From f491c7fc9a170590e41b17007b1d9f5fd94b161b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 04:49:05 -0400 Subject: [PATCH] feat: bound SDK-internal OAuth on the transport chain too (#2319 round 11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The transport chain carried the SDK's own OAuth work — the discovery it runs from inside the transport on the 401/refresh path — and that leg was left to the SDK's incidental per-request timeout, reporting a bare `Request timed out` naming no endpoint. On a direct (non-proxied) path the /api/fetch backstop cannot reach it either. `withOAuthRequestTimeout` takes an optional `isExempt` predicate; an exempt request passes through completely untouched — no deadline, no signal, and no body buffering, which is what a stream needs. `exemptMcpEndpoint` exempts the MCP endpoint by origin + pathname, and FAILS OPEN: no server URL, or either side unparseable, means exempt. The errors are not symmetric. Bounding what should not be bounded severs a long-running tool call withholding its headers, or an SSE body; failing to bound leaves the SDK's timeout as the backstop it already was. Two test corrections, both real: - The metadata-preservation case asserted url/redirected/type against a synthetic Response, whose values are the rebuild's own defaults — so it stayed green with the defineProperty loop deleted. It now seeds non-default values first; verified by mutation that removing the loop makes it fail. - Two signal assertions compared against the controller rather than `request.signal`. The Fetch standard gives a Request a dependent signal, so those encoded non-standard identity. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../src/test/core/auth/requestTimeout.test.ts | 99 ++++++++++++++++++- .../core/mcp/remote/createRemoteFetch.test.ts | 25 +++-- core/auth/index.ts | 1 + core/auth/requestTimeout.ts | 48 +++++++++ core/mcp/inspectorClient.ts | 22 ++++- 5 files changed, 181 insertions(+), 14 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 0bf8dfa0..8c8be1cd 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -25,6 +25,7 @@ import { describe, it, expect, vi, afterEach } from "vitest"; import { DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, OAuthRequestTimeoutError, + exemptMcpEndpoint, withOAuthRequestTimeout, } from "@inspector/core/auth/requestTimeout.js"; import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js"; @@ -270,6 +271,24 @@ describe("withOAuthRequestTimeout", () => { statusText: "Created", headers: { "content-type": "application/json", "x-probe": "kept" }, }); + // Seeded to NON-default values, which is the whole point of the case: a + // synthetic `new Response(...)` already has `url === ""`, `redirected === + // false` and `type === "default"` — exactly the rebuilt response's own + // defaults — so asserting against those would stay green with the + // `Object.defineProperty` loop deleted from `rebuildResponse` (Copilot). + // These are read-only getters, hence the same mechanism the source uses. + Object.defineProperty(original, "url", { + value: "https://as.example.com/redirected", + configurable: true, + }); + Object.defineProperty(original, "redirected", { + value: true, + configurable: true, + }); + Object.defineProperty(original, "type", { + value: "cors", + configurable: true, + }); const wrapped = withOAuthRequestTimeout( vi.fn().mockResolvedValue(original), 1000, @@ -283,9 +302,9 @@ describe("withOAuthRequestTimeout", () => { expect(response.headers.get("x-probe")).toBe("kept"); expect(response.headers.get("content-encoding")).toBeNull(); expect(response.headers.get("content-length")).toBeNull(); - expect(response.url).toBe(original.url); - expect(response.redirected).toBe(original.redirected); - expect(response.type).toBe(original.type); + expect(response.url).toBe("https://as.example.com/redirected"); + expect(response.redirected).toBe(true); + expect(response.type).toBe("cors"); await expect(response.json()).resolves.toEqual({ issuer: "https://as.example.com", }); @@ -385,6 +404,80 @@ describe("withOAuthRequestTimeout", () => { }); }); + describe("exemptMcpEndpoint (the transport chain's mixed traffic)", () => { + const SERVER = "https://srv.example.com/mcp"; + + it("exempts the MCP endpoint itself, whatever method or query", () => { + const isExempt = exemptMcpEndpoint(() => SERVER); + + expect(isExempt(SERVER)).toBe(true); + expect(isExempt(`${SERVER}?sessionId=abc`)).toBe(true); + expect(isExempt(`${SERVER}#frag`)).toBe(true); + }); + + it("bounds OAuth work on the same chain", () => { + 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); + }); + + 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 + // SDK's own per-request timeout as the backstop it already was. + expect( + exemptMcpEndpoint(() => undefined)("https://as.example.com/token"), + ).toBe(true); + expect(exemptMcpEndpoint(() => "")("https://as.example.com/token")).toBe( + true, + ); + expect(exemptMcpEndpoint(() => "not a url")(SERVER)).toBe(true); + expect(exemptMcpEndpoint(() => SERVER)("not a url")).toBe(true); + }); + + it("reads the server URL per call, since it changes between connects", () => { + let current = SERVER; + const isExempt = exemptMcpEndpoint(() => current); + + expect(isExempt("https://other.example.com/mcp")).toBe(false); + current = "https://other.example.com/mcp"; + expect(isExempt("https://other.example.com/mcp")).toBe(true); + }); + }); + + it("passes an exempt request through with no deadline and no buffering", async () => { + vi.useFakeTimers(); + // Untouched means untouched: no signal of ours, and the body is left as a + // live stream rather than buffered — which is what an SSE response needs. + const body = new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode("data: hi\n\n")); + }, + }); + const original = new Response(body, { + headers: { "content-type": "text/event-stream" }, + }); + const inner = vi.fn().mockResolvedValue(original); + const wrapped = withOAuthRequestTimeout(inner, 1000, () => true); + + const response = await wrapped(URL_UNDER_TEST); + + expect(response).toBe(original); + expect( + (inner.mock.calls[0][1] as RequestInit | undefined)?.signal, + ).toBeUndefined(); + // Nothing is armed, so advancing past the budget cannot sever it. + await vi.advanceTimersByTimeAsync(5000); + expect(response.bodyUsed).toBe(false); + }); + describe("composed under withRfc8414OidcCompat (#2319 ordering)", () => { // `withRfc8414OidcCompat` re-fetches an OIDC candidate after a failed RFC // 8414 discovery, so where the deadline sits relative to it decides which diff --git a/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts b/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts index f5be8fb0..0eaa397b 100644 --- a/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts +++ b/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts @@ -206,25 +206,32 @@ describe("createRemoteFetch", () => { it("forwards a Request's own signal when init has none", async () => { const { fetchFn, remoteFetch } = capture(); const caller = new AbortController(); + // Compared against `request.signal`, not `caller.signal`: the Fetch + // standard gives `new Request(url, { signal })` a *dependent* signal, so + // the two are distinct objects in a spec-compliant runtime. Asserting + // against the controller's would encode non-standard identity and could + // pass or fail on the host rather than on the behaviour (Copilot). + const request = new Request("http://upstream.example/", { + signal: caller.signal, + }); - await remoteFetch( - new Request("http://upstream.example/", { signal: caller.signal }), - ); + await remoteFetch(request); - expect(signalOf(fetchFn)).toBe(caller.signal); + expect(signalOf(fetchFn)).toBe(request.signal); }); it("treats an undefined init.signal as absent, keeping the Request's", async () => { // WebIDL dictionary conversion: a member present as `undefined` is absent. const { fetchFn, remoteFetch } = capture(); const caller = new AbortController(); + const request = new Request("http://upstream.example/", { + signal: caller.signal, + }); - await remoteFetch( - new Request("http://upstream.example/", { signal: caller.signal }), - { signal: undefined }, - ); + await remoteFetch(request, { signal: undefined }); - expect(signalOf(fetchFn)).toBe(caller.signal); + // Against `request.signal` for the dependent-signal reason above. + expect(signalOf(fetchFn)).toBe(request.signal); }); it("honours an explicit null init.signal as no signal", async () => { diff --git a/core/auth/index.ts b/core/auth/index.ts index 4cfde438..e38812eb 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -56,6 +56,7 @@ export { DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, OAUTH_TIMEOUT_WIRE_CODE, OAuthRequestTimeoutError, + exemptMcpEndpoint, isOAuthRequestTimeoutWire, withOAuthRequestTimeout, } from "./requestTimeout.js"; diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 65372ddc..d58d06e9 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -117,6 +117,37 @@ export function isOAuthRequestTimeoutWire( ); } +/** + * Build the `isExempt` predicate for a chain that carries MCP traffic as well as + * OAuth work: it returns true for the MCP endpoint itself. + * + * ⚠️ **Fails open, deliberately.** The two errors are not symmetric. Bounding a + * request that should not be bounded severs a long-running tool call or an SSE + * stream — a severe, user-visible regression. Failing to bound one leaves the + * SDK's own per-request timeout as the backstop it has always been. So anything + * 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. + */ +export function exemptMcpEndpoint( + getServerUrl: () => string | undefined, +): (url: string) => boolean { + return (url) => { + 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; + } catch { + return true; + } + }; +} + /** The request URL, whatever form `fetch`'s first argument took. */ function requestUrlOf(input: RequestInfo | URL): string { if (typeof input === "string") return input; @@ -208,6 +239,17 @@ function rebuildResponse(response: Response, body: ArrayBuffer): Response { * `buildEffectiveAuthFetch` rather than wrapping `this.fetchFn`, which the * transport is handed directly. * + * `isExempt` lets one wrapped fetch serve a mixed chain. It is how the + * *transport* fetch can be bounded at all: that chain carries both the SDK's + * OAuth work and the MCP traffic itself, and the MCP traffic must not be + * timed — a Streamable HTTP POST for a long-running tool call legitimately + * withholds its response headers for minutes, and an SSE body stays open + * indefinitely. So `InspectorClient` exempts the MCP endpoint by URL and bounds + * everything else on that chain, and **fails open**: if the server URL cannot + * be determined or parsed, everything is exempt. Bounding a request that should + * not be bounded breaks a working tool call; failing to bound one leaves the + * SDK's own per-request timeout as the backstop it already was. + * * A caller's own signal is preserved — whether it arrived in `init` or embedded * in a `Request` — and is both forwarded to the inner fetch (composed with * `AbortSignal.any`) and raced here, so an outer cancellation still wins even on @@ -220,6 +262,7 @@ function rebuildResponse(response: Response, body: ArrayBuffer): Response { export function withOAuthRequestTimeout( fetchFn: typeof fetch, timeoutMs: number = DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + isExempt?: (url: string) => boolean, ): typeof fetch { // Whole milliseconds. Not for the reason `revocation.ts:253` gives — that one // calls `AbortSignal.timeout`, which really does throw `ERR_OUT_OF_RANGE` on @@ -239,6 +282,11 @@ export function withOAuthRequestTimeout( return async (input, init) => { const url = requestUrlOf(input); + // An exempt request is passed through completely untouched — no deadline, + // no signal of ours, and crucially no body buffering, since the responses + // this exists for are streams that must stay open (see `isExempt` above). + if (isExempt?.(url)) return fetchFn(input, init); + const controller = new AbortController(); const callerSignal = callerSignalOf(input, init); // Before anything is constructed or sent. Racing an already-aborted signal diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 7ccbefa3..31d6b311 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -256,7 +256,11 @@ import { } from "../auth/challenge.js"; import { withOAuthEndpointOverrides } from "../auth/endpointOverrides.js"; import { withRfc8414OidcCompat } from "../auth/oidcDiscoveryCompat.js"; -import { withOAuthRequestTimeout } from "../auth/requestTimeout.js"; +import { + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + exemptMcpEndpoint, + withOAuthRequestTimeout, +} from "../auth/requestTimeout.js"; import type { TokenRevocationOutcome } from "../auth/revocation.js"; import type { OAuthTokens } from "@modelcontextprotocol/client"; import { silentLogger, type InspectorLogger } from "../logging/logger.js"; @@ -824,7 +828,21 @@ export class InspectorClient extends InspectorClientEventTarget { // still hit the upstream failure. The substituted response is stamped with // `COMPAT_SOURCE_HEADER` so a captured entry names the URL its body came // from rather than appearing to be a 200 from the RFC 8414 path. - this.fetchFn = withRfc8414OidcCompat(withOverrides); + // The transport chain is bounded too, but only for the requests on it that + // are *not* MCP traffic — the OAuth work the SDK runs from inside the + // transport on the 401/refresh path, which otherwise waits out the SDK's + // incidental per-request timeout and reports a bare `Request timed out` + // naming no endpoint (Copilot). The MCP endpoint itself is exempt, and the + // predicate fails open: a long-running tool call withholding its response + // headers for minutes, or an SSE body that stays open, must never be timed + // here. Deadline innermost, for the same reason as the auth chain below. + this.fetchFn = withRfc8414OidcCompat( + withOAuthRequestTimeout( + withOverrides, + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + exemptMcpEndpoint(() => this.getServerUrl()), + ), + ); // #2319: the auth chain is composed separately so the deadline sits // *inside* the compat wrapper. Reusing `this.fetchFn` as the base would put // it outside, and then a stalled OIDC probe would be reported under the