diff --git a/client/src/components/OAuthCallback.tsx b/client/src/components/OAuthCallback.tsx index ccfd6d928..0c27efc7e 100644 --- a/client/src/components/OAuthCallback.tsx +++ b/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 diff --git a/client/src/components/__tests__/OAuthCallback.test.tsx b/client/src/components/__tests__/OAuthCallback.test.tsx new file mode 100644 index 000000000..a0373dd2b --- /dev/null +++ b/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; + +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(); + + 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(); + + 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(); + + 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(); + + 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(); + + 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(); + + await waitFor(() => + expect( + sessionStorage.getItem( + getServerSpecificKey(SESSION_KEYS.OAUTH_STATE, SERVER_URL), + ), + ).toBeNull(), + ); + }); + }); +}); diff --git a/client/src/lib/__tests__/auth.test.ts b/client/src/lib/__tests__/auth.test.ts index 03c503d81..86d28dae4 100644 --- a/client/src/lib/__tests__/auth.test.ts +++ b/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); + }); +}); diff --git a/client/src/lib/auth.ts b/client/src/lib/auth.ts index f0fc2fc4b..54adc1fce 100644 --- a/client/src/lib/auth.ts +++ b/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 { - 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); } } diff --git a/client/src/lib/constants.ts b/client/src/lib/constants.ts index d986d3802..c3590faed 100644 --- a/client/src/lib/constants.ts +++ b/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