From 6a5018d7feb7775bf336384ac38be99d7e3f825e Mon Sep 17 00:00:00 2001 From: tuanaiseo Date: Sun, 12 Apr 2026 06:44:51 +0700 Subject: [PATCH 1/4] fix(security): oauth callback does not validate `state` parameter The OAuth callback flow accepts an authorization `code` from the URL and proceeds with token exchange, but there is no validation of the OAuth `state` parameter to bind the response to the original auth request. This can enable login CSRF/session mix-up attacks where an attacker injects a valid code for a different authorization context. Affected files: OAuthCallback.tsx, AppRenderer.tsx Signed-off-by: tuanaiseo <221258316+tuanaiseo@users.noreply.github.com> --- client/src/components/OAuthCallback.tsx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/client/src/components/OAuthCallback.tsx b/client/src/components/OAuthCallback.tsx index ccfd6d928..282e31e0c 100644 --- a/client/src/components/OAuthCallback.tsx +++ b/client/src/components/OAuthCallback.tsx @@ -12,6 +12,8 @@ interface OAuthCallbackProps { onConnect: (serverUrl: string) => void; } +const OAUTH_STATE_SESSION_KEY = "oauth_state"; + const OAuthCallback = ({ onConnect }: OAuthCallbackProps) => { const { toast } = useToast(); const hasProcessedRef = useRef(false); @@ -36,6 +38,13 @@ const OAuthCallback = ({ onConnect }: OAuthCallbackProps) => { return notifyError(generateOAuthErrorDescription(params)); } + const callbackState = new URLSearchParams(window.location.search).get("state"); + const storedState = sessionStorage.getItem(OAUTH_STATE_SESSION_KEY); + if (!callbackState || !storedState || callbackState !== storedState) { + return notifyError("Invalid OAuth state"); + } + sessionStorage.removeItem(OAUTH_STATE_SESSION_KEY); + const serverUrl = sessionStorage.getItem(SESSION_KEYS.SERVER_URL); if (!serverUrl) { return notifyError("Missing Server URL"); From 4a10a2e26efa83dd5332ece77db5852f7b469eae Mon Sep 17 00:00:00 2001 From: tuanaiseo Date: Sun, 12 Apr 2026 06:44:52 +0700 Subject: [PATCH 2/4] fix(security): oauth callback does not validate `state` parameter The OAuth callback flow accepts an authorization `code` from the URL and proceeds with token exchange, but there is no validation of the OAuth `state` parameter to bind the response to the original auth request. This can enable login CSRF/session mix-up attacks where an attacker injects a valid code for a different authorization context. Affected files: OAuthCallback.tsx, AppRenderer.tsx Signed-off-by: tuanaiseo <221258316+tuanaiseo@users.noreply.github.com> --- client/src/components/AppRenderer.tsx | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/client/src/components/AppRenderer.tsx b/client/src/components/AppRenderer.tsx index e25f35c91..d8c44416b 100644 --- a/client/src/components/AppRenderer.tsx +++ b/client/src/components/AppRenderer.tsx @@ -31,6 +31,8 @@ interface AppRendererProps { onNotification?: (notification: ServerNotification) => void; } +const OAUTH_STATE_SESSION_KEY = "oauth_state"; + const AppRenderer = ({ sandboxPath, tool, @@ -74,8 +76,22 @@ const AppRenderer = ({ const handleOpenLink = async ({ url }: { url: string }) => { let isError = true; if (url.startsWith("https://") || url.startsWith("http://")) { - window.open(url, "_blank"); - isError = false; + try { + const nextUrl = new URL(url); + if (nextUrl.searchParams.get("response_type") === "code") { + const stateBytes = new Uint8Array(16); + window.crypto.getRandomValues(stateBytes); + const state = Array.from(stateBytes, (byte) => + byte.toString(16).padStart(2, "0"), + ).join(""); + sessionStorage.setItem(OAUTH_STATE_SESSION_KEY, state); + nextUrl.searchParams.set("state", state); + } + window.open(nextUrl.toString(), "_blank"); + isError = false; + } catch { + isError = true; + } } return { isError }; }; From 6cd5a8e01d5e2bff05b74c71df7adb942e7d0209 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 19 Aug 2026 16:58:35 -0400 Subject: [PATCH 3/4] fix(security): persist the OAuth `state` so the callback can verify it Addresses the Copilot review: as submitted, `OAuthCallback` required a `sessionStorage` key (`oauth_state`) that the normal OAuth path never wrote. `useConnection` calls the SDK's `auth()`, which redirects through `InspectorOAuthClientProvider.redirectToAuthorization()`; the only setter was the unrelated MCP Apps open-link handler. Every real callback would therefore have failed with "Invalid OAuth state". The provider already implements `state()` (the SDK calls it just before building the /authorize URL) but discarded the value, so the `state` was sent and never verified. Persist it under a server-specific session key and compare it in the callback: - `SESSION_KEYS.OAUTH_STATE` + `get/clearOAuthStateFromSessionStorage()`. - `InspectorOAuthClientProvider.state()` stores the generated value; `clear()` drops it with the rest of the session data. - `OAuthCallback` resolves the server URL first, then compares the echoed `state` against the stored one, clearing it either way so a replayed callback cannot re-validate against it. - Revert the `AppRenderer` open-link change: it rewrote arbitrary app-supplied URLs, clobbered any `state` they already carried, and wrote a global key that collided with this flow. Tests: state persistence/scoping/overwrite/clear in `lib/__tests__/auth.test.ts`, and match / mismatch / missing-state / no-stored-state callback behavior in the new `components/__tests__/OAuthCallback.test.tsx`. Signed-off-by: cliffhall --- client/src/components/AppRenderer.tsx | 20 +--- client/src/components/OAuthCallback.tsx | 32 ++++-- .../__tests__/OAuthCallback.test.tsx | 102 ++++++++++++++++++ client/src/lib/__tests__/auth.test.ts | 60 ++++++++++- client/src/lib/auth.ts | 32 +++++- client/src/lib/constants.ts | 2 + 6 files changed, 217 insertions(+), 31 deletions(-) create mode 100644 client/src/components/__tests__/OAuthCallback.test.tsx diff --git a/client/src/components/AppRenderer.tsx b/client/src/components/AppRenderer.tsx index d8c44416b..e25f35c91 100644 --- a/client/src/components/AppRenderer.tsx +++ b/client/src/components/AppRenderer.tsx @@ -31,8 +31,6 @@ interface AppRendererProps { onNotification?: (notification: ServerNotification) => void; } -const OAUTH_STATE_SESSION_KEY = "oauth_state"; - const AppRenderer = ({ sandboxPath, tool, @@ -76,22 +74,8 @@ const AppRenderer = ({ const handleOpenLink = async ({ url }: { url: string }) => { let isError = true; if (url.startsWith("https://") || url.startsWith("http://")) { - try { - const nextUrl = new URL(url); - if (nextUrl.searchParams.get("response_type") === "code") { - const stateBytes = new Uint8Array(16); - window.crypto.getRandomValues(stateBytes); - const state = Array.from(stateBytes, (byte) => - byte.toString(16).padStart(2, "0"), - ).join(""); - sessionStorage.setItem(OAUTH_STATE_SESSION_KEY, state); - nextUrl.searchParams.set("state", state); - } - window.open(nextUrl.toString(), "_blank"); - isError = false; - } catch { - isError = true; - } + window.open(url, "_blank"); + isError = false; } return { isError }; }; diff --git a/client/src/components/OAuthCallback.tsx b/client/src/components/OAuthCallback.tsx index 282e31e0c..e7cf7d393 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"; @@ -12,8 +16,6 @@ interface OAuthCallbackProps { onConnect: (serverUrl: string) => void; } -const OAUTH_STATE_SESSION_KEY = "oauth_state"; - const OAuthCallback = ({ onConnect }: OAuthCallbackProps) => { const { toast } = useToast(); const hasProcessedRef = useRef(false); @@ -38,18 +40,28 @@ const OAuthCallback = ({ onConnect }: OAuthCallbackProps) => { return notifyError(generateOAuthErrorDescription(params)); } - const callbackState = new URLSearchParams(window.location.search).get("state"); - const storedState = sessionStorage.getItem(OAUTH_STATE_SESSION_KEY); - if (!callbackState || !storedState || callbackState !== storedState) { - return notifyError("Invalid OAuth state"); - } - sessionStorage.removeItem(OAUTH_STATE_SESSION_KEY); - 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, so the code must not be exchanged. + const callbackState = new URLSearchParams(window.location.search).get( + "state", + ); + const expectedState = getOAuthStateFromSessionStorage(serverUrl); + // Single-use: drop the stored value whether or not it matched, 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.", + ); + } + 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..747be4625 --- /dev/null +++ b/client/src/components/__tests__/OAuthCallback.test.tsx @@ -0,0 +1,102 @@ +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(); + }); +}); 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 From 5bc171335ec643658a4c6cc31c76ac02f280d0b1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 19 Aug 2026 17:03:11 -0400 Subject: [PATCH 4/4] fix(security): validate the OAuth `state` on error callbacks too Addresses the round-2 Copilot review (suppressed comment). `parseOAuthCallbackParams` returned before the state check on any error response, so `?error=access_denied&state=wrong` skipped validation entirely and left the expected state stored rather than consumed. Move the server-URL lookup and the state check ahead of the success/error branch: an error response MUST echo `state` too (RFC 6749 4.1.2.1), and validating first also keeps an unsolicited callback from putting attacker-chosen `error_description` text in front of the user. Tests: error response with a matching state still surfaces the server's error; mismatched, missing, and unsolicited states on an error response are rejected without rendering the supplied description; and the stored state is consumed on the error path as well. Signed-off-by: cliffhall --- client/src/components/OAuthCallback.tsx | 20 +++--- .../__tests__/OAuthCallback.test.tsx | 65 +++++++++++++++++++ 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/client/src/components/OAuthCallback.tsx b/client/src/components/OAuthCallback.tsx index e7cf7d393..0c27efc7e 100644 --- a/client/src/components/OAuthCallback.tsx +++ b/client/src/components/OAuthCallback.tsx @@ -35,11 +35,6 @@ 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"); @@ -48,13 +43,17 @@ const OAuthCallback = ({ onConnect }: OAuthCallbackProps) => { // 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, so the code must not be exchanged. + // 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, so a replayed - // callback cannot be validated against it a second time. + // 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( @@ -62,6 +61,11 @@ const OAuthCallback = ({ onConnect }: OAuthCallbackProps) => { ); } + 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 index 747be4625..a0373dd2b 100644 --- a/client/src/components/__tests__/OAuthCallback.test.tsx +++ b/client/src/components/__tests__/OAuthCallback.test.tsx @@ -99,4 +99,69 @@ describe("OAuthCallback state validation", () => { 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(), + ); + }); + }); });