From 3d419d312f60e3b7aa25ce2847aeed89611c5feb Mon Sep 17 00:00:00 2001 From: Jonathan Singer Date: Wed, 22 Jul 2026 16:14:09 -0400 Subject: [PATCH] review: harden OAuth state check and tighten error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address self-review findings ahead of code review: - auth_cli: require the OAuth `state` on the automated loopback callback (reject missing/mismatched — CSRF), keep manual paste lenient since it's user-initiated. Add tests for _finish state handling. - codex: drop the redundant chatgpt-account-id from client default_headers; the per-request auth hook already stamps it (single source of truth). - main: tighten the "sign-in expired" hint to match `error code: 401` / `http 401` rather than a bare "401" substring that could misfire. Co-Authored-By: Claude Fable 5 --- strix/auth/codex.py | 11 +++++++---- strix/interface/auth_cli.py | 17 ++++++++++++++--- strix/interface/main.py | 7 ++++++- tests/test_auth_cli.py | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 8 deletions(-) diff --git a/strix/auth/codex.py b/strix/auth/codex.py index 986c6503..6b5551e9 100644 --- a/strix/auth/codex.py +++ b/strix/auth/codex.py @@ -333,12 +333,16 @@ def build_openai_client() -> AsyncOpenAI: # Validate up front so sign-in problems surface at configure time, not # mid-scan, and to fail fast if the stored refresh token is dead. - _access, account_id = get_valid_token() + get_valid_token() async def _auth_hook(request: httpx.Request) -> None: - access, acct = await asyncio.to_thread(get_valid_token) + # Refresh-aware auth stamped per request so a scan that outlives one + # access token keeps working. The account id can only change with the + # token, so it is set here alongside the bearer rather than as a static + # default header. + access, account_id = await asyncio.to_thread(get_valid_token) request.headers["Authorization"] = f"Bearer {access}" - request.headers["chatgpt-account-id"] = acct + request.headers["chatgpt-account-id"] = account_id http_client = httpx.AsyncClient( timeout=httpx.Timeout(600.0, connect=30.0), @@ -349,7 +353,6 @@ def build_openai_client() -> AsyncOpenAI: base_url=CODEX_BASE_URL, http_client=http_client, default_headers={ - "chatgpt-account-id": account_id, "OpenAI-Beta": "responses=experimental", "originator": ORIGINATOR, }, diff --git a/strix/interface/auth_cli.py b/strix/interface/auth_cli.py index 312fc3f0..bc3941a1 100644 --- a/strix/interface/auth_cli.py +++ b/strix/interface/auth_cli.py @@ -188,7 +188,7 @@ def _run_oauth_flow( code, returned_state, error = result if error: raise codex.CodexAuthError("oauth_error", error) - return _finish(code, returned_state, verifier, state) + return _finish(code, returned_state, verifier, state, require_state=True) console.print("[yellow]Timed out waiting for the browser. Falling back to manual paste.[/]") # Manual fallback: the user completes sign-in and pastes the redirect URL @@ -200,14 +200,25 @@ def _run_oauth_flow( except EOFError as exc: raise codex.CodexAuthError("no_input", "no redirect URL provided") from exc code, returned_state = codex.parse_redirect_input(pasted) - return _finish(code, returned_state, verifier, state) + return _finish(code, returned_state, verifier, state, require_state=False) def _finish( - code: str | None, returned_state: str | None, verifier: str, expected_state: str + code: str | None, + returned_state: str | None, + verifier: str, + expected_state: str, + *, + require_state: bool, ) -> dict[str, Any]: if not code: raise codex.CodexAuthError("no_code", "no authorization code found in the redirect") + # The loopback callback from OpenAI always carries state, so a missing or + # mismatched value there is forged (CSRF) and must be rejected. Manual paste + # is user-initiated (the user copies their own redirect), so state is only + # validated when the pasted value includes it. + if require_state and returned_state is None: + raise codex.CodexAuthError("state_mismatch", "missing state in callback; possible CSRF") if returned_state is not None and returned_state != expected_state: raise codex.CodexAuthError("state_mismatch", "state did not match; possible CSRF") return codex.exchange_code(code, verifier) diff --git a/strix/interface/main.py b/strix/interface/main.py index b06d3e89..d3117a38 100644 --- a/strix/interface/main.py +++ b/strix/interface/main.py @@ -372,7 +372,12 @@ def _subscription_error_hint(exc: BaseException) -> str | None: "The ChatGPT backend requires streamed requests, but this call wasn't " "streamed. This is an internal Strix issue on this path — please report it." ) - if "401" in joined or "unauthorized" in joined or "invalid_grant" in joined: + if ( + "error code: 401" in joined + or "http 401" in joined + or "unauthorized" in joined + or "invalid_grant" in joined + ): return ( "Your ChatGPT sign-in has expired or was revoked. Sign in again:\n" " strix auth login chatgpt" diff --git a/tests/test_auth_cli.py b/tests/test_auth_cli.py index 9cbf444d..3c32b89e 100644 --- a/tests/test_auth_cli.py +++ b/tests/test_auth_cli.py @@ -46,6 +46,40 @@ def test_login_rejects_unsupported_provider(monkeypatch: pytest.MonkeyPatch) -> assert auth_cli.run_auth(["login", "gemini"]) == 2 +def test_finish_requires_state_on_loopback(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(codex, "exchange_code", lambda *_: {"ok": True}) + + # Loopback (require_state=True): missing or mismatched state is rejected. + with pytest.raises(codex.CodexAuthError) as missing: + auth_cli._finish("code", None, "verifier", "expected", require_state=True) + assert missing.value.code == "state_mismatch" + with pytest.raises(codex.CodexAuthError) as mismatch: + auth_cli._finish("code", "wrong", "verifier", "expected", require_state=True) + assert mismatch.value.code == "state_mismatch" + + # Matching state proceeds to the exchange. + assert auth_cli._finish("code", "expected", "verifier", "expected", require_state=True) == { + "ok": True + } + + +def test_finish_manual_paste_allows_absent_state(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(codex, "exchange_code", lambda *_: {"ok": True}) + # Manual paste (require_state=False): a bare code with no state is accepted, + # but a present-and-wrong state is still rejected. + assert auth_cli._finish("code", None, "verifier", "expected", require_state=False) == { + "ok": True + } + with pytest.raises(codex.CodexAuthError): + auth_cli._finish("code", "wrong", "verifier", "expected", require_state=False) + + +def test_finish_rejects_missing_code() -> None: + with pytest.raises(codex.CodexAuthError) as exc: + auth_cli._finish(None, "expected", "verifier", "expected", require_state=True) + assert exc.value.code == "no_code" + + @pytest.mark.parametrize("provider", ["chatgpt", "codex", "ChatGPT"]) def test_login_accepts_provider_aliases(provider: str, monkeypatch: pytest.MonkeyPatch) -> None: reached = {"flow": False}