diff --git a/src/mcp/client-auth-policy.test.ts b/src/mcp/client-auth-policy.test.ts index d83856747..03d3b988a 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -188,10 +188,13 @@ describe("HTTP MCP auth policy", () => { }); expect(callbackStarts).toBe(0); expect(providerCreates).toBe(0); - // No signal and no provider: leave redirectPolicy unset so the SDK - // default `same-origin` applies. Do not pin `follow`. + // No signal and no provider: same-origin is pinned so the SDK default + // can never widen to `follow`. Do not pin `follow`. expect(transportOptions).toEqual([ - { reconnectionOptions: MCP_HTTP_RECONNECTION_OPTIONS }, + { + redirectPolicy: "same-origin", + reconnectionOptions: MCP_HTTP_RECONNECTION_OPTIONS, + }, ]); }); @@ -268,6 +271,7 @@ describe("HTTP MCP auth policy", () => { if (!result.ok) return; expect(transportOptions).toEqual([ { + redirectPolicy: "same-origin", reconnectionOptions: MCP_HTTP_RECONNECTION_OPTIONS, authProvider, requestInit: { signal: expect.any(AbortSignal) }, @@ -305,6 +309,7 @@ describe("HTTP MCP auth policy", () => { expect(providerServerURL).toBe("https://custom.example/mcp?mode=full"); expect(transportOptions).toEqual([ { + redirectPolicy: "same-origin", reconnectionOptions: MCP_HTTP_RECONNECTION_OPTIONS, authProvider, requestInit: { signal: expect.any(AbortSignal) }, diff --git a/src/mcp/client-redirect-policy.test.ts b/src/mcp/client-redirect-policy.test.ts index 540e606f1..63f88669c 100644 --- a/src/mcp/client-redirect-policy.test.ts +++ b/src/mcp/client-redirect-policy.test.ts @@ -10,6 +10,18 @@ import { createOAuthProvider } from "./oauth-provider.js"; const SERVER_URL = "https://mcp.example/mcp"; const OFF_ORIGIN = "https://evil.example/collect"; +const RESOURCE_METADATA = { + resource: SERVER_URL, + authorization_servers: ["https://mcp.example"], +}; + +const AUTH_SERVER_METADATA = { + issuer: "https://mcp.example", + authorization_endpoint: "https://mcp.example/authorize", + token_endpoint: "https://mcp.example/oauth/token", + response_types_supported: ["code"], +}; + function json(body: unknown): Response { return new Response(JSON.stringify(body), { status: 200, @@ -17,37 +29,46 @@ function json(body: unknown): Response { }); } +function redirectTo(location: string): Response { + return new Response(null, { status: 308, headers: { location } }); +} + +async function saveTestProvider(home: string) { + const identity = { serverName: "example", serverURL: SERVER_URL }; + await saveAuthState( + identity, + { + clientInformation: { + client_id: "client", + redirect_uris: ["http://127.0.0.1:1/callback"], + client_id_issued_at: 1, + token_endpoint_auth_method: "none", + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code"], + client_name: "interchange-code", + }, + }, + home, + ); + const provider = await createOAuthProvider({ + serverName: identity.serverName, + serverURL: identity.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + }); + await provider.saveCodeVerifier("verifier"); + return provider; +} + describe("HTTP MCP redirect policy", () => { test("discovery and token fetches do not follow a cross-origin redirect", async () => { const home = await mkdtemp(join(tmpdir(), "mcp-redirect-")); - const identity = { serverName: "example", serverURL: SERVER_URL }; - await saveAuthState( - identity, - { - clientInformation: { - client_id: "client", - redirect_uris: ["http://127.0.0.1:1/callback"], - client_id_issued_at: 1, - token_endpoint_auth_method: "none", - grant_types: ["authorization_code", "refresh_token"], - response_types: ["code"], - client_name: "interchange-code", - }, - }, - home, - ); - const provider = await createOAuthProvider({ - serverName: identity.serverName, - serverURL: identity.serverURL, - redirectUrl: "http://127.0.0.1:1/callback", - onAuthURL: () => undefined, - home, - }); - await provider.saveCodeVerifier("verifier"); + const provider = await saveTestProvider(home); const abort = new AbortController(); const options = streamableHTTPTransportOptions(provider, abort.signal); - expect(options).not.toHaveProperty("redirectPolicy"); + expect(options).toMatchObject({ redirectPolicy: "same-origin" }); const requested: string[] = []; const fetchFn = async ( @@ -65,18 +86,10 @@ describe("HTTP MCP redirect policy", () => { target.pathname === "/oauth/token"; if (!crossOrigin) { if (target.pathname === "/.well-known/oauth-protected-resource") { - return json({ - resource: SERVER_URL, - authorization_servers: ["https://mcp.example"], - }); + return json(RESOURCE_METADATA); } if (target.pathname === "/.well-known/oauth-authorization-server") { - return json({ - issuer: "https://mcp.example", - authorization_endpoint: "https://mcp.example/authorize", - token_endpoint: "https://mcp.example/oauth/token", - response_types_supported: ["code"], - }); + return json(AUTH_SERVER_METADATA); } return new Response(null, { status: 404 }); } @@ -106,4 +119,122 @@ describe("HTTP MCP redirect policy", () => { const tokenPost = requested.find((href) => href.includes("/oauth/token")); expect(tokenPost?.startsWith("https://mcp.example/")).toBe(true); }); + + test("same-origin discovery and token redirects are followed (CL-10758)", async () => { + const home = await mkdtemp(join(tmpdir(), "mcp-redirect-")); + const provider = await saveTestProvider(home); + + const abort = new AbortController(); + const options = streamableHTTPTransportOptions(provider, abort.signal); + expect(options).toMatchObject({ redirectPolicy: "same-origin" }); + + const requested: string[] = []; + const fetchFn = async ( + url: string | URL, + init?: RequestInit, + ): Promise => { + const href = String(url); + requested.push(href); + const target = new URL(href); + // The stub only answers manual redirects: the SDK follows same-origin + // chains itself and never lets fetch follow across origins. + if (init?.redirect !== "manual") + throw new Error(`expected redirect manual, got ${init?.redirect}`); + if (target.hostname !== "mcp.example") + return new Response("collected", { status: 200 }); + if ( + target.pathname === "/.well-known/oauth-protected-resource/mcp" || + target.pathname === "/.well-known/oauth-protected-resource" + ) { + return redirectTo( + "https://mcp.example/.well-known/oauth-protected-resource/mcp-2", + ); + } + if (target.pathname === "/.well-known/oauth-protected-resource/mcp-2") { + return json(RESOURCE_METADATA); + } + if (target.pathname === "/.well-known/oauth-authorization-server") { + return json(AUTH_SERVER_METADATA); + } + if (target.pathname === "/oauth/token") { + return redirectTo("https://mcp.example/oauth/token-final"); + } + if (target.pathname === "/oauth/token-final") { + return json({ access_token: "access", token_type: "Bearer" }); + } + return new Response(null, { status: 404 }); + }; + + const transport = new StreamableHTTPClientTransport(new URL(SERVER_URL), { + ...options, + fetch: fetchFn, + }); + // finishAuth resolves (void) once the code exchange completes; the + // redirect targets below prove the same-origin hops were followed. + await expect(transport.finishAuth("auth-code")).resolves.toBeUndefined(); + + expect(requested.some((href) => href.includes("evil.example"))).toBe(false); + expect( + requested.some((href) => + href.includes("/.well-known/oauth-protected-resource/mcp-2"), + ), + ).toBe(true); + expect(requested.some((href) => href.includes("/oauth/token-final"))).toBe( + true, + ); + }); + + test("a same-origin token redirect chain past the SDK cap fails bounded (CL-10758)", async () => { + const home = await mkdtemp(join(tmpdir(), "mcp-redirect-")); + const provider = await saveTestProvider(home); + + const abort = new AbortController(); + const options = streamableHTTPTransportOptions(provider, abort.signal); + + const requested: string[] = []; + const fetchFn = async ( + url: string | URL, + init?: RequestInit, + ): Promise => { + const href = String(url); + requested.push(href); + const target = new URL(href); + if (init?.redirect !== "manual") + throw new Error(`expected redirect manual, got ${init?.redirect}`); + if ( + target.pathname === "/.well-known/oauth-protected-resource/mcp" || + target.pathname === "/.well-known/oauth-protected-resource" + ) { + return json(RESOURCE_METADATA); + } + if (target.pathname === "/.well-known/oauth-authorization-server") { + return json(AUTH_SERVER_METADATA); + } + const hop = /^\/oauth\/token(?:-(\d+))?$/.exec(target.pathname); + if (hop !== null) { + const depth = hop[1] === undefined ? 0 : Number(hop[1]); + // Eight same-origin hops with a 200 past the end: the SDK must stop + // following before it gets there. + if (depth < 8) + return redirectTo(`https://mcp.example/oauth/token-${depth + 1}`); + return json({ access_token: "access", token_type: "Bearer" }); + } + return new Response(null, { status: 404 }); + }; + + const transport = new StreamableHTTPClientTransport(new URL(SERVER_URL), { + ...options, + fetch: fetchFn, + }); + await expect(transport.finishAuth("auth-code")).rejects.toThrow(); + + expect(requested.some((href) => href.includes("evil.example"))).toBe(false); + const tokenHits = requested.filter((href) => href.includes("/oauth/token")); + // SDK MAX_REDIRECTS (5) plus the initial POST: the 200 at depth 8 is + // never reached, and the chain cannot loop forever. + expect(tokenHits).toHaveLength(6); + expect( + tokenHits.every((href) => href.startsWith("https://mcp.example/")), + ).toBe(true); + }); }); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index ef2fca151..eb75b480b 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -410,10 +410,13 @@ export function streamableHTTPTransportOptions( authProvider: CorbitsOAuthProvider | undefined, signal: AbortSignal | undefined, ) { - // Leave redirectPolicy unset. SDK 1.32 defaults to `same-origin`; `follow` - // also wraps OAuth discovery and token requests, so a 307/308 can post - // refresh tokens or auth codes to another host. + // Pin same-origin explicitly rather than relying on the SDK default: + // `follow` also wraps OAuth discovery and token requests, so a 307/308 + // could post refresh tokens or auth codes to another host. Aligned with + // the ^1.32.1 SDK range in package.json, where `same-origin` follows + // within-origin chains (capped) for discovery, token, and SSE traffic. return { + redirectPolicy: "same-origin" as const, reconnectionOptions: MCP_HTTP_RECONNECTION_OPTIONS, ...(authProvider === undefined ? {} : { authProvider }), ...(signal === undefined