From d13dc9fa2b7d47de4e29e023ffb76d89f176ed8b Mon Sep 17 00:00:00 2001 From: abrichr Date: Mon, 27 Jul 2026 23:00:09 -0400 Subject: [PATCH] fix(auth): stop the unwired browser-PKCE provider from stalling login `available_providers()` puts `BrowserPkceProvider` first on every interactive desktop, and its `is_available()` returned True on macOS/Windows regardless of configuration. `_exchange_code` is the first step that needs a Supabase project, but it only runs *after* the loopback wait -- so `openadapt-desktop login` opened a browser tab and then blocked for the full 180s default timeout before failing over to `PasteTokenProvider`. Reproduced against this commit's parent on macOS with no configuration: `is_available()` -> True, one tab opened to `https://app.openadapt.ai/login?redirect_to=http://127.0.0.1:/callback&...`, then `RuntimeError: Timed out waiting for the browser login to complete.` The provider is not wired end to end, and the missing pieces are all hosted-side: * `GET /login` reads only `checkout_session_id` and `next`. It ignores `redirect_to`, `code_challenge`, `code_challenge_method` and `state` -- verified live against app.openadapt.ai, where those four land in the RSC `searchParams` payload and are never consumed. The browser therefore never navigates to the loopback listener. * The Supabase `uri_allow_list` is rewritten on every production deploy and holds no loopback entry, so a `127.0.0.1` redirect would be rejected anyway. * `POST /api/ingest-tokens` authenticates the browser session cookie and requires a `name` field; it answers 401 to the `Authorization: Bearer ` this provider sends, and the provider sends `label` rather than `name`. `OPENADAPT_SUPABASE_URL` / `OPENADAPT_SUPABASE_ANON_KEY` are referenced nowhere except this module, so the provider has never been able to complete anywhere. Make `is_available()` require the Supabase configuration up front. That is a necessary condition it already depends on, so the provider now reports unavailable instead of costing the operator a silent multi-minute stall, and the paste provider -- which works -- runs immediately. Nothing is removed and the chain order is unchanged: configuring the environment plus the hosted contract restores the flow. The existing `test_full_flow` passed only because it stubs both `_exchange_code` and `_mint_ingest_token` -- exactly the two halves the server does not implement. It now runs against a configured provider so it still covers the wired path, and new regression tests cover the unconfigured case, partial configuration, and the user-visible symptom (no stray browser tab). The shipped path to a credential remains the one-time pairing flow in `engine/auth/pairing.py`, which the hosted control plane does implement. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM --- engine/auth/browser_pkce.py | 37 ++++++++++++++- tests/test_engine/test_auth_browser_pkce.py | 51 +++++++++++++++++++-- 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/engine/auth/browser_pkce.py b/engine/auth/browser_pkce.py index bc19f67..0bcb079 100644 --- a/engine/auth/browser_pkce.py +++ b/engine/auth/browser_pkce.py @@ -17,6 +17,31 @@ ``is_available()`` is False on a headless server (no browser / no loopback), so the UI falls back to :class:`~engine.auth.paste.PasteTokenProvider`. +NOT WIRED END TO END -- do not enable this provider by setting the Supabase +environment variables alone. Steps 2-4 above each depend on a hosted contract +that ``app.openadapt.ai`` does not implement today: + +* ``GET /login`` reads only ``checkout_session_id`` and ``next`` (clamped to a + same-origin ``/dashboard`` destination). It ignores ``redirect_to``, + ``code_challenge``, ``code_challenge_method`` and ``state``, so the browser + never navigates to the loopback listener and step 3 cannot happen. +* The Supabase ``uri_allow_list`` is rewritten on every production deploy and + contains no loopback entry, so Supabase would reject a ``127.0.0.1`` redirect + even if the page forwarded one. +* ``POST /api/ingest-tokens`` authenticates the browser session cookie and + requires a ``name`` field. It rejects a ``Authorization: Bearer `` with 401, so step 4 cannot mint. + +Because ``_exchange_code`` is only reached AFTER the loopback wait, leaving the +provider enabled makes ``login`` open a browser tab and then block for the full +``timeout`` before failing over to token paste. ``is_available()`` therefore +requires the Supabase configuration up front: the provider reports unavailable +rather than costing the operator a silent multi-minute stall. Restoring it is a +hosted-contract change, not a client change -- see the three bullets above. + +The working, shipped path to a credential is the one-time pairing flow +(:mod:`engine.auth.pairing`), which the hosted control plane does implement. + Reconciliation note: the shared store (:mod:`engine.auth.store`) keys ONE active credential per host and ``auth_header()`` returns exactly one bearer. We therefore fold the Supabase session and the minted ingest token into a single @@ -161,7 +186,17 @@ def _default_open_browser(url: str) -> None: webbrowser.open(url) def is_available(self) -> bool: - """False on a headless box (no browser / no display).""" + """False on a headless box, and False while the flow is unconfigured. + + The Supabase check is deliberately first. ``_exchange_code`` cannot run + without a project URL and anon key, but it is only reached after the + loopback wait -- so an unconfigured provider would open a browser tab + and stall for the whole ``timeout`` before failing over to token paste. + Reporting unavailable up front keeps ``login`` responsive and keeps the + provider chain honest about what it can actually complete. + """ + if not (self._supabase_url and self._supabase_anon_key): + return False if os.environ.get("OPENADAPT_HEADLESS", "").strip(): return False if sys.platform.startswith("linux"): diff --git a/tests/test_engine/test_auth_browser_pkce.py b/tests/test_engine/test_auth_browser_pkce.py index 81165ee..4247505 100644 --- a/tests/test_engine/test_auth_browser_pkce.py +++ b/tests/test_engine/test_auth_browser_pkce.py @@ -49,22 +49,65 @@ def _deliver(): assert receiver.state == "s1" +def _configured(**kwargs) -> BrowserPkceProvider: + """A provider whose Supabase project is configured, as in a wired deployment.""" + kwargs.setdefault("supabase_url", "https://project.supabase.co") + kwargs.setdefault("supabase_anon_key", "anon_key") + return BrowserPkceProvider(**kwargs) + + class TestIsAvailable: def test_headless_env_false(self, monkeypatch) -> None: monkeypatch.setenv("OPENADAPT_HEADLESS", "1") - assert BrowserPkceProvider().is_available() is False + assert _configured().is_available() is False def test_linux_without_display_false(self, monkeypatch) -> None: monkeypatch.delenv("OPENADAPT_HEADLESS", raising=False) monkeypatch.setattr("engine.auth.browser_pkce.sys.platform", "linux") monkeypatch.delenv("DISPLAY", raising=False) monkeypatch.delenv("WAYLAND_DISPLAY", raising=False) + assert _configured().is_available() is False + + def test_macos_true_when_configured(self, monkeypatch) -> None: + monkeypatch.delenv("OPENADAPT_HEADLESS", raising=False) + monkeypatch.setattr("engine.auth.browser_pkce.sys.platform", "darwin") + assert _configured().is_available() is True + + def test_unconfigured_is_unavailable_even_on_a_desktop(self, monkeypatch) -> None: + """An unconfigured provider must not lead the chain and stall the login. + + Without a Supabase project the exchange cannot succeed, but it is only + attempted after the loopback wait -- so reporting available here would + open a browser tab and block for the full timeout before falling back + to token paste. Regression guard for that stall. + """ + monkeypatch.delenv("OPENADAPT_HEADLESS", raising=False) + monkeypatch.delenv("OPENADAPT_SUPABASE_URL", raising=False) + monkeypatch.delenv("OPENADAPT_SUPABASE_ANON_KEY", raising=False) + monkeypatch.setattr("engine.auth.browser_pkce.sys.platform", "darwin") assert BrowserPkceProvider().is_available() is False - def test_macos_true(self, monkeypatch) -> None: + def test_partial_configuration_is_unavailable(self, monkeypatch) -> None: monkeypatch.delenv("OPENADAPT_HEADLESS", raising=False) monkeypatch.setattr("engine.auth.browser_pkce.sys.platform", "darwin") - assert BrowserPkceProvider().is_available() is True + url_only = BrowserPkceProvider( + supabase_url="https://project.supabase.co", supabase_anon_key="" + ) + key_only = BrowserPkceProvider(supabase_url="", supabase_anon_key="anon_key") + assert url_only.is_available() is False + assert key_only.is_available() is False + + def test_unconfigured_provider_never_opens_a_browser(self, monkeypatch) -> None: + """The stall is user-visible as a stray tab; assert none is opened.""" + monkeypatch.delenv("OPENADAPT_HEADLESS", raising=False) + monkeypatch.delenv("OPENADAPT_SUPABASE_URL", raising=False) + monkeypatch.delenv("OPENADAPT_SUPABASE_ANON_KEY", raising=False) + monkeypatch.setattr("engine.auth.browser_pkce.sys.platform", "darwin") + opened: list[str] = [] + provider = BrowserPkceProvider(open_browser=opened.append) + with pytest.raises(RuntimeError, match="unavailable"): + provider.login() + assert opened == [] class TestLogin: @@ -84,7 +127,7 @@ def _deliver(): threading.Thread(target=_deliver, daemon=True).start() - provider = BrowserPkceProvider(host="https://app.openadapt.ai", open_browser=_open_browser) + provider = _configured(host="https://app.openadapt.ai", open_browser=_open_browser) provider._exchange_code = lambda code, verifier, redirect_uri: { # type: ignore[assignment] "access_token": "supabase_access", "refresh_token": "supabase_refresh",