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

[cliffhall]

Summary

Two small OAuth hardening fixes:

1. generateOAuthState() (core/auth/utils.ts) throws when crypto.getRandomValues is unavailable instead of silently falling back to Math.random().
2. The web OAuth callback handler (App.tsx) parses the returned state param and rejects anything that doesn't match the expected {mode}:{authId} shape, surfacing a clear error instead of proceeding.

Why

Math.random() is not a CSRF-grade entropy source — better to fail loudly on exotic runtimes than mint guessable state. And an unparseable state on the callback is a forgery indicator; rejecting it defeats simple CSRF attempts against the callback route.

Reference implementation (PR #1510)

Re-implement informed by these changes at 33fac3f:

• core/auth/utils.ts — generateOAuthState() throws without WebCrypto (+10/−13)
• clients/web/src/App.tsx — OAuth callback state validation (small part of a larger diff; only the callback-handler hunks are in scope here)

Depends on

Nothing — Wave 1 foundation, parallel-safe.

Notes

• In tests that exercise the throw path, suppress expected error output from the console per AGENTS.md.
• Coverage gate ≥90 on all four dimensions for touched files.

Part of the PR #1510 decomposition (see tracking issue).

[cliffhall]

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.}

[cliffhall]

🔗 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.