mirror of
https://github.com/usestrix/strix.git
synced 2026-08-21 18:52:47 +02:00
review: harden OAuth state check and tighten error handling
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
d35af02e47
commit
3d419d312f
+7
-4
@@ -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,
|
||||
},
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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}
|
||||
|
||||
Reference in New Issue
Block a user