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 7c47fe6f0d..546066d2f5 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..6cec55c1cf --- /dev/null +++ b/clients/web/src/test/core/auth/pkce-flow-store.test.ts @@ -0,0 +1,102 @@ +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 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(); + }); + + 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 16145dec0a..a0cdaa2679 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 d54681598e..3191b5f752 100644 --- a/core/auth/node/runner-interactive-oauth.ts +++ b/core/auth/node/runner-interactive-oauth.ts @@ -14,7 +14,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, options?: CheckAuthChallengeSatisfiedOptions, @@ -148,7 +152,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 8ccb383ca5..0af16c58a3 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -6341,8 +6341,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(); } @@ -6370,9 +6375,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 4982d543d1..474d6be6d3 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -195,6 +195,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()); @@ -395,9 +397,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()) { @@ -427,6 +435,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, @@ -440,6 +451,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).