From 3fbf2a9f20d0c5ae5b3a37b318f098e67ffa4a71 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 20 Oct 2026 00:39:16 -0400 Subject: [PATCH 1/2] fix(auth): key in-flight PKCE verifiers by flow, not server URL Overlapping OAuth flows to the same server (two tabs, or several users of one deployed Inspector) shared one codeVerifier slot in the backend's oauth.json, so the later flow overwrote the earlier one's verifier and the earlier exchange failed PKCE with invalid_grant. The web client now keeps each flow's verifier in the initiating tab's sessionStorage, keyed by the OAuth `state` authId, and the callback exchange reads the verifier for the `state` it returned with. Keying inside servers[url] would not help, since the remote backend merges whole server entries. A callback whose `state` has no stored verifier fails with a clear error rather than borrowing another flow's. CLI/TUI set no flow store and keep the existing storage path; the runner now forwards the callback `state` too. Closes #2726 Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: cliffhall --- clients/tui/__tests__/App.test.tsx | 15 +- clients/web/src/hooks/useOAuthRecovery.ts | 2 + clients/web/src/lib/environmentFactory.ts | 6 + .../test/core/auth/pkce-flow-store.test.ts | 99 +++++++ .../web/src/test/core/auth/providers.test.ts | 117 +++++++++ .../auth/runner-interactive-oauth.test.ts | 6 +- ...rClient-oauth-concurrent-flows-e2e.test.ts | 241 ++++++++++++++++++ core/auth/index.ts | 7 + core/auth/node/runner-interactive-oauth.ts | 12 +- core/auth/pkce-flow-store.ts | 98 +++++++ core/auth/providers.ts | 67 ++++- core/mcp/inspectorClient.ts | 15 +- core/mcp/oauthManager.ts | 13 + core/mcp/types.ts | 8 + specification/v2_storage.md | 2 + 15 files changed, 698 insertions(+), 10 deletions(-) create mode 100644 clients/web/src/test/core/auth/pkce-flow-store.test.ts create mode 100644 clients/web/src/test/integration/mcp/inspectorClient-oauth-concurrent-flows-e2e.test.ts create mode 100644 core/auth/pkce-flow-store.ts diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index 07f2ad9dbf..a7271ab249 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -108,7 +108,11 @@ const h = vi.hoisted(() => { // Captured options from the most recent callbackServer.start(), so a test can // drive the onCallback / onError handlers the OAuth flows register. interface CallbackOpts { - onCallback: (p: { code: string; iss?: string }) => Promise | void; + onCallback: (p: { + code: string; + iss?: string; + state?: string; + }) => Promise | void; onError: (p: { error?: string; error_description?: string }) => void; } const cb: { opts: CallbackOpts | null } = { opts: null }; @@ -1551,11 +1555,18 @@ describe("App (OAuth flows)", () => { const r = await mount(oneHttp()); await press(r, ["c"]); await waitUntil(() => h.cb.opts !== null); - await h.cb.opts!.onCallback({ code: "abc", iss: "https://as.example" }); + // #2726: the redirect's `state` selects the flow's PKCE verifier. + const state = "d".repeat(64); + await h.cb.opts!.onCallback({ + code: "abc", + iss: "https://as.example", + state, + }); await tick(); expect(h.clientSpies.completeOAuthFlow).toHaveBeenCalledWith( "abc", "https://as.example", + state, ); }); diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 313143a6a2..8a0bfd8b64 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -1381,6 +1381,8 @@ export function useOAuthRecovery({ await client.resumeAfterOAuth(params.code, { remoteSessionId: resumeSnapshot?.remoteSessionId, iss: params.iss, + // #2726: selects this flow's PKCE verifier from the tab's store. + state: stateParam ?? undefined, }); } catch (err) { connectStartRef.current = undefined; diff --git a/clients/web/src/lib/environmentFactory.ts b/clients/web/src/lib/environmentFactory.ts index 3b08460bb8..c82959d676 100644 --- a/clients/web/src/lib/environmentFactory.ts +++ b/clients/web/src/lib/environmentFactory.ts @@ -6,6 +6,7 @@ import { } from "@inspector/core/mcp/remote/index.js"; import { BrowserNavigation } from "@inspector/core/auth/browser/index.js"; import type { RedirectUrlProvider } from "@inspector/core/auth/index.js"; +import { createWebStoragePkceFlowStore } from "@inspector/core/auth/pkce-flow-store.js"; import { getWebRemoteOAuthStorage } from "./remoteOAuthStorage.js"; export interface WebEnvironmentResult { @@ -20,6 +21,10 @@ export interface WebEnvironmentResult { * dev-backend wires this in `/api/*`). * - OAuth storage uses `RemoteOAuthStorage` (shared `oauth.json` via * `/api/storage/oauth`); navigation uses `BrowserNavigation`. + * - In-flight PKCE verifiers stay in this tab's `sessionStorage`, keyed by + * the flow's OAuth `state`, so overlapping flows for one server — two tabs, + * or two people on one deployed Inspector — cannot overwrite each other's + * in the shared store (#2726). * * Returns both the assembled environment and the logger so callers can share * the same pino instance for any direct logging they need to do, instead of @@ -69,6 +74,7 @@ export function createWebEnvironment( storage: getWebRemoteOAuthStorage(authToken), navigation: new BrowserNavigation(undefined, onBeforeOAuthRedirect), redirectUrlProvider, + flowStore: createWebStoragePkceFlowStore(), }, }; diff --git a/clients/web/src/test/core/auth/pkce-flow-store.test.ts b/clients/web/src/test/core/auth/pkce-flow-store.test.ts new file mode 100644 index 0000000000..708a633dd2 --- /dev/null +++ b/clients/web/src/test/core/auth/pkce-flow-store.test.ts @@ -0,0 +1,99 @@ +import { describe, it, expect, afterEach, vi } from "vitest"; +import { + createWebStoragePkceFlowStore, + PKCE_FLOW_STORAGE_KEY_PREFIX, + type PkceFlowStorageArea, +} from "@inspector/core/auth/pkce-flow-store.js"; + +const SERVER = "https://mcp.example.com"; +const FLOW = "f".repeat(64); + +function makeArea(): PkceFlowStorageArea & { items: Map } { + const items = new Map(); + return { + items, + getItem: (key) => items.get(key) ?? null, + setItem: (key, value) => void items.set(key, value), + removeItem: (key) => void items.delete(key), + }; +} + +describe("createWebStoragePkceFlowStore (#2726)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("round-trips a verifier under a per-flow key", async () => { + const area = makeArea(); + const store = createWebStoragePkceFlowStore(() => area); + + await store.saveCodeVerifier(FLOW, SERVER, "cv"); + + expect([...area.items.keys()]).toEqual([ + `${PKCE_FLOW_STORAGE_KEY_PREFIX}${FLOW}`, + ]); + expect(await store.getCodeVerifier(FLOW, SERVER)).toBe("cv"); + }); + + it("keeps flows for the same server apart", async () => { + const otherTab = createWebStoragePkceFlowStore(() => makeArea()); + const area = makeArea(); + const shared = createWebStoragePkceFlowStore(() => area); + const other = "e".repeat(64); + + await shared.saveCodeVerifier(FLOW, SERVER, "cv-1"); + await shared.saveCodeVerifier(other, SERVER, "cv-2"); + + expect(await shared.getCodeVerifier(FLOW, SERVER)).toBe("cv-1"); + expect(await shared.getCodeVerifier(other, SERVER)).toBe("cv-2"); + // A separate storage area (another tab) sees neither. + expect(await otherTab.getCodeVerifier(FLOW, SERVER)).toBeUndefined(); + }); + + it("does not answer for a different server than the flow was started for", async () => { + const store = createWebStoragePkceFlowStore(() => makeArea()); + await store.saveCodeVerifier(FLOW, SERVER, "cv"); + + expect( + await store.getCodeVerifier(FLOW, "https://other.example.com"), + ).toBeUndefined(); + }); + + it("clears a flow's verifier", async () => { + const area = makeArea(); + const store = createWebStoragePkceFlowStore(() => area); + await store.saveCodeVerifier(FLOW, SERVER, "cv"); + + await store.clearCodeVerifier(FLOW); + + expect(area.items.size).toBe(0); + expect(await store.getCodeVerifier(FLOW, SERVER)).toBeUndefined(); + }); + + it.each([ + ["not JSON", "{"], + ["not an object", "42"], + ["null", "null"], + ["missing the verifier", JSON.stringify({ serverUrl: SERVER })], + [ + "a non-string verifier", + JSON.stringify({ serverUrl: SERVER, codeVerifier: 1 }), + ], + ])("treats a corrupt entry (%s) as absent", async (_label, raw) => { + const area = makeArea(); + area.items.set(`${PKCE_FLOW_STORAGE_KEY_PREFIX}${FLOW}`, raw); + const store = createWebStoragePkceFlowStore(() => area); + + expect(await store.getCodeVerifier(FLOW, SERVER)).toBeUndefined(); + }); + + it("defaults to sessionStorage", async () => { + const area = makeArea(); + vi.stubGlobal("sessionStorage", area); + const store = createWebStoragePkceFlowStore(); + + await store.saveCodeVerifier(FLOW, SERVER, "cv"); + + expect(area.items.has(`${PKCE_FLOW_STORAGE_KEY_PREFIX}${FLOW}`)).toBe(true); + }); +}); diff --git a/clients/web/src/test/core/auth/providers.test.ts b/clients/web/src/test/core/auth/providers.test.ts index 1992e7fcfa..359e5c7cc3 100644 --- a/clients/web/src/test/core/auth/providers.test.ts +++ b/clients/web/src/test/core/auth/providers.test.ts @@ -11,6 +11,10 @@ import { OAuthStorageBase } from "@inspector/core/auth/oauth-storage.js"; import { OAuthMemoryStore } from "@inspector/core/auth/store.js"; import type { OAuthPersistBackend } from "@inspector/core/auth/oauth-persist.js"; import { ensureCimdClientRegistration } from "@inspector/core/auth/cimd.js"; +import { + createWebStoragePkceFlowStore, + type PkceFlowStore, +} from "@inspector/core/auth/pkce-flow-store.js"; import { BrowserNavigation, BrowserOAuthClientProvider, @@ -1108,5 +1112,118 @@ describe("OAuthNavigation", () => { expect(storage.clearDiscoveryState).toHaveBeenCalledWith(SERVER); }); }); + + describe("per-flow PKCE verifier (#2726)", () => { + function makeFlowStore(): PkceFlowStore { + const store = new Map(); + return createWebStoragePkceFlowStore(() => ({ + getItem: (key) => store.get(key) ?? null, + setItem: (key, value) => void store.set(key, value), + removeItem: (key) => void store.delete(key), + })); + } + + it("keys the verifier by the state it generated, not the shared slot", async () => { + const storage = makeStorage(); + const flowStore = makeFlowStore(); + const provider = makeProvider(storage, vi.fn(), { flowStore }); + + const state = await provider.state(); + await provider.saveCodeVerifier("cv-a"); + + expect(storage.saveCodeVerifier).not.toHaveBeenCalled(); + expect(await flowStore.getCodeVerifier(state, SERVER)).toBe("cv-a"); + expect(await provider.codeVerifier()).toBe("cv-a"); + }); + + it("gives two overlapping flows for one server their own verifiers", async () => { + const storage = makeStorage(); + const flowStore = makeFlowStore(); + const providerA = makeProvider(storage, vi.fn(), { flowStore }); + const providerB = makeProvider(storage, vi.fn(), { flowStore }); + + const stateA = await providerA.state(); + await providerA.saveCodeVerifier("cv-a"); + const stateB = await providerB.state(); + await providerB.saveCodeVerifier("cv-b"); + + // The callback legs run on fresh providers, as after a page reload. + const callbackB = makeProvider(storage, vi.fn(), { flowStore }); + callbackB.resumeAuthorizationFlow(stateB); + expect(await callbackB.codeVerifier()).toBe("cv-b"); + await callbackB.clearAuthorizationFlow(); + + const callbackA = makeProvider(storage, vi.fn(), { flowStore }); + callbackA.resumeAuthorizationFlow(stateA); + expect(await callbackA.codeVerifier()).toBe("cv-a"); + }); + + it("fails clearly for a state with no stored verifier instead of using another flow's", async () => { + const storage = makeStorage(); + vi.mocked(storage.getCodeVerifier).mockResolvedValue("someone-elses"); + const flowStore = makeFlowStore(); + const other = makeProvider(storage, vi.fn(), { flowStore }); + await other.state(); + await other.saveCodeVerifier("cv-other"); + + const provider = makeProvider(storage, vi.fn(), { flowStore }); + provider.resumeAuthorizationFlow("a".repeat(64)); + + await expect(provider.codeVerifier()).rejects.toThrow( + /No code verifier saved for this authorization flow/, + ); + expect(storage.getCodeVerifier).not.toHaveBeenCalled(); + }); + + it("binds no flow for a state that is not one of ours", async () => { + const storage = makeStorage(); + const provider = makeProvider(storage, vi.fn(), { + flowStore: makeFlowStore(), + }); + + provider.resumeAuthorizationFlow("not-our-state"); + + await expect(provider.codeVerifier()).rejects.toThrow( + /No code verifier saved for this authorization flow/, + ); + }); + + it("invalidateCredentials('verifier') clears only the bound flow", async () => { + const storage = makeStorage(); + const flowStore = makeFlowStore(); + const provider = makeProvider(storage, vi.fn(), { flowStore }); + const state = await provider.state(); + await provider.saveCodeVerifier("cv"); + + await provider.invalidateCredentials("verifier"); + + expect(await flowStore.getCodeVerifier(state, SERVER)).toBeUndefined(); + expect(storage.clearCodeVerifier).not.toHaveBeenCalled(); + }); + + it("clearAuthorizationFlow() is a no-op without a flow store or a bound flow", async () => { + const storage = makeStorage(); + await makeProvider(storage).clearAuthorizationFlow(); + + const flowStore = makeFlowStore(); + const clear = vi.spyOn(flowStore, "clearCodeVerifier"); + await makeProvider(storage, vi.fn(), { + flowStore, + }).clearAuthorizationFlow(); + + expect(clear).not.toHaveBeenCalled(); + expect(storage.clearCodeVerifier).not.toHaveBeenCalled(); + }); + + it("keeps the shared slot when no flow store is configured", async () => { + const storage = makeStorage(); + const provider = makeProvider(storage); + + await provider.state(); + await provider.saveCodeVerifier("cv"); + + expect(storage.saveCodeVerifier).toHaveBeenCalledWith(SERVER, "cv"); + }); + }); }); }); diff --git a/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts b/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts index e885e06d26..9dc9320149 100644 --- a/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts +++ b/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts @@ -58,6 +58,8 @@ function createMockCallbackServer(handlers: { } const CALLBACK_ISS = "https://as.example"; +/** The OAuth `state` the redirect echoes; it selects the flow's verifier (#2726). */ +const CALLBACK_STATE = "c".repeat(64); /** * Drive the loopback callback the way a browser redirect would, including the @@ -71,7 +73,7 @@ async function simulateCallback( if (!handlers.onCallback) { throw new Error("onCallback not registered"); } - await handlers.onCallback({ code, iss }); + await handlers.onCallback({ code, iss, state: CALLBACK_STATE }); } describe("runRunnerInteractiveOAuth", () => { @@ -155,6 +157,7 @@ describe("runRunnerInteractiveOAuth", () => { expect(client.completeOAuthFlow).toHaveBeenCalledWith( "auth-code-123", CALLBACK_ISS, + CALLBACK_STATE, ); expect(redirectUrlProvider.redirectUrl).toBe( "http://127.0.0.1:6276/oauth/callback", @@ -192,6 +195,7 @@ describe("runRunnerInteractiveOAuth", () => { expect(client.completeOAuthFlow).toHaveBeenCalledWith( "step-up-code", CALLBACK_ISS, + CALLBACK_STATE, ); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-concurrent-flows-e2e.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-concurrent-flows-e2e.test.ts new file mode 100644 index 0000000000..9d32e702d9 --- /dev/null +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-concurrent-flows-e2e.test.ts @@ -0,0 +1,241 @@ +/** + * Overlapping OAuth flows for the same server through the shared remote store + * (#2726). + * + * Each "tab" is what the web client builds per page: its own + * `RemoteOAuthStorage` view over one backend `oauth.json`, and its own + * sessionStorage-like area for in-flight PKCE verifiers. Tab A starts a flow, + * tab B starts and finishes one for the same server, then tab A's callback page + * (a fresh page, so a fresh client and remote view, but the same tab storage) + * finishes. The test AS verifies PKCE S256, so A succeeds only if its exchange + * sends A's own verifier. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { serve } from "@hono/node-server"; +import type { ServerType } from "@hono/node-server"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createRemoteTransport } from "@inspector/core/mcp/remote/createRemoteTransport.js"; +import { createRemoteFetch } from "@inspector/core/mcp/remote/createRemoteFetch.js"; +import { RemoteOAuthStorage } from "@inspector/core/auth/remote/storage-remote.js"; +import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; +import { + createWebStoragePkceFlowStore, + type PkceFlowStore, +} from "@inspector/core/auth/pkce-flow-store.js"; +import { ConsoleNavigation } from "@inspector/core/auth/providers.js"; +import { + TestServerHttp, + waitForOAuthWellKnown, + getDefaultServerConfig, + createOAuthTestServerConfig, + clearOAuthTestData, +} from "@modelcontextprotocol/inspector-test-server"; +import { + completeOAuthAuthorization, + createStaticRedirectUrlProvider, +} from "../helpers/oauth-client-fixtures.js"; +import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; + +const REDIRECT_URL = "http://localhost:3001/oauth/callback"; +const CLIENT_ID = "concurrent-flows-client"; +const CLIENT_SECRET = "concurrent-flows-secret"; + +/** One tab's sessionStorage stand-in. */ +function createTabFlowStore(): PkceFlowStore { + const items = new Map(); + return createWebStoragePkceFlowStore(() => ({ + getItem: (key) => items.get(key) ?? null, + setItem: (key, value) => void items.set(key, value), + removeItem: (key) => void items.delete(key), + })); +} + +describe("InspectorClient overlapping OAuth flows to one server (#2726)", () => { + let mcpServer: TestServerHttp | undefined; + let remoteServer: ServerType | undefined; + let tempDir: string | undefined; + const clients: InspectorClient[] = []; + + beforeEach(() => { + clearOAuthTestData(); + tempDir = mkdtempSync(join(tmpdir(), "inspector-concurrent-oauth-")); + vi.spyOn(console, "log").mockImplementation(() => {}); + }); + + afterEach(async () => { + for (const client of clients.splice(0)) { + await client.disconnect(); + } + await mcpServer?.stop(); + mcpServer = undefined; + if (remoteServer) { + const server = remoteServer; + await new Promise((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + }); + remoteServer = undefined; + } + if (tempDir) { + rmSync(tempDir, { recursive: true, force: true }); + tempDir = undefined; + } + vi.restoreAllMocks(); + }); + + async function startBackend(): Promise<{ + baseUrl: string; + authToken: string; + }> { + const { app, authToken } = createRemoteApp({ + storageDir: tempDir, + initialConfig: { defaultEnvironment: {} }, + }); + return new Promise((resolve, reject) => { + const server = serve( + { fetch: app.fetch, port: 0, hostname: "127.0.0.1" }, + (info) => { + resolve({ baseUrl: `http://127.0.0.1:${info.port}`, authToken }); + }, + ); + remoteServer = server; + server.on("error", reject); + }); + } + + async function startMcpServer(): Promise { + mcpServer = new TestServerHttp({ + ...getDefaultServerConfig(), + serverType: "streamable-http", + ...createOAuthTestServerConfig({ + requireAuth: true, + staticClients: [ + { + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + redirectUris: [REDIRECT_URL], + }, + ], + }), + }); + const port = await mcpServer.start(); + const serverUrl = `http://localhost:${port}`; + await waitForOAuthWellKnown(serverUrl); + return serverUrl; + } + + /** A client as one page load of one tab builds it. */ + function createPageClient( + backend: { baseUrl: string; authToken: string }, + serverUrl: string, + flowStore: PkceFlowStore | undefined, + ): InspectorClient { + const client = new InspectorClient( + { type: "streamable-http", url: `${serverUrl}/mcp` } as MCPServerConfig, + { + environment: { + transport: createRemoteTransport(backend), + fetch: createRemoteFetch(backend), + oauth: { + storage: new RemoteOAuthStorage(backend), + navigation: new ConsoleNavigation(), + redirectUrlProvider: createStaticRedirectUrlProvider(REDIRECT_URL), + flowStore, + }, + }, + oauth: { clientId: CLIENT_ID, clientSecret: CLIENT_SECRET }, + }, + ); + clients.push(client); + return client; + } + + /** Tab A starts, tab B starts and finishes, then tab A finishes. */ + async function runOverlappingFlows(withFlowStore: boolean): Promise<{ + finishA: () => Promise; + clientB: InspectorClient; + }> { + const backend = await startBackend(); + const serverUrl = await startMcpServer(); + const tabA = withFlowStore ? createTabFlowStore() : undefined; + const tabB = withFlowStore ? createTabFlowStore() : undefined; + + const urlA = await createPageClient( + backend, + serverUrl, + tabA, + ).authenticate(); + const urlB = await createPageClient( + backend, + serverUrl, + tabB, + ).authenticate(); + if (!urlA || !urlB) throw new Error("Expected two authorization URLs"); + + const approvedA = await completeOAuthAuthorization(urlA); + const approvedB = await completeOAuthAuthorization(urlB); + + const clientB = createPageClient(backend, serverUrl, tabB); + await clientB.completeOAuthFlow( + approvedB.code, + approvedB.iss, + urlB.searchParams.get("state") ?? undefined, + ); + await clientB.connect(); + + const finishA = async () => { + const clientA = createPageClient(backend, serverUrl, tabA); + await clientA.completeOAuthFlow( + approvedA.code, + approvedA.iss, + urlA.searchParams.get("state") ?? undefined, + ); + await clientA.connect(); + return clientA; + }; + return { finishA, clientB }; + } + + it("completes both flows, each with its own verifier", async () => { + const { finishA, clientB } = await runOverlappingFlows(true); + const clientA = await finishA(); + + expect(clientB.getStatus()).toBe("connected"); + expect(clientA.getStatus()).toBe("connected"); + }); + + it("without a flow store, the earlier flow fails PKCE (the #2726 defect)", async () => { + const { finishA, clientB } = await runOverlappingFlows(false); + + expect(clientB.getStatus()).toBe("connected"); + // A's exchange sends B's verifier and fails PKCE. The test AS consumes a + // code on first read, so the SDK's retry after that rejection reports the + // code as expired instead; either message is the same defect. + await expect(finishA()).rejects.toThrow( + /Invalid code_verifier|Invalid or expired authorization code/, + ); + }); + + it("a callback whose state has no stored verifier fails clearly", async () => { + const backend = await startBackend(); + const serverUrl = await startMcpServer(); + const tab = createTabFlowStore(); + + const url = await createPageClient(backend, serverUrl, tab).authenticate(); + if (!url) throw new Error("Expected an authorization URL"); + const approved = await completeOAuthAuthorization(url); + + // The callback lands in a different tab, which never saw this flow. + const client = createPageClient(backend, serverUrl, createTabFlowStore()); + await expect( + client.completeOAuthFlow( + approved.code, + approved.iss, + url.searchParams.get("state") ?? undefined, + ), + ).rejects.toThrow(/No code verifier saved for this authorization flow/); + }); +}); diff --git a/core/auth/index.ts b/core/auth/index.ts index 918eaac3d4..3f2117de11 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -85,6 +85,13 @@ export { BaseOAuthClientProvider, } from "./providers.js"; +// Per-flow PKCE verifier store (#2726) +export type { PkceFlowStore, PkceFlowStorageArea } from "./pkce-flow-store.js"; +export { + createWebStoragePkceFlowStore, + PKCE_FLOW_STORAGE_KEY_PREFIX, +} from "./pkce-flow-store.js"; + // Utilities export { parseHttpUrl, diff --git a/core/auth/node/runner-interactive-oauth.ts b/core/auth/node/runner-interactive-oauth.ts index 6e084d85f8..78f35c2fd9 100644 --- a/core/auth/node/runner-interactive-oauth.ts +++ b/core/auth/node/runner-interactive-oauth.ts @@ -11,7 +11,11 @@ import type { RunnerOAuthCallbackConfig } from "./runner-oauth-callback.js"; export interface RunnerInteractiveOAuthClient { authenticate(): Promise; beginInteractiveAuthorization(authorizationUrl: URL): Promise; - completeOAuthFlow(authorizationCode: string, iss?: string): Promise; + completeOAuthFlow( + authorizationCode: string, + iss?: string, + state?: string, + ): Promise; checkAuthChallengeSatisfied(challenge: AuthChallenge): Promise; } @@ -142,7 +146,11 @@ export async function runRunnerInteractiveOAuth( path: options.callbackListen.pathname, onCallback: async (params) => { try { - await options.client.completeOAuthFlow(params.code, params.iss); + await options.client.completeOAuthFlow( + params.code, + params.iss, + params.state, + ); flowResolve(); } catch (err) { flowReject(toRunnerOAuthError(err)); diff --git a/core/auth/pkce-flow-store.ts b/core/auth/pkce-flow-store.ts new file mode 100644 index 0000000000..29fc9abe50 --- /dev/null +++ b/core/auth/pkce-flow-store.ts @@ -0,0 +1,98 @@ +/** + * Per-flow PKCE verifier store (#2726). + * + * The web client's OAuth state lives in the backend's shared `oauth.json`, + * merged at whole `servers[url]` granularity, and keyed by server URL only. Two + * authorization flows for the same server that overlap in time — two tabs, or + * two people on one deployed Inspector — therefore share one `codeVerifier` + * slot: the later flow overwrites the earlier one's verifier, and the earlier + * flow's token exchange fails PKCE with `invalid_grant`. + * + * A verifier is in-flight state that belongs to exactly one flow, so this store + * keys it by the auth session id `generateOAuthState()` puts in the OAuth + * `state` parameter (the 64-hex `authId`), and the callback looks it up by the + * `state` it was redirected back with. Only the outcome of a completed flow + * (client information, tokens) goes to the shared store. + * + * Keying inside the shared `servers[url]` entry instead would not help: the + * remote backend merges whole server entries, so a second tab's write would + * still replace the first tab's map. The web client therefore backs this with + * the initiating tab's `sessionStorage`, which survives the full-page redirect + * to the authorization server and back and is never visible to another tab. + */ + +/** In-flight PKCE verifiers, keyed by the OAuth `state` authId of their flow. */ +export interface PkceFlowStore { + saveCodeVerifier( + flowId: string, + serverUrl: string, + codeVerifier: string, + ): Promise; + /** + * The verifier saved for `flowId`, or `undefined` when there is none — or + * when the flow was started for a different server, so a `state` replayed + * against another server can never borrow that server's verifier. + */ + getCodeVerifier( + flowId: string, + serverUrl: string, + ): Promise; + clearCodeVerifier(flowId: string): Promise; +} + +/** The subset of the Web Storage API the store uses. */ +export type PkceFlowStorageArea = Pick< + Storage, + "getItem" | "setItem" | "removeItem" +>; + +export const PKCE_FLOW_STORAGE_KEY_PREFIX = "mcp-inspector:pkce-flow:"; + +interface StoredPkceFlow { + serverUrl: string; + codeVerifier: string; +} + +function parseStoredFlow(raw: string | null): StoredPkceFlow | undefined { + if (raw === null) return undefined; + try { + const value: unknown = JSON.parse(raw); + if ( + typeof value === "object" && + value !== null && + "serverUrl" in value && + "codeVerifier" in value && + typeof value.serverUrl === "string" && + typeof value.codeVerifier === "string" + ) { + return { serverUrl: value.serverUrl, codeVerifier: value.codeVerifier }; + } + } catch { + // A corrupt entry is treated as absent: the callback then fails with the + // "no verifier for this flow" error rather than throwing a parse error. + } + return undefined; +} + +/** + * A {@link PkceFlowStore} over a Web Storage area — `sessionStorage` by + * default, so each verifier is scoped to the tab that started its flow. + */ +export function createWebStoragePkceFlowStore( + getStorage: () => PkceFlowStorageArea = () => sessionStorage, +): PkceFlowStore { + const key = (flowId: string) => `${PKCE_FLOW_STORAGE_KEY_PREFIX}${flowId}`; + return { + async saveCodeVerifier(flowId, serverUrl, codeVerifier) { + const entry: StoredPkceFlow = { serverUrl, codeVerifier }; + getStorage().setItem(key(flowId), JSON.stringify(entry)); + }, + async getCodeVerifier(flowId, serverUrl) { + const entry = parseStoredFlow(getStorage().getItem(key(flowId))); + return entry?.serverUrl === serverUrl ? entry.codeVerifier : undefined; + }, + async clearCodeVerifier(flowId) { + getStorage().removeItem(key(flowId)); + }, + }; +} diff --git a/core/auth/providers.ts b/core/auth/providers.ts index 92512c0a23..2ca5968e2a 100644 --- a/core/auth/providers.ts +++ b/core/auth/providers.ts @@ -10,7 +10,8 @@ import type { OAuthDiscoveryState, } from "@modelcontextprotocol/client"; import type { OAuthStorage, SaveClientInformationOptions } from "./storage.js"; -import { generateOAuthState } from "./utils.js"; +import { generateOAuthState, parseOAuthState } from "./utils.js"; +import type { PkceFlowStore } from "./pkce-flow-store.js"; import { applyAuthorizationParams } from "./authorizationParams.js"; import { scopeForDeclinedRefreshGrant } from "./scopes.js"; @@ -136,6 +137,13 @@ export type OAuthProviderConfig = { * the request when {@link requestRefreshToken} is false. (#2068) */ configuredScope?: string; + /** + * Where the in-flight PKCE verifier lives (#2726). When set, the verifier is + * keyed by the OAuth `state` of the flow that created it instead of sharing + * the server-URL-keyed slot in {@link storage}, so overlapping flows for the + * same server cannot overwrite each other's. See `pkce-flow-store.ts`. + */ + flowStore?: PkceFlowStore; }; /** @@ -161,6 +169,13 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { protected requestRefreshToken: boolean; /** Server-configured scope, for the #2068 `offline_access` distinction. */ protected configuredScope?: string; + /** Per-flow verifier store (#2726); absent → the shared storage slot. */ + protected flowStore?: PkceFlowStore; + /** + * The authId of the flow this provider is driving: set by {@link state} on the + * redirect leg and by {@link resumeAuthorizationFlow} on the callback leg. + */ + private flowId: string | undefined; constructor(serverUrl: string, oauthConfig: OAuthProviderConfig) { this.serverUrl = serverUrl; @@ -171,6 +186,7 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { this.authorizationParams = oauthConfig.authorizationParams; this.requestRefreshToken = oauthConfig.requestRefreshToken ?? true; this.configuredScope = oauthConfig.configuredScope; + this.flowStore = oauthConfig.flowStore; } /** @@ -266,7 +282,28 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { } state(): string | Promise { - return generateOAuthState(); + const state = generateOAuthState(); + // The SDK calls `state()` immediately before `saveCodeVerifier()`, so this + // is the flow the verifier about to be saved belongs to (#2726). + this.flowId = state; + return state; + } + + /** + * Bind this provider to the flow a callback's `state` names, so + * {@link codeVerifier} reads that flow's verifier and no other (#2726). A + * `state` that is not one of ours binds nothing — and, with a flow store, a + * flow-less read then fails rather than reaching for the shared slot. + */ + resumeAuthorizationFlow(state: string): void { + this.flowId = parseOAuthState(state)?.authId; + } + + /** Drop the bound flow's verifier once its exchange has completed. */ + async clearAuthorizationFlow(): Promise { + if (this.flowStore && this.flowId) { + await this.flowStore.clearCodeVerifier(this.flowId); + } } async clientInformation( @@ -471,10 +508,32 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { } async saveCodeVerifier(codeVerifier: string): Promise { + if (this.flowStore && this.flowId) { + await this.flowStore.saveCodeVerifier( + this.flowId, + this.serverUrl, + codeVerifier, + ); + return; + } await this.storage.saveCodeVerifier(this.serverUrl, codeVerifier); } async codeVerifier(): Promise { + if (this.flowStore) { + // Never fall back to the shared slot here: whatever it holds belongs to + // some other flow, and exchanging with it is exactly the PKCE failure + // this store exists to prevent (#2726). + const verifier = this.flowId + ? await this.flowStore.getCodeVerifier(this.flowId, this.serverUrl) + : undefined; + if (!verifier) { + throw new Error( + "No code verifier saved for this authorization flow. It may have been started in another browser tab or already completed; please re-authenticate.", + ); + } + return verifier; + } const verifier = await this.storage.getCodeVerifier(this.serverUrl); if (!verifier) { throw new Error("No code verifier saved for session"); @@ -529,6 +588,10 @@ export class BaseOAuthClientProvider implements OAuthClientProvider { await this.storage.clearTokens(this.serverUrl); return; case "verifier": + if (this.flowStore) { + await this.clearAuthorizationFlow(); + return; + } await this.storage.clearCodeVerifier(this.serverUrl); return; case "discovery": diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 818c4f60eb..48ccb87118 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -6336,8 +6336,13 @@ export class InspectorClient extends InspectorClientEventTarget { async completeOAuthFlow( authorizationCode: string, iss?: string, + state?: string, ): Promise { - await this.ensureOAuthManager().completeOAuthFlow(authorizationCode, iss); + await this.ensureOAuthManager().completeOAuthFlow( + authorizationCode, + iss, + state, + ); if (this.usesDirectAuthRecovery()) { await this.reconnectAfterAuthRecovery(); } @@ -6365,9 +6370,13 @@ export class InspectorClient extends InspectorClientEventTarget { */ async resumeAfterOAuth( authorizationCode: string, - options?: { remoteSessionId?: string; iss?: string }, + options?: { remoteSessionId?: string; iss?: string; state?: string }, ): Promise { - await this.completeOAuthFlow(authorizationCode, options?.iss); + await this.completeOAuthFlow( + authorizationCode, + options?.iss, + options?.state, + ); const remoteSessionId = options?.remoteSessionId; const transport = this.baseTransport; diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 0f8f919ae7..5cd68a29bf 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -194,6 +194,8 @@ export class OAuthManager { // #2068: lets the provider tell an `offline_access` the user asked for // from one inherited from a previous grant's persisted scope. configuredScope: this.oauthConfig.scope, + // #2726: in-flight PKCE verifiers keyed by flow, not by server URL. + flowStore: this.oauthConfig.flowStore, }); provider.setEventTarget(this.params.getEventTarget()); @@ -394,9 +396,15 @@ export class OAuthManager { authorizationUrl.searchParams.get("scope")?.trim() || requestedScope; } + /** + * `state` is the OAuth `state` the callback was redirected back with. It + * selects the flow whose PKCE verifier the exchange uses (#2726); without it + * a provider with a flow store has no verifier to send. + */ async completeOAuthFlow( authorizationCode: string, iss?: string, + state?: string, ): Promise { try { if (this.isEnterpriseManaged()) { @@ -426,6 +434,9 @@ export class OAuthManager { const provider = await this.createOAuthProvider(); const serverUrl = this.getServerUrl(); + if (state !== undefined) { + provider.resumeAuthorizationFlow(state); + } const result = await mcpAuth(provider, { serverUrl, @@ -439,6 +450,8 @@ export class OAuthManager { `Expected AUTHORIZED after providing authorization code, got: ${result}`, ); } + // The code is single-use, so the verifier that redeemed it is spent. + await provider.clearAuthorizationFlow(); const tokens = await provider.tokens(); if (!tokens) { diff --git a/core/mcp/types.ts b/core/mcp/types.ts index b9c5606bd8..7cde329006 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -39,6 +39,7 @@ import type { RedirectUrlProvider, } from "../auth/providers.js"; import type { OAuthStorage } from "../auth/storage.js"; +import type { PkceFlowStore } from "../auth/pkce-flow-store.js"; import type { AuthChallenge } from "../auth/challenge.js"; /** @@ -1265,6 +1266,13 @@ export interface InspectorClientEnvironment { * - Browser: from window.location or callback route */ redirectUrlProvider?: RedirectUrlProvider; + + /** + * Where in-flight PKCE verifiers live, keyed by the OAuth `state` of their + * flow (#2726). Omit to keep the verifier in {@link storage}. + * - Browser: `createWebStoragePkceFlowStore()` (the tab's sessionStorage) + */ + flowStore?: PkceFlowStore; }; } diff --git a/specification/v2_storage.md b/specification/v2_storage.md index 79f3cac4f4..485f559625 100644 --- a/specification/v2_storage.md +++ b/specification/v2_storage.md @@ -62,6 +62,8 @@ OAuth and EMA **runtime state** (access/refresh tokens, PKCE verifiers, granted **Stack (#1549):** `OAuthStorage` interface → `OAuthStorageBase` + `OAuthMemoryStore` + `OAuthPersistBackend` (`core/auth/oauth-storage.ts`, `core/auth/store.ts`, `core/auth/oauth-persist.ts`). Reads still promote legacy `{ state, version }` envelopes (migrate-on-write). All getters are async; setters auto-persist. Web shares one store via `getWebRemoteOAuthStorage()`. +**In-flight PKCE verifiers stay in the tab (#2726):** the web client does **not** put a flow's PKCE verifier in the shared store. Its environment sets an `oauth.flowStore` (`createWebStoragePkceFlowStore()`, `core/auth/pkce-flow-store.ts`) that keeps each verifier in the initiating tab's `sessionStorage`, keyed by the flow's OAuth `state` authId, and the `/oauth/callback` exchange reads the verifier for the `state` it came back with. A server-URL-keyed slot would be shared by every overlapping flow for that server — two tabs, or several people on one deployed Inspector — so the later flow would overwrite the earlier one's verifier and fail its exchange with `invalid_grant`; and keying inside `servers[url]` would not help, since the remote backend merges whole server entries. Only a completed flow's outcome (client information, tokens) reaches `oauth.json`. CLI / TUI set no flow store and keep the verifier in their store as before. + **Secret split (#2481):** `oauth.json` holds only the **non-secret residue** — server URLs, granted scopes, cached authorization-server metadata, and non-secret client registration fields. Secret material (access/refresh tokens, `client_secret`, `registration_access_token`, IdP session payloads) lives in the OS **secret store** (`core/auth/node/secret-store.ts`), keyed per server. Reads rejoin the residue with the stored secrets (lazily migrating any legacy plaintext into the store); writes are section-scoped merges under the state-file lock, so a writer never clobbers sections it did not read. A present-but-unrecognized `oauth.json` refuses mutation rather than being treated as empty. See [docs/secret-storage.md](../docs/secret-storage.md) for store selection and fallbacks. Details: [EMA auth](v2_auth_ema.md#oauth-persistence-1549--done), [Mid-session auth](v2_auth_mid_session.md). From 3fd5b5b52faef509a89cb39ea79f988f395695e8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 20 Oct 2026 00:46:31 -0400 Subject: [PATCH 2/2] test(auth): keep one storage area in the cross-server flow-store test Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: cliffhall --- clients/web/src/test/core/auth/pkce-flow-store.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clients/web/src/test/core/auth/pkce-flow-store.test.ts b/clients/web/src/test/core/auth/pkce-flow-store.test.ts index 708a633dd2..6cec55c1cf 100644 --- a/clients/web/src/test/core/auth/pkce-flow-store.test.ts +++ b/clients/web/src/test/core/auth/pkce-flow-store.test.ts @@ -51,9 +51,12 @@ describe("createWebStoragePkceFlowStore (#2726)", () => { }); it("does not answer for a different server than the flow was started for", async () => { - const store = createWebStoragePkceFlowStore(() => makeArea()); + const area = makeArea(); + const store = createWebStoragePkceFlowStore(() => area); await store.saveCodeVerifier(FLOW, SERVER, "cv"); + // Same area, so only the server binding can make this read miss. + expect(await store.getCodeVerifier(FLOW, SERVER)).toBe("cv"); expect( await store.getCodeVerifier(FLOW, "https://other.example.com"), ).toBeUndefined();