From 2b84281346452559252b8a7210c6bd839963717f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 1 Jul 2026 18:23:38 -0400 Subject: [PATCH 1/2] feat(auth): harden OAuth state generation and callback validation (closes #1562) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit generateOAuthState() now throws when crypto.getRandomValues is unavailable instead of silently degrading to a non-cryptographic Math.random() fallback — OAuth state is a CSRF token and must be unpredictable. The web /oauth/callback handler now rejects a returned `state` param that does not parse to the expected 64-char-hex authId shape (a forgery indicator), surfacing a clear error toast instead of proceeding with an undefined session. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX --- clients/web/src/App.test.tsx | 53 ++++++++++++++++++++ clients/web/src/App.tsx | 21 ++++++-- clients/web/src/test/core/auth/utils.test.ts | 38 +++++++++++++- core/auth/utils.ts | 23 ++++----- 4 files changed, 118 insertions(+), 17 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 23808833..dfa0f352 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -1589,3 +1589,56 @@ describe("App history pin/replay", () => { ); }); }); + +// The `/oauth/callback` handler must reject a returned `state` that does not +// parse to the expected 64-char-hex authId shape (a forgery indicator) instead +// of silently proceeding. See #1562. +describe("App OAuth callback state validation", () => { + const originalUrl = window.location.href; + + beforeEach(() => { + notificationsMock.show.mockClear(); + window.sessionStorage.clear(); + }); + + afterEach(() => { + window.history.replaceState({}, "", originalUrl); + }); + + it("rejects an unparseable state param with a clear error toast", async () => { + window.history.replaceState( + {}, + "", + "/oauth/callback?code=abc123&state=not-a-valid-state", + ); + + renderWithMantine(); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "OAuth callback rejected", + color: "red", + }), + ), + ); + }); + + it("does not reject when the state param parses to a valid authId", async () => { + // A well-formed 64-char-hex state is accepted; the handler proceeds past + // the rejection guard (it fails later for other reasons in this harness, + // but never shows the 'OAuth callback rejected' toast). + window.history.replaceState( + {}, + "", + `/oauth/callback?code=abc123&state=${"a".repeat(64)}`, + ); + + renderWithMantine(); + + await waitFor(() => expect(notificationsMock.show).toHaveBeenCalled()); + expect(notificationsMock.show).not.toHaveBeenCalledWith( + expect.objectContaining({ title: "OAuth callback rejected" }), + ); + }); +}); diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index a08dedbc..176f6fcf 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1472,10 +1472,15 @@ function App() { // session key the pre-redirect page saved the fetch log under, so the // rebuilt client can restore those `auth` entries. Read it before the // URL is cleared below. + // + // CSRF hardening: a `state` that is present but does not parse to our + // expected 64-char-hex authId shape is a forgery indicator — the value + // did not originate from `generateOAuthState`. Reject the callback with a + // clear error instead of silently proceeding with an undefined sessionId. const stateParam = new URLSearchParams(window.location.search).get("state"); - const sessionId = stateParam - ? (parseOAuthState(stateParam)?.authId ?? undefined) - : undefined; + const parsedState = stateParam ? parseOAuthState(stateParam) : null; + const stateRejected = stateParam !== null && parsedState === null; + const sessionId = parsedState?.authId ?? undefined; const pendingId = window.sessionStorage.getItem(OAUTH_PENDING_SERVER_KEY) ?? undefined; window.sessionStorage.removeItem(OAUTH_PENDING_SERVER_KEY); @@ -1484,6 +1489,16 @@ function App() { // the (now single-use) authorization code through the exchange again. window.history.replaceState({}, "", "/"); + if (stateRejected) { + notifications.show({ + title: "OAuth callback rejected", + message: + "OAuth callback carried an unrecognized state parameter; rejecting to prevent a cross-site request. Please try connecting again.", + color: "red", + }); + return; + } + if (!params.successful) { notifications.show({ title: "OAuth authorization failed", diff --git a/clients/web/src/test/core/auth/utils.test.ts b/clients/web/src/test/core/auth/utils.test.ts index 712b95ee..08189dca 100644 --- a/clients/web/src/test/core/auth/utils.test.ts +++ b/clients/web/src/test/core/auth/utils.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, afterEach } from "vitest"; import { parseHttpUrl, parseOAuthCallbackParams, @@ -51,6 +51,17 @@ describe("parseOAuthCallbackParams", () => { }); describe("generateOAuthState", () => { + const originalCrypto = globalThis.crypto; + + afterEach(() => { + // Restore the real WebCrypto after tests that stub/remove it. + Object.defineProperty(globalThis, "crypto", { + configurable: true, + writable: true, + value: originalCrypto, + }); + }); + it("should generate 64-char hex state", () => { const state = generateOAuthState(); expect(state).toMatch(/^[a-f0-9]{64}$/i); @@ -61,6 +72,31 @@ describe("generateOAuthState", () => { const s2 = generateOAuthState(); expect(s1).not.toBe(s2); }); + + it("throws when the crypto global is entirely absent (no silent Math.random fallback)", () => { + // Simulate an exotic runtime with no WebCrypto at all. Rather than minting + // a guessable CSRF token, generateOAuthState must fail loudly. + Object.defineProperty(globalThis, "crypto", { + configurable: true, + writable: true, + value: undefined, + }); + expect(() => generateOAuthState()).toThrow( + /crypto\.getRandomValues is not available/, + ); + }); + + it("throws when crypto.getRandomValues is missing", () => { + // crypto exists but lacks getRandomValues — still refuse to degrade. + Object.defineProperty(globalThis, "crypto", { + configurable: true, + writable: true, + value: {}, + }); + expect(() => generateOAuthState()).toThrow( + /crypto\.getRandomValues is not available/, + ); + }); }); describe("parseOAuthState", () => { diff --git a/core/auth/utils.ts b/core/auth/utils.ts index 638a4080..123274f8 100644 --- a/core/auth/utils.ts +++ b/core/auth/utils.ts @@ -50,20 +50,17 @@ export const parseOAuthCallbackParams = (location: string): CallbackParams => { * @returns A random state for the OAuth 2.0 flow. */ export const generateOAuthState = (): string => { - // Generate a random state - const array = new Uint8Array(32); - - // Use crypto.getRandomValues (available in both browser and Node.js) - if (typeof crypto !== "undefined" && crypto.getRandomValues) { - crypto.getRandomValues(array); - } else { - // Fallback for environments without crypto.getRandomValues - // This should not happen in modern environments - for (let i = 0; i < array.length; i++) { - array[i] = Math.floor(Math.random() * 256); - } + // OAuth state is a CSRF token — it MUST be unpredictable. crypto.getRandomValues + // is available in every supported runtime (browsers, Node ≥15); if it's somehow + // missing, fail loudly rather than silently degrading to Math.random (whose + // output is predictable from a small amount of observed state). + 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, (byte) => byte.toString(16).padStart(2, "0")).join( "", ); From d21da16983d8e59a65c264af5ad3f3eff8b5315f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 1 Jul 2026 19:13:17 -0400 Subject: [PATCH 2/2] =?UTF-8?q?refactor(auth):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20accurate=20toast=20copy,=20document=20state=20asymm?= =?UTF-8?q?etry,=20tighten=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Soften the OAuth-callback-rejected toast: the shape guard is defense-in-depth, not full CSRF prevention (PKCE remains primary), so the message no longer overstates ("rejecting to prevent a cross-site request" -> "did not originate from this session"). - Document the intentional asymmetry between a present-but-malformed `state` (rejected) and a wholly absent `state` (accepted, matched via OAUTH_PENDING_SERVER_KEY) — rejecting the null case would mask real provider error redirects that omit `state`. - Make the "valid state is not rejected" test assert the specific downstream "could not be matched" toast (seeding OAUTH_PENDING_SERVER_KEY) instead of an indirect "some toast fired" check. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX --- clients/web/src/App.test.tsx | 22 ++++++++++++++++++---- clients/web/src/App.tsx | 20 +++++++++++++++----- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index dfa0f352..1d1c42f9 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -404,6 +404,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ })); import App from "./App"; +import { OAUTH_PENDING_SERVER_KEY } from "./utils/oauthFlow"; import * as McpIndex from "@inspector/core/mcp/index.js"; import * as FetchLogModule from "@inspector/core/mcp/state/fetchRequestLogState.js"; import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js"; @@ -1625,9 +1626,16 @@ describe("App OAuth callback state validation", () => { }); it("does not reject when the state param parses to a valid authId", async () => { - // A well-formed 64-char-hex state is accepted; the handler proceeds past - // the rejection guard (it fails later for other reasons in this harness, - // but never shows the 'OAuth callback rejected' toast). + // A well-formed 64-char-hex state passes the shape guard, so the handler + // proceeds to the server-matching step. With no server registered under the + // seeded pending id, that step surfaces the "could not be matched" toast — + // asserting on that specific downstream toast proves the state was accepted + // (never the "OAuth callback rejected" toast) rather than relying on an + // indirect "some toast fired" check. + window.sessionStorage.setItem( + OAUTH_PENDING_SERVER_KEY, + "server-that-does-not-exist", + ); window.history.replaceState( {}, "", @@ -1636,7 +1644,13 @@ describe("App OAuth callback state validation", () => { renderWithMantine(); - await waitFor(() => expect(notificationsMock.show).toHaveBeenCalled()); + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "OAuth callback could not be matched", + }), + ), + ); expect(notificationsMock.show).not.toHaveBeenCalledWith( expect.objectContaining({ title: "OAuth callback rejected" }), ); diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 176f6fcf..01f977df 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1473,10 +1473,20 @@ function App() { // rebuilt client can restore those `auth` entries. Read it before the // URL is cleared below. // - // CSRF hardening: a `state` that is present but does not parse to our - // expected 64-char-hex authId shape is a forgery indicator — the value - // did not originate from `generateOAuthState`. Reject the callback with a - // clear error instead of silently proceeding with an undefined sessionId. + // Defense-in-depth: a `state` that is present but does not parse to our + // expected 64-char-hex authId shape did not originate from + // `generateOAuthState`, so reject the callback instead of silently + // proceeding with an undefined sessionId. This is a shape check, not full + // state-matching — the primary CSRF protection remains PKCE + // (`code_verifier`); this layer catches malformed/forged `state` early. + // + // Intentional asymmetry: a present-but-malformed `state` (including the + // empty string `?state=`) is rejected, but a wholly absent `state` + // (`stateParam === null`) is *not* — it falls through with + // `sessionId = undefined` and is matched via `OAUTH_PENDING_SERVER_KEY` + // instead. Rejecting the null case would turn any provider error redirect + // that omits `state` into a misleading "rejected" toast, hiding the real + // OAuth error surfaced by the `!params.successful` branch below. const stateParam = new URLSearchParams(window.location.search).get("state"); const parsedState = stateParam ? parseOAuthState(stateParam) : null; const stateRejected = stateParam !== null && parsedState === null; @@ -1493,7 +1503,7 @@ function App() { notifications.show({ title: "OAuth callback rejected", message: - "OAuth callback carried an unrecognized state parameter; rejecting to prevent a cross-site request. Please try connecting again.", + "OAuth callback carried an unrecognized state parameter that did not originate from this session. Please try connecting again.", color: "red", }); return;