auth hardening: generateOAuthState requires WebCrypto; OAuth callback rejects unparseable state

[cliffhall]

Closes #1562

Wave 1 of the PR #1510 decomposition (tracking #1579). Two small OAuth hardening fixes, scoped to the core auth state generation + web OAuth callback validation.

Changes

1. generateOAuthState() (core/auth/utils.ts) now throws when crypto.getRandomValues is unavailable instead of silently falling back to Math.random(). Math.random() is not a CSRF-grade entropy source — better to fail loudly on exotic runtimes than mint a guessable state. The throw guards both the missing-crypto-global and missing-getRandomValues cases.

2. The web /oauth/callback handler (App.tsx) now parses the returned state param and rejects anything that does not match the expected 64-char-hex authId shape produced by generateOAuthState. An unparseable state is a forgery indicator; the handler surfaces a clear "OAuth callback rejected" toast and returns instead of silently proceeding with an undefined session id.

Tests

• core/auth/utils.ts — added throw-path tests for both the absent-crypto and missing-getRandomValues branches (expected error is asserted via toThrow, no stray console output). utils.ts clears the ≥90 per-file gate on all four dimensions.
• App.test.tsx — added a /oauth/callback describe covering the reject-unparseable-state path (asserts the rejection toast) and the accept-valid-state path (asserts the rejection toast is not shown).

Validation

• cd clients/web && npm run validate — pass (format, lint, build, 2442 unit tests).
• npm run test:integration — pass (799 tests).
• Coverage gate: core/auth/utils.ts at 100% lines/functions/statements, ≥90% branches.

Notes

The reference PR #1510 used a {mode}:{authId} state shape (guided auth), but guided auth was since removed on v2/main, so this re-implementation adapts to the current single authId shape. The reference also routed the callback error through recordConnectError (part of a separate Wave 1 EMA/data-status change not present here), so this uses the current handler's notifications.show style.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 19s —— View job

---

Review — PR #1589 (OAuth state hardening)

• Read changed files & diff against origin/v2/main
• Review generateOAuthState() hardening
• Review /oauth/callback state validation
• Review tests

Overall this is a clean, tightly-scoped, well-documented hardening PR. Both changes are correct and the tests cover the new branches. A few observations below — none blocking.

✅ What's solid

• generateOAuthState() (core/auth/utils.ts:52-67) — failing loudly instead of degrading to Math.random() is the right call for a CSRF token. The guard covers both the missing-global (typeof crypto === "undefined") and missing-method (!crypto.getRandomValues) cases, and crypto.getRandomValues(array) is invoked with crypto as the receiver so this-binding is fine.
• Callback guard (App.tsx:1480-1500) — the logic is precise. stateRejected = stateParam !== null && parsedState === null correctly rejects a present-but-malformed state, and replaceState({}, "", "/") still runs before the early return, so the single-use code/state can't be replayed on reload regardless of the outcome. Ordering the rejection before the !params.successful branch is also correct.
• Tests exercise both new throw branches and both callback branches, with crypto properly restored in afterEach.

🔎 Observations / possible follow-ups

1. Shape validation is not full state-matching (defense-in-depth only). The guard checks that state looks like a 64-char-hex authId, but does not compare it against a value this client actually generated and stored before the redirect. An attacker can trivially craft a well-formed 64-hex string that passes. The genuine CSRF protection here remains PKCE (code_verifier), so this is a reasonable defense-in-depth layer — but the toast copy "rejecting to prevent a cross-site request" slightly overstates what shape validation alone achieves. Consider softening the wording (e.g. "unrecognized state parameter") or, as a stronger follow-up, comparing parsedState.authId against the generated state persisted alongside OAUTH_PENDING_SERVER_KEY. Fine to defer given the narrow scope.

2. Missing state is silently accepted; empty state is rejected. With ?state= (empty), stateParam === "" → stateRejected is true. With no state param at all, stateParam === null → stateRejected is false and the flow proceeds with sessionId = undefined. Since the inspector's own flow always emits a state (generateOAuthState), a genuinely absent state is just as anomalous as an empty one. This preserves prior behavior, so it's a judgment call, but worth a comment noting the intentional asymmetry (or rejecting the null case too).

3. Minor test fragility (App.test.tsx:1627). The "valid state is not rejected" test asserts some toast fired and that it isn't the rejection toast — it passes because the flow falls through to the "could not be matched" toast (no pendingId in cleared sessionStorage). That's an indirect assertion on a downstream failure. Seeding OAUTH_PENDING_SERVER_KEY (or asserting the negative alone) would make intent clearer and less coupled to unrelated downstream behavior.

None of these block merge — the security posture strictly improves. Items 1–2 are the ones worth a maintainer decision.
· v2/1562-oauth-state-hardening

[cliffhall]

Thanks for the review. Addressed all three observations in d21da16:

1. Toast copy overstated the guarantee. Softened the rejection message from "rejecting to prevent a cross-site request" to "…did not originate from this session", and expanded the inline comment to state explicitly that this is a defense-in-depth shape check with PKCE (code_verifier) as the primary CSRF protection — not full state-matching.

2. Missing vs. empty state asymmetry. Kept the current behavior (present-but-malformed rejected; wholly absent accepted and matched via OAUTH_PENDING_SERVER_KEY) but documented it as intentional: rejecting the null case would turn any provider error redirect that omits state into a misleading "rejected" toast, masking the real OAuth error surfaced by the !params.successful branch.

3. Indirect test assertion. The "valid state is not rejected" test now seeds OAUTH_PENDING_SERVER_KEY and asserts the specific downstream "OAuth callback could not be matched" toast, proving the state was accepted and the flow advanced past the guard — rather than the previous "some toast fired AND not the rejection toast" check.

npm run validate (unit: 2442 passed) and npm run test:integration (799 passed) both green.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 48s —— View job

---

Re-review — PR #1589 (OAuth state hardening)

• Read latest diff vs origin/v2/main
• Verify the 3 addressed observations
• Review current generateOAuthState() + callback + tests
• Post findings

Reviewed commit d21da169 against the prior round. All three observations are addressed correctly, and I found no new issues. LGTM.

✅ Observations resolved

1. Toast copy overstatement — Fixed well. The message is now "…an unrecognized state parameter that did not originate from this session" (App.tsx:1505-1506), and the inline comment (App.tsx:1475-1483) now states plainly that this is a shape check, not full state-matching, with PKCE (code_verifier) as the primary CSRF protection. The copy no longer promises more than shape validation delivers. 👍

2. Missing vs. empty state asymmetry — Documented as intentional (App.tsx:1484-1491), with the right rationale: rejecting the wholly-absent (null) case would convert a provider error redirect that omits state into a misleading "rejected" toast and mask the real OAuth error from the !params.successful branch. The distinction is correctly implemented: stateRejected = stateParam !== null && parsedState === null, so ?state= (empty → falsy → parsedState === null → rejected) and no-state (null → not rejected) diverge exactly as documented.

3. Indirect test assertion — Tightened as promised (App.test.tsx:1631-1657). The accept-path test now seeds OAUTH_PENDING_SERVER_KEY and asserts the specific downstream "OAuth callback could not be matched" toast (plus a negative assertion on the rejection toast), proving the state was accepted and the flow advanced past the guard rather than relying on "some toast fired."

🔎 Spot checks (all pass)

• generateOAuthState() (core/auth/utils.ts:52-66) — guard covers both typeof crypto === "undefined" and !crypto.getRandomValues; array is now allocated only on the success path; receiver binding on crypto.getRandomValues(array) is correct. Throw message is descriptive.
• parseOAuthState shape (^[a-f0-9]{64}$/i) matches what generateOAuthState mints (32 bytes → 64 hex), so the accept-path test's "a".repeat(64) is a faithful positive case.
• replaceState({}, "", "/") still runs before the stateRejected early return, so the single-use code/state can't be replayed on reload.
• utils.test.ts restores the real crypto in afterEach via Object.defineProperty, so the stub/remove tests don't leak into sibling tests.

Nothing blocking — the security posture strictly improves and the follow-up items from the first round are all handled. Ship it.
· v2/1562-oauth-state-hardening

[cliffhall]

📋 Smoke test & AGENTS.md audit for this PR: #1562 (comment) (posted on the linked issue #1562).