diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index acb354aa..511f307b 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -504,6 +504,45 @@ describe("probe cancellation (#2319)", () => { expect(inner).toHaveBeenCalledTimes(1); }); + it("rethrows an abort that lands while the probe body is pending", async () => { + // `fetch` resolves on headers, so a late abort rejects `probe.text()` + // rather than the fetch — a separate catch, which used to answer with the + // preceding 404. + const caller = new AbortController(); + const inner = vi.fn((input, init) => { + if (String(input) === RFC8414) { + return Promise.resolve(new Response(null, { status: 404 })); + } + // Headers now, body never. + return Promise.resolve( + new Response( + new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode('{"issuer":')); + init?.signal?.addEventListener("abort", () => + ctrl.error(init.signal?.reason as Error), + ); + }, + }), + { headers: { "content-type": "application/json" } }, + ), + ); + }); + const wrapped = withRfc8414OidcCompat(inner); + + const settled = wrapped(RFC8414, { signal: caller.signal }).then( + (r) => r, + (e: unknown) => e, + ); + await vi.waitFor(() => { + expect(inner).toHaveBeenCalledTimes(2); + }); + const reason = new Error("caller gave up mid-body"); + caller.abort(reason); + + expect(await settled).toBe(reason); + }); + it("still falls back to the original response on an ordinary probe failure", async () => { const inner = vi.fn((input) => { if (String(input) === RFC8414) { diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 95f23034..8ee0559f 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -506,17 +506,46 @@ describe("withOAuthRequestTimeout", () => { expect(isExempt(`${SERVER}#frag`)).toBe(true); }); - it("bounds OAuth work on the same chain", () => { + 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 + // JSON-RPC message to. Bounding those would sever a slow tool call over + // SSE and report it as an OAuth timeout. The SDK enforces that the + // message endpoint shares the stream URL's origin, which is what makes + // an origin rule exact rather than approximate. + const isExempt = exemptMcpEndpoint(() => "https://srv.example.com/sse"); + + expect(isExempt("https://srv.example.com/messages?sessionId=abc")).toBe( + true, + ); + expect(isExempt("https://srv.example.com/")).toBe(true); + }); + + it("bounds OAuth work on a different origin", () => { + const isExempt = exemptMcpEndpoint(() => SERVER); + + expect(isExempt("https://as.example.com/token")).toBe(false); + expect( + isExempt("https://as.example.com/.well-known/openid-configuration"), + ).toBe(false); + // Origin is scheme + host + port, so any of the three differing bounds it. + expect(isExempt("http://srv.example.com/token")).toBe(false); + 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(false); - expect(isExempt("https://as.example.com/token")).toBe(false); - // Same origin, different path — still not the endpoint. - expect(isExempt("https://srv.example.com/mcp/register")).toBe(false); + ).toBe(true); }); it("fails open when the server URL is unknown or unparseable", () => { @@ -540,6 +569,7 @@ describe("withOAuthRequestTimeout", () => { expect(isExempt("https://other.example.com/mcp")).toBe(false); current = "https://other.example.com/mcp"; expect(isExempt("https://other.example.com/mcp")).toBe(true); + expect(isExempt(SERVER)).toBe(false); }); }); diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index 06ba7f43..760b3540 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -290,8 +290,10 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { if (err instanceof OAuthRequestTimeoutError) throw err; // Likewise a caller that gave up: substituting the preceding 404 for // its own abort would report a discovery failure for a request it - // deliberately cancelled. - if (callerSignal?.aborted) throw err; + // deliberately cancelled. The caller's own `reason` is what surfaces, + // here and at the two sites below, rather than whatever shape the + // underlying layer happened to reject with. + if (callerSignal?.aborted) throw callerSignal.reason; return response; } if (!probe.ok) { @@ -301,6 +303,11 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // candidate exhaust the origin's pool (Copilot). Same discipline as // `core/mcp/node/authChallengeFetch.ts`. await releaseBody(probe); + // Rechecked after the release, which awaits: an abort that lands while + // the body is being discarded must not be answered with the preceding + // response either, and `continue` would otherwise carry on probing for + // a caller that has stopped waiting. + if (callerSignal?.aborted) throw callerSignal.reason; if (continuesDiscovery(probe.status)) continue; return response; } @@ -317,6 +324,14 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { body = await probe.text(); parsed = JSON.parse(body); } catch { + // `fetch` resolves on headers, so the abort can land here rather than + // in the catch above: the probe's headers arrived, the caller gave up, + // and `probe.text()` rejects on the partial body. Substituting the + // preceding 404 would hand a standalone user of this wrapper a + // discovery failure in place of its own abort reason (Copilot). A body + // that merely will not parse still falls through, which is the case + // this catch exists for. + if (callerSignal?.aborted) throw callerSignal.reason; return response; } if (!isRfc8414OnlyMetadata(parsed)) { diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 511e566a..3e6c478e 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -157,9 +157,25 @@ 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 + pathname: the SDK varies method and query across its MCP - * requests but not the endpoint, while OAuth discovery goes to a different path - * (`/.well-known/…`) and usually a different origin entirely. + * 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). + * + * 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. + * + * 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. */ export function exemptMcpEndpoint( getServerUrl: () => string | undefined, @@ -168,9 +184,7 @@ export function exemptMcpEndpoint( const serverUrl = getServerUrl(); if (!serverUrl) return true; try { - const a = new URL(url); - const b = new URL(serverUrl); - return a.origin === b.origin && a.pathname === b.pathname; + return new URL(url).origin === new URL(serverUrl).origin; } catch { return true; }