Security: OAuth callback does not validate state parameter

client/src/components/OAuthCallback.tsx

@@ -1,5 +1,9 @@
 import { useEffect, useRef } from "react";
-import { InspectorOAuthClientProvider } from "../lib/auth";
+import {
+  clearOAuthStateFromSessionStorage,
+  getOAuthStateFromSessionStorage,
+  InspectorOAuthClientProvider,
+} from "../lib/auth";
 import { SESSION_KEYS } from "../lib/constants";
 import { auth } from "@modelcontextprotocol/sdk/client/auth.js";
 import { useToast } from "@/lib/hooks/useToast";
@@ -31,16 +35,37 @@ const OAuthCallback = ({ onConnect }: OAuthCallbackProps) => {
           variant: "destructive",
         });
 
-      const params = parseOAuthCallbackParams(window.location.search);
-      if (!params.successful) {
-        return notifyError(generateOAuthErrorDescription(params));
-      }
-
       const serverUrl = sessionStorage.getItem(SESSION_KEYS.SERVER_URL);
       if (!serverUrl) {
         return notifyError("Missing Server URL");
       }
 
+      // Verify the CSRF `state` the authorization server echoed back matches the
+      // one this browser session sent on the /authorize request. A mismatch (or a
+      // missing value on either side) means the callback did not originate from a
+      // flow we started. This runs before the success/error branch: an error
+      // response MUST carry the `state` too (RFC 6749 §4.1.2.1), and validating it
+      // first keeps an unsolicited callback from putting attacker-chosen
+      // `error_description` text in front of the user.
+      const callbackState = new URLSearchParams(window.location.search).get(
+        "state",
+      );
+      const expectedState = getOAuthStateFromSessionStorage(serverUrl);
+      // Single-use: drop the stored value whether or not it matched, and whether the
+      // response was a code or an error, so a replayed callback cannot be validated
+      // against it a second time.
+      clearOAuthStateFromSessionStorage(serverUrl);
+      if (!callbackState || !expectedState || callbackState !== expectedState) {
+        return notifyError(
+          "Invalid OAuth state parameter. The authorization response did not match the request this session started.",
+        );
+      }
+
+      const params = parseOAuthCallbackParams(window.location.search);
+      if (!params.successful) {
+        return notifyError(generateOAuthErrorDescription(params));
+      }
+
       let result;
       try {
         // Create an auth provider with the current server URL

client/src/components/__tests__/OAuthCallback.test.tsx

@@ -0,0 +1,167 @@
+import { render, waitFor } from "@testing-library/react";
+import OAuthCallback from "../OAuthCallback";
+import { SESSION_KEYS, getServerSpecificKey } from "../../lib/constants";
+import { auth } from "@modelcontextprotocol/sdk/client/auth.js";
+
+jest.mock("@modelcontextprotocol/sdk/client/auth.js", () => ({
+  auth: jest.fn(),
+  discoverAuthorizationServerMetadata: jest.fn(),
+}));
+
+const mockToast = jest.fn();
+jest.mock("@/lib/hooks/useToast", () => ({
+  useToast: () => ({ toast: mockToast }),
+}));
+
+const mockAuth = auth as jest.MockedFunction<typeof auth>;
+
+const SERVER_URL = "https://example.com/mcp";
+const STORED_STATE = "a".repeat(64);
+
+const setSearch = (search: string) => {
+  window.history.replaceState({}, "", `/oauth/callback${search}`);
+};
+
+const errorDescriptions = () =>
+  mockToast.mock.calls
+    .filter(([arg]) => arg?.variant === "destructive")
+    .map(([arg]) => String(arg.description));
+
+describe("OAuthCallback state validation", () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+    sessionStorage.clear();
+    mockAuth.mockResolvedValue("AUTHORIZED");
+    sessionStorage.setItem(SESSION_KEYS.SERVER_URL, SERVER_URL);
+  });
+
+  const storeExpectedState = (state: string) =>
+    sessionStorage.setItem(
+      getServerSpecificKey(SESSION_KEYS.OAUTH_STATE, SERVER_URL),
+      state,
+    );
+
+  it("exchanges the code when the returned state matches the stored one", async () => {
+    storeExpectedState(STORED_STATE);
+    setSearch(`?code=abc123&state=${STORED_STATE}`);
+    const onConnect = jest.fn();
+
+    render(<OAuthCallback onConnect={onConnect} />);
+
+    await waitFor(() => expect(onConnect).toHaveBeenCalledWith(SERVER_URL));
+    expect(mockAuth).toHaveBeenCalledTimes(1);
+    expect(errorDescriptions()).toHaveLength(0);
+  });
+
+  it("consumes the stored state so a replayed callback cannot reuse it", async () => {
+    storeExpectedState(STORED_STATE);
+    setSearch(`?code=abc123&state=${STORED_STATE}`);
+
+    render(<OAuthCallback onConnect={jest.fn()} />);
+
+    await waitFor(() =>
+      expect(
+        sessionStorage.getItem(
+          getServerSpecificKey(SESSION_KEYS.OAUTH_STATE, SERVER_URL),
+        ),
+      ).toBeNull(),
+    );
+  });
+
+  it.each([
+    {
+      name: "the returned state does not match",
+      stored: STORED_STATE,
+      search: `?code=abc123&state=${"b".repeat(64)}`,
+    },
+    {
+      name: "the callback carries no state",
+      stored: STORED_STATE,
+      search: "?code=abc123",
+    },
+    {
+      name: "this session never started an authorization request",
+      stored: undefined,
+      search: `?code=abc123&state=${STORED_STATE}`,
+    },
+  ])("rejects the callback when $name", async ({ stored, search }) => {
+    if (stored) storeExpectedState(stored);
+    setSearch(search);
+    const onConnect = jest.fn();
+
+    render(<OAuthCallback onConnect={onConnect} />);
+
+    await waitFor(() =>
+      expect(errorDescriptions().join("\n")).toContain(
+        "Invalid OAuth state parameter",
+      ),
+    );
+    expect(mockAuth).not.toHaveBeenCalled();
+    expect(onConnect).not.toHaveBeenCalled();
+  });
+
+  describe("error responses", () => {
+    it("surfaces the server's error once the state checks out", async () => {
+      storeExpectedState(STORED_STATE);
+      setSearch(
+        `?error=access_denied&error_description=User+said+no&state=${STORED_STATE}`,
+      );
+
+      render(<OAuthCallback onConnect={jest.fn()} />);
+
+      await waitFor(() =>
+        expect(errorDescriptions().join("\n")).toContain("access_denied"),
+      );
+      expect(errorDescriptions().join("\n")).toContain("User said no");
+      expect(mockAuth).not.toHaveBeenCalled();
+    });
+
+    it.each([
+      {
+        name: "mismatched",
+        stored: STORED_STATE,
+        search: `?error=access_denied&error_description=Attacker+text&state=${"b".repeat(64)}`,
+      },
+      {
+        name: "missing",
+        stored: STORED_STATE,
+        search: "?error=access_denied&error_description=Attacker+text",
+      },
+      {
+        name: "unsolicited (nothing stored)",
+        stored: undefined,
+        search: `?error=access_denied&error_description=Attacker+text&state=${STORED_STATE}`,
+      },
+    ])(
+      "rejects a $name state on an error response without showing its description",
+      async ({ stored, search }) => {
+        if (stored) storeExpectedState(stored);
+        setSearch(search);
+
+        render(<OAuthCallback onConnect={jest.fn()} />);
+
+        await waitFor(() =>
+          expect(errorDescriptions().join("\n")).toContain(
+            "Invalid OAuth state parameter",
+          ),
+        );
+        expect(errorDescriptions().join("\n")).not.toContain("Attacker text");
+      },
+    );
+
+    it("consumes the stored state on an error response too", async () => {
+      storeExpectedState(STORED_STATE);
+      setSearch(`?error=access_denied&state=${STORED_STATE}`);
+
+      render(<OAuthCallback onConnect={jest.fn()} />);
+
+      await waitFor(() =>
+        expect(
+          sessionStorage.getItem(
+            getServerSpecificKey(SESSION_KEYS.OAUTH_STATE, SERVER_URL),
+          ),
+        ).toBeNull(),
+      );
+    });
+  });
+});

client/src/lib/__tests__/auth.test.ts

@@ -1,4 +1,9 @@
-import { discoverScopes } from "../auth";
+import {
+  clearOAuthStateFromSessionStorage,
+  discoverScopes,
+  getOAuthStateFromSessionStorage,
+  InspectorOAuthClientProvider,
+} from "../auth";
 import { discoverAuthorizationServerMetadata } from "@modelcontextprotocol/sdk/client/auth.js";
 
 jest.mock("@modelcontextprotocol/sdk/client/auth.js", () => ({
@@ -156,3 +161,56 @@ describe("discoverScopes", () => {
     },
   );
 });
+
+describe("InspectorOAuthClientProvider state", () => {
+  const serverUrl = "https://example.com/mcp";
+
+  beforeEach(() => {
+    sessionStorage.clear();
+  });
+
+  it("persists the generated state under a server-specific key", () => {
+    const provider = new InspectorOAuthClientProvider(serverUrl);
+    const state = provider.state();
+
+    expect(state).toMatch(/^[0-9a-f]{64}$/);
+    expect(getOAuthStateFromSessionStorage(serverUrl)).toBe(state);
+  });
+
+  it("scopes the stored state to the server URL", () => {
+    const provider = new InspectorOAuthClientProvider(serverUrl);
+    const state = provider.state();
+
+    expect(getOAuthStateFromSessionStorage("https://other.com/mcp")).toBe(
+      undefined,
+    );
+    expect(getOAuthStateFromSessionStorage(serverUrl)).toBe(state);
+  });
+
+  it("overwrites the stored state on each new authorization request", () => {
+    const provider = new InspectorOAuthClientProvider(serverUrl);
+    const first = provider.state();
+    const second = provider.state();
+
+    expect(second).not.toBe(first);
+    expect(getOAuthStateFromSessionStorage(serverUrl)).toBe(second);
+  });
+
+  it("clearOAuthStateFromSessionStorage removes the stored state", () => {
+    const provider = new InspectorOAuthClientProvider(serverUrl);
+    provider.state();
+
+    clearOAuthStateFromSessionStorage(serverUrl);
+
+    expect(getOAuthStateFromSessionStorage(serverUrl)).toBe(undefined);
+  });
+
+  it("clear() drops the stored state along with the other session data", () => {
+    const provider = new InspectorOAuthClientProvider(serverUrl);
+    provider.state();
+
+    provider.clear();
+
+    expect(getOAuthStateFromSessionStorage(serverUrl)).toBe(undefined);
+  });
+});

client/src/lib/auth.ts

@@ -129,6 +129,23 @@ export const clearScopeFromSessionStorage = (serverUrl: string) => {
   sessionStorage.removeItem(key);
 };
 
+/**
+ * Reads the CSRF `state` that was sent on the most recent /authorize request
+ * for `serverUrl`, so the OAuth callback can verify the value the
+ * authorization server echoed back.
+ */
+export const getOAuthStateFromSessionStorage = (
+  serverUrl: string,
+): string | undefined => {
+  const key = getServerSpecificKey(SESSION_KEYS.OAUTH_STATE, serverUrl);
+  return sessionStorage.getItem(key) || undefined;
+};
+
+export const clearOAuthStateFromSessionStorage = (serverUrl: string) => {
+  const key = getServerSpecificKey(SESSION_KEYS.OAUTH_STATE, serverUrl);
+  sessionStorage.removeItem(key);
+};
+
 export class InspectorOAuthClientProvider implements OAuthClientProvider {
   constructor(protected serverUrl: string) {
     // Save the server URL to session storage
@@ -178,8 +195,18 @@ export class InspectorOAuthClientProvider implements OAuthClientProvider {
     return metadata;
   }
 
-  state(): string | Promise<string> {
-    return generateOAuthState();
+  /**
+   * Called by the SDK immediately before it builds the /authorize URL. The
+   * generated value is persisted so `OAuthCallback` can verify the `state` the
+   * authorization server echoes back (CSRF protection, OAuth 2.1 §7.6).
+   */
+  state(): string {
+    const state = generateOAuthState();
+    sessionStorage.setItem(
+      getServerSpecificKey(SESSION_KEYS.OAUTH_STATE, this.serverUrl),
+      state,
+    );
+    return state;
   }
 
   async clientInformation() {
@@ -262,6 +289,7 @@ export class InspectorOAuthClientProvider implements OAuthClientProvider {
     sessionStorage.removeItem(
       getServerSpecificKey(SESSION_KEYS.CODE_VERIFIER, this.serverUrl),
     );
+    clearOAuthStateFromSessionStorage(this.serverUrl);
   }
 }
 

client/src/lib/constants.ts

@@ -18,6 +18,8 @@ export const SESSION_KEYS = {
   SERVER_METADATA: "mcp_server_metadata",
   AUTH_DEBUGGER_STATE: "mcp_auth_debugger_state",
   SCOPE: "mcp_scope",
+  // CSRF `state` sent on the /authorize request, verified on the OAuth callback.
+  OAUTH_STATE: "mcp_oauth_state",
 } as const;
 
 // Generate server-specific session storage keys