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

Activity

1. cliffhall

   added

   v2Issues and PRs for v2Issues and PRs for v2

   on Jul 1, 2026
2. cliffhall

   added a parent issue on Jul 1, 2026

   • Tracking: re-implement PR #1510 (spec-conformant Apps host + programmatic review path) as scoped issues #1579
3. cliffhall

   self-assigned this

   on Jul 1, 2026
4. cliffhall

   linked a pull request that will close this issueauth hardening: generateOAuthState requires WebCrypto; OAuth callback rejects unparseable state #1589on Jul 1, 2026
5. cliffhall commented on Jul 2, 2026

   

   cliffhall

   on Jul 2, 2026

   MemberAuthor

   More actions

   Smoke test & AGENTS.md audit — PR #1589

   Checked out v2/1562-oauth-state-hardening (d21da16), npm install at root, and exercised both hardening paths plus a full compliance/coverage audit. Verdict: LGTM — ship it.

   Smoke test (real output)

   A standalone script re-implements the two guarded paths verbatim from core/auth/utils.ts and drives them directly (throw path + shape guard). It stubs crypto away to prove generateOAuthState() throws instead of falling back to Math.random(), and feeds malformed vs. valid state through the parseOAuthState shape guard:

   === generateOAuthState ===
   PASS  returns 64-char hex when WebCrypto present -> 8ae0312d146315d4…
   PASS  two calls produce different values
   PASS  throws when crypto global is absent -> "crypto.getRandomValues is not available; refusing to generate an OAuth state with a non-cryptographic RNG."
   PASS  throws when crypto.getRandomValues is missing
   
   === parseOAuthState (callback shape guard) ===
   PASS  rejects malformed state "not-a-valid-state" -> null
   PASS  rejects empty string -> null
   PASS  rejects short hex (63 chars) -> null
   PASS  rejects 64 non-hex chars -> null
   PASS  accepts valid 64-char-hex -> {authId}
   PASS  round-trips a freshly generated state
   
   ALL PASS: 10 passed, 0 failed
   

   The throw fires for both the missing-crypto-global and missing-getRandomValues cases; the callback guard rejects anything that is not a 64-char-hex authId while accepting the well-formed shape.

   Screenshot: skipped intentionally (no rabbit-holing). The browser rejection toast is already covered by the App.test.tsx component test — App OAuth callback state validation asserts the "OAuth callback rejected" red toast on an unparseable state and its absence on a valid one (both pass, see below). The log proof above plus that component test cover the behavior end to end.

   AGENTS.md compliance audit (diff-scoped)

   • Mantine idioms — the App.tsx callback-rejection path uses notifications.show({ title, message, color: "red" }). No inline styles, no raw hex/rgba(), no new CSS classes or classNames introduced in the diff. ✅
   • TypeScript — no any, no eslint/ts suppressions. The new parsedState/stateRejected/sessionId locals are properly narrowed (stateParam !== null && parsedState === null). ✅
   • Tests present — throw-path tests for both branches in utils.test.ts (asserted via toThrow, no stray console output); accept/reject callback tests in App.test.tsx. ✅

   Coverage & integration

   cd clients/web && npm run test:coverage — the real gate (--project=unit --project=integration) passes. core/auth/utils.ts:

   File	% Stmts	% Branch	% Funcs	% Lines
   core/auth/utils.ts	100	96.66	100	100

   All four dimensions clear the ≥90 per-file gate. (The single uncovered branch is on line 12, inside the pre-existing parseHttpUrl, untouched by this PR.)

   npm run test:integration — 799 passed (38 files), including the OAuth callback paths.

   Note: on the first coverage pass, 12 tests in unrelated components (ServerConfigModal, InspectorView, PromptArgumentsForm, the import modals — no auth files) flaked at the default 5s waitFor/userEvent timeout under heavy v8-instrumented load (durations clustered right at 5000ms). Re-running with --testTimeout=30000 on an unloaded machine, the full suite passes clean (229 files, exit 0, no threshold errors). These are environmental timeout flakes, not regressions from this PR.

   Verdict

   LGTM. Both fixes do exactly what the PR describes: generateOAuthState fails loudly rather than minting a Math.random() CSRF token, and the callback rejects a forged/malformed state with a clear toast instead of proceeding with an undefined session id. Diff is Mantine/TS-clean, coverage clears the gate, integration green.

   Smoke-test script (run with node oauth-hardening-smoke.mjs; not committed)

   // Smoke test for PR #1589 (OAuth state hardening).
   // Re-implements the two guarded code paths verbatim from the PR sources so we
   // can exercise them from a plain node/tsx runtime without the vite alias.
   //
   //   1. generateOAuthState()  (core/auth/utils.ts) — MUST throw when
   //      crypto.getRandomValues is unavailable; returns 64-char hex otherwise.
   //   2. parseOAuthState()     (core/auth/utils.ts) — the callback shape guard;
   //      rejects malformed `state`, accepts a 64-char-hex authId.
   
   const generateOAuthState = () => {
     if (typeof crypto === "undefined" || !crypto.getRandomValues) {
       throw new Error(
         "crypto.getRandomValues is not available; refusing to generate an OAuth state with a non-cryptographic RNG.",
       );
     }
     const array = new Uint8Array(32);
     crypto.getRandomValues(array);
     return Array.from(array, (b) => b.toString(16).padStart(2, "0")).join("");
   };
   
   const parseOAuthState = (state) => {
     if (!state || typeof state !== "string") return null;
     if (/^[a-f0-9]{64}$/i.test(state)) return { authId: state };
     return null;
   };
   
   let pass = 0;
   let fail = 0;
   const ok = (label, cond) => {
     console.log(`${cond ? "PASS" : "FAIL"}  ${label}`);
     cond ? pass++ : fail++;
   };
   
   console.log("=== generateOAuthState ===");
   const realCrypto = globalThis.crypto;
   
   // (a) WebCrypto present -> returns 64-char hex
   const state = generateOAuthState();
   ok(`returns 64-char hex when WebCrypto present -> ${state.slice(0, 16)}…`, /^[a-f0-9]{64}$/.test(state));
   
   // (b) two calls differ
   ok("two calls produce different values", generateOAuthState() !== generateOAuthState());
   
   // (c) crypto global entirely absent -> throws
   Object.defineProperty(globalThis, "crypto", { configurable: true, writable: true, value: undefined });
   let threwAbsent = false;
   let msgAbsent = "";
   try { generateOAuthState(); } catch (e) { threwAbsent = true; msgAbsent = e.message; }
   ok(`throws when crypto global is absent -> "${msgAbsent}"`, threwAbsent && /getRandomValues is not available/.test(msgAbsent));
   
   // (d) crypto present but getRandomValues missing -> throws
   Object.defineProperty(globalThis, "crypto", { configurable: true, writable: true, value: {} });
   let threwMissing = false;
   try { generateOAuthState(); } catch { threwMissing = true; }
   ok("throws when crypto.getRandomValues is missing", threwMissing);
   
   // restore
   Object.defineProperty(globalThis, "crypto", { configurable: true, writable: true, value: realCrypto });
   
   console.log("\n=== parseOAuthState (callback shape guard) ===");
   ok('rejects malformed state "not-a-valid-state" -> null', parseOAuthState("not-a-valid-state") === null);
   ok('rejects empty string -> null', parseOAuthState("") === null);
   ok('rejects short hex (63 chars) -> null', parseOAuthState("a".repeat(63)) === null);
   ok('rejects 64 non-hex chars -> null', parseOAuthState("z".repeat(64)) === null);
   const valid = parseOAuthState("a".repeat(64));
   ok('accepts valid 64-char-hex -> {authId}', valid !== null && valid.authId === "a".repeat(64));
   const rt = parseOAuthState(state);
   ok("round-trips a freshly generated state", rt !== null && rt.authId === state);
   
   console.log(`\n${fail === 0 ? "ALL PASS" : "FAILURES"}: ${pass} passed, ${fail} failed`);
   process.exit(fail === 0 ? 0 : 1);

   _{Smoke test & audit performed by Claude Code on behalf of @cliffhall.}
6. cliffhall commented on Jul 2, 2026

   

   cliffhall

   on Jul 2, 2026

   MemberAuthor

   More actions

   🔗 Implemented by PR #1589. Its code is already integrated into the Wave 1 rollup branch 1579-wave-1 (see rollup PR #1601 / verification #1600). #1589 currently targets v2/main and will auto-show as Merged once the rollup lands in v2/main. The Development panel doesn't link it now because the closing keyword is on a non-default base branch.
7. cliffhall

   closed this as completedon Jul 9, 2026
8. pull

   added a commit that references this issue on Jul 28, 2026

   feat(auth): harden OAuth state generation and callback validation (cl…

   ...

   This issue will close once commit 2b84281 is merged into the 'main' branch.

   2b84281
9. cliffhall

   added this to the v2.0.0 milestone on Jul 28, 2026