merge: v2/1562-oauth-state-hardening (#1589)

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";
@@ -1589,3 +1590,69 @@ 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(<App />);
+
+    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 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(
+      {},
+      "",
+      `/oauth/callback?code=abc123&state=${"a".repeat(64)}`,
+    );
+
+    renderWithMantine(<App />);
+
+    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" }),
+    );
+  });
+});

clients/web/src/App.tsx

@@ -1472,10 +1472,25 @@ 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.
+    //
+    // 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 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 +1499,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 that did not originate from this session. Please try connecting again.",
+        color: "red",
+      });
+      return;
+    }
+
     if (!params.successful) {
       notifications.show({
         title: "OAuth authorization failed",

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", () => {