diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 8ee0559f..6cd232e7 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -506,6 +506,58 @@ describe("withOAuthRequestTimeout", () => { expect(isExempt(`${SERVER}#frag`)).toBe(true); }); + /** Streamable HTTP: every MCP request goes to the configured URL. */ + const streamable = (serverUrl: string) => + exemptMcpEndpoint( + () => serverUrl, + () => false, + ); + /** Legacy SSE: the message endpoint arrives inside the stream. */ + const sse = (serverUrl: string) => + exemptMcpEndpoint( + () => serverUrl, + () => true, + ); + + it("bounds same-origin OAuth work on a Streamable HTTP connection", () => { + // The narrow rule, available because the endpoint is the configured URL. + const isExempt = streamable(SERVER); + + expect(isExempt(SERVER)).toBe(true); + expect( + isExempt( + "https://srv.example.com/.well-known/oauth-protected-resource/mcp", + ), + ).toBe(false); + expect(isExempt("https://srv.example.com/token")).toBe(false); + }); + + it("widens to the origin on legacy SSE, whose message path is unknowable", () => { + // The cost of the wider rule, asserted rather than implied: same-origin + // OAuth is exempt here, and falls back to the SDK's per-request timeout. + const isExempt = sse("https://srv.example.com/sse"); + + expect( + isExempt("https://srv.example.com/.well-known/openid-configuration"), + ).toBe(true); + expect(isExempt("https://as.example.com/token")).toBe(false); + }); + + it("falls back to the origin rule when the transport is unknown", () => { + // Fails open on this uncertainty like every other: a path rule applied to + // a connection that turns out to be SSE severs real MCP traffic. + const noHint = exemptMcpEndpoint(() => SERVER); + const throws = exemptMcpEndpoint( + () => SERVER, + () => { + throw new Error("not yet connected"); + }, + ); + + expect(noHint("https://srv.example.com/token")).toBe(true); + expect(throws("https://srv.example.com/token")).toBe(true); + }); + it("exempts legacy SSE's separate message endpoint", () => { // The case a path rule gets wrong: `SSEClientTransport` opens the // configured URL and is handed a *different* pathname to POST every @@ -533,21 +585,6 @@ describe("withOAuthRequestTimeout", () => { expect(isExempt("https://srv.example.com:8443/token")).toBe(false); }); - it("exempts a same-origin OAuth endpoint, which is the cost of the rule", () => { - // Stated as a test rather than left implicit: protected-resource metadata - // lives on the resource's own origin, so it is exempt here. The OAuth - // chain still bounds the Inspector's own requests to it; only the SDK's - // transport-internal OAuth against a same-origin server falls back to the - // SDK's per-request timeout. - const isExempt = exemptMcpEndpoint(() => SERVER); - - expect( - isExempt( - "https://srv.example.com/.well-known/oauth-protected-resource/mcp", - ), - ).toBe(true); - }); - it("fails open when the server URL is unknown or unparseable", () => { // The two errors are not symmetric: bounding what should not be bounded // severs a long-running tool call, while failing to bound leaves the diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 3e6c478e..46943308 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -157,34 +157,65 @@ export function isOAuthRequestTimeoutWire( * uncertain — no server URL yet, a URL that will not parse, either side — is * treated as the MCP endpoint and exempted. * - * Compared on **origin alone**, not origin + pathname. Path matching looks more - * precise and is wrong: legacy SSE opens the configured URL (`/sse`), is handed - * a *separate* message endpoint by the server, and POSTs every JSON-RPC message - * to that second pathname — so a path rule bounds and body-buffers real MCP - * traffic, and a slow tool call over SSE is severed at 30s and reported as an - * `OAuthRequestTimeoutError` (Copilot). + * The comparison is **as narrow as the transport allows**, which is not the same + * rule for both of them: * - * Origin is not a loose approximation of the right rule; it is exact, because - * the SDK *enforces* that the SSE message endpoint shares the stream URL's - * origin and rejects one that does not. Every MCP request this chain carries is - * therefore same-origin with the configured URL, by construction. + * - **Streamable HTTP** sends every MCP request to the configured URL, so + * `origin + pathname` is exact. Same-origin OAuth work — a protected-resource + * document, or an authorization server deployed beside the resource — is a + * different path and stays bounded, which is what #2319 asks for (Copilot). + * - **Legacy SSE** opens the configured URL (`/sse`), is handed a *separate* + * message endpoint by the server, and POSTs every JSON-RPC message to that + * second pathname. We never see that URL — it arrives inside the stream, in + * the SDK's own parser — so a path rule there bounds and body-buffers real + * MCP traffic, severing a slow tool call at 30s and reporting it as an + * `OAuthRequestTimeoutError`. Origin is the narrowest rule available, and it + * is exact rather than approximate: the SDK *enforces* that the message + * endpoint shares the stream URL's origin and rejects one that does not. * - * What it costs: an OAuth endpoint hosted on the MCP server's own origin — the - * protected-resource metadata document, or an authorization server deployed - * beside the resource — is exempt here and so unbounded on this chain. That is - * the fail-open direction again, and the loss is narrow: the OAuth *chain* - * bounds the Inspector's own discovery and token requests to those same URLs, - * so only the SDK's transport-internal OAuth against a same-origin server falls - * back to the SDK's per-request timeout, which is where it was before #2319. + * So the residual gap is confined to legacy SSE against a server that also + * hosts its own OAuth endpoints, where transport-internal OAuth falls back to + * the SDK's per-request timeout — where it was before #2319. The Inspector's + * own discovery and token requests to those URLs are bounded regardless, on the + * OAuth chain. + * + * ⚠️ **Fails open** on every uncertainty, and the uncertainties differ by rule: + * no server URL, an unparseable URL on either side, or *not knowing which + * transport this is* all resolve to exempt — the last one by falling back to + * the wider origin rule. */ +/** + * Read the transport hint, treating "absent" and "threw" alike as the wide + * case. A path rule applied to a connection that turns out to be legacy SSE + * severs real MCP traffic, so every uncertainty resolves toward the origin rule. + */ +function readNegotiatesEndpoint(hint: (() => boolean) | undefined): boolean { + if (!hint) return true; + try { + return hint(); + } catch { + return true; + } +} + export function exemptMcpEndpoint( getServerUrl: () => string | undefined, + /** + * Whether the message endpoint is negotiated rather than configured — true + * for legacy SSE. Read per call, since the transport can change between + * connects. Absent, or throwing, is treated as `true`. + */ + negotiatesEndpoint?: () => boolean, ): (url: string) => boolean { return (url) => { const serverUrl = getServerUrl(); if (!serverUrl) return true; + const negotiated = readNegotiatesEndpoint(negotiatesEndpoint); try { - return new URL(url).origin === new URL(serverUrl).origin; + const a = new URL(url); + const b = new URL(serverUrl); + if (a.origin !== b.origin) return false; + return negotiated || a.pathname === b.pathname; } catch { return true; } diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 94abbc87..29456ff4 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -840,7 +840,14 @@ export class InspectorClient extends InspectorClientEventTarget { withOAuthRequestTimeout( withOverrides, DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, - exemptMcpEndpoint(() => this.getServerUrl()), + exemptMcpEndpoint( + () => this.getServerUrl(), + // Legacy SSE negotiates its message endpoint inside the stream, so + // its path is unknowable here and the exemption has to widen to the + // origin. Streamable HTTP sends everything to the configured URL, so + // the path rule is exact there and same-origin OAuth stays bounded. + () => this.transportConfig?.type === "sse", + ), ), ); // #2319: the auth chain is composed separately so the deadline sits