From 8dd7bc2920f7833e3386f471773f7e65cfd86c12 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 00:45:04 -0400 Subject: [PATCH 01/23] feat: bound every OAuth-path request with a deadline Protected-resource-metadata discovery, authorization-server metadata discovery, dynamic client registration, the token exchange and the refresh all went out with no AbortSignal at all. Token revocation was the single exception, and its withDeadline race is the pattern this generalizes. Adds core/auth/requestTimeout.ts: withOAuthRequestTimeout wraps a fetch so every call through it is bounded, composing any caller signal with AbortSignal.any and racing a timer so the deadline holds even where the signal is dropped in flight (the browser's remote fetch re-issues the call to /api/fetch and forwards no signal). The rejection is an OAuthRequestTimeoutError naming the URL, so a stall says which endpoint stalled rather than riding the SDK handshake timeout and reporting "Request timed out". Applied inside InspectorClient.buildEffectiveAuthFetch rather than to this.fetchFn, because the transport is handed this.fetchFn directly and a deadline on a Streamable HTTP / SSE response would sever the stream it is supposed to hold open. Also applied to the CLI's storedAuthFetch, which calls SDK discovery directly and so has nothing else bounding it. Closes #2319 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- clients/cli/src/cli.ts | 10 +- .../src/test/core/auth/requestTimeout.test.ts | 186 ++++++++++++++++++ core/auth/index.ts | 7 + core/auth/requestTimeout.ts | 150 ++++++++++++++ core/mcp/inspectorClient.ts | 11 +- 5 files changed, 362 insertions(+), 2 deletions(-) create mode 100644 clients/web/src/test/core/auth/requestTimeout.test.ts create mode 100644 core/auth/requestTimeout.ts diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 28ccd0dc..990267d0 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -53,6 +53,7 @@ import { getAuthorizationServerUrlCandidates, } from "@inspector/core/auth/discovery.js"; import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js"; +import { withOAuthRequestTimeout } from "@inspector/core/auth/requestTimeout.js"; import { writeStoreFile } from "@inspector/core/storage/store-io.js"; import { refreshAuthorization, @@ -353,7 +354,14 @@ export async function refreshStoredAuthToken( // else puts a proxy under it, and a server reachable only through // `HTTPS_PROXY` would otherwise be probed directly. The same fetch is handed // to the token request below, so neither leg bypasses the proxy (Copilot). - const storedAuthFetch = withRfc8414OidcCompat(createProxyFetch() ?? fetch); + // #2319: this path runs outside `InspectorClient`, so nothing else bounds it + // — the discovery and the token request below would otherwise hang forever + // against an authorization server that accepts the connection and never + // answers. Innermost, so the compat wrapper's own probe requests inherit the + // deadline too. + const storedAuthFetch = withRfc8414OidcCompat( + withOAuthRequestTimeout(createProxyFetch() ?? fetch), + ); const discover: typeof discoverAuthorizationServerMetadata = deps.discover ?? ((authorizationServerUrl, options) => diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts new file mode 100644 index 00000000..05c92029 --- /dev/null +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -0,0 +1,186 @@ +import { describe, it, expect, vi, afterEach } from "vitest"; +import { + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + OAuthRequestTimeoutError, + withOAuthRequestTimeout, +} from "@inspector/core/auth/requestTimeout.js"; + +const URL_UNDER_TEST = + "https://as.example.com/.well-known/oauth-authorization-server"; + +/** A fetch that never settles — the wedged authorization server of #2319. */ +const neverSettles: typeof fetch = () => new Promise(() => {}); + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +describe("withOAuthRequestTimeout", () => { + it("passes a prompt response straight through", async () => { + const ok = new Response("{}", { status: 200 }); + const inner = vi.fn().mockResolvedValue(ok); + const wrapped = withOAuthRequestTimeout(inner, 1000); + + await expect(wrapped(URL_UNDER_TEST)).resolves.toBe(ok); + expect(inner).toHaveBeenCalledTimes(1); + }); + + it("rejects a stalled request with an error naming the endpoint", async () => { + vi.useFakeTimers(); + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const pending = wrapped(URL_UNDER_TEST); + const assertion = expect(pending).rejects.toThrow( + `OAuth request to ${URL_UNDER_TEST} timed out after 1000ms`, + ); + await vi.advanceTimersByTimeAsync(1000); + await assertion; + }); + + it("reports the timeout as OAuthRequestTimeoutError carrying url and budget", async () => { + vi.useFakeTimers(); + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const pending = wrapped(new URL(URL_UNDER_TEST)); + const assertion = pending.catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + const err = await assertion; + + expect(err).toBeInstanceOf(OAuthRequestTimeoutError); + const timeout = err as OAuthRequestTimeoutError; + expect(timeout.name).toBe("OAuthRequestTimeoutError"); + expect(timeout.url).toBe(URL_UNDER_TEST); + expect(timeout.timeoutMs).toBe(1000); + }); + + it("takes the URL off a Request object too", async () => { + vi.useFakeTimers(); + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const pending = wrapped(new Request(URL_UNDER_TEST, { method: "POST" })); + const assertion = pending.catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + url: URL_UNDER_TEST, + }); + }); + + it("aborts the underlying fetch rather than leaving it running detached", async () => { + vi.useFakeTimers(); + let seen: AbortSignal | undefined; + const inner: typeof fetch = (_input, init) => { + seen = init?.signal ?? undefined; + return new Promise((_resolve, reject) => { + init?.signal?.addEventListener("abort", () => + reject(new Error("aborted by signal")), + ); + }); + }; + const wrapped = withOAuthRequestTimeout(inner, 1000); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect(seen?.aborted).toBe(true); + // The underlying rejection can win the race against our own; either way the + // caller must be told it was a timeout, and which endpoint stalled. + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + }); + + it("still enforces the deadline when the signal is dropped in flight", async () => { + vi.useFakeTimers(); + // The browser's `createRemoteFetch` re-issues the call to `/api/fetch` and + // forwards no signal, so aborting locally cancels nothing. The race is what + // has to bound it. + const signalIgnoring: typeof fetch = () => new Promise(() => {}); + const wrapped = withOAuthRequestTimeout(signalIgnoring, 1000); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + }); + + it("preserves a caller-supplied signal, which still wins on its own", async () => { + const inner: typeof fetch = (_input, init) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener("abort", () => + reject(new Error("caller cancelled")), + ); + }); + const wrapped = withOAuthRequestTimeout(inner, 60_000); + + const caller = new AbortController(); + const pending = wrapped(URL_UNDER_TEST, { signal: caller.signal }); + caller.abort(); + + await expect(pending).rejects.toThrow("caller cancelled"); + }); + + it("propagates a non-timeout failure unchanged", async () => { + const boom = new TypeError("network error"); + const inner = vi.fn().mockRejectedValue(boom); + const wrapped = withOAuthRequestTimeout(inner, 1000); + + await expect(wrapped(URL_UNDER_TEST)).rejects.toBe(boom); + }); + + it("rounds a fractional budget, because setTimeout takes an integer", async () => { + vi.useFakeTimers(); + // A budget derived from `performance.now()` arrives fractional; a fractional + // delay throws ERR_OUT_OF_RANGE before the request is ever made. + const wrapped = withOAuthRequestTimeout(neverSettles, 1000.4); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + timeoutMs: 1000, + }); + }); + + it("clamps a negative budget to zero rather than throwing", async () => { + vi.useFakeTimers(); + const wrapped = withOAuthRequestTimeout(neverSettles, -1); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(0); + + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + timeoutMs: 0, + }); + }); + + it("defaults to a generous budget a slow authorization server can meet", async () => { + vi.useFakeTimers(); + const wrapped = withOAuthRequestTimeout(neverSettles); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(DEFAULT_OAUTH_REQUEST_TIMEOUT_MS - 1); + expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe( + "pending", + ); + + await vi.advanceTimersByTimeAsync(1); + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + timeoutMs: DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + }); + }); + + it("clears its timer once the request settles", async () => { + vi.useFakeTimers(); + const clearSpy = vi.spyOn(globalThis, "clearTimeout"); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(new Response(null)), + 1000, + ); + + await wrapped(URL_UNDER_TEST); + + expect(clearSpy).toHaveBeenCalled(); + // Nothing is left to fire, so no unhandled rejection can surface later. + await vi.advanceTimersByTimeAsync(5000); + }); +}); diff --git a/core/auth/index.ts b/core/auth/index.ts index c74702ce..6dd49b3b 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -51,6 +51,13 @@ export { withRfc8414OidcCompat, } from "./oidcDiscoveryCompat.js"; +// OAuth-path request deadline (#2319) +export { + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + OAuthRequestTimeoutError, + withOAuthRequestTimeout, +} from "./requestTimeout.js"; + // Storage export type { OAuthStorage, diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts new file mode 100644 index 00000000..e89aa261 --- /dev/null +++ b/core/auth/requestTimeout.ts @@ -0,0 +1,150 @@ +/** + * A bound on every network call the Inspector makes on the OAuth path (#2319). + * + * ## Why this exists + * + * Protected-resource-metadata discovery, authorization-server metadata + * discovery, dynamic client registration, the token exchange and the refresh + * all went out with no `AbortSignal` at all. Token revocation was the single + * exception — `revocation.ts` has carried its own deadline since #2144, and it + * is the pattern this module generalizes. + * + * An unbounded fetch matters more here than it usually would, because of where + * this work runs and what the UI does while it runs: + * + * - It runs **server-side**, in the Inspector's own Node process, proxied + * through the backend. A stall is therefore invisible in browser devtools — + * #2188's reporter correctly observed that no request left the browser at + * all while a Node-side socket sat established and idle. + * - Connect-time auth errors deliberately hold the connection status at + * `"connecting"` rather than moving it to `"error"` + * (`isConnectAuthRecoveryError`, `challenge.ts`), on the theory that a + * redirect is about to end the attempt. A stalled fetch inside that window is + * a silent, unbounded spinner rather than a surfaced failure. + * + * Today the common case is bounded, but only incidentally: a discovery stall + * that happens to sit inside an SDK `initialize` rides that request's own + * timeout and surfaces after 60s as `Request timed out`. This module makes the + * bound deliberate, applies it to the stalls that sit *outside* an SDK request + * too, and — the point of naming the URL in the error — says *which* endpoint + * stalled instead of blaming the handshake. + * + * ## Why a race, and not just the signal + * + * The signal alone cannot enforce this. In the browser the OAuth fetch is + * `createRemoteFetch`, which re-issues the call as a POST to `/api/fetch` and + * does not forward `init.signal`; the backend's outbound fetch gets no signal + * either. So on exactly the path this bound exists for — a browser session + * against a wedged authorization server — aborting the local promise would + * change nothing. The race is what actually enforces the deadline; the signal + * is kept because it does cancel the direct-fetch paths (CLI, TUI, backend) + * rather than merely abandoning them. Same reasoning, same shape, as + * `revokeToken`. + */ + +/** + * 30 seconds. Deliberately generous: discovery against a slow or cold-starting + * authorization server is legitimate, and a bound that fires on a server that + * was going to answer is worse than the unbounded wait it replaced. It still + * halves the 60s the SDK handshake incidentally provides, and it is the only + * bound at all on the legs that sit outside an SDK request. + */ +export const DEFAULT_OAUTH_REQUEST_TIMEOUT_MS = 30_000; + +/** + * Raised when an OAuth-path request outlives its budget. + * + * Carries the URL because "discovery timed out" without one is only marginally + * better than a spinner: the OAuth path makes several requests to several + * hosts, and which of them stalled is the whole diagnostic. + */ +export class OAuthRequestTimeoutError extends Error { + readonly url: string; + readonly timeoutMs: number; + + constructor(url: string, timeoutMs: number) { + super(`OAuth request to ${url} timed out after ${timeoutMs}ms`); + this.name = "OAuthRequestTimeoutError"; + this.url = url; + this.timeoutMs = timeoutMs; + } +} + +/** The request URL, whatever form `fetch`'s first argument took. */ +function requestUrlOf(input: RequestInfo | URL): string { + if (typeof input === "string") return input; + if (input instanceof URL) return input.href; + return input.url; +} + +/** + * Wrap a `fetch` so every call through it is bounded by `timeoutMs`. + * + * Apply this to an **OAuth-path** fetch only. It must never wrap the transport + * fetch: a Streamable HTTP or SSE response is a long-lived stream that is + * *supposed* to stay open, and a deadline there would sever every connection + * after the budget. `InspectorClient` keeps the two apart by wrapping inside + * `buildEffectiveAuthFetch` rather than wrapping `this.fetchFn`, which the + * transport is handed directly. + * + * A caller-supplied `init.signal` is preserved — the two are composed with + * `AbortSignal.any`, so an outer cancellation still wins and this wrapper only + * ever *adds* a reason to give up. + */ +export function withOAuthRequestTimeout( + fetchFn: typeof fetch, + timeoutMs: number = DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, +): typeof fetch { + // Whole milliseconds, because `setTimeout` and `AbortSignal.timeout` both + // take an integer: Node throws `ERR_OUT_OF_RANGE` on a fractional delay — + // before the fetch, so the request is never sent and the caller sees a failed + // operation rather than a timeout. A budget derived from `performance.now()` + // is fractional, so this is reachable. `revocation.ts:253` carries the same + // note for the same reason. + const budget = Math.max(0, Math.round(timeoutMs)); + + return async (input, init) => { + const url = requestUrlOf(input); + const controller = new AbortController(); + // `init.signal` is `AbortSignal | null | undefined`; only an actual signal + // is worth composing. + const callerSignal = init?.signal ?? undefined; + const signal = callerSignal + ? AbortSignal.any([controller.signal, callerSignal]) + : controller.signal; + + let timer: ReturnType | undefined; + let timedOut = false; + const deadline = new Promise((_resolve, reject) => { + timer = setTimeout(() => { + timedOut = true; + // Abort first so a direct fetch is actually cancelled rather than left + // running detached, then reject so the proxied fetch — which never saw + // the signal — is abandoned on schedule too. + controller.abort(new OAuthRequestTimeoutError(url, budget)); + reject(new OAuthRequestTimeoutError(url, budget)); + }, budget); + }); + + try { + return await Promise.race([ + fetchFn(input, { ...init, signal }), + deadline, + ]); + } catch (err) { + // `controller.abort()` above can reject the underlying fetch *before* + // `reject` runs, in which case the race settles with undici's + // `AbortError` instead of ours. Which of the two wins is an ordering + // detail of the fetch implementation, so normalize on the flag rather + // than on the error that surfaced: a timeout must always be reported as + // one, naming the endpoint. + if (timedOut) throw new OAuthRequestTimeoutError(url, budget); + throw err; + } finally { + /* v8 ignore next -- the Promise executor runs synchronously, so `timer` + is always assigned by the time this runs; the guard exists only because + TypeScript cannot see that. */ + if (timer !== undefined) clearTimeout(timer); + } + }; +} diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index cec72b49..180243ad 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -256,6 +256,7 @@ import { } from "../auth/challenge.js"; import { withOAuthEndpointOverrides } from "../auth/endpointOverrides.js"; import { withRfc8414OidcCompat } from "../auth/oidcDiscoveryCompat.js"; +import { 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"; @@ -991,7 +992,15 @@ export class InspectorClient extends InspectorClientEventTarget { } private buildEffectiveAuthFetch(): typeof fetch { - const base = this.fetchFn ?? fetch; + // #2319: bound every OAuth-path request — discovery, dynamic client + // registration, the token exchange, the refresh. Applied here rather than + // to `this.fetchFn` because the transport is handed `this.fetchFn` + // directly, and a deadline on a Streamable HTTP / SSE response would sever + // the very stream it is supposed to hold open. The cost of that placement + // is that the discovery the SDK runs from *inside* the transport (the + // 401/refresh path) is not covered here; that leg sits inside an SDK + // request and is bounded by its per-request timeout. + const base = withOAuthRequestTimeout(this.fetchFn ?? fetch); // Capture auth response bodies (OAuth discovery, DCR, token exchange) so // they're inspectable in the Network tab. Token-exchange responses carry // `access_token` / `refresh_token`; the Network UI masks those (and other From e89ea4cf19bb15d1ca71ac6f4dfab8ebd7ed5392 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 02:24:30 -0400 Subject: [PATCH 02/23] fix: address Copilot review round 1 on #2319 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all real: - The deadline stopped at the response headers. `fetch` resolves once headers arrive, so a server that sent headers — or half a JSON document — and then stalled left the caller's `response.json()` hanging with nothing watching it. The body is now buffered under the same race and the response rebuilt around it, preserving `url`, `redirected` and `type`, which have no constructor option, and passing `null` for a null-body status. - The caller's signal was only forwarded, not raced. On the browser's `createRemoteFetch` path the signal is dropped in flight, so an abort left the promise pending for the whole budget rather than the outer cancellation winning as documented. It is raced now, with the caller's own `reason` preserved, and the listener removed in `finally` since one caller signal outlives several requests. - A `Request` carries its signal on the input, not in `init`, so always passing our own `signal` in `init` silently overrode it. The caller signal is now derived from the `Request` when `init` has no `signal` key — and an explicit `null` there still wins, per the fetch spec. Eight new tests cover each: the stalled body, caller cancellation against a signal-ignoring fetch, an already-aborted signal, the Request-embedded signal, the explicit-null override, listener removal, the rebuilt response's metadata and a null-body status. 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 | 151 +++++++++++++++++- core/auth/requestTimeout.ts | 125 +++++++++++++-- 2 files changed, 260 insertions(+), 16 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 05c92029..5100083b 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -17,12 +17,18 @@ afterEach(() => { }); describe("withOAuthRequestTimeout", () => { - it("passes a prompt response straight through", async () => { - const ok = new Response("{}", { status: 200 }); - const inner = vi.fn().mockResolvedValue(ok); + it("passes a prompt response through", async () => { + // Not the same object: the body is buffered under the deadline (see the + // stalled-body case below) and the response rebuilt around it. + const inner = vi + .fn() + .mockResolvedValue(new Response('{"ok":true}', { status: 200 })); const wrapped = withOAuthRequestTimeout(inner, 1000); - await expect(wrapped(URL_UNDER_TEST)).resolves.toBe(ok); + const response = await wrapped(URL_UNDER_TEST); + + expect(response.status).toBe(200); + await expect(response.json()).resolves.toEqual({ ok: true }); expect(inner).toHaveBeenCalledTimes(1); }); @@ -103,7 +109,7 @@ describe("withOAuthRequestTimeout", () => { expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); }); - it("preserves a caller-supplied signal, which still wins on its own", async () => { + it("forwards a caller-supplied signal to the inner fetch", async () => { const inner: typeof fetch = (_input, init) => new Promise((_resolve, reject) => { init?.signal?.addEventListener("abort", () => @@ -119,6 +125,139 @@ describe("withOAuthRequestTimeout", () => { await expect(pending).rejects.toThrow("caller cancelled"); }); + it("races caller cancellation too, so it wins on a signal-ignoring fetch", async () => { + // Forwarding alone is not enough on the browser's `createRemoteFetch` path, + // which drops the signal: without the race the caller's abort would sit + // pending for the whole budget instead of winning. + const wrapped = withOAuthRequestTimeout(neverSettles, 60_000); + + const caller = new AbortController(); + const pending = wrapped(URL_UNDER_TEST, { signal: caller.signal }); + const reason = new Error("caller gave up"); + caller.abort(reason); + + await expect(pending).rejects.toBe(reason); + }); + + it("rejects immediately when the caller's signal is already aborted", async () => { + const wrapped = withOAuthRequestTimeout(neverSettles, 60_000); + const reason = new Error("already gone"); + + await expect( + wrapped(URL_UNDER_TEST, { signal: AbortSignal.abort(reason) }), + ).rejects.toBe(reason); + }); + + it("honours the signal embedded in a Request input", async () => { + // `init.signal` is absent here, so the Request's own signal is the caller's + // — reading `init` alone would silently override it. + const wrapped = withOAuthRequestTimeout(neverSettles, 60_000); + + const caller = new AbortController(); + const pending = wrapped( + new Request(URL_UNDER_TEST, { signal: caller.signal }), + ); + const reason = new Error("request cancelled"); + caller.abort(reason); + + await expect(pending).rejects.toBe(reason); + }); + + it("lets an explicit null init.signal override a Request's own", async () => { + vi.useFakeTimers(); + // Per the fetch spec a present `signal` key wins, `null` included. + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const caller = new AbortController(); + const pending = wrapped( + new Request(URL_UNDER_TEST, { signal: caller.signal }), + { signal: null }, + ); + const assertion = pending.catch((err: unknown) => err); + caller.abort(new Error("ignored")); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + }); + + it("removes its abort listener once the request settles", async () => { + const caller = new AbortController(); + const removeSpy = vi.spyOn(caller.signal, "removeEventListener"); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(new Response("{}")), + 60_000, + ); + + await wrapped(URL_UNDER_TEST, { signal: caller.signal }); + + // A caller signal outlives one request — one connect attempt makes several + // — so a listener left behind per request would accumulate on it. + expect(removeSpy).toHaveBeenCalledWith("abort", expect.any(Function)); + }); + + it("bounds a stalled response body, not just the headers", async () => { + vi.useFakeTimers(); + // `fetch` resolves on headers. A server that sends them and then stalls + // would leave the caller's `response.json()` hanging unwatched. + const stalledBody: typeof fetch = () => + Promise.resolve( + new Response( + new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode('{"issuer":')); + }, + }), + { headers: { "content-type": "application/json" } }, + ), + ); + const wrapped = withOAuthRequestTimeout(stalledBody, 1000); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + }); + + it("returns a readable response whose metadata survives the rebuild", async () => { + const original = new Response('{"issuer":"https://as.example.com"}', { + status: 201, + statusText: "Created", + headers: { "content-type": "application/json", "x-probe": "kept" }, + }); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(original), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + + expect(response.status).toBe(201); + expect(response.statusText).toBe("Created"); + expect(response.ok).toBe(true); + expect(response.headers.get("x-probe")).toBe("kept"); + expect(response.url).toBe(original.url); + expect(response.redirected).toBe(original.redirected); + expect(response.type).toBe(original.type); + await expect(response.json()).resolves.toEqual({ + issuer: "https://as.example.com", + }); + }); + + it("rebuilds a null-body status without handing it a body", async () => { + // `new Response(body, { status: 204 })` throws; the rebuild has to pass null. + const wrapped = withOAuthRequestTimeout( + vi + .fn() + .mockResolvedValue(new Response(null, { status: 204 })), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + + expect(response.status).toBe(204); + expect(response.body).toBeNull(); + }); + it("propagates a non-timeout failure unchanged", async () => { const boom = new TypeError("network error"); const inner = vi.fn().mockRejectedValue(boom); @@ -173,7 +312,7 @@ describe("withOAuthRequestTimeout", () => { vi.useFakeTimers(); const clearSpy = vi.spyOn(globalThis, "clearTimeout"); const wrapped = withOAuthRequestTimeout( - vi.fn().mockResolvedValue(new Response(null)), + vi.fn().mockResolvedValue(new Response("{}")), 1000, ); diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index e89aa261..8fae3dcb 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -22,6 +22,13 @@ * redirect is about to end the attempt. A stalled fetch inside that window is * a silent, unbounded spinner rather than a surfaced failure. * + * The deadline covers the **whole exchange**, not just the headers: `fetch` + * resolves as soon as response headers arrive, so a server that sends headers + * — or half a JSON document — and then stalls would leave the caller's + * `response.json()` hanging with nothing watching it. Every response on this + * path is a small finite document, so the body is buffered under the same race + * and the response rebuilt around it. + * * Today the common case is bounded, but only incidentally: a discovery stall * that happens to sit inside an SDK `initialize` rides that request's own * timeout and surfaces after 60s as `Request timed out`. This module makes the @@ -77,6 +84,63 @@ function requestUrlOf(input: RequestInfo | URL): string { return input.url; } +/** + * The caller's own `AbortSignal`, if it supplied one. + * + * A `Request` carries its signal on the *input*, not in `init`, and this + * wrapper always passes a `signal` of its own in `init` — so reading `init` + * alone would silently override the embedded one and change ordinary `fetch` + * cancellation semantics for `withOAuthRequestTimeout(new Request(url, { signal }))` + * (Copilot). + * + * `init.signal` still wins whenever the key is *present*, per the fetch spec: + * an explicit `null` there means "no signal" even when `input` is a `Request` + * that has one. + */ +function callerSignalOf( + input: RequestInfo | URL, + init: RequestInit | undefined, +): AbortSignal | undefined { + if (init && "signal" in init) return init.signal ?? undefined; + if (typeof input !== "string" && !(input instanceof URL)) return input.signal; + return undefined; +} + +/** + * Statuses the Fetch Standard defines as null-body. Handing the `Response` + * constructor a body with one of these throws a `TypeError`, so the rebuilt + * response below must pass `null` instead. + */ +const NULL_BODY_STATUSES = new Set([101, 103, 204, 205, 304]); + +/** + * Rebuild a `Response` around an already-buffered body. + * + * `url`, `redirected` and `type` are getters with no constructor option, and + * callers do read them — the RFC 8414/OIDC compat wrapper stamps a header + * naming the URL a body came from, and the SDK's discovery reports the + * responding URL in its errors. So they are copied across explicitly rather + * than silently reset to `""` / `false` / `"default"`. + */ +function rebuildResponse(response: Response, body: ArrayBuffer): Response { + const rebuilt = new Response( + NULL_BODY_STATUSES.has(response.status) ? null : body, + { + status: response.status, + statusText: response.statusText, + headers: response.headers, + }, + ); + for (const key of ["url", "redirected", "type"] as const) { + Object.defineProperty(rebuilt, key, { + value: response[key], + enumerable: true, + configurable: true, + }); + } + return rebuilt; +} + /** * Wrap a `fetch` so every call through it is bounded by `timeoutMs`. * @@ -87,9 +151,14 @@ function requestUrlOf(input: RequestInfo | URL): string { * `buildEffectiveAuthFetch` rather than wrapping `this.fetchFn`, which the * transport is handed directly. * - * A caller-supplied `init.signal` is preserved — the two are composed with - * `AbortSignal.any`, so an outer cancellation still wins and this wrapper only - * ever *adds* a reason to give up. + * 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 + * a fetch that drops the signal. This wrapper only ever *adds* a reason to give + * up; it never removes the caller's. + * + * The response **body** is buffered under the same deadline, because `fetch` + * resolves on headers and a stalled body would otherwise be unbounded. */ export function withOAuthRequestTimeout( fetchFn: typeof fetch, @@ -106,16 +175,18 @@ export function withOAuthRequestTimeout( return async (input, init) => { const url = requestUrlOf(input); const controller = new AbortController(); - // `init.signal` is `AbortSignal | null | undefined`; only an actual signal - // is worth composing. - const callerSignal = init?.signal ?? undefined; + const callerSignal = callerSignalOf(input, init); const signal = callerSignal ? AbortSignal.any([controller.signal, callerSignal]) : controller.signal; let timer: ReturnType | undefined; let timedOut = false; - const deadline = new Promise((_resolve, reject) => { + let onCallerAbort: (() => void) | undefined; + + // The losing side of every race below: it rejects when the budget runs out, + // and when the caller cancels. + const abandoned = new Promise((_resolve, reject) => { timer = setTimeout(() => { timedOut = true; // Abort first so a direct fetch is actually cancelled rather than left @@ -124,20 +195,48 @@ export function withOAuthRequestTimeout( controller.abort(new OAuthRequestTimeoutError(url, budget)); reject(new OAuthRequestTimeoutError(url, budget)); }, budget); + + // The caller's cancellation is raced too, not merely forwarded. Forwarding + // it is enough on a direct fetch, but on the browser's `createRemoteFetch` + // path — the one this wrapper exists for — the signal is dropped in + // flight, so an abort would otherwise leave this promise pending for the + // whole budget instead of the outer cancellation winning as documented + // (Copilot). The caller's own `reason` is preserved, so it still sees its + // abort rather than a substituted error. + if (callerSignal) { + if (callerSignal.aborted) { + reject(callerSignal.reason); + } else { + onCallerAbort = () => reject(callerSignal.reason); + callerSignal.addEventListener("abort", onCallerAbort, { once: true }); + } + } }); try { - return await Promise.race([ + const response = await Promise.race([ fetchFn(input, { ...init, signal }), - deadline, + abandoned, ]); + // `fetch` resolves once the response *headers* arrive, so stopping here + // would leave the body unbounded: an authorization server can send + // headers, or half a JSON document, and then stall, and the caller's + // `response.json()` would hang with nothing watching it (Copilot). Every + // response on this path is a small, finite document — metadata, + // a registration, a token — so buffering it under the same deadline + // bounds the whole exchange. This is the other reason the wrapper must + // never be applied to the transport fetch, whose bodies are streams that + // are supposed to stay open. + const body = await Promise.race([response.arrayBuffer(), abandoned]); + return rebuildResponse(response, body); } catch (err) { // `controller.abort()` above can reject the underlying fetch *before* // `reject` runs, in which case the race settles with undici's // `AbortError` instead of ours. Which of the two wins is an ordering // detail of the fetch implementation, so normalize on the flag rather // than on the error that surfaced: a timeout must always be reported as - // one, naming the endpoint. + // one, naming the endpoint. A caller-driven abort leaves the flag false + // and so passes through with its own reason intact. if (timedOut) throw new OAuthRequestTimeoutError(url, budget); throw err; } finally { @@ -145,6 +244,12 @@ export function withOAuthRequestTimeout( is always assigned by the time this runs; the guard exists only because TypeScript cannot see that. */ if (timer !== undefined) clearTimeout(timer); + // Drop the listener even though it is `once`: a caller signal can outlive + // this request (one `AbortController` per connect attempt, several + // requests through it), and a listener per request would accumulate on it. + if (onCallerAbort && callerSignal) { + callerSignal.removeEventListener("abort", onCallerAbort); + } } }; } From 2710c98681ae05483eddd09dd684a03178552408 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 02:44:43 -0400 Subject: [PATCH 03/23] fix: address Copilot review round 2 on #2319 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings are on the caller-signal handling added in round 1: - `"signal" in init` treated a member present as `undefined` as an explicit override. `RequestInit` is a WebIDL dictionary, where such a member converts as absent — so `{ ...base, signal: undefined }`, which is what a spread over an options object with no signal produces, was clearing a Request's embedded signal instead of inheriting it. Only a non-`undefined` value overrides now; an explicit `null` is still the genuine no-signal case. - An already-aborted caller signal rejected the race, but `Promise.race` still evaluated `fetchFn(...)`, so on the signal-dropping proxy path the OAuth request went out anyway. It now short-circuits before anything is constructed or sent, and the now-unreachable aborted branch inside the race is gone. 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 | 25 +++++++++++++++-- core/auth/requestTimeout.ts | 27 ++++++++++++------- 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 5100083b..190a2640 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -139,13 +139,34 @@ describe("withOAuthRequestTimeout", () => { await expect(pending).rejects.toBe(reason); }); - it("rejects immediately when the caller's signal is already aborted", async () => { - const wrapped = withOAuthRequestTimeout(neverSettles, 60_000); + it("rejects an already-aborted signal without sending the request", async () => { + const inner = vi.fn(neverSettles); + const wrapped = withOAuthRequestTimeout(inner, 60_000); const reason = new Error("already gone"); await expect( wrapped(URL_UNDER_TEST, { signal: AbortSignal.abort(reason) }), ).rejects.toBe(reason); + // Racing it would still have evaluated the inner fetch, which on the + // signal-dropping proxy path means the request actually goes out. + expect(inner).not.toHaveBeenCalled(); + }); + + it("treats an undefined init.signal as absent, keeping a Request's own", async () => { + // `RequestInit` is a WebIDL dictionary: a member present as `undefined` is + // converted as absent. `{ ...base, signal: undefined }` is what a spread + // over an options object with no signal produces. + const wrapped = withOAuthRequestTimeout(neverSettles, 60_000); + + const caller = new AbortController(); + const pending = wrapped( + new Request(URL_UNDER_TEST, { signal: caller.signal }), + { signal: undefined }, + ); + const reason = new Error("request cancelled"); + caller.abort(reason); + + await expect(pending).rejects.toBe(reason); }); it("honours the signal embedded in a Request input", async () => { diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 8fae3dcb..80cb5aba 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -93,15 +93,19 @@ function requestUrlOf(input: RequestInfo | URL): string { * cancellation semantics for `withOAuthRequestTimeout(new Request(url, { signal }))` * (Copilot). * - * `init.signal` still wins whenever the key is *present*, per the fetch spec: - * an explicit `null` there means "no signal" even when `input` is a `Request` - * that has one. + * An `init.signal` still wins, but only when it is not `undefined`. `RequestInit` + * is a WebIDL dictionary, where a member present with the value `undefined` is + * converted as *absent* — so `{ ...base, signal: undefined }`, which is what a + * spread over an options object that had no signal produces, must inherit the + * `Request`'s signal rather than clear it (Copilot). An explicit `null` is the + * genuine "no signal" override and is honoured as one. */ function callerSignalOf( input: RequestInfo | URL, init: RequestInit | undefined, ): AbortSignal | undefined { - if (init && "signal" in init) return init.signal ?? undefined; + const explicit = init?.signal; + if (explicit !== undefined) return explicit ?? undefined; if (typeof input !== "string" && !(input instanceof URL)) return input.signal; return undefined; } @@ -176,6 +180,12 @@ export function withOAuthRequestTimeout( const url = requestUrlOf(input); const controller = new AbortController(); const callerSignal = callerSignalOf(input, init); + // Before anything is constructed or sent. Racing an already-aborted signal + // would still evaluate `fetchFn(...)`, and on the signal-dropping + // `createRemoteFetch` path that means the OAuth request goes out even + // though the caller cancelled before the call (Copilot). + if (callerSignal?.aborted) throw callerSignal.reason; + const signal = callerSignal ? AbortSignal.any([controller.signal, callerSignal]) : controller.signal; @@ -203,13 +213,10 @@ export function withOAuthRequestTimeout( // whole budget instead of the outer cancellation winning as documented // (Copilot). The caller's own `reason` is preserved, so it still sees its // abort rather than a substituted error. + // Not aborted — the short-circuit above returned for that case. if (callerSignal) { - if (callerSignal.aborted) { - reject(callerSignal.reason); - } else { - onCallerAbort = () => reject(callerSignal.reason); - callerSignal.addEventListener("abort", onCallerAbort, { once: true }); - } + onCallerAbort = () => reject(callerSignal.reason); + callerSignal.addEventListener("abort", onCallerAbort, { once: true }); } }); From 42139c2d230f8ab77dbabc6d2b1a1d1555828204 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 03:02:37 -0400 Subject: [PATCH 04/23] fix: put the OAuth deadline inside the compat wrapper (#2319 round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildEffectiveAuthFetch` took `this.fetchFn` as its base, which already carried `withRfc8414OidcCompat` — so the deadline sat outside it. A stalled OIDC probe was then timed on whatever budget the preceding RFC 8414 request had left, and reported under that request's URL rather than the probe's, defeating the endpoint-level diagnostic this change exists to add. The auth chain is now composed separately, deadline innermost, matching the CLI's storedAuthFetch: withRfc8414OidcCompat(withOAuthRequestTimeout(withOverrides)) `this.fetchFn` keeps the transport chain, unbounded, so an SSE or Streamable HTTP body is still free to stay open. Two tests pin the ordering by contrast: outside, a stalled probe is misreported under the preceding URL; innermost, it is bounded on its own budget and cannot be attributed to the RFC 8414 request. 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 | 73 +++++++++++++++++++ core/mcp/inspectorClient.ts | 38 ++++++---- 2 files changed, 97 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 190a2640..df8bbb74 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -4,6 +4,7 @@ import { OAuthRequestTimeoutError, withOAuthRequestTimeout, } from "@inspector/core/auth/requestTimeout.js"; +import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js"; const URL_UNDER_TEST = "https://as.example.com/.well-known/oauth-authorization-server"; @@ -329,6 +330,78 @@ describe("withOAuthRequestTimeout", () => { }); }); + 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 + // request is being timed. `InspectorClient` and the CLI both put it + // innermost; these two cases are why. + const RFC8414 = + "https://as.example.com/.well-known/oauth-authorization-server/tenant"; + // A path-suffixed RFC 8414 candidate has two OIDC siblings, and the compat + // wrapper tries both. + const PROBES = [ + "https://as.example.com/.well-known/openid-configuration/tenant", + "https://as.example.com/tenant/.well-known/openid-configuration", + ]; + + /** Answers the RFC 8414 leg with a prompt 404, then stalls on the probe. */ + function stallingProbeFetch() { + return vi.fn((input) => { + if (String(input) === RFC8414) { + return Promise.resolve(new Response(null, { status: 404 })); + } + return new Promise(() => {}); + }); + } + + it("outside, a stalled probe is misreported under the preceding URL", async () => { + vi.useFakeTimers(); + const inner = stallingProbeFetch(); + const wrapped = withOAuthRequestTimeout( + withRfc8414OidcCompat(inner), + 1000, + ); + + const assertion = wrapped(RFC8414).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + const err = (await assertion) as OAuthRequestTimeoutError; + expect(err).toBeInstanceOf(OAuthRequestTimeoutError); + // The endpoint that stalled was PROBE. One budget covers both legs, and + // the error names the request the caller made rather than the one that + // hung — exactly the diagnostic this change exists to provide. + expect(err.url).toBe(RFC8414); + }); + + it("innermost, each leg is bounded on its own and the probe is abandoned", async () => { + vi.useFakeTimers(); + const inner = stallingProbeFetch(); + const wrapped = withRfc8414OidcCompat( + withOAuthRequestTimeout(inner, 1000), + ); + + const settled = wrapped(RFC8414).then( + (response) => response, + (err: unknown) => err, + ); + // The probe is timed from when it starts, on a budget of its own — not on + // whatever an outer one had left after the RFC 8414 leg. + await vi.advanceTimersByTimeAsync(1000); + + const result = await settled; + // The compat wrapper is deliberately inert when a probe *throws* — it + // hands the original response back and lets discovery run its normal + // course — so a timed-out probe surfaces as the original 404 rather than + // as an error, and the second candidate is not tried. What innermost buys + // is that the probe is bounded at all, and that a stall in it can never be + // attributed to the RFC 8414 request the caller actually made. + expect(result).toBeInstanceOf(Response); + expect((result as Response).status).toBe(404); + expect(inner).toHaveBeenCalledTimes(2); + expect(String(inner.mock.calls[1][0])).toBe(PROBES[0]); + }); + }); + it("clears its timer once the request settles", async () => { vi.useFakeTimers(); const clearSpy = vi.spyOn(globalThis, "clearTimeout"); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 180243ad..7ccbefa3 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -811,8 +811,9 @@ export class InspectorClient extends InspectorClientEventTarget { // `this.fetchFn` directly. The overrides are read lazily — `oauthManager` is // created a few lines below, and `setOAuthConfig` can change them later — so // the wrapper is inert until a server actually configures one. - this.fetchFn = withOAuthEndpointOverrides(this.fetchFn ?? fetch, () => - this.oauthManager?.getEndpointOverrides(), + const withOverrides = withOAuthEndpointOverrides( + this.fetchFn ?? fetch, + () => this.oauthManager?.getEndpointOverrides(), ); // #2172: recover discovery when a plain OAuth 2.0 authorization server // publishes RFC 8414 metadata at `/.well-known/openid-configuration`, which @@ -823,8 +824,18 @@ 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(this.fetchFn); - this.effectiveAuthFetch = this.buildEffectiveAuthFetch(); + this.fetchFn = withRfc8414OidcCompat(withOverrides); + // #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 + // URL of the RFC 8414 request that preceded it — defeating the whole point + // of naming the endpoint — while every probe in the loop shared that one + // request's budget (Copilot). Innermost here matches the CLI's + // `storedAuthFetch`. The transport keeps `this.fetchFn`, deliberately + // unbounded: its bodies are streams that are supposed to stay open. + this.effectiveAuthFetch = this.buildEffectiveAuthFetch( + withRfc8414OidcCompat(withOAuthRequestTimeout(withOverrides)), + ); this.sessionId = options.sessionId; @@ -991,16 +1002,15 @@ export class InspectorClient extends InspectorClientEventTarget { ); } - private buildEffectiveAuthFetch(): typeof fetch { - // #2319: bound every OAuth-path request — discovery, dynamic client - // registration, the token exchange, the refresh. Applied here rather than - // to `this.fetchFn` because the transport is handed `this.fetchFn` - // directly, and a deadline on a Streamable HTTP / SSE response would sever - // the very stream it is supposed to hold open. The cost of that placement - // is that the discovery the SDK runs from *inside* the transport (the - // 401/refresh path) is not covered here; that leg sits inside an SDK - // request and is bounded by its per-request timeout. - const base = withOAuthRequestTimeout(this.fetchFn ?? fetch); + /** + * @param base - the composed OAuth-path fetch, deadline innermost (#2319). + * Passed in rather than read off `this.fetchFn`, which is the *transport* + * chain and must stay unbounded. The cost of keeping the two apart is that + * the discovery the SDK runs from inside the transport (the 401/refresh path) + * is not covered by the deadline; that leg sits inside an SDK request and is + * bounded by its per-request timeout. + */ + private buildEffectiveAuthFetch(base: typeof fetch): typeof fetch { // Capture auth response bodies (OAuth discovery, DCR, token exchange) so // they're inspectable in the Network tab. Token-exchange responses carry // `access_token` / `refresh_token`; the Network UI masks those (and other From d8483000775360246797f7bd9245e1be014dd8f9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 03:23:26 -0400 Subject: [PATCH 05/23] fix: enforce the OAuth deadline across the proxy hop (#2319 round 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, both real. The race bounded only the caller's promise on the browser path. `createRemoteFetch` dropped the signal, so an abort — including the deadline's own — left the POST to /api/fetch in flight; the backend never saw its request cancelled and its outbound fetch ran on with neither signal nor timeout, holding a handler and an authorization-server socket per timed-out attempt. `createRemoteFetch` now forwards the caller's signal (init.signal when present and not undefined, else a Request's own), and the /api/fetch route composes `c.req.raw.signal` with a server-side deadline matching the client budget, cleared in `finally` so a streaming response is not severed. A probe's own timeout was swallowed by `withRfc8414OidcCompat`, which returned the preceding 404 and discarded the endpoint name the deadline exists to provide. A deadline the Inspector imposed is its own error, not the server's answer, so it escapes now; every other probe failure still falls back as documented. Nothing regresses: every candidate the loop probes is one the SDK's own buildDiscoveryUrls emits, so a stall there is one the SDK would have hit itself, unbounded. 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 | 23 ++---- .../core/mcp/remote/createRemoteFetch.test.ts | 73 +++++++++++++++++++ core/auth/oidcDiscoveryCompat.ts | 14 +++- core/mcp/remote/createRemoteFetch.ts | 17 +++++ core/mcp/remote/node/server.ts | 31 ++++++++ 5 files changed, 142 insertions(+), 16 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index df8bbb74..ab453c52 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -373,32 +373,25 @@ describe("withOAuthRequestTimeout", () => { expect(err.url).toBe(RFC8414); }); - it("innermost, each leg is bounded on its own and the probe is abandoned", async () => { + it("innermost, the probe's own timeout escapes and names the probe", async () => { vi.useFakeTimers(); const inner = stallingProbeFetch(); const wrapped = withRfc8414OidcCompat( withOAuthRequestTimeout(inner, 1000), ); - const settled = wrapped(RFC8414).then( - (response) => response, - (err: unknown) => err, - ); + const assertion = wrapped(RFC8414).catch((err: unknown) => err); // The probe is timed from when it starts, on a budget of its own — not on // whatever an outer one had left after the RFC 8414 leg. await vi.advanceTimersByTimeAsync(1000); - const result = await settled; - // The compat wrapper is deliberately inert when a probe *throws* — it - // hands the original response back and lets discovery run its normal - // course — so a timed-out probe surfaces as the original 404 rather than - // as an error, and the second candidate is not tried. What innermost buys - // is that the probe is bounded at all, and that a stall in it can never be - // attributed to the RFC 8414 request the caller actually made. - expect(result).toBeInstanceOf(Response); - expect((result as Response).status).toBe(404); + const err = (await assertion) as OAuthRequestTimeoutError; + // The compat wrapper is inert on an ordinary probe failure, but a deadline + // the Inspector imposed escapes it, so the caller is told which endpoint + // stalled rather than being handed the preceding 404. + expect(err).toBeInstanceOf(OAuthRequestTimeoutError); + expect(err.url).toBe(PROBES[0]); expect(inner).toHaveBeenCalledTimes(2); - expect(String(inner.mock.calls[1][0])).toBe(PROBES[0]); }); }); 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 7ae9edbf..ea0d7f66 100644 --- a/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts +++ b/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts @@ -168,4 +168,77 @@ describe("createRemoteFetch", () => { expect(res.headers.get("x-test")).toBe("yes"); expect(await res.text()).toBe("hello"); }); + + describe("cancellation forwarding (#2319)", () => { + // Without this the caller's abort settled only its own promise: the POST + // stayed in flight, the backend never saw its request cancelled, and its + // outbound fetch to the authorization server ran on detached. + function capture() { + const fetchFn = vi + .fn() + .mockResolvedValue(ok(standardRemoteBody)); + const remoteFetch = createRemoteFetch({ + baseUrl: "http://remote.example", + fetchFn: fetchFn as unknown as typeof fetch, + }); + return { fetchFn, remoteFetch }; + } + + function signalOf(fetchFn: ReturnType>) { + return (fetchFn.mock.calls[0][1] as RequestInit).signal; + } + + it("forwards init.signal onto the proxy hop", async () => { + const { fetchFn, remoteFetch } = capture(); + const caller = new AbortController(); + + await remoteFetch("http://upstream.example/", { signal: caller.signal }); + + expect(signalOf(fetchFn)).toBe(caller.signal); + }); + + it("forwards a Request's own signal when init has none", async () => { + const { fetchFn, remoteFetch } = capture(); + const caller = new AbortController(); + + await remoteFetch( + new Request("http://upstream.example/", { signal: caller.signal }), + ); + + expect(signalOf(fetchFn)).toBe(caller.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(); + + await remoteFetch( + new Request("http://upstream.example/", { signal: caller.signal }), + { signal: undefined }, + ); + + expect(signalOf(fetchFn)).toBe(caller.signal); + }); + + it("honours an explicit null init.signal as no signal", async () => { + const { fetchFn, remoteFetch } = capture(); + const caller = new AbortController(); + + await remoteFetch( + new Request("http://upstream.example/", { signal: caller.signal }), + { signal: null }, + ); + + expect(signalOf(fetchFn)).toBeUndefined(); + }); + + it("sends no signal when the caller supplied none", async () => { + const { fetchFn, remoteFetch } = capture(); + + await remoteFetch("http://upstream.example/"); + + expect(signalOf(fetchFn)).toBeUndefined(); + }); + }); }); diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index bb30e722..4404adc0 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -90,6 +90,7 @@ * wiring in `core/mcp/inspectorClient.ts`. */ +import { OAuthRequestTimeoutError } from "./requestTimeout.js"; import { OAuthMetadataSchema, OpenIdProviderDiscoveryMetadataSchema, @@ -262,7 +263,18 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { let probe: Response; try { probe = await fetchFn(candidate, { headers }); - } catch { + } catch (err) { + // A deadline the Inspector imposed is *our* error, not the server's + // answer, so it escapes rather than being folded back into the original + // response (#2319, Copilot). Swallowing it would hand the caller the + // preceding 404 and discard the one thing the deadline adds — the name + // of the endpoint that stalled — leaving discovery to fail later under + // a message that points at the wrong URL. There is no flow this can + // break that was not already broken: every candidate this loop probes + // is one the SDK's own `buildDiscoveryUrls` emits, so a stall here is a + // stall the SDK would have hit itself, unbounded. Everything else — a + // CORS rejection, DNS, a reset — still falls back as documented above. + if (err instanceof OAuthRequestTimeoutError) throw err; return response; } if (!probe.ok) { diff --git a/core/mcp/remote/createRemoteFetch.ts b/core/mcp/remote/createRemoteFetch.ts index 4dcbf689..36bbba2d 100644 --- a/core/mcp/remote/createRemoteFetch.ts +++ b/core/mcp/remote/createRemoteFetch.ts @@ -133,10 +133,27 @@ export function createRemoteFetch(options: RemoteFetchOptions): typeof fetch { reqHeaders["x-mcp-remote-auth"] = `Bearer ${options.authToken}`; } + // #2319: forward the caller's cancellation onto the proxy hop. Without it + // an abort — including the OAuth deadline's own — settled only the caller's + // promise: the POST stayed in flight, so the backend never saw its request + // cancelled and its outbound fetch to the authorization server was left + // running detached, holding a handler and a socket for as long as the + // server cared to stall (Copilot). `init.signal` wins when present and not + // `undefined` (a WebIDL dictionary member present as `undefined` converts + // as absent), otherwise a `Request` carries its own. + const explicitSignal = init?.signal; + const signal = + explicitSignal !== undefined + ? (explicitSignal ?? undefined) + : input instanceof Request + ? input.signal + : undefined; + const res = await fetchFn(`${baseUrl}/api/fetch`, { method: "POST", headers: reqHeaders, body: JSON.stringify({ url, method, headers, body }), + signal, }); if (!res.ok) { diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 69439f6d..071bcd49 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -28,6 +28,7 @@ import { bodyLimit } from "hono/body-limit"; import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; import { createProxyFetch } from "../../node/proxyFetch.js"; +import { DEFAULT_OAUTH_REQUEST_TIMEOUT_MS } from "../../../auth/requestTimeout.js"; import type { RemoteConnectRequest, RemoteSendRequest, @@ -1216,6 +1217,33 @@ export function createRemoteApp( return c.json({ error: "Missing url" }, 400); } + // #2319: bound the outbound call, and release it when the browser gives up. + // This hop is where a client-side deadline used to stop being enforceable: + // the browser abandons its promise, but the request it made is served here, + // and an outbound fetch with neither a signal nor a timeout holds a handler + // and an authorization-server socket open indefinitely — once per timed-out + // retry (Copilot). `c.req.raw.signal` is the propagated cancellation, now + // that `createRemoteFetch` forwards it; the timer is the backstop for a + // client that vanishes without aborting, and it matches the client-side + // budget so the two agree on when this is hopeless. + const controller = new AbortController(); + const clientSignal: AbortSignal | undefined = c.req.raw.signal; + const signal = clientSignal + ? AbortSignal.any([controller.signal, clientSignal]) + : controller.signal; + // Cleared in `finally`, before the handler returns. A streaming response is + // returned here without its body being read, so a timer left armed would + // sever a live stream 30 seconds in. + const timer = setTimeout( + () => + controller.abort( + new Error( + `proxied request to ${url} timed out after ${DEFAULT_OAUTH_REQUEST_TIMEOUT_MS}ms`, + ), + ), + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + ); + try { // Proxy-aware, not the bare global. This route is the browser's ONLY way // out to the network: the web client's `environment.fetch` is @@ -1228,6 +1256,7 @@ export function createRemoteApp( method, headers: new Headers(headers), body: reqBody, + signal, }); const resHeaders: Record = {}; @@ -1254,6 +1283,8 @@ export function createRemoteApp( } catch (err) { const msg = err instanceof Error ? err.message : String(err); return c.json({ error: msg }, 500); + } finally { + clearTimeout(timer); } }); From f3e4023e350724cde802e5d41a4bfbf5b612347b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 03:43:57 -0400 Subject: [PATCH 06/23] fix: sanitize rebuilt headers, refresh stale docs, cover the route (#2319 round 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all real. `arrayBuffer()` hands back the decoded entity body, so the rebuilt response was carrying the upstream `content-encoding` and the compressed `content-length` — a gzipped metadata document came back as plain JSON bytes still claiming gzip and the wrong size, visible in the Network capture. Both headers are deleted now, as `responseWithBody` in endpointOverrides.ts already did for the same reason. The "why a race, and not just the signal" rationale described a world this PR had since changed: the signal does now reach the backend's outbound fetch. The module doc and three implementation comments are rewritten to say what the race is actually still for — a fetchFn that ignores AbortSignal, and a backend that is itself wedged — rather than a proxy hop that no longer drops it. The backend cancellation path had only mock-level coverage. A route-level test drives /api/fetch at an upstream that accepts and never answers, aborts the browser side, and asserts the upstream observes its connection closing. Verified by mutation: dropping `signal` from the route's outbound fetch makes it time out rather than pass. 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 | 48 +++++++++++--- .../mcp/remote/server-extra-coverage.test.ts | 47 ++++++++++++++ core/auth/requestTimeout.ts | 65 ++++++++++++------- 3 files changed, 128 insertions(+), 32 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index ab453c52..5308437d 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -96,11 +96,12 @@ describe("withOAuthRequestTimeout", () => { expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); }); - it("still enforces the deadline when the signal is dropped in flight", async () => { + it("still enforces the deadline when the fetch ignores the signal", async () => { vi.useFakeTimers(); - // The browser's `createRemoteFetch` re-issues the call to `/api/fetch` and - // forwards no signal, so aborting locally cancels nothing. The race is what - // has to bound it. + // The signal now reaches all the way out — `createRemoteFetch` forwards it + // onto the proxy hop and `/api/fetch` composes it into its outbound call — + // but a `fetchFn` that ignores `AbortSignal` cancels nothing, so the race + // is what bounds this case. const signalIgnoring: typeof fetch = () => new Promise(() => {}); const wrapped = withOAuthRequestTimeout(signalIgnoring, 1000); @@ -127,9 +128,9 @@ describe("withOAuthRequestTimeout", () => { }); it("races caller cancellation too, so it wins on a signal-ignoring fetch", async () => { - // Forwarding alone is not enough on the browser's `createRemoteFetch` path, - // which drops the signal: without the race the caller's abort would sit - // pending for the whole budget instead of winning. + // Forwarding alone is not enough against a `fetchFn` that ignores the + // signal: without the race the caller's abort would sit pending for the + // whole budget instead of winning. const wrapped = withOAuthRequestTimeout(neverSettles, 60_000); const caller = new AbortController(); @@ -148,8 +149,8 @@ describe("withOAuthRequestTimeout", () => { await expect( wrapped(URL_UNDER_TEST, { signal: AbortSignal.abort(reason) }), ).rejects.toBe(reason); - // Racing it would still have evaluated the inner fetch, which on the - // signal-dropping proxy path means the request actually goes out. + // Racing it would still have evaluated the inner fetch, and a fetch that + // does not check an already-aborted signal would send the request. expect(inner).not.toHaveBeenCalled(); }); @@ -257,6 +258,8 @@ describe("withOAuthRequestTimeout", () => { expect(response.statusText).toBe("Created"); expect(response.ok).toBe(true); 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); @@ -265,6 +268,33 @@ describe("withOAuthRequestTimeout", () => { }); }); + it("drops content-encoding and content-length, which the buffer invalidates", async () => { + // `arrayBuffer()` hands back the *decoded* entity body, so an inherited + // `content-encoding` would describe an encoding the bytes no longer have + // and the inherited `content-length` would be the compressed size. Both are + // wrong, and both show up in the Network capture. + const decoded = '{"issuer":"https://as.example.com"}'; + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue( + new Response(decoded, { + headers: { + "content-type": "application/json", + "content-encoding": "gzip", + "content-length": "42", + }, + }), + ), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + + expect(response.headers.get("content-encoding")).toBeNull(); + expect(response.headers.get("content-length")).toBeNull(); + expect(response.headers.get("content-type")).toBe("application/json"); + await expect(response.text()).resolves.toBe(decoded); + }); + it("rebuilds a null-body status without handing it a body", async () => { // `new Response(body, { status: 204 })` throws; the rebuild has to pass null. const wrapped = withOAuthRequestTimeout( diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index aabda5f2..f1d6ef9c 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -300,6 +300,10 @@ describe("server.ts supplemental coverage", () => { let h: Harness; let target: ServerType; let targetUrl: string; + /** Resolves when an upstream `/stall` request is closed by its client. */ + let stallClosed: Promise; + /** Paths the upstream fixture has actually received. */ + let upstreamHits: string[]; // Isolate the WHOLE suite from the developer's (or CI's) proxy environment. // Setting only uppercase HTTP_PROXY in the one proxy test would not be @@ -336,7 +340,21 @@ describe("server.ts supplemental coverage", () => { h = await start(); // A tiny upstream HTTP server we can point /api/fetch at. const { createServer } = await import("node:http"); + let markClosed: () => void = () => {}; + stallClosed = new Promise((resolve) => { + markClosed = resolve; + }); + upstreamHits = []; const srv = createServer((req, res) => { + upstreamHits.push(req.url ?? ""); + if (req.url === "/stall") { + // Accept the request, answer nothing, and report when the client + // gives up — the #2319 wedged-authorization-server shape. `close` on + // the response fires whether the socket was torn down or the handler + // simply ended, and nothing here ever ends it. + res.on("close", () => markClosed()); + return; + } if (req.url === "/stream") { res.writeHead(200, { "Content-Type": "text/event-stream" }); res.write("data: hi\n\n"); @@ -381,6 +399,35 @@ describe("server.ts supplemental coverage", () => { await stop(h); }); + it("releases the upstream request when the browser cancels (#2319)", async () => { + // The point of forwarding the signal through `createRemoteFetch`: without + // it the browser's abort settled only its own promise, and this handler + // plus the upstream socket stayed pending for as long as the server cared + // to stall — once per timed-out attempt. Asserted at the route rather + // than on a mock, because the mock cannot show the socket being released. + const caller = new AbortController(); + const pending = fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/stall` }), + signal: caller.signal, + }); + const rejected = pending.catch((err: unknown) => err); + + // Wait until the upstream has actually received the request, so the abort + // cannot race ahead of it and pass for the wrong reason. + await vi.waitFor(() => { + expect(upstreamHits).toContain("/stall"); + }); + caller.abort(); + + expect(await rejected).toBeInstanceOf(Error); + // The upstream sees its connection go away. Without the signal composed + // into the route's outbound fetch this never resolves and the test times + // out. + await stallClosed; + }); + it("routes the outbound request through HTTP_PROXY (#2067)", async () => { // /api/fetch is the browser's ONLY way out to the network — the web // client's `environment.fetch` is `createRemoteFetch()`, which forwards diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 80cb5aba..7c2dadf0 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -36,17 +36,23 @@ * too, and — the point of naming the URL in the error — says *which* endpoint * stalled instead of blaming the handshake. * - * ## Why a race, and not just the signal + * ## The signal, the proxy hop, and why the race is still here * - * The signal alone cannot enforce this. In the browser the OAuth fetch is - * `createRemoteFetch`, which re-issues the call as a POST to `/api/fetch` and - * does not forward `init.signal`; the backend's outbound fetch gets no signal - * either. So on exactly the path this bound exists for — a browser session - * against a wedged authorization server — aborting the local promise would - * change nothing. The race is what actually enforces the deadline; the signal - * is kept because it does cancel the direct-fetch paths (CLI, TUI, backend) - * rather than merely abandoning them. Same reasoning, same shape, as - * `revokeToken`. + * The signal is the primary mechanism and it now reaches all the way out. On a + * direct fetch (CLI, TUI, backend) it cancels the request outright. In the + * browser the OAuth fetch is `createRemoteFetch`, which re-issues the call as a + * POST to `/api/fetch` — and as of #2319 it forwards the signal onto that hop, + * where the route composes `c.req.raw.signal` into its own outbound fetch. So + * an abort here tears down the backend's request to the authorization server + * rather than leaving it running detached, which is what it used to do. + * + * The race is kept anyway, and it is not redundant. It bounds what the signal + * cannot: a `fetchFn` that ignores `AbortSignal` altogether — an injected or + * test double, or any future transport that drops it the way the proxy used to + * — and a backend that is itself wedged, where the outbound request is + * cancelled but the hop back never completes. A deadline that depends on every + * layer beneath it honouring a signal is not a deadline. Same reasoning, same + * shape, as `revokeToken`. */ /** @@ -120,19 +126,32 @@ const NULL_BODY_STATUSES = new Set([101, 103, 204, 205, 304]); /** * Rebuild a `Response` around an already-buffered body. * + * Two things have to be carried across by hand. + * * `url`, `redirected` and `type` are getters with no constructor option, and * callers do read them — the RFC 8414/OIDC compat wrapper stamps a header * naming the URL a body came from, and the SDK's discovery reports the - * responding URL in its errors. So they are copied across explicitly rather - * than silently reset to `""` / `false` / `"default"`. + * responding URL in its errors. So they are copied explicitly rather than + * silently reset to `""` / `false` / `"default"`. + * + * `content-encoding` and `content-length`, by contrast, must be *dropped*. + * `arrayBuffer()` hands back the **decoded** entity body, so a gzipped metadata + * document arrives here as plain JSON bytes: an inherited `content-encoding` + * would describe an encoding the body no longer has, and the inherited + * `content-length` would be the compressed size — both of them wrong, and both + * visible in the Network capture (Copilot). `responseWithBody` in + * `endpointOverrides.ts` deletes the same two headers for the same reason. */ function rebuildResponse(response: Response, body: ArrayBuffer): Response { + const headers = new Headers(response.headers); + headers.delete("content-encoding"); + headers.delete("content-length"); const rebuilt = new Response( NULL_BODY_STATUSES.has(response.status) ? null : body, { status: response.status, statusText: response.statusText, - headers: response.headers, + headers, }, ); for (const key of ["url", "redirected", "type"] as const) { @@ -181,9 +200,10 @@ export function withOAuthRequestTimeout( const controller = new AbortController(); const callerSignal = callerSignalOf(input, init); // Before anything is constructed or sent. Racing an already-aborted signal - // would still evaluate `fetchFn(...)`, and on the signal-dropping - // `createRemoteFetch` path that means the OAuth request goes out even - // though the caller cancelled before the call (Copilot). + // would still evaluate `fetchFn(...)`, and a `fetchFn` that does not check + // an already-aborted signal — the proxy hop was one until #2319 — would + // send the OAuth request even though the caller cancelled before the call + // (Copilot). if (callerSignal?.aborted) throw callerSignal.reason; const signal = callerSignal @@ -199,18 +219,17 @@ export function withOAuthRequestTimeout( const abandoned = new Promise((_resolve, reject) => { timer = setTimeout(() => { timedOut = true; - // Abort first so a direct fetch is actually cancelled rather than left - // running detached, then reject so the proxied fetch — which never saw - // the signal — is abandoned on schedule too. + // Abort first, which is what actually cancels the request — through + // the proxy hop too, since #2319 — then reject, so a `fetchFn` that + // ignores the signal is still abandoned on schedule. controller.abort(new OAuthRequestTimeoutError(url, budget)); reject(new OAuthRequestTimeoutError(url, budget)); }, budget); // The caller's cancellation is raced too, not merely forwarded. Forwarding - // it is enough on a direct fetch, but on the browser's `createRemoteFetch` - // path — the one this wrapper exists for — the signal is dropped in - // flight, so an abort would otherwise leave this promise pending for the - // whole budget instead of the outer cancellation winning as documented + // it is enough wherever the signal is honoured, but a `fetchFn` that + // ignores it would otherwise leave this promise pending for the whole + // budget instead of the outer cancellation winning as documented // (Copilot). The caller's own `reason` is preserved, so it still sees its // abort rather than a substituted error. // Not aborted — the short-circuit above returned for that case. From a452305204363c9c3f3b685c1a89115402c0b856 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 04:00:44 -0400 Subject: [PATCH 07/23] fix: cancel the discarded proxy stream, refresh revocation docs (#2319 round 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /api/fetch route classifies an event-stream or NDJSON response as a stream and then answers with JSON and no body — so nobody downstream owns the upstream stream and nothing will ever read it, while the `finally` clears the only deadline on the way out. It is now cancelled explicitly, best-effort, the way `releaseBody` in oidcDiscoveryCompat already does. Measured, undici reclaims an unread body here on its own, so this is belt and braces rather than a demonstrated leak — kept because relying on that is an implementation detail of the fetch beneath us, not a contract. The route test and the source comment both say so rather than implying a stronger claim. Forwarding the signal also made revocation.ts's rationale stale: it still described a proxy that drops `init.signal` and a backend that cannot cancel. Both comments there, and the matching one in revocation.test.ts, now describe the post-#2319 path — the signal cancels, the race guarantees, and what the race is still for. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../web/src/test/core/auth/revocation.test.ts | 10 ++--- .../mcp/remote/server-extra-coverage.test.ts | 42 +++++++++++++++++++ core/auth/revocation.ts | 28 +++++++------ core/mcp/remote/node/server.ts | 20 +++++++-- 4 files changed, 79 insertions(+), 21 deletions(-) diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index e50fa1e9..c108b530 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -393,11 +393,11 @@ describe("revokeToken", () => { }); }); - // The web fetch is `createRemoteFetch`, which re-issues the call as a POST to - // `/api/fetch` and drops `init.signal`; the backend's outbound fetch gets no - // signal either. A signal-only bound is therefore inert on exactly the path - // the timeout exists for, so the deadline has to hold against a fetch that - // ignores the signal entirely. + // The signal reaches every path since #2319 — `createRemoteFetch` forwards it + // onto the `/api/fetch` hop and that route composes it into its outbound + // fetch — but a signal-only bound is still only as good as the fetch beneath + // it. A `fetchFn` that ignores `AbortSignal`, or a wedged backend, would + // leave a signal-only deadline inert, so it has to hold without one. it("gives up on a fetch that never settles and ignores the signal", async () => { const outcome = await revokeToken({ endpoint: REVOKE_URL, diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index f1d6ef9c..648da1c9 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -304,6 +304,8 @@ describe("server.ts supplemental coverage", () => { let stallClosed: Promise; /** Paths the upstream fixture has actually received. */ let upstreamHits: string[]; + /** Resolves when the never-ending `/stream-open` response is closed. */ + let openStreamClosed: Promise; // Isolate the WHOLE suite from the developer's (or CI's) proxy environment. // Setting only uppercase HTTP_PROXY in the one proxy test would not be @@ -345,8 +347,22 @@ describe("server.ts supplemental coverage", () => { markClosed = resolve; }); upstreamHits = []; + let markStreamClosed: () => void = () => {}; + openStreamClosed = new Promise((resolve) => { + markStreamClosed = resolve; + }); const srv = createServer((req, res) => { upstreamHits.push(req.url ?? ""); + if (req.url === "/stream-open") { + // Event-stream headers, one frame, and then nothing — the shape that + // used to leave a socket open for good, because the route classifies + // it as a stream, returns JSON without the body, and clears its + // deadline on the way out. + res.writeHead(200, { "Content-Type": "text/event-stream" }); + res.write("data: hi\n\n"); + res.on("close", () => markStreamClosed()); + return; + } if (req.url === "/stall") { // Accept the request, answer nothing, and report when the client // gives up — the #2319 wedged-authorization-server shape. `close` on @@ -428,6 +444,32 @@ describe("server.ts supplemental coverage", () => { await stallClosed; }); + it("cancels a discarded stream that sends headers and never ends (#2319)", async () => { + // The route answers with JSON and no body, so nobody downstream owns the + // upstream stream and nothing will ever read it; the route cancels it + // explicitly rather than relying on that being reclaimed for it. + // + // ⚠️ This pins the OUTCOME, not the mechanism: measured, it still passes + // with the explicit `cancel()` removed, because undici reclaims an unread + // response body here on its own. So the cancel is defensive — it makes + // the release explicit and independent of that behaviour — and this test + // will catch the route starting to hold the stream open, not the cancel + // being deleted. Said plainly rather than left to imply a stronger claim. + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/stream-open` }), + }); + + expect(res.status).toBe(200); + const payload = (await res.json()) as { status: number; body?: string }; + expect(payload.status).toBe(200); + expect(payload.body).toBeUndefined(); + // The upstream sees its connection go away once the handler returns. + // Without the cancel this never resolves and the test times out. + await openStreamClosed; + }); + it("routes the outbound request through HTTP_PROXY (#2067)", async () => { // /api/fetch is the browser's ONLY way out to the network — the web // client's `environment.fetch` is `createRemoteFetch()`, which forwards diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index f2d16c0f..1ffc8e6b 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -268,14 +268,15 @@ export async function revokeToken( // outcome — and every caller has already cleared its local state by now, so // a rejection would break the documented best-effort guarantee. const { url, init } = buildRevocationRequest(params); - // The signal alone is not enough to bound this. In the browser the fetch is - // `createRemoteFetch`, which re-issues the call as a POST to `/api/fetch` - // and does not forward `init.signal`; the backend's outbound fetch gets no - // signal either. So a wedged authorization server would hold the teardown - // open indefinitely on exactly the path the timeout exists for. The race is - // what actually enforces the deadline; the signal is kept because it does - // cancel the direct-fetch paths (CLI, TUI, backend) rather than merely - // abandoning them. + // The signal cancels; the race is what guarantees the deadline. Since #2319 + // the signal reaches every path — `createRemoteFetch` forwards it onto the + // `/api/fetch` hop and that route composes it into its outbound fetch, so a + // browser-side abort now tears down the backend's request to the + // authorization server as well as the direct-fetch paths (CLI, TUI, + // backend) it always did. The race is still not redundant: it bounds a + // `fetchFn` that ignores `AbortSignal` altogether, and a backend that is + // itself wedged. Same reasoning, and the same shape, as + // `withOAuthRequestTimeout`. const response = await withDeadline( params.fetchFn(url, { ...init, signal: AbortSignal.timeout(timeoutMs) }), timeoutMs, @@ -307,11 +308,12 @@ export async function revokeToken( /** * Reject with a timeout error if `promise` has not settled within `timeoutMs`. * - * The underlying request is abandoned rather than cancelled — nothing here can - * cancel a fetch the proxy already stripped the signal from. That is the right - * trade for this caller: the point is that the *teardown* proceeds, and the - * response, if it ever arrives, is a revocation we no longer need to wait for. - * The timer is cleared on the settled path so a caller is never held awake by it. + * This abandons the promise; cancelling the request is the caller's signal's job + * (see `revokeToken`, where the two are used together). Abandonment alone is an + * acceptable floor for this caller even where the signal is ignored: the point + * is that the *teardown* proceeds, and the response, if it ever arrives, is a + * revocation we no longer need to wait for. The timer is cleared on the settled + * path so a caller is never held awake by it. */ async function withDeadline( promise: Promise, diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 071bcd49..42c20fbf 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1231,9 +1231,10 @@ export function createRemoteApp( const signal = clientSignal ? AbortSignal.any([controller.signal, clientSignal]) : controller.signal; - // Cleared in `finally`, before the handler returns. A streaming response is - // returned here without its body being read, so a timer left armed would - // sever a live stream 30 seconds in. + // Cleared in `finally`, before the handler returns. Safe because by then + // the body has been either read or explicitly cancelled — this route never + // hands a live stream to its caller, so there is nothing left for the timer + // to protect and nothing it could sever. const timer = setTimeout( () => controller.abort( @@ -1271,6 +1272,19 @@ export function createRemoteApp( let resBody: string | undefined; if (!isStream && res.body) { resBody = await res.text(); + } else if (isStream) { + // This route does not return the stream — it answers with JSON and no + // body — so nobody downstream owns it and nothing will ever read it. + // Left un-cancelled, an OAuth endpoint that sends event-stream or + // NDJSON headers and never closes would hold the upstream socket open + // for good, since the `finally` below clears the only deadline once + // this handler returns (Copilot). Best-effort, like `releaseBody` in + // `oidcDiscoveryCompat`: a body already consumed, locked or absent is + // not an error here. Measured, undici does reclaim an unread body on + // its own in this configuration — so this is belt and braces rather + // than a demonstrated leak, and it is kept because relying on that is + // an implementation detail of the fetch beneath us, not a contract. + await res.body?.cancel().catch(() => {}); } return c.json({ From baefc96806f3bbae6f641a90099c3e0c816c18d8 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 04:16:15 -0400 Subject: [PATCH 08/23] fix: carry the proxy deadline's identity, correct the rounding note (#2319 round 9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A deadline enforced by the backend could not arrive as an OAuthRequestTimeoutError: /api/fetch serialized it as a 500 and createRemoteFetch turned that into a plain Error, so every instanceof check downstream was false — including the one that lets a stalled probe escape withRfc8414OidcCompat. That matters most where no client-side wrapper exists to have fired first: the discovery the SDK runs from inside the transport. The route now answers 504 with a typed marker (OAUTH_TIMEOUT_WIRE_CODE plus url and timeoutMs) and createRemoteFetch reconstructs the error from it, guarded by a shape check so an upstream's own JSON cannot forge one. The route's deadline becomes a `fetchTimeoutMs` option defaulting to the shared constant, so the backstop can be proven to fire in milliseconds instead of thirty seconds of wall clock — and because a very slow authorization server is a legitimate reason to want a longer one. The rounding rationale was wrong: setTimeout accepts a fractional delay and truncates it (verified on Node 26); ERR_OUT_OF_RANGE is AbortSignal.timeout's behaviour, which this wrapper never calls. The rounding stays, for the reason that is actually true — the budget is interpolated into the message and exposed as timeoutMs, and a performance.now() budget would otherwise report 1000.4000000953674ms. Also drops a sentence in the discarded-stream test that contradicted the qualification above it. 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 | 8 +- .../core/mcp/remote/createRemoteFetch.test.ts | 83 +++++++++++++++++++ .../mcp/remote/server-extra-coverage.test.ts | 39 ++++++++- core/auth/index.ts | 3 + core/auth/requestTimeout.ts | 54 ++++++++++-- core/mcp/remote/createRemoteFetch.ts | 21 +++++ core/mcp/remote/node/server.ts | 59 +++++++++++-- 7 files changed, 247 insertions(+), 20 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 5308437d..6ff65333 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -318,10 +318,12 @@ describe("withOAuthRequestTimeout", () => { await expect(wrapped(URL_UNDER_TEST)).rejects.toBe(boom); }); - it("rounds a fractional budget, because setTimeout takes an integer", async () => { + it("rounds a fractional budget, which is reported to the caller", async () => { vi.useFakeTimers(); - // A budget derived from `performance.now()` arrives fractional; a fractional - // delay throws ERR_OUT_OF_RANGE before the request is ever made. + // `setTimeout` would accept the fraction and truncate it. The rounding is + // for the budget's *reported* form: a `performance.now()` subtraction would + // otherwise name "1000.4000000953674ms" in the message and expose a + // non-integer `timeoutMs`. const wrapped = withOAuthRequestTimeout(neverSettles, 1000.4); const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); 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 ea0d7f66..12de59ce 100644 --- a/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts +++ b/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts @@ -1,5 +1,9 @@ import { describe, it, expect, vi } from "vitest"; import { createRemoteFetch } from "@inspector/core/mcp/remote/createRemoteFetch.js"; +import { + OAUTH_TIMEOUT_WIRE_CODE, + OAuthRequestTimeoutError, +} from "@inspector/core/auth/requestTimeout.js"; function ok(body: object): Response { return new Response(JSON.stringify(body), { @@ -241,4 +245,83 @@ describe("createRemoteFetch", () => { expect(signalOf(fetchFn)).toBeUndefined(); }); }); + + describe("proxy-side deadline (#2319)", () => { + function remoteReturning(status: number, body: string) { + const fetchFn = vi.fn().mockResolvedValue( + new Response(body, { + status, + headers: { "content-type": "application/json" }, + }), + ); + return createRemoteFetch({ + baseUrl: "http://remote.example", + fetchFn: fetchFn as unknown as typeof fetch, + }); + } + + it("rebuilds an OAuthRequestTimeoutError from the route's marker", async () => { + // The one path where a timeout reaches the browser without a client-side + // wrapper having fired first: the discovery the SDK runs from inside the + // transport has no wrapper at all, so the backend's deadline is the only + // one, and it arrives as an ordinary error response. + const remoteFetch = remoteReturning( + 504, + JSON.stringify({ + error: "proxied request to https://as.example.com/x timed out", + code: OAUTH_TIMEOUT_WIRE_CODE, + url: "https://as.example.com/x", + timeoutMs: 30000, + }), + ); + + const err = await remoteFetch("https://as.example.com/x").catch( + (e: unknown) => e, + ); + + expect(err).toBeInstanceOf(OAuthRequestTimeoutError); + const timeout = err as OAuthRequestTimeoutError; + expect(timeout.url).toBe("https://as.example.com/x"); + expect(timeout.timeoutMs).toBe(30000); + }); + + it("leaves an ordinary error response as a plain Error", async () => { + const remoteFetch = remoteReturning( + 500, + JSON.stringify({ error: "connect ECONNREFUSED" }), + ); + + const err = await remoteFetch("https://as.example.com/x").catch( + (e: unknown) => e, + ); + + expect(err).not.toBeInstanceOf(OAuthRequestTimeoutError); + expect((err as Error).message).toMatch(/Remote fetch failed \(500\)/); + }); + + it("ignores a non-JSON error body rather than throwing on the parse", async () => { + const remoteFetch = remoteReturning(502, "bad gateway"); + + const err = await remoteFetch("https://as.example.com/x").catch( + (e: unknown) => e, + ); + + expect((err as Error).message).toMatch(/Remote fetch failed \(502\)/); + }); + + it("ignores a marker whose fields are the wrong shape", async () => { + // An upstream could serve JSON of its own through a failing proxy; the + // guard checks the field types, not just the code. + const remoteFetch = remoteReturning( + 504, + JSON.stringify({ code: OAUTH_TIMEOUT_WIRE_CODE, url: 5 }), + ); + + const err = await remoteFetch("https://as.example.com/x").catch( + (e: unknown) => e, + ); + + expect(err).not.toBeInstanceOf(OAuthRequestTimeoutError); + }); + }); }); diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 648da1c9..9613d2af 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -51,6 +51,7 @@ interface StartOpts { secretStore?: SecretStore; logger?: pinoType.Logger; seedConfig?: string; + fetchTimeoutMs?: number; } async function start(opts: StartOpts = {}): Promise { @@ -65,6 +66,9 @@ async function start(opts: StartOpts = {}): Promise { initialConfig: { defaultEnvironment: {} }, secretStore: opts.secretStore ?? new InMemorySecretStore(), logger: opts.logger, + ...(opts.fetchTimeoutMs !== undefined && { + fetchTimeoutMs: opts.fetchTimeoutMs, + }), }); const { baseUrl, server } = await new Promise<{ baseUrl: string; @@ -465,11 +469,42 @@ describe("server.ts supplemental coverage", () => { const payload = (await res.json()) as { status: number; body?: string }; expect(payload.status).toBe(200); expect(payload.body).toBeUndefined(); - // The upstream sees its connection go away once the handler returns. - // Without the cancel this never resolves and the test times out. + // The observable contract: the route does not leave the upstream stream + // running after it has answered. (Per the qualification above, this does + // not isolate the explicit `cancel()` as the cause.) await openStreamClosed; }); + it("answers 504 with a typed marker when its own deadline fires (#2319)", async () => { + // The backstop, on its own short budget so this costs milliseconds rather + // than the production thirty seconds. It matters because this is the only + // deadline on the transport-internal discovery path, which has no + // client-side wrapper — so its error has to survive the hop as something + // an `instanceof OAuthRequestTimeoutError` check can still recognize. + await stop(h); + h = await start({ fetchTimeoutMs: 120 }); + + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/stall` }), + }); + + expect(res.status).toBe(504); + const payload = (await res.json()) as { + error: string; + code: string; + url: string; + timeoutMs: number; + }; + expect(payload.code).toBe("oauth_request_timeout"); + expect(payload.url).toBe(`${targetUrl}/stall`); + expect(payload.timeoutMs).toBe(120); + expect(payload.error).toContain("timed out after 120ms"); + // And the upstream is released rather than left running. + await stallClosed; + }); + it("routes the outbound request through HTTP_PROXY (#2067)", async () => { // /api/fetch is the browser's ONLY way out to the network — the web // client's `environment.fetch` is `createRemoteFetch()`, which forwards diff --git a/core/auth/index.ts b/core/auth/index.ts index 6dd49b3b..4cfde438 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -54,9 +54,12 @@ export { // OAuth-path request deadline (#2319) export { DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + OAUTH_TIMEOUT_WIRE_CODE, OAuthRequestTimeoutError, + isOAuthRequestTimeoutWire, withOAuthRequestTimeout, } from "./requestTimeout.js"; +export type { OAuthRequestTimeoutWire } from "./requestTimeout.js"; // Storage export type { diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 7c2dadf0..65372ddc 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -83,6 +83,40 @@ export class OAuthRequestTimeoutError extends Error { } } +/** + * How a proxy-side deadline is carried back to the browser (#2319). + * + * `/api/fetch` serializes its failures as a JSON error response, and + * `createRemoteFetch` turns any non-OK answer into a plain `Error` — so a + * timeout enforced by the *backend* would arrive as an untyped error and the + * `instanceof OAuthRequestTimeoutError` checks downstream would all be false + * (Copilot). That matters most exactly where there is no client-side wrapper to + * have fired first: the discovery the SDK runs from inside the transport. So + * the route stamps this marker and `createRemoteFetch` reconstructs the typed + * error from it. + */ +export const OAUTH_TIMEOUT_WIRE_CODE = "oauth_request_timeout"; + +/** The body `/api/fetch` returns when its own deadline fired. */ +export interface OAuthRequestTimeoutWire { + code: typeof OAUTH_TIMEOUT_WIRE_CODE; + url: string; + timeoutMs: number; +} + +/** Whether a parsed `/api/fetch` error body is a proxy-side deadline. */ +export function isOAuthRequestTimeoutWire( + value: unknown, +): value is OAuthRequestTimeoutWire { + if (typeof value !== "object" || value === null) return false; + const candidate = value as Record; + return ( + candidate.code === OAUTH_TIMEOUT_WIRE_CODE && + typeof candidate.url === "string" && + typeof candidate.timeoutMs === "number" + ); +} + /** The request URL, whatever form `fetch`'s first argument took. */ function requestUrlOf(input: RequestInfo | URL): string { if (typeof input === "string") return input; @@ -187,12 +221,20 @@ export function withOAuthRequestTimeout( fetchFn: typeof fetch, timeoutMs: number = DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, ): typeof fetch { - // Whole milliseconds, because `setTimeout` and `AbortSignal.timeout` both - // take an integer: Node throws `ERR_OUT_OF_RANGE` on a fractional delay — - // before the fetch, so the request is never sent and the caller sees a failed - // operation rather than a timeout. A budget derived from `performance.now()` - // is fractional, so this is reachable. `revocation.ts:253` carries the same - // note for the same reason. + // Whole milliseconds. Not for the reason `revocation.ts:253` gives — that one + // calls `AbortSignal.timeout`, which really does throw `ERR_OUT_OF_RANGE` on + // a fractional delay (verified on Node 26) and so would fail before the + // request was sent. This wrapper drives its deadline with `setTimeout` and an + // `AbortController`, and `setTimeout` accepts a fractional delay and + // truncates it, so nothing here would throw (Copilot). + // + // The real reason is that the budget is *reported*: it is interpolated into + // the timeout message and exposed as `OAuthRequestTimeoutError.timeoutMs`. A + // caller whose budget came from a `performance.now()` subtraction would + // otherwise produce "timed out after 1000.4000000953674ms" and a non-integer + // public field. Rounding rather than flooring so a caller's own + // whole-millisecond timeout survives the trip through that clock and is still + // the number the message names. const budget = Math.max(0, Math.round(timeoutMs)); return async (input, init) => { diff --git a/core/mcp/remote/createRemoteFetch.ts b/core/mcp/remote/createRemoteFetch.ts index 36bbba2d..bd54db8c 100644 --- a/core/mcp/remote/createRemoteFetch.ts +++ b/core/mcp/remote/createRemoteFetch.ts @@ -21,6 +21,11 @@ * the web client against a strict modern server, same as from the CLI/TUI. */ +import { + isOAuthRequestTimeoutWire, + OAuthRequestTimeoutError, +} from "../../auth/requestTimeout.js"; + export interface RemoteFetchOptions { /** Base URL of the remote server (e.g. http://localhost:3000) */ baseUrl: string; @@ -158,6 +163,22 @@ export function createRemoteFetch(options: RemoteFetchOptions): typeof fetch { if (!res.ok) { const text = await res.text(); + // A deadline the backend enforced arrives as an ordinary error response; + // rebuild the typed error from its marker so the `instanceof` checks + // downstream — most importantly the one that lets a stalled probe escape + // `withRfc8414OidcCompat` — still see a timeout for what it is (#2319, + // Copilot). This is the only path on which such a timeout can reach the + // browser without a client-side wrapper having fired first: the discovery + // the SDK runs from inside the transport has no wrapper at all. + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch { + parsed = undefined; + } + if (isOAuthRequestTimeoutWire(parsed)) { + throw new OAuthRequestTimeoutError(parsed.url, parsed.timeoutMs); + } throw new Error(`Remote fetch failed (${res.status}): ${text}`); } diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 42c20fbf..3e8a121b 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -28,7 +28,10 @@ import { bodyLimit } from "hono/body-limit"; import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; import { createProxyFetch } from "../../node/proxyFetch.js"; -import { DEFAULT_OAUTH_REQUEST_TIMEOUT_MS } from "../../../auth/requestTimeout.js"; +import { + DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + OAUTH_TIMEOUT_WIRE_CODE, +} from "../../../auth/requestTimeout.js"; import type { RemoteConnectRequest, RemoteSendRequest, @@ -235,6 +238,21 @@ export interface RemoteServerOptions { /** Optional path for the user's server list file (/api/servers). Default: ~/.mcp-inspector/mcp.json */ mcpConfigPath?: string; + /** + * Deadline for the outbound request `/api/fetch` makes, in milliseconds + * (#2319). Defaults to {@link DEFAULT_OAUTH_REQUEST_TIMEOUT_MS}, the same + * budget the browser-side `withOAuthRequestTimeout` uses, so the two agree on + * when an attempt is hopeless. + * + * This is the backstop for a client that vanishes without aborting; the + * primary mechanism is the caller's own signal, which `createRemoteFetch` + * forwards onto this hop. Exposed rather than hardcoded so a test can prove + * the backstop actually fires without spending thirty seconds of wall clock + * on it — and because a very slow authorization server is a legitimate reason + * to want a longer one. + */ + fetchTimeoutMs?: number; + /** * When false, the server list is read-only for this session: all * `/api/servers` mutations (POST/PUT/DELETE/order) are rejected with 403, @@ -570,6 +588,10 @@ export function createRemoteApp( options: RemoteServerOptions, ): CreateRemoteAppResult { const dangerouslyOmitAuth = !!options.dangerouslyOmitAuth; + const fetchTimeoutMs = Math.max( + 0, + Math.round(options.fetchTimeoutMs ?? DEFAULT_OAUTH_REQUEST_TIMEOUT_MS), + ); // Determine auth token when auth is enabled: options > env var > generate const authToken = dangerouslyOmitAuth @@ -1227,6 +1249,11 @@ export function createRemoteApp( // client that vanishes without aborting, and it matches the client-side // budget so the two agree on when this is hopeless. const controller = new AbortController(); + // Set by the timer below and read in the `catch`. The fetch rejects with + // the signal's abort reason, but which of several aborts won is not worth + // inferring from the error — the flag says it directly, the same + // normalization `withOAuthRequestTimeout` uses. + let deadlineFired = false; const clientSignal: AbortSignal | undefined = c.req.raw.signal; const signal = clientSignal ? AbortSignal.any([controller.signal, clientSignal]) @@ -1235,15 +1262,14 @@ export function createRemoteApp( // the body has been either read or explicitly cancelled — this route never // hands a live stream to its caller, so there is nothing left for the timer // to protect and nothing it could sever. - const timer = setTimeout( - () => - controller.abort( - new Error( - `proxied request to ${url} timed out after ${DEFAULT_OAUTH_REQUEST_TIMEOUT_MS}ms`, - ), + const timer = setTimeout(() => { + deadlineFired = true; + controller.abort( + new Error( + `proxied request to ${url} timed out after ${fetchTimeoutMs}ms`, ), - DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, - ); + ); + }, fetchTimeoutMs); try { // Proxy-aware, not the bare global. This route is the browser's ONLY way @@ -1296,6 +1322,21 @@ export function createRemoteApp( }); } catch (err) { const msg = err instanceof Error ? err.message : String(err); + if (deadlineFired) { + // Stamped so the browser can rebuild an `OAuthRequestTimeoutError` + // rather than receiving a plain `Error` that no `instanceof` check + // downstream can recognize (Copilot). 504 because that is what this + // is — the gateway gave up on the upstream, not a fault in the route. + return c.json( + { + error: msg, + code: OAUTH_TIMEOUT_WIRE_CODE, + url, + timeoutMs: fetchTimeoutMs, + }, + 504, + ); + } return c.json({ error: msg }, 500); } finally { clearTimeout(timer); From 19718c540279b4cb2c2450eb4ac1f0434cfc63e4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 04:31:36 -0400 Subject: [PATCH 09/23] chore: file header and forwarding mocks on the new tests (#2319 round 10) requestTimeout.test.ts had no file-level header, which AGENTS.md requires of every file; it now explains what each group of cases pins and why, including why the composition cases are written as a contrast. The two mock fetches I added used `as unknown as typeof fetch`, which AGENTS.md rules out without an inline justification. A contextually typed forwarding function keeps the vitest mock inspectable and needs no cast at all. The PR description still described the pre-fix cancellation flow; it is rewritten to say that the signal now reaches the backend's outbound fetch and what the race is still for. 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 | 23 +++++++++++++++++++ .../core/mcp/remote/createRemoteFetch.test.ts | 7 ++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 6ff65333..0bf8dfa0 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -1,3 +1,26 @@ +/** + * Unit coverage for `withOAuthRequestTimeout` (#2319) — the deadline every + * OAuth-path request runs under. + * + * What is pinned here, and why each case exists rather than being obvious: + * + * - **The bound itself**, including the *body*: `fetch` resolves on headers, so + * a server that sends them and then stalls has to be caught by the buffering + * race, not by the fetch promise. + * - **The rebuilt response.** Buffering means the caller gets a different + * `Response` object, so `url` / `redirected` / `type` are asserted to survive + * and `content-encoding` / `content-length` to be dropped — the buffer holds + * the decoded body, which makes both of those headers lies. + * - **Cancellation semantics.** The caller's signal is forwarded *and* raced, + * so the tests distinguish the two: one uses a fetch that honours the signal, + * another uses one that ignores it entirely. + * - **Composition with `withRfc8414OidcCompat`**, written as a contrast between + * the two orderings. It is the ordering that is under test, not one + * arrangement's behaviour, so the wrong one is asserted to misattribute. + * + * Fake timers throughout: every budget here is exercised by advancing the clock + * rather than by waiting, so the suite costs milliseconds. + */ import { describe, it, expect, vi, afterEach } from "vitest"; import { DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, 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 12de59ce..f5be8fb0 100644 --- a/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts +++ b/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts @@ -183,7 +183,9 @@ describe("createRemoteFetch", () => { .mockResolvedValue(ok(standardRemoteBody)); const remoteFetch = createRemoteFetch({ baseUrl: "http://remote.example", - fetchFn: fetchFn as unknown as typeof fetch, + // A contextually typed forwarding function rather than a double cast: + // the mock stays inspectable and nothing bypasses the type system. + fetchFn: (input, init) => fetchFn(input, init), }); return { fetchFn, remoteFetch }; } @@ -256,7 +258,8 @@ describe("createRemoteFetch", () => { ); return createRemoteFetch({ baseUrl: "http://remote.example", - fetchFn: fetchFn as unknown as typeof fetch, + // Forwarding function, not a double cast — see `capture` above. + fetchFn: (input, init) => fetchFn(input, init), }); } From f491c7fc9a170590e41b17007b1d9f5fd94b161b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 04:49:05 -0400 Subject: [PATCH 10/23] 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 From 234d0a9ac43a35c43178867e60ec82c0ee6039ba Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 05:07:31 -0400 Subject: [PATCH 11/23] fix: preserve Response enumerability, refresh transport docs (#2319 round 12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copying url/redirected/type onto the rebuilt response as enumerable own properties changed what a consumer sees: `Object.keys`, object spread and `JSON.stringify` are all affected. Which answer is correct turns out to be the host's business — under the Fetch standard these are prototype getters with no own enumerable key, while happy-dom defines them as own enumerable data properties — so the descriptor is copied from the freshly built Response rather than either answer being hardcoded. The test compares against a baseline built in the same runtime for the same reason. Three comments still described the transport fetch as unbounded, which round 11 changed: - the wrapper's own docblock said it must never wrap the transport, obscuring that isExempt is what makes doing so safe; - the auth-chain comment in inspectorClient repeated it; - buildEffectiveAuthFetch's docblock said transport-internal discovery is not covered by a deadline. All three now state the real distinction — both chains are bounded, and they differ only in that the transport chain exempts the MCP endpoint — and the docblock says why reusing the transport chain here would be wrong rather than merely different. 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 | 10 +++++++ core/auth/requestTimeout.ts | 27 ++++++++++++++----- core/mcp/inspectorClient.ts | 22 ++++++++++----- 3 files changed, 45 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 8c8be1cd..0738add7 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -305,6 +305,16 @@ describe("withOAuthRequestTimeout", () => { expect(response.url).toBe("https://as.example.com/redirected"); expect(response.redirected).toBe(true); expect(response.type).toBe("cors"); + // The rebuild must not change what enumerates. Which keys a `Response` owns + // is the host's business — under the Fetch standard these three are + // prototype getters and a native response has no own enumerable keys, while + // happy-dom makes them own enumerable data properties — so this compares + // against a baseline built in the same runtime rather than asserting either + // answer. Hard-coding one would make the rebuilt response observably + // different from a response that never passed through the wrapper, in + // `Object.keys`, object spread and `JSON.stringify`. + const baseline = Object.keys(new Response("{}")).sort(); + expect(Object.keys(response).sort()).toEqual(baseline); await expect(response.json()).resolves.toEqual({ issuer: "https://as.example.com", }); diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index d58d06e9..1f1df46c 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -220,9 +220,19 @@ function rebuildResponse(response: Response, body: ArrayBuffer): Response { }, ); for (const key of ["url", "redirected", "type"] as const) { + // Enumerability is taken from whatever the freshly built `Response` already + // has for this key, rather than asserted. Hard-coding either answer makes + // the rebuilt response observably different from a native one — and which + // answer is right is the host's business: under the Fetch standard (and + // undici) these are prototype getters with no own enumerable key at all, + // while happy-dom defines them as own enumerable data properties. Copying + // the descriptor keeps `Object.keys`, object spread and `JSON.stringify` + // identical to a response that never passed through here, in both + // (Copilot). `false` for the getter case, where there is no own descriptor. + const existing = Object.getOwnPropertyDescriptor(rebuilt, key); Object.defineProperty(rebuilt, key, { value: response[key], - enumerable: true, + enumerable: existing?.enumerable ?? false, configurable: true, }); } @@ -232,12 +242,15 @@ function rebuildResponse(response: Response, body: ArrayBuffer): Response { /** * Wrap a `fetch` so every call through it is bounded by `timeoutMs`. * - * Apply this to an **OAuth-path** fetch only. It must never wrap the transport - * fetch: a Streamable HTTP or SSE response is a long-lived stream that is - * *supposed* to stay open, and a deadline there would sever every connection - * after the budget. `InspectorClient` keeps the two apart by wrapping inside - * `buildEffectiveAuthFetch` rather than wrapping `this.fetchFn`, which the - * transport is handed directly. + * Every request that reaches it unexempted is bounded — so on a chain that + * carries anything other than OAuth work, `isExempt` is not optional. A + * Streamable HTTP or SSE response is a long-lived stream that is *supposed* to + * stay open, and a Streamable HTTP POST for a long-running tool call + * legitimately withholds its response headers for minutes; a deadline over + * either severs a working connection. `InspectorClient` uses the wrapper on + * both of its chains and the two differ only in this: the OAuth chain passes no + * predicate because every request on it is OAuth work, while the transport + * chain passes `exemptMcpEndpoint`. * * `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 diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 31d6b311..94abbc87 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -849,8 +849,9 @@ export class InspectorClient extends InspectorClientEventTarget { // URL of the RFC 8414 request that preceded it — defeating the whole point // of naming the endpoint — while every probe in the loop shared that one // request's budget (Copilot). Innermost here matches the CLI's - // `storedAuthFetch`. The transport keeps `this.fetchFn`, deliberately - // unbounded: its bodies are streams that are supposed to stay open. + // `storedAuthFetch`. It also passes no `isExempt`, unlike the transport + // chain above: every request on this chain is OAuth work, so there is + // nothing here to exempt. this.effectiveAuthFetch = this.buildEffectiveAuthFetch( withRfc8414OidcCompat(withOAuthRequestTimeout(withOverrides)), ); @@ -1022,11 +1023,18 @@ export class InspectorClient extends InspectorClientEventTarget { /** * @param base - the composed OAuth-path fetch, deadline innermost (#2319). - * Passed in rather than read off `this.fetchFn`, which is the *transport* - * chain and must stay unbounded. The cost of keeping the two apart is that - * the discovery the SDK runs from inside the transport (the 401/refresh path) - * is not covered by the deadline; that leg sits inside an SDK request and is - * bounded by its per-request timeout. + * + * Passed in rather than read off `this.fetchFn` because the two chains are + * bounded on different terms, not because one of them is unbounded. Both + * carry a deadline; the transport chain additionally exempts the MCP endpoint + * (`exemptMcpEndpoint`), since its bodies are streams meant to stay open and + * a long-running tool call may withhold its headers for minutes. Reusing it + * here would extend that exemption to the OAuth manager's own requests — and + * an authorization server published at the MCP endpoint's path would then go + * unbounded on the one chain that exists to bound it. + * + * The SDK's transport-internal discovery (the 401/refresh path) is covered by + * the transport chain's own deadline, not by this one. */ private buildEffectiveAuthFetch(base: typeof fetch): typeof fetch { // Capture auth response bodies (OAuth discovery, DCR, token exchange) so From de6960ec5a6ce3cf4620fd83082f1a4d47fbba6d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 05:24:24 -0400 Subject: [PATCH 12/23] fix: make the proxy deadline per-request, not unconditional (#2319 round 13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server-side timer added in round 9 applied to every /api/fetch request. But that route carries MCP traffic as well as OAuth work, so a Streamable HTTP tool call that legitimately withholds its response headers past the budget was aborted there — reintroducing on the backend exactly the regression exemptMcpEndpoint prevents on the client, and reporting it as an OAuth timeout besides. The caller now decides. `withOAuthRequestTimeout` stamps its budget on the RequestInit it hands down, `createRemoteFetch` reads it back and puts it in the /api/fetch envelope, and the route applies a timer only when one is present — so an exempt MCP request, which never gets an init of its own, is bounded by nothing. The carrier is a WeakMap keyed on the init rather than a header: serializeRequest copies request headers verbatim into the payload and the route re-sends them to the authorization server, so a marker header would leak to a third party. It needs no cast onto RequestInit and is collected with the object. The `fetchTimeoutMs` option from round 9 is gone — the deadline now lives in exactly one place, the client wrapper, which is also what makes the 504 marker honest: only a request the OAuth wrapper bounded can produce one. Ten tests: the stamping and its rounding, nothing stamped on an exempt call or an init that never went through the wrapper, the envelope carrying the budget (and not a header), its omission on both exempt and direct calls, and at the route the 504 marker, no deadline without one, and a non-numeric deadline ignored. The no-deadline case is verified by mutation: an unconditional timer makes it fail. 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 | 41 ++++++++++ .../core/mcp/remote/createRemoteFetch.test.ts | 58 ++++++++++++++ .../mcp/remote/server-extra-coverage.test.ts | 80 +++++++++++++++---- core/auth/index.ts | 1 + core/auth/requestTimeout.ts | 40 +++++++++- core/mcp/remote/createRemoteFetch.ts | 19 ++++- core/mcp/remote/node/server.ts | 70 ++++++++-------- 7 files changed, 259 insertions(+), 50 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 0738add7..dbd89bcc 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, + deadlineForRequestInit, exemptMcpEndpoint, withOAuthRequestTimeout, } from "@inspector/core/auth/requestTimeout.js"; @@ -414,6 +415,46 @@ describe("withOAuthRequestTimeout", () => { }); }); + describe("the deadline stamped on the init (#2319 proxy hop)", () => { + // How the budget reaches `/api/fetch` without travelling upstream: a header + // would be copied verbatim into the payload and re-sent to the + // authorization server, so the carrier is a WeakMap keyed on the init. + it("stamps the budget on the init it hands down", async () => { + const inner = vi.fn().mockResolvedValue(new Response("{}")); + const wrapped = withOAuthRequestTimeout(inner, 1234); + + await wrapped(URL_UNDER_TEST); + + expect(deadlineForRequestInit(inner.mock.calls[0][1])).toBe(1234); + }); + + it("stamps the rounded budget, matching what is reported", async () => { + const inner = vi.fn().mockResolvedValue(new Response("{}")); + const wrapped = withOAuthRequestTimeout(inner, 1000.4); + + await wrapped(URL_UNDER_TEST); + + expect(deadlineForRequestInit(inner.mock.calls[0][1])).toBe(1000); + }); + + it("stamps nothing on an exempt request", async () => { + // The route must apply no deadline to MCP traffic, and it decides that + // from the absence of a budget in the envelope. + const inner = vi.fn().mockResolvedValue(new Response("{}")); + const wrapped = withOAuthRequestTimeout(inner, 1234, () => true); + + await wrapped(URL_UNDER_TEST, { method: "POST" }); + + expect(deadlineForRequestInit(inner.mock.calls[0][1])).toBeUndefined(); + }); + + it("reads nothing off an init that never went through the wrapper", () => { + expect(deadlineForRequestInit(undefined)).toBeUndefined(); + expect(deadlineForRequestInit({})).toBeUndefined(); + expect(deadlineForRequestInit({ method: "GET" })).toBeUndefined(); + }); + }); + describe("exemptMcpEndpoint (the transport chain's mixed traffic)", () => { const SERVER = "https://srv.example.com/mcp"; 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 0eaa397b..37e71d9d 100644 --- a/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts +++ b/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts @@ -3,6 +3,7 @@ import { createRemoteFetch } from "@inspector/core/mcp/remote/createRemoteFetch. import { OAUTH_TIMEOUT_WIRE_CODE, OAuthRequestTimeoutError, + withOAuthRequestTimeout, } from "@inspector/core/auth/requestTimeout.js"; function ok(body: object): Response { @@ -334,4 +335,61 @@ describe("createRemoteFetch", () => { expect(err).not.toBeInstanceOf(OAuthRequestTimeoutError); }); }); + + describe("carrying the caller's deadline to the route (#2319)", () => { + function envelopeOf(fetchFn: ReturnType>) { + const init = fetchFn.mock.calls[0][1] as RequestInit; + return JSON.parse(init.body as string) as Record; + } + + it("puts a bounded call's budget in the envelope, not in the headers", async () => { + const fetchFn = vi + .fn() + .mockResolvedValue(ok(standardRemoteBody)); + const remoteFetch = createRemoteFetch({ + baseUrl: "http://remote.example", + fetchFn: (input, init) => fetchFn(input, init), + }); + // Composed the way `InspectorClient` composes it: the wrapper outside, + // the remote fetch beneath. + const bounded = withOAuthRequestTimeout(remoteFetch, 7000); + + await bounded("http://upstream.example/token"); + + const envelope = envelopeOf(fetchFn); + expect(envelope.timeoutMs).toBe(7000); + // Not a header: `headers` is re-sent verbatim to the upstream server, so + // a marker there would leak the Inspector's internals to a third party. + expect(JSON.stringify(envelope.headers)).not.toContain("7000"); + }); + + it("omits the budget for an exempt call, so the route bounds nothing", async () => { + const fetchFn = vi + .fn() + .mockResolvedValue(ok(standardRemoteBody)); + const remoteFetch = createRemoteFetch({ + baseUrl: "http://remote.example", + fetchFn: (input, init) => fetchFn(input, init), + }); + const bounded = withOAuthRequestTimeout(remoteFetch, 7000, () => true); + + await bounded("http://upstream.example/mcp"); + + expect(envelopeOf(fetchFn)).not.toHaveProperty("timeoutMs"); + }); + + it("omits the budget for a direct call that no wrapper bounded", async () => { + const fetchFn = vi + .fn() + .mockResolvedValue(ok(standardRemoteBody)); + const remoteFetch = createRemoteFetch({ + baseUrl: "http://remote.example", + fetchFn: (input, init) => fetchFn(input, init), + }); + + await remoteFetch("http://upstream.example/mcp"); + + expect(envelopeOf(fetchFn)).not.toHaveProperty("timeoutMs"); + }); + }); }); diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 9613d2af..09b756e8 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -51,7 +51,6 @@ interface StartOpts { secretStore?: SecretStore; logger?: pinoType.Logger; seedConfig?: string; - fetchTimeoutMs?: number; } async function start(opts: StartOpts = {}): Promise { @@ -66,9 +65,6 @@ async function start(opts: StartOpts = {}): Promise { initialConfig: { defaultEnvironment: {} }, secretStore: opts.secretStore ?? new InMemorySecretStore(), logger: opts.logger, - ...(opts.fetchTimeoutMs !== undefined && { - fetchTimeoutMs: opts.fetchTimeoutMs, - }), }); const { baseUrl, server } = await new Promise<{ baseUrl: string; @@ -475,19 +471,16 @@ describe("server.ts supplemental coverage", () => { await openStreamClosed; }); - it("answers 504 with a typed marker when its own deadline fires (#2319)", async () => { - // The backstop, on its own short budget so this costs milliseconds rather - // than the production thirty seconds. It matters because this is the only - // deadline on the transport-internal discovery path, which has no - // client-side wrapper — so its error has to survive the hop as something - // an `instanceof OAuthRequestTimeoutError` check can still recognize. - await stop(h); - h = await start({ fetchTimeoutMs: 120 }); - + it("answers 504 with a typed marker when the request's deadline fires (#2319)", async () => { + // The caller's budget travels in the envelope, so this costs milliseconds + // rather than the production thirty seconds. The marker matters because + // a backend-enforced deadline is the only one on the transport-internal + // discovery path, and its error has to survive the hop as something an + // `instanceof OAuthRequestTimeoutError` check can still recognize. const res = await fetch(`${h.baseUrl}/api/fetch`, { method: "POST", headers: { "content-type": "application/json" }, - body: JSON.stringify({ url: `${targetUrl}/stall` }), + body: JSON.stringify({ url: `${targetUrl}/stall`, timeoutMs: 120 }), }); expect(res.status).toBe(504); @@ -505,6 +498,65 @@ describe("server.ts supplemental coverage", () => { await stallClosed; }); + it("applies no deadline to a request that carries none (#2319)", async () => { + // ⚠️ The regression this guards: an unconditional timer here would abort + // a Streamable HTTP tool call that legitimately withholds its response + // headers — reintroducing on the backend exactly what `exemptMcpEndpoint` + // prevents on the client, and reporting it as an OAuth timeout besides. + const caller = new AbortController(); + const pending = fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/stall` }), + signal: caller.signal, + }); + const settled = pending.then( + (r) => `responded ${r.status}`, + () => "aborted by us", + ); + + await vi.waitFor(() => { + expect(upstreamHits).toContain("/stall"); + }); + // Well past any budget the route might have applied on its own. Nothing + // should have answered: an unbounded request is still in flight. + await new Promise((r) => setTimeout(r, 400)); + expect( + await Promise.race([settled, Promise.resolve("still pending")]), + ).toBe("still pending"); + + caller.abort(); + expect(await settled).toBe("aborted by us"); + }); + + it("ignores a non-numeric deadline rather than trusting it (#2319)", async () => { + const caller = new AbortController(); + const pending = fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + url: `${targetUrl}/stall`, + timeoutMs: "soon", + }), + signal: caller.signal, + }); + const settled = pending.then( + (r) => `responded ${r.status}`, + () => "aborted by us", + ); + + await vi.waitFor(() => { + expect(upstreamHits).toContain("/stall"); + }); + await new Promise((r) => setTimeout(r, 300)); + expect( + await Promise.race([settled, Promise.resolve("still pending")]), + ).toBe("still pending"); + + caller.abort(); + expect(await settled).toBe("aborted by us"); + }); + it("routes the outbound request through HTTP_PROXY (#2067)", async () => { // /api/fetch is the browser's ONLY way out to the network — the web // client's `environment.fetch` is `createRemoteFetch()`, which forwards diff --git a/core/auth/index.ts b/core/auth/index.ts index e38812eb..918eaac3 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, + deadlineForRequestInit, exemptMcpEndpoint, isOAuthRequestTimeoutWire, withOAuthRequestTimeout, diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 1f1df46c..d74bff90 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -148,6 +148,39 @@ export function exemptMcpEndpoint( }; } +/** + * The deadline a wrapped call is running under, keyed by the exact `RequestInit` + * the wrapper handed down (#2319). + * + * This is how the budget crosses the proxy hop without travelling *upstream*. + * `/api/fetch` must not time every request it serves — `createRemoteFetch` also + * carries MCP traffic, and a Streamable HTTP tool call can legitimately withhold + * its response headers for minutes — so the route needs to know which requests + * are bounded and which are not (Copilot). A header would be the obvious + * carrier and is the wrong one: `serializeRequest` copies request headers + * verbatim into the payload and the route re-sends them to the authorization + * server, so a marker header would leak to a third party. + * + * A `WeakMap` keyed on the init object has neither problem. It is invisible to + * serialization, it needs no cast onto `RequestInit`, and it is collected with + * the object — the wrapper builds a fresh init per call, so there is one entry + * per in-flight request and nothing accumulates. + */ +const REQUEST_DEADLINES = new WeakMap(); + +/** + * The deadline `withOAuthRequestTimeout` stamped on this init, if any. + * + * `undefined` means the request is not bounded by this wrapper and must not be + * bounded by anything downstream either — an exempt MCP request reads as + * `undefined` because the exempt path never builds an init of its own. + */ +export function deadlineForRequestInit( + init: RequestInit | undefined, +): number | undefined { + return init ? REQUEST_DEADLINES.get(init) : undefined; +} + /** The request URL, whatever form `fetch`'s first argument took. */ function requestUrlOf(input: RequestInfo | URL): string { if (typeof input === "string") return input; @@ -342,9 +375,14 @@ export function withOAuthRequestTimeout( } }); + // Built once and stamped, so `createRemoteFetch` can read the budget off it + // and tell `/api/fetch` this particular request is bounded. + const nextInit: RequestInit = { ...init, signal }; + REQUEST_DEADLINES.set(nextInit, budget); + try { const response = await Promise.race([ - fetchFn(input, { ...init, signal }), + fetchFn(input, nextInit), abandoned, ]); // `fetch` resolves once the response *headers* arrive, so stopping here diff --git a/core/mcp/remote/createRemoteFetch.ts b/core/mcp/remote/createRemoteFetch.ts index bd54db8c..144f6681 100644 --- a/core/mcp/remote/createRemoteFetch.ts +++ b/core/mcp/remote/createRemoteFetch.ts @@ -22,6 +22,7 @@ */ import { + deadlineForRequestInit, isOAuthRequestTimeoutWire, OAuthRequestTimeoutError, } from "../../auth/requestTimeout.js"; @@ -154,10 +155,26 @@ export function createRemoteFetch(options: RemoteFetchOptions): typeof fetch { ? input.signal : undefined; + // #2319: tell the route whether *this* request is bounded, and by how much. + // The route serves MCP traffic as well as OAuth work, and a Streamable HTTP + // tool call can legitimately withhold its response headers for minutes — so + // a timer there must be per-request rather than unconditional (Copilot). An + // exempt request carries no deadline and the route applies none. + // + // In the JSON envelope rather than a header: `headers` is re-sent verbatim + // to the upstream server, so a marker header would leak to a third party. + const timeoutMs = deadlineForRequestInit(init); + const res = await fetchFn(`${baseUrl}/api/fetch`, { method: "POST", headers: reqHeaders, - body: JSON.stringify({ url, method, headers, body }), + body: JSON.stringify({ + url, + method, + headers, + body, + ...(timeoutMs !== undefined && { timeoutMs }), + }), signal, }); diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 3e8a121b..b584bb1c 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -28,10 +28,7 @@ import { bodyLimit } from "hono/body-limit"; import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; import { createProxyFetch } from "../../node/proxyFetch.js"; -import { - DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, - OAUTH_TIMEOUT_WIRE_CODE, -} from "../../../auth/requestTimeout.js"; +import { OAUTH_TIMEOUT_WIRE_CODE } from "../../../auth/requestTimeout.js"; import type { RemoteConnectRequest, RemoteSendRequest, @@ -238,21 +235,6 @@ export interface RemoteServerOptions { /** Optional path for the user's server list file (/api/servers). Default: ~/.mcp-inspector/mcp.json */ mcpConfigPath?: string; - /** - * Deadline for the outbound request `/api/fetch` makes, in milliseconds - * (#2319). Defaults to {@link DEFAULT_OAUTH_REQUEST_TIMEOUT_MS}, the same - * budget the browser-side `withOAuthRequestTimeout` uses, so the two agree on - * when an attempt is hopeless. - * - * This is the backstop for a client that vanishes without aborting; the - * primary mechanism is the caller's own signal, which `createRemoteFetch` - * forwards onto this hop. Exposed rather than hardcoded so a test can prove - * the backstop actually fires without spending thirty seconds of wall clock - * on it — and because a very slow authorization server is a legitimate reason - * to want a longer one. - */ - fetchTimeoutMs?: number; - /** * When false, the server list is read-only for this session: all * `/api/servers` mutations (POST/PUT/DELETE/order) are rejected with 403, @@ -588,10 +570,6 @@ export function createRemoteApp( options: RemoteServerOptions, ): CreateRemoteAppResult { const dangerouslyOmitAuth = !!options.dangerouslyOmitAuth; - const fetchTimeoutMs = Math.max( - 0, - Math.round(options.fetchTimeoutMs ?? DEFAULT_OAUTH_REQUEST_TIMEOUT_MS), - ); // Determine auth token when auth is enabled: options > env var > generate const authToken = dangerouslyOmitAuth @@ -1227,6 +1205,8 @@ export function createRemoteApp( method?: string; headers?: Record; body?: string; + /** Per-request deadline, set only by a bounded OAuth call (#2319). */ + timeoutMs?: number; }; try { body = (await c.req.json()) as typeof body; @@ -1234,7 +1214,13 @@ export function createRemoteApp( return c.json({ error: "Invalid JSON body" }, 400); } - const { url, method = "GET", headers = {}, body: reqBody } = body; + const { + url, + method = "GET", + headers = {}, + body: reqBody, + timeoutMs: requestedTimeoutMs, + } = body; if (!url) { return c.json({ error: "Missing url" }, 400); } @@ -1262,14 +1248,30 @@ export function createRemoteApp( // the body has been either read or explicitly cancelled — this route never // hands a live stream to its caller, so there is nothing left for the timer // to protect and nothing it could sever. - const timer = setTimeout(() => { - deadlineFired = true; - controller.abort( - new Error( - `proxied request to ${url} timed out after ${fetchTimeoutMs}ms`, - ), - ); - }, fetchTimeoutMs); + // ⚠️ Only the caller decides whether this request is bounded, and by how + // much. This route is the browser's way out to the network for MCP traffic + // as well as OAuth work, so an unconditional timer here would abort a + // Streamable HTTP tool call that legitimately withholds its response + // headers for longer than the budget — reintroducing on the backend exactly + // the regression `exemptMcpEndpoint` exists to prevent on the client, and + // reporting it as an OAuth timeout besides (Copilot). No deadline in the + // envelope means no timer at all. + const deadlineMs = + typeof requestedTimeoutMs === "number" && + Number.isFinite(requestedTimeoutMs) + ? Math.max(0, Math.round(requestedTimeoutMs)) + : undefined; + const timer = + deadlineMs === undefined + ? undefined + : setTimeout(() => { + deadlineFired = true; + controller.abort( + new Error( + `proxied request to ${url} timed out after ${deadlineMs}ms`, + ), + ); + }, deadlineMs); try { // Proxy-aware, not the bare global. This route is the browser's ONLY way @@ -1332,14 +1334,14 @@ export function createRemoteApp( error: msg, code: OAUTH_TIMEOUT_WIRE_CODE, url, - timeoutMs: fetchTimeoutMs, + timeoutMs: deadlineMs, }, 504, ); } return c.json({ error: msg }, 500); } finally { - clearTimeout(timer); + if (timer !== undefined) clearTimeout(timer); } }); From 93965936ba220dd88ea52925f96b8c2e2e915b03 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 05:42:00 -0400 Subject: [PATCH 13/23] docs: describe the shipped timeout architecture (#2319 round 14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three comments still said transport-internal discovery has no client-side wrapper, or that the wrapper must never be applied to the transport fetch. Round 11 made both false, and the second actively tells a caller to do the opposite of what InspectorClient does — while hiding that isExempt is the safety-critical part. - createRemoteFetch's marker comment now gives the real reason the reconstruction matters: both ends run the same budget, so this path is the backend winning the race, most plausibly a backgrounded tab throttling setTimeout while the server's fires on schedule. Which deadline fired must not decide whether the error names its endpoint. - The body-buffering comment now says buffering is the second reason isExempt is not optional on a mixed chain, since an exempt request skips it and an SSE body stays open rather than being drained. - The matching test comment is reframed as testing the proxy boundary independently of which deadline wins. The PR description is rewritten around what is actually shipped: both chains bounded and differing only in the exemption, the per-request envelope that replaced the fetchTimeoutMs option, and the three mutation-verified tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../test/core/mcp/remote/createRemoteFetch.test.ts | 10 ++++++---- core/auth/requestTimeout.ts | 7 ++++--- core/mcp/remote/createRemoteFetch.ts | 11 ++++++++--- 3 files changed, 18 insertions(+), 10 deletions(-) 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 37e71d9d..27a42d61 100644 --- a/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts +++ b/clients/web/src/test/core/mcp/remote/createRemoteFetch.test.ts @@ -272,10 +272,12 @@ describe("createRemoteFetch", () => { } it("rebuilds an OAuthRequestTimeoutError from the route's marker", async () => { - // The one path where a timeout reaches the browser without a client-side - // wrapper having fired first: the discovery the SDK runs from inside the - // transport has no wrapper at all, so the backend's deadline is the only - // one, and it arrives as an ordinary error response. + // The proxy boundary in isolation: whichever end's deadline fires, a + // timeout that comes back as an ordinary error response must still + // reconstruct as a typed one. Both ends run the same budget in + // production, so this path is the backend winning the race — a + // backgrounded tab throttling `setTimeout` is the plausible case — and + // which one fired must not decide whether the error names its endpoint. const remoteFetch = remoteReturning( 504, JSON.stringify({ diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index d74bff90..28efdbc4 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -391,9 +391,10 @@ export function withOAuthRequestTimeout( // `response.json()` would hang with nothing watching it (Copilot). Every // response on this path is a small, finite document — metadata, // a registration, a token — so buffering it under the same deadline - // bounds the whole exchange. This is the other reason the wrapper must - // never be applied to the transport fetch, whose bodies are streams that - // are supposed to stay open. + // bounds the whole exchange. It is also the second reason `isExempt` is + // not optional on a mixed chain: an exempt request skips this buffering + // entirely, which is what lets an SSE body stay open rather than being + // drained into memory here. const body = await Promise.race([response.arrayBuffer(), abandoned]); return rebuildResponse(response, body); } catch (err) { diff --git a/core/mcp/remote/createRemoteFetch.ts b/core/mcp/remote/createRemoteFetch.ts index 144f6681..d0757956 100644 --- a/core/mcp/remote/createRemoteFetch.ts +++ b/core/mcp/remote/createRemoteFetch.ts @@ -184,9 +184,14 @@ export function createRemoteFetch(options: RemoteFetchOptions): typeof fetch { // rebuild the typed error from its marker so the `instanceof` checks // downstream — most importantly the one that lets a stalled probe escape // `withRfc8414OidcCompat` — still see a timeout for what it is (#2319, - // Copilot). This is the only path on which such a timeout can reach the - // browser without a client-side wrapper having fired first: the discovery - // the SDK runs from inside the transport has no wrapper at all. + // Copilot). + // + // Both ends run the same budget, so the client's race usually settles + // first and this path is not taken. It matters when the backend's timer + // wins anyway — most plausibly a backgrounded tab, where the browser + // throttles `setTimeout` while the server's fires on schedule. Without + // this, which of two equal deadlines happened to fire would decide + // whether the error carried its endpoint. let parsed: unknown; try { parsed = JSON.parse(text); From e234343702386248c8b9d20f3d19214326f76a8c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 06:06:36 -0400 Subject: [PATCH 14/23] fix: cancellable compat probes and a validated budget (#2319 round 16) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real bugs and two stale rationales. The OIDC fallback probes were issued with `{ headers }` and no signal, so a caller that aborted after the initial 404 got a timeout reported in place of its own cancellation, while the probe ran on to its own deadline. The probes now carry the caller's signal — via the shared `callerSignalOf`, so the precedence rule has one definition — a pre-aborted caller is not probed at all, and an abort is rethrown rather than folded back into the preceding response. `Math.max(0, Math.round(timeoutMs))` let NaN and Infinity through. `setTimeout(fn, NaN)` fires immediately, and a delay above 2**31-1 overflows a 32-bit signed int and falls back to 1ms — so a caller asking for a very large budget, or one whose arithmetic produced NaN, got an immediate OAuth timeout. `normalizeBudget` falls back to the default on non-finite and clamps to the timer's range; the route's envelope-supplied deadline gets the same clamp, but ignores a non-finite value outright since a malformed deadline reads safest as no deadline. The wire-marker docblock and the matching route test still said a backend deadline is the only one on the transport-internal discovery path, which round 11 changed. Both now give the real reason the marker matters: both ends run the same budget, and it is the backend winning the race — a backgrounded tab throttling setTimeout — that must not decide whether the error names its endpoint. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../core/auth/oidcDiscoveryCompat.test.ts | 79 +++++++++++++++++++ .../src/test/core/auth/requestTimeout.test.ts | 40 ++++++++++ .../mcp/remote/server-extra-coverage.test.ts | 10 ++- core/auth/oidcDiscoveryCompat.ts | 21 ++++- core/auth/requestTimeout.ts | 41 ++++++++-- core/mcp/remote/node/server.ts | 7 +- 6 files changed, 185 insertions(+), 13 deletions(-) diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index e3bf9d4d..acb354aa 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -439,3 +439,82 @@ describe("withRfc8414OidcCompat", () => { expect(base).toHaveBeenCalledTimes(2); }); }); + +describe("probe cancellation (#2319)", () => { + const RFC8414 = + "https://as.example.com/.well-known/oauth-authorization-server/tenant"; + + /** 404 on the RFC 8414 leg, then never answers the OIDC probe. */ + function stallingProbeFetch() { + return vi.fn((input, init) => { + if (String(input) === RFC8414) { + return Promise.resolve(new Response(null, { status: 404 })); + } + return new Promise((_resolve, reject) => { + // A real `fetch` rejects immediately on a signal that is already + // aborted, so the double is only useful if it does the same. + if (init?.signal?.aborted) { + reject(init.signal.reason as Error); + return; + } + init?.signal?.addEventListener("abort", () => + reject(init.signal?.reason as Error), + ); + }); + }); + } + + it("carries the caller's signal into the probe", async () => { + const inner = stallingProbeFetch(); + const wrapped = withRfc8414OidcCompat(inner); + const caller = new AbortController(); + + const settled = wrapped(RFC8414, { signal: caller.signal }).then( + (r) => r, + (e: unknown) => e, + ); + // Wait until the probe is actually in flight, so the abort exercises + // cancellation rather than the pre-aborted short-circuit. + await vi.waitFor(() => { + expect(inner).toHaveBeenCalledTimes(2); + }); + const reason = new Error("caller gave up"); + caller.abort(reason); + + // Not the preceding 404: substituting it would report a discovery failure + // for a request the caller deliberately cancelled. + expect(await settled).toBe(reason); + expect((inner.mock.calls[1][1] as RequestInit).signal).toBe(caller.signal); + }); + + it("does not issue a probe the caller already cancelled", async () => { + const inner = stallingProbeFetch(); + const wrapped = withRfc8414OidcCompat(inner); + const reason = new Error("already gone"); + + const settled = wrapped(RFC8414, { + signal: AbortSignal.abort(reason), + }).then( + (r) => r, + (e: unknown) => e, + ); + + expect(await settled).toBe(reason); + // Only the RFC 8414 leg, which the caller did make. + expect(inner).toHaveBeenCalledTimes(1); + }); + + it("still falls back to the original response on an ordinary probe failure", async () => { + const inner = vi.fn((input) => { + if (String(input) === RFC8414) { + return Promise.resolve(new Response(null, { status: 404 })); + } + return Promise.reject(new TypeError("network error")); + }); + const wrapped = withRfc8414OidcCompat(inner); + + const response = await wrapped(RFC8414); + + expect(response.status).toBe(404); + }); +}); diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index dbd89bcc..95f23034 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -387,6 +387,46 @@ describe("withOAuthRequestTimeout", () => { }); }); + it("falls back to the default on a non-finite budget", async () => { + vi.useFakeTimers(); + // `NaN` survives `Math.max(0, Math.round(NaN))` and `setTimeout(fn, NaN)` + // fires immediately, so without this every OAuth request under a budget + // that came out of bad arithmetic would fail at once with "timed out after + // NaNms". + for (const bad of [NaN, Infinity, -Infinity]) { + const wrapped = withOAuthRequestTimeout(neverSettles, bad); + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + + await vi.advanceTimersByTimeAsync(DEFAULT_OAUTH_REQUEST_TIMEOUT_MS - 1); + expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe( + "pending", + ); + + await vi.advanceTimersByTimeAsync(1); + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + timeoutMs: DEFAULT_OAUTH_REQUEST_TIMEOUT_MS, + }); + } + }); + + it("clamps an over-large budget to what setTimeout can schedule", async () => { + vi.useFakeTimers(); + // Past 2**31-1 the delay overflows a 32-bit signed int and Node falls back + // to 1ms — an immediate timeout, the opposite of what was asked for. + const wrapped = withOAuthRequestTimeout(neverSettles, 2 ** 40); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe( + "pending", + ); + + await vi.advanceTimersByTimeAsync(2_147_483_647); + expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({ + timeoutMs: 2_147_483_647, + }); + }); + it("clamps a negative budget to zero rather than throwing", async () => { vi.useFakeTimers(); const wrapped = withOAuthRequestTimeout(neverSettles, -1); diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 09b756e8..96e3bd15 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -473,10 +473,12 @@ describe("server.ts supplemental coverage", () => { it("answers 504 with a typed marker when the request's deadline fires (#2319)", async () => { // The caller's budget travels in the envelope, so this costs milliseconds - // rather than the production thirty seconds. The marker matters because - // a backend-enforced deadline is the only one on the transport-internal - // discovery path, and its error has to survive the hop as something an - // `instanceof OAuthRequestTimeoutError` check can still recognize. + // rather than the production thirty seconds. What it pins is typed-marker + // preservation when the *backend* wins the client-side race — both ends + // run the same budget, and a backgrounded tab throttling `setTimeout` is + // the plausible way the server's fires first. The error has to survive + // the hop as something an `instanceof OAuthRequestTimeoutError` check can + // still recognize, whichever end produced it. const res = await fetch(`${h.baseUrl}/api/fetch`, { method: "POST", headers: { "content-type": "application/json" }, diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index 4404adc0..06ba7f43 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -90,7 +90,7 @@ * wiring in `core/mcp/inspectorClient.ts`. */ -import { OAuthRequestTimeoutError } from "./requestTimeout.js"; +import { callerSignalOf, OAuthRequestTimeoutError } from "./requestTimeout.js"; import { OAuthMetadataSchema, OpenIdProviderDiscoveryMetadataSchema, @@ -249,6 +249,13 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { if (candidates.length === 0) return response; const headers = discoveryHeaders(input, init); + // The probe is a request the caller never made, issued on its behalf — so + // it has to be cancellable by the caller too. Without the signal, aborting + // after the initial 404 left the probe running to its own deadline and the + // caller was handed a timeout in place of its own abort (Copilot). Same + // precedence rule as everywhere else, hence the shared helper rather than a + // second copy of it. + const callerSignal = callerSignalOf(input, init); // The loop advances to the next candidate only where the SDK's own loop // would. Anywhere else it hands the original response back and lets // discovery run its normal course, because *promoting a later candidate @@ -260,9 +267,15 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // those, returning `response` leaves the SDK to make the same request and // reach the same verdict it always would. for (const candidate of candidates) { + // Don't issue a probe the caller has already given up on. A real `fetch` + // rejects immediately on a pre-aborted signal, so this is mostly about + // not sending the request at all — the same reasoning as the + // short-circuit in `withOAuthRequestTimeout`. + if (callerSignal?.aborted) throw callerSignal.reason; + let probe: Response; try { - probe = await fetchFn(candidate, { headers }); + probe = await fetchFn(candidate, { headers, signal: callerSignal }); } catch (err) { // A deadline the Inspector imposed is *our* error, not the server's // answer, so it escapes rather than being folded back into the original @@ -275,6 +288,10 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // stall the SDK would have hit itself, unbounded. Everything else — a // CORS rejection, DNS, a reset — still falls back as documented above. 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; return response; } if (!probe.ok) { diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 28efdbc4..511e566a 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -83,6 +83,33 @@ export class OAuthRequestTimeoutError extends Error { } } +/** + * The largest delay `setTimeout` can actually schedule. Past `2 ** 31 - 1` the + * delay overflows a 32-bit signed integer and Node falls back to **1ms**, so an + * `Infinity` or an over-large budget produces an *immediate* timeout — the exact + * opposite of what the caller asked for (Copilot). + */ +const MAX_TIMER_DELAY_MS = 2_147_483_647; + +/** + * Coerce a caller's budget into something `setTimeout` can honour and the error + * can honestly report. + * + * - Non-finite (`NaN`, `±Infinity`) falls back to the default. `NaN` survives + * `Math.max(0, Math.round(...))` and `setTimeout(fn, NaN)` fires immediately, + * so a caller whose arithmetic produced a `NaN` would otherwise see every + * OAuth request fail at once with "timed out after NaNms". + * - Above `MAX_TIMER_DELAY_MS` it clamps rather than overflowing to 1ms. + * - Negative clamps to 0, which is a real budget (fire on the next tick), not + * an error. + * + * Rounded because the budget is *reported* — see the note at the call site. + */ +function normalizeBudget(timeoutMs: number): number { + if (!Number.isFinite(timeoutMs)) return DEFAULT_OAUTH_REQUEST_TIMEOUT_MS; + return Math.min(MAX_TIMER_DELAY_MS, Math.max(0, Math.round(timeoutMs))); +} + /** * How a proxy-side deadline is carried back to the browser (#2319). * @@ -90,10 +117,12 @@ export class OAuthRequestTimeoutError extends Error { * `createRemoteFetch` turns any non-OK answer into a plain `Error` — so a * timeout enforced by the *backend* would arrive as an untyped error and the * `instanceof OAuthRequestTimeoutError` checks downstream would all be false - * (Copilot). That matters most exactly where there is no client-side wrapper to - * have fired first: the discovery the SDK runs from inside the transport. So - * the route stamps this marker and `createRemoteFetch` reconstructs the typed - * error from it. + * (Copilot). Both ends run the same budget, so the client's race usually + * settles first; this matters when the backend's timer wins anyway — most + * plausibly a backgrounded tab, where the browser throttles `setTimeout` while + * the server's fires on schedule. Which of two equal deadlines happened to fire + * must not decide whether the error names its endpoint. So the route stamps + * this marker and `createRemoteFetch` reconstructs the typed error from it. */ export const OAUTH_TIMEOUT_WIRE_CODE = "oauth_request_timeout"; @@ -204,7 +233,7 @@ function requestUrlOf(input: RequestInfo | URL): string { * `Request`'s signal rather than clear it (Copilot). An explicit `null` is the * genuine "no signal" override and is honoured as one. */ -function callerSignalOf( +export function callerSignalOf( input: RequestInfo | URL, init: RequestInit | undefined, ): AbortSignal | undefined { @@ -324,7 +353,7 @@ export function withOAuthRequestTimeout( // public field. Rounding rather than flooring so a caller's own // whole-millisecond timeout survives the trip through that clock and is still // the number the message names. - const budget = Math.max(0, Math.round(timeoutMs)); + const budget = normalizeBudget(timeoutMs); return async (input, init) => { const url = requestUrlOf(input); diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index b584bb1c..62819249 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1256,10 +1256,15 @@ export function createRemoteApp( // the regression `exemptMcpEndpoint` exists to prevent on the client, and // reporting it as an OAuth timeout besides (Copilot). No deadline in the // envelope means no timer at all. + // Clamped to what `setTimeout` can schedule: past 2**31-1 the delay + // overflows and Node falls back to 1ms, turning an over-large budget into + // an immediate timeout. Non-finite is ignored outright rather than + // defaulted — an envelope carrying `NaN` is a malformed request, and the + // safe reading of a malformed deadline is "no deadline". const deadlineMs = typeof requestedTimeoutMs === "number" && Number.isFinite(requestedTimeoutMs) - ? Math.max(0, Math.round(requestedTimeoutMs)) + ? Math.min(2_147_483_647, Math.max(0, Math.round(requestedTimeoutMs))) : undefined; const timer = deadlineMs === undefined From 5a1c157a7965c519b615173fa9894a2225f6fe43 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 06:24:24 -0400 Subject: [PATCH 15/23] fix: exempt MCP traffic by origin, not path (#2319 round 17) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit exemptMcpEndpoint compared origin + pathname, which misses legacy SSE entirely: SSEClientTransport opens the configured URL and is handed a SEPARATE message endpoint by the server, then POSTs every JSON-RPC message to that second pathname. Those were bounded and body-buffered as OAuth traffic, so a slow tool call over SSE would be severed at 30s and reported as an OAuthRequestTimeoutError. It compares origin alone now. That is exact rather than approximate: the SDK enforces that the SSE message endpoint shares the stream URL's origin and rejects one that does not, so every MCP request this chain carries is same-origin with the configured URL by construction. The cost, stated in the docblock and pinned by a test: an OAuth endpoint on the MCP server's own origin is exempt here and so unbounded on this chain. The OAuth chain still bounds the Inspector's own requests to it, so only the SDK's transport-internal OAuth against a same-origin server falls back to where it was before #2319. Caller cancellation was also still swallowed in two later places in the compat probe loop — `probe.text()` has its own catch, and `fetch` resolves on headers, so an abort mid-body landed there and was answered with the preceding 404; and an abort during releaseBody's await would have carried on probing. Both recheck the signal now, and all three sites throw the caller's own `reason` rather than whatever shape the layer beneath rejected with. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../core/auth/oidcDiscoveryCompat.test.ts | 39 ++++++++++++++++++ .../src/test/core/auth/requestTimeout.test.ts | 40 ++++++++++++++++--- core/auth/oidcDiscoveryCompat.ts | 19 ++++++++- core/auth/requestTimeout.ts | 26 +++++++++--- 4 files changed, 111 insertions(+), 13 deletions(-) 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; } From 42339ff4a45e1f983d343655e35c1971d8259194 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 06:40:07 -0400 Subject: [PATCH 16/23] fix: narrow the exemption per transport (#2319 round 18) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 17 widened exemptMcpEndpoint to the origin for everything, which exempted same-origin OAuth work as well as MCP traffic — on the very 401/discovery path #2319 exists to bound. The rule is now as narrow as each transport allows. Streamable HTTP sends every MCP request to the configured URL, so origin + pathname is exact there and same-origin OAuth stays bounded. Legacy SSE negotiates its message endpoint inside the stream, where we never see it, so the origin rule is the narrowest available — and it is exact rather than approximate, since the SDK enforces that the message endpoint shares the stream URL's origin. The residual gap is therefore confined to legacy SSE against a server hosting its own OAuth endpoints, and it is stated in the docblock and pinned by a test rather than left implicit. Neither alternative was reachable: the SDK takes a single fetch for all network requests, so there is no separate auth seam to route through, and the negotiated SSE endpoint is parsed inside the SDK's own stream handler. Fails open on every uncertainty, including not knowing the transport — a path rule applied to what turns out to be SSE severs real MCP traffic, so an absent or throwing hint falls back to the origin rule. 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 | 67 ++++++++++++++----- core/auth/requestTimeout.ts | 67 ++++++++++++++----- core/mcp/inspectorClient.ts | 9 ++- 3 files changed, 109 insertions(+), 34 deletions(-) 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 From e71b8b9dfb57ec57c4d363e3addc7f9e1c0c36b2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 07:10:00 -0400 Subject: [PATCH 17/23] fix: never exempt a form-encoded request (#2319 round 20) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The exemption was URL-only, so an authorization server configured at the MCP endpoint's own URL — `oauthTokenUrl` takes any absolute URL — made the transport-internal refresh take the exempt branch and run with no signal, no body buffering and no deadline. That is the one request this change exists to bound. `isExempt` now receives the request's headers as well as its URL, and `exemptMcpEndpoint` refuses to exempt anything sent as `application/x-www-form-urlencoded`, before any URL comparison. That is a discriminator rather than a heuristic: the token exchange, the refresh and revocation are all form-encoded per RFC 6749 §4.1.3 and RFC 7009 §2.1, and MCP is JSON-RPC over application/json and never is. Also records, on the no-deadline route test, what it does not catch: at a 400ms wait an unconditional timer restored at the production 30s budget would still leave the request pending and the test would pass. Catching that needs either a ~31s wait on every CI run or a route-level deadline knob existing only for the test, and a second source of truth for the deadline is the defect round 13 removed. 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 | 113 +++++++++++++----- .../mcp/remote/server-extra-coverage.test.ts | 9 ++ core/auth/requestTimeout.ts | 51 +++++++- 3 files changed, 142 insertions(+), 31 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 6cd232e7..3fb90a68 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -498,12 +498,11 @@ 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); + /** Most cases are plain JSON-RPC traffic, which carries no form media type. */ + const JSON_HEADERS = new Headers({ "content-type": "application/json" }); + /** The token family: RFC 6749 §4.1.3 and RFC 7009 §2.1 are form-encoded. */ + const FORM_HEADERS = new Headers({ + "content-type": "application/x-www-form-urlencoded;charset=UTF-8", }); /** Streamable HTTP: every MCP request goes to the configured URL. */ @@ -519,17 +518,46 @@ describe("withOAuthRequestTimeout", () => { () => true, ); + it("exempts the MCP endpoint itself, whatever method or query", () => { + const isExempt = exemptMcpEndpoint(() => SERVER); + + expect(isExempt(SERVER, JSON_HEADERS)).toBe(true); + expect(isExempt(`${SERVER}?sessionId=abc`, JSON_HEADERS)).toBe(true); + expect(isExempt(`${SERVER}#frag`, JSON_HEADERS)).toBe(true); + }); + + it("never exempts a form-encoded request, whatever URL it uses", () => { + // `oauthTokenUrl` takes any absolute URL, including the MCP endpoint's + // own — at which point a URL rule would exempt the transport-internal + // refresh, the one request this change exists to bound. The token family + // is form-encoded and MCP never is, so the media type settles it first. + for (const isExempt of [ + streamable(SERVER), + sse(SERVER), + exemptMcpEndpoint(() => SERVER), + ]) { + expect(isExempt(SERVER, FORM_HEADERS)).toBe(false); + } + // And a missing server URL no longer fails open past it either. + expect(exemptMcpEndpoint(() => undefined)(SERVER, FORM_HEADERS)).toBe( + false, + ); + }); + 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(SERVER, JSON_HEADERS)).toBe(true); expect( isExempt( "https://srv.example.com/.well-known/oauth-protected-resource/mcp", + JSON_HEADERS, ), ).toBe(false); - expect(isExempt("https://srv.example.com/token")).toBe(false); + expect(isExempt("https://srv.example.com/token", JSON_HEADERS)).toBe( + false, + ); }); it("widens to the origin on legacy SSE, whose message path is unknowable", () => { @@ -538,9 +566,14 @@ describe("withOAuthRequestTimeout", () => { const isExempt = sse("https://srv.example.com/sse"); expect( - isExempt("https://srv.example.com/.well-known/openid-configuration"), + isExempt( + "https://srv.example.com/.well-known/openid-configuration", + JSON_HEADERS, + ), ).toBe(true); - expect(isExempt("https://as.example.com/token")).toBe(false); + expect(isExempt("https://as.example.com/token", JSON_HEADERS)).toBe( + false, + ); }); it("falls back to the origin rule when the transport is unknown", () => { @@ -554,8 +587,8 @@ describe("withOAuthRequestTimeout", () => { }, ); - expect(noHint("https://srv.example.com/token")).toBe(true); - expect(throws("https://srv.example.com/token")).toBe(true); + expect(noHint("https://srv.example.com/token", JSON_HEADERS)).toBe(true); + expect(throws("https://srv.example.com/token", JSON_HEADERS)).toBe(true); }); it("exempts legacy SSE's separate message endpoint", () => { @@ -567,22 +600,34 @@ describe("withOAuthRequestTimeout", () => { // 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); + expect( + isExempt( + "https://srv.example.com/messages?sessionId=abc", + JSON_HEADERS, + ), + ).toBe(true); + expect(isExempt("https://srv.example.com/", JSON_HEADERS)).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/token", JSON_HEADERS)).toBe( + false, + ); expect( - isExempt("https://as.example.com/.well-known/openid-configuration"), + isExempt( + "https://as.example.com/.well-known/openid-configuration", + JSON_HEADERS, + ), ).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); + expect(isExempt("http://srv.example.com/token", JSON_HEADERS)).toBe( + false, + ); + expect(isExempt("https://srv.example.com:8443/token", JSON_HEADERS)).toBe( + false, + ); }); it("fails open when the server URL is unknown or unparseable", () => { @@ -590,23 +635,37 @@ describe("withOAuthRequestTimeout", () => { // 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"), + exemptMcpEndpoint(() => undefined)( + "https://as.example.com/token", + JSON_HEADERS, + ), ).toBe(true); - expect(exemptMcpEndpoint(() => "")("https://as.example.com/token")).toBe( + expect( + exemptMcpEndpoint(() => "")( + "https://as.example.com/token", + JSON_HEADERS, + ), + ).toBe(true); + expect(exemptMcpEndpoint(() => "not a url")(SERVER, JSON_HEADERS)).toBe( + true, + ); + expect(exemptMcpEndpoint(() => SERVER)("not a url", JSON_HEADERS)).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); + expect(isExempt("https://other.example.com/mcp", JSON_HEADERS)).toBe( + false, + ); current = "https://other.example.com/mcp"; - expect(isExempt("https://other.example.com/mcp")).toBe(true); - expect(isExempt(SERVER)).toBe(false); + expect(isExempt("https://other.example.com/mcp", JSON_HEADERS)).toBe( + true, + ); + expect(isExempt(SERVER, JSON_HEADERS)).toBe(false); }); }); diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 96e3bd15..7d2fea1f 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -505,6 +505,15 @@ describe("server.ts supplemental coverage", () => { // a Streamable HTTP tool call that legitimately withholds its response // headers — reintroducing on the backend exactly what `exemptMcpEndpoint` // prevents on the client, and reporting it as an OAuth timeout besides. + // + // ⚠️ What it does NOT catch, stated so nobody reads more into it: the + // wait below is 400ms, so an unconditional timer restored at the + // production 30s budget would still leave the request pending here and + // the test would pass (Copilot). Catching that needs either a ~31s wait + // on every CI run or a route-level deadline knob existing only for the + // test — and a second source of truth for the deadline is the defect + // round 13 removed. Measured, this fails against an unconditional timer + // at any budget shorter than the wait. const caller = new AbortController(); const pending = fetch(`${h.baseUrl}/api/fetch`, { method: "POST", diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 46943308..5a963fe8 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -198,6 +198,15 @@ function readNegotiatesEndpoint(hint: (() => boolean) | undefined): boolean { } } +/** + * The media type every OAuth token-family request uses — the token exchange, + * the refresh, and revocation are all `application/x-www-form-urlencoded` per + * RFC 6749 §4.1.3 and RFC 7009 §2.1. MCP is JSON-RPC over `application/json` + * and never uses it, which is what makes this a safe discriminator rather than + * a heuristic. + */ +const OAUTH_FORM_MEDIA_TYPE = "application/x-www-form-urlencoded"; + export function exemptMcpEndpoint( getServerUrl: () => string | undefined, /** @@ -206,8 +215,22 @@ export function exemptMcpEndpoint( * connects. Absent, or throwing, is treated as `true`. */ negotiatesEndpoint?: () => boolean, -): (url: string) => boolean { - return (url) => { +): (url: string, headers: Headers) => boolean { + return (url, headers) => { + // A URL match is not by itself proof of MCP traffic. `oauthTokenUrl` takes + // any absolute URL, so a user may point it at the MCP endpoint's own URL, + // and the transport-internal refresh then travels this chain to a path the + // rules below would exempt — leaving the one request this whole change + // exists to bound running unbounded (Copilot). The token family is + // form-encoded and MCP never is, so the media type settles it before any + // URL comparison happens. + if ( + (headers.get("content-type") ?? "").split(";")[0].trim().toLowerCase() === + OAUTH_FORM_MEDIA_TYPE + ) { + return false; + } + const serverUrl = getServerUrl(); if (!serverUrl) return true; const negotiated = readNegotiatesEndpoint(negotiatesEndpoint); @@ -255,6 +278,21 @@ export function deadlineForRequestInit( return init ? REQUEST_DEADLINES.get(init) : undefined; } +/** + * The request's headers, whatever form `fetch`'s arguments took. `init` wins + * over a `Request`'s own, matching `fetch`. + */ +function requestHeadersOf( + input: RequestInfo | URL, + init: RequestInit | undefined, +): Headers { + if (init?.headers) return new Headers(init.headers); + if (typeof input !== "string" && !(input instanceof URL)) { + return new Headers(input.headers); + } + return new Headers(); +} + /** The request URL, whatever form `fetch`'s first argument took. */ function requestUrlOf(input: RequestInfo | URL): string { if (typeof input === "string") return input; @@ -359,6 +397,9 @@ function rebuildResponse(response: Response, body: ArrayBuffer): Response { * predicate because every request on it is OAuth work, while the transport * chain passes `exemptMcpEndpoint`. * + * `isExempt` receives the request's URL **and headers**, because a URL alone + * cannot always tell the two kinds of traffic apart — see `exemptMcpEndpoint`. + * * `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 @@ -382,7 +423,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, + isExempt?: (url: string, headers: Headers) => 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 @@ -405,7 +446,9 @@ export function withOAuthRequestTimeout( // 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); + if (isExempt?.(url, requestHeadersOf(input, init))) { + return fetchFn(input, init); + } const controller = new AbortController(); const callerSignal = callerSignalOf(input, init); From bb01c1e73b8558db628d14c78d58a946474738c9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 07:25:56 -0400 Subject: [PATCH 18/23] fix: bound the body by draining a clone, not by rebuilding (#2319 round 21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebuilding a Response around the buffered bytes could never be faithful. `url`, `redirected` and `type` are internal slots, so shadowing them as own properties satisfied a direct read and was lost the moment a caller called clone() — which returns a fresh native Response. The headers guard differed too, and the decoded bytes made the inherited content-encoding and content-length wrong, so those had to be stripped: four observable differences from the response fetch actually produced. The wrapper now drains `response.clone()` under the deadline and returns the original. Cloning tees the body, so draining one branch buffers the other: the bound is unchanged, the caller reads without a second trip, and every native slot stays intact. rebuildResponse and its null-body handling are gone, and with them the whole class. Also: getServerUrl() throws for a non-HTTP configuration, and this wrapped fetch can still be handed to a transport or a custom factory that calls it — so the documented fail-open was reachable only until that getter threw, at which point a configuration error propagated out of a fetch. Caught, for the same reason readNegotiatesEndpoint catches. Tests: response identity, the body still readable after the drain, a re-clone keeping its metadata (the case a rebuild could not satisfy), content-encoding preserved because nothing invalidates it now, a null-body status passed through, and the throwing getter failing open while still not slipping past the form-encoded check. 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 | 138 ++++++++++-------- core/auth/requestTimeout.ts | 99 +++++-------- 2 files changed, 110 insertions(+), 127 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 3fb90a68..832a147b 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -266,30 +266,12 @@ describe("withOAuthRequestTimeout", () => { expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); }); - it("returns a readable response whose metadata survives the rebuild", async () => { + it("hands back the original response, not a copy", async () => { const original = new Response('{"issuer":"https://as.example.com"}', { status: 201, 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, @@ -297,68 +279,84 @@ describe("withOAuthRequestTimeout", () => { const response = await wrapped(URL_UNDER_TEST); - expect(response.status).toBe(201); - expect(response.statusText).toBe("Created"); - expect(response.ok).toBe(true); - 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("https://as.example.com/redirected"); - expect(response.redirected).toBe(true); - expect(response.type).toBe("cors"); - // The rebuild must not change what enumerates. Which keys a `Response` owns - // is the host's business — under the Fetch standard these three are - // prototype getters and a native response has no own enumerable keys, while - // happy-dom makes them own enumerable data properties — so this compares - // against a baseline built in the same runtime rather than asserting either - // answer. Hard-coding one would make the rebuilt response observably - // different from a response that never passed through the wrapper, in - // `Object.keys`, object spread and `JSON.stringify`. - const baseline = Object.keys(new Response("{}")).sort(); - expect(Object.keys(response).sort()).toEqual(baseline); + // Identity, which is the strongest form of "nothing was changed": every + // native slot, the headers guard and `clone()` behave as they would if this + // wrapper were not in the chain at all. + expect(response).toBe(original); await expect(response.json()).resolves.toEqual({ issuer: "https://as.example.com", }); }); - it("drops content-encoding and content-length, which the buffer invalidates", async () => { - // `arrayBuffer()` hands back the *decoded* entity body, so an inherited - // `content-encoding` would describe an encoding the bytes no longer have - // and the inherited `content-length` would be the compressed size. Both are - // wrong, and both show up in the Network capture. - const decoded = '{"issuer":"https://as.example.com"}'; + it("leaves the body readable after draining the clone", async () => { + // The bound works by draining a clone; teeing buffers the other branch, so + // the caller must still be able to read the original without a second trip. const wrapped = withOAuthRequestTimeout( - vi.fn().mockResolvedValue( - new Response(decoded, { - headers: { - "content-type": "application/json", - "content-encoding": "gzip", - "content-length": "42", - }, - }), - ), + vi.fn().mockResolvedValue(new Response("hello")), 1000, ); const response = await wrapped(URL_UNDER_TEST); - expect(response.headers.get("content-encoding")).toBeNull(); - expect(response.headers.get("content-length")).toBeNull(); - expect(response.headers.get("content-type")).toBe("application/json"); - await expect(response.text()).resolves.toBe(decoded); + expect(response.bodyUsed).toBe(false); + await expect(response.text()).resolves.toBe("hello"); }); - it("rebuilds a null-body status without handing it a body", async () => { - // `new Response(body, { status: 204 })` throws; the rebuild has to pass null. + it("leaves the caller free to clone it again, with metadata intact", async () => { + // The case a rebuilt response could never satisfy: `url` / `redirected` / + // `type` are internal slots, so a shadowed own property survives a direct + // read and is lost the moment anyone calls `clone()`. + const original = new Response("{}", { status: 200 }); + Object.defineProperty(original, "url", { + value: "https://as.example.com/redirected", + configurable: true, + }); + Object.defineProperty(original, "redirected", { + value: true, + configurable: true, + }); const wrapped = withOAuthRequestTimeout( - vi - .fn() - .mockResolvedValue(new Response(null, { status: 204 })), + vi.fn().mockResolvedValue(original), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + const copy = response.clone(); + + expect(copy.url).toBe("https://as.example.com/redirected"); + expect(copy.redirected).toBe(true); + await expect(copy.json()).resolves.toEqual({}); + }); + + it("keeps content-encoding and content-length, which are the server's own", async () => { + // Nothing is rebuilt, so nothing invalidates them: the caller gets the + // response `fetch` produced and decodes it the way `fetch` intends. + const original = new Response("{}", { + headers: { + "content-type": "application/json", + "content-encoding": "gzip", + }, + }); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(original), 1000, ); const response = await wrapped(URL_UNDER_TEST); + expect(response.headers.get("content-encoding")).toBe("gzip"); + }); + + it("passes a null-body status through untouched", async () => { + const original = new Response(null, { status: 204 }); + const wrapped = withOAuthRequestTimeout( + vi.fn().mockResolvedValue(original), + 1000, + ); + + const response = await wrapped(URL_UNDER_TEST); + + expect(response).toBe(original); expect(response.status).toBe(204); expect(response.body).toBeNull(); }); @@ -654,6 +652,22 @@ describe("withOAuthRequestTimeout", () => { ); }); + it("fails open when the server-URL getter throws", () => { + // `InspectorClient.getServerUrl()` throws for a non-HTTP configuration, + // and this wrapped fetch can still be handed to a transport or a custom + // factory that calls it — propagating a configuration error out of a + // fetch would turn the documented fail-open into a hard failure. + const isExempt = exemptMcpEndpoint(() => { + throw new Error("Server URL is only available for HTTP transports"); + }); + + expect(isExempt("https://as.example.com/token", JSON_HEADERS)).toBe(true); + // Still not past the form-encoded check, which runs first. + expect(isExempt("https://as.example.com/token", FORM_HEADERS)).toBe( + false, + ); + }); + it("reads the server URL per call, since it changes between connects", () => { let current = SERVER; const isExempt = exemptMcpEndpoint(() => current); diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 5a963fe8..3ccca28d 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -26,8 +26,9 @@ * resolves as soon as response headers arrive, so a server that sends headers * — or half a JSON document — and then stalls would leave the caller's * `response.json()` hanging with nothing watching it. Every response on this - * path is a small finite document, so the body is buffered under the same race - * and the response rebuilt around it. + * path is a small finite document, so a *clone* of it is drained under the same + * race and the original — untouched, with every native slot intact — is what + * the caller gets. * * Today the common case is bounded, but only incidentally: a discovery stall * that happens to sit inside an SDK `initialize` rides that request's own @@ -231,7 +232,17 @@ export function exemptMcpEndpoint( return false; } - const serverUrl = getServerUrl(); + // `getServerUrl()` throws for a non-HTTP configuration, and this wrapped + // fetch can be handed to a transport or a custom factory that calls it + // anyway — propagating a configuration error out of a fetch would turn the + // documented fail-open into a hard failure (Copilot). Caught here for the + // same reason `readNegotiatesEndpoint` catches. + let serverUrl: string | undefined; + try { + serverUrl = getServerUrl(); + } catch { + return true; + } if (!serverUrl) return true; const negotiated = readNegotiatesEndpoint(negotiatesEndpoint); try { @@ -326,64 +337,6 @@ export function callerSignalOf( return undefined; } -/** - * Statuses the Fetch Standard defines as null-body. Handing the `Response` - * constructor a body with one of these throws a `TypeError`, so the rebuilt - * response below must pass `null` instead. - */ -const NULL_BODY_STATUSES = new Set([101, 103, 204, 205, 304]); - -/** - * Rebuild a `Response` around an already-buffered body. - * - * Two things have to be carried across by hand. - * - * `url`, `redirected` and `type` are getters with no constructor option, and - * callers do read them — the RFC 8414/OIDC compat wrapper stamps a header - * naming the URL a body came from, and the SDK's discovery reports the - * responding URL in its errors. So they are copied explicitly rather than - * silently reset to `""` / `false` / `"default"`. - * - * `content-encoding` and `content-length`, by contrast, must be *dropped*. - * `arrayBuffer()` hands back the **decoded** entity body, so a gzipped metadata - * document arrives here as plain JSON bytes: an inherited `content-encoding` - * would describe an encoding the body no longer has, and the inherited - * `content-length` would be the compressed size — both of them wrong, and both - * visible in the Network capture (Copilot). `responseWithBody` in - * `endpointOverrides.ts` deletes the same two headers for the same reason. - */ -function rebuildResponse(response: Response, body: ArrayBuffer): Response { - const headers = new Headers(response.headers); - headers.delete("content-encoding"); - headers.delete("content-length"); - const rebuilt = new Response( - NULL_BODY_STATUSES.has(response.status) ? null : body, - { - status: response.status, - statusText: response.statusText, - headers, - }, - ); - for (const key of ["url", "redirected", "type"] as const) { - // Enumerability is taken from whatever the freshly built `Response` already - // has for this key, rather than asserted. Hard-coding either answer makes - // the rebuilt response observably different from a native one — and which - // answer is right is the host's business: under the Fetch standard (and - // undici) these are prototype getters with no own enumerable key at all, - // while happy-dom defines them as own enumerable data properties. Copying - // the descriptor keeps `Object.keys`, object spread and `JSON.stringify` - // identical to a response that never passed through here, in both - // (Copilot). `false` for the getter case, where there is no own descriptor. - const existing = Object.getOwnPropertyDescriptor(rebuilt, key); - Object.defineProperty(rebuilt, key, { - value: response[key], - enumerable: existing?.enumerable ?? false, - configurable: true, - }); - } - return rebuilt; -} - /** * Wrap a `fetch` so every call through it is bounded by `timeoutMs`. * @@ -417,8 +370,9 @@ function rebuildResponse(response: Response, body: ArrayBuffer): Response { * a fetch that drops the signal. This wrapper only ever *adds* a reason to give * up; it never removes the caller's. * - * The response **body** is buffered under the same deadline, because `fetch` - * resolves on headers and a stalled body would otherwise be unbounded. + * The response **body** is drained through a clone under the same deadline, + * because `fetch` resolves on headers and a stalled body would otherwise be + * unbounded. The caller receives the original response, not a copy. */ export function withOAuthRequestTimeout( fetchFn: typeof fetch, @@ -512,8 +466,23 @@ export function withOAuthRequestTimeout( // not optional on a mixed chain: an exempt request skips this buffering // entirely, which is what lets an SSE body stay open rather than being // drained into memory here. - const body = await Promise.race([response.arrayBuffer(), abandoned]); - return rebuildResponse(response, body); + // Read a *clone* under the deadline and hand back the original. Cloning + // tees the body, so draining one branch buffers the other: by the time + // this resolves the original is fully in memory and the caller reads it + // without touching the network again — the bound is the same, and the + // response the caller gets is the one `fetch` produced. + // + // The alternative, rebuilding a `Response` around the buffered bytes, + // could never be faithful (Copilot). `url`, `redirected` and `type` are + // internal slots; shadowing them as own properties satisfies a direct + // read and is lost again the moment a caller calls `clone()`, which + // returns a fresh native `Response`. The headers guard differs too, and + // the decoded bytes made the inherited `content-encoding` and + // `content-length` wrong, so those had to be stripped — a rebuilt + // response was observably not the original in at least four ways. Not + // rebuilding removes the whole class. + await Promise.race([response.clone().arrayBuffer(), abandoned]); + return response; } catch (err) { // `controller.abort()` above can reject the underlying fetch *before* // `reject` runs, in which case the race settles with undici's From 45d87911ebec180d8151dfc35b6886ee3f47fdad Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 07:42:17 -0400 Subject: [PATCH 19/23] fix: release the discarded discovery response on every throw (#2319 round 22) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The timeout and cancellation exits added over the last few rounds drop the original failed discovery response, which nobody will ever receive — and an unread body holds its connection open on Node/undici. The successful-substitution path already released it for that reason; the five exceptional exits now do too (pre-abort, timeout, fetch-abort, post-release abort, body-read abort). Matters most for a standalone user of the exported wrapper, where no timeout wrapper underneath has already drained it. Also drops the last descriptions of the superseded rebuild: the test file's header, an inline comment asserting the response is "not the same object" when the file later asserts identity, and the PR description's account of copied metadata and deleted encoding headers. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../core/auth/oidcDiscoveryCompat.test.ts | 35 +++++++++++++++++++ .../src/test/core/auth/requestTimeout.test.ts | 14 ++++---- core/auth/oidcDiscoveryCompat.ts | 33 ++++++++++++++--- 3 files changed, 71 insertions(+), 11 deletions(-) diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index 511f307b..14096711 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -543,6 +543,41 @@ describe("probe cancellation (#2319)", () => { expect(await settled).toBe(reason); }); + it("releases the discarded discovery response on an exceptional exit", async () => { + // Nobody receives `response` when the loop throws, and an unread body holds + // its connection open on Node/undici — the same reason the successful + // substitution path releases it. + const cancels = vi.fn<() => Promise>().mockResolvedValue(undefined); + const discovery = new Response( + new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode("not found")); + }, + }), + { status: 404 }, + ); + // Spy on the body's cancel, which is what `releaseBody` calls. + const body = discovery.body; + if (body) vi.spyOn(body, "cancel").mockImplementation(cancels); + + const inner = vi.fn((input) => + String(input) === RFC8414 + ? Promise.resolve(discovery) + : new Promise(() => {}), + ); + const wrapped = withRfc8414OidcCompat(inner); + + const settled = wrapped(RFC8414, { + signal: AbortSignal.abort(new Error("gone")), + }).then( + () => "resolved", + () => "threw", + ); + + expect(await settled).toBe("threw"); + expect(cancels).toHaveBeenCalled(); + }); + 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 832a147b..1875d878 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -7,10 +7,11 @@ * - **The bound itself**, including the *body*: `fetch` resolves on headers, so * a server that sends them and then stalls has to be caught by the buffering * race, not by the fetch promise. - * - **The rebuilt response.** Buffering means the caller gets a different - * `Response` object, so `url` / `redirected` / `type` are asserted to survive - * and `content-encoding` / `content-length` to be dropped — the buffer holds - * the decoded body, which makes both of those headers lies. + * - **The response's identity.** The bound drains a *clone* and hands back the + * original, so the cases assert identity, that the body is still readable + * after the drain, and that a caller's own `clone()` keeps its metadata — + * the property a rebuilt response could not have, since `url` / `redirected` + * / `type` are internal slots. * - **Cancellation semantics.** The caller's signal is forwarded *and* raced, * so the tests distinguish the two: one uses a fetch that honours the signal, * another uses one that ignores it entirely. @@ -44,8 +45,9 @@ afterEach(() => { describe("withOAuthRequestTimeout", () => { it("passes a prompt response through", async () => { - // Not the same object: the body is buffered under the deadline (see the - // stalled-body case below) and the response rebuilt around it. + // The same object: the body is drained through a clone under the deadline + // (see the stalled-body case below), so nothing about the response the + // caller receives changes. const inner = vi .fn() .mockResolvedValue(new Response('{"ok":true}', { status: 200 })); diff --git a/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index 760b3540..d6ca43d3 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -271,7 +271,18 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // rejects immediately on a pre-aborted signal, so this is mostly about // not sending the request at all — the same reasoning as the // short-circuit in `withOAuthRequestTimeout`. - if (callerSignal?.aborted) throw callerSignal.reason; + // + // Every exceptional exit below releases `response` first. On a normal + // return it is the value the caller gets and reads; on a throw nobody + // will ever read it, and an unread body holds its connection open on + // Node/undici — the same reason the successful-substitution path + // releases it (Copilot). Matters most for a standalone user of this + // exported wrapper, where no timeout wrapper is underneath to have + // already drained it. + if (callerSignal?.aborted) { + await releaseBody(response); + throw callerSignal.reason; + } let probe: Response; try { @@ -287,13 +298,19 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // is one the SDK's own `buildDiscoveryUrls` emits, so a stall here is a // stall the SDK would have hit itself, unbounded. Everything else — a // CORS rejection, DNS, a reset — still falls back as documented above. - if (err instanceof OAuthRequestTimeoutError) throw err; + if (err instanceof OAuthRequestTimeoutError) { + await releaseBody(response); + 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. 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; + if (callerSignal?.aborted) { + await releaseBody(response); + throw callerSignal.reason; + } return response; } if (!probe.ok) { @@ -307,7 +324,10 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // 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 (callerSignal?.aborted) { + await releaseBody(response); + throw callerSignal.reason; + } if (continuesDiscovery(probe.status)) continue; return response; } @@ -331,7 +351,10 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // 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; + if (callerSignal?.aborted) { + await releaseBody(response); + throw callerSignal.reason; + } return response; } if (!isRfc8414OnlyMetadata(parsed)) { From 1e501fa5cbaf28909c0982cf573019251035aa0b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 07:57:38 -0400 Subject: [PATCH 20/23] fix: redact sensitive query values from timeout diagnostics (#2319 round 23) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The timeout message interpolated the raw request URL, and createFetchTracker records that message verbatim into the Network log and the persisted session. An OAuth endpoint's query string can carry a `code`, an `access_token` or a `client_secret` — exactly the fields fetchTracking already redacts everywhere else it records a URL — so a stalled signed endpoint would have written its secret to both. Redacted with the existing `redactUrlQuery`, in the OAuthRequestTimeoutError constructor rather than at each display site, so both the message and the public `url` field are covered by construction and no future consumer can reintroduce it. The route's own proxied-timeout message and the URL in its 504 marker get the same treatment, since both cross back to the browser and are recorded there. The path and every non-sensitive parameter survive, so the endpoint stays identifiable — which was the point of naming it. 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 | 30 +++++++++++++++++++ core/auth/requestTimeout.ts | 19 ++++++++++-- core/mcp/remote/node/server.ts | 10 +++++-- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 1875d878..c7cb609f 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -88,6 +88,36 @@ describe("withOAuthRequestTimeout", () => { expect(timeout.timeoutMs).toBe(1000); }); + it("redacts sensitive query values from the endpoint it names", async () => { + vi.useFakeTimers(); + // This message is recorded verbatim by `createFetchTracker` into the + // Network log and the persisted session, and an OAuth endpoint's query + // string can carry a `code`, an `access_token` or a `client_secret` — + // which `fetchTracking` already redacts everywhere else it records a URL. + const sensitive = + "https://as.example.com/token?code=abc123&client_secret=shhh&state=keepme"; + const wrapped = withOAuthRequestTimeout(neverSettles, 1000); + + const assertion = wrapped(sensitive).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + const err = (await assertion) as OAuthRequestTimeoutError; + + for (const secret of ["abc123", "shhh"]) { + expect(err.message).not.toContain(secret); + expect(err.url).not.toContain(secret); + } + // Still identifiable, which was the point of naming the endpoint: the path + // and every non-sensitive parameter survive. Read through `URL`, since + // `URLSearchParams.toString()` percent-encodes the redaction marker. + const redacted = new URL(err.url); + expect(redacted.origin + redacted.pathname).toBe( + "https://as.example.com/token", + ); + expect(redacted.searchParams.get("state")).toBe("keepme"); + expect(redacted.searchParams.get("code")).toBe("[REDACTED]"); + expect(redacted.searchParams.get("client_secret")).toBe("[REDACTED]"); + }); + it("takes the URL off a Request object too", async () => { vi.useFakeTimers(); const wrapped = withOAuthRequestTimeout(neverSettles, 1000); diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 3ccca28d..ba06e53f 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -63,6 +63,8 @@ * halves the 60s the SDK handshake incidentally provides, and it is the only * bound at all on the legs that sit outside an SDK request. */ +import { redactUrlQuery } from "../mcp/fetchTracking.js"; + export const DEFAULT_OAUTH_REQUEST_TIMEOUT_MS = 30_000; /** @@ -73,13 +75,26 @@ export const DEFAULT_OAUTH_REQUEST_TIMEOUT_MS = 30_000; * hosts, and which of them stalled is the whole diagnostic. */ export class OAuthRequestTimeoutError extends Error { + /** + * The endpoint that stalled, with sensitive query values redacted. + * + * Redacted in the constructor rather than at each display site, so every + * consumer is covered by construction: this error's message is recorded + * verbatim by `createFetchTracker` into the Network log and the persisted + * session, and an OAuth endpoint can carry a `code`, an `access_token` or a + * `client_secret` in its query string — which `fetchTracking` already + * deliberately redacts everywhere else it records a URL (Copilot). The path + * and every non-sensitive parameter survive, so the endpoint stays + * identifiable, which was the point of naming it. + */ readonly url: string; readonly timeoutMs: number; constructor(url: string, timeoutMs: number) { - super(`OAuth request to ${url} timed out after ${timeoutMs}ms`); + const safeUrl = redactUrlQuery(url); + super(`OAuth request to ${safeUrl} timed out after ${timeoutMs}ms`); this.name = "OAuthRequestTimeoutError"; - this.url = url; + this.url = safeUrl; this.timeoutMs = timeoutMs; } } diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 62819249..ccdc7a8b 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -29,6 +29,7 @@ import { watch as chokidarWatch, type FSWatcher } from "chokidar"; import { createTransportNode } from "../../node/transport.js"; import { createProxyFetch } from "../../node/proxyFetch.js"; import { OAUTH_TIMEOUT_WIRE_CODE } from "../../../auth/requestTimeout.js"; +import { redactUrlQuery } from "../../fetchTracking.js"; import type { RemoteConnectRequest, RemoteSendRequest, @@ -1266,6 +1267,11 @@ export function createRemoteApp( Number.isFinite(requestedTimeoutMs) ? Math.min(2_147_483_647, Math.max(0, Math.round(requestedTimeoutMs))) : undefined; + // Redacted for the same reason `OAuthRequestTimeoutError` redacts: this + // message and this URL are handed back to the browser, recorded in the + // Network log and persisted, and an OAuth endpoint's query string can carry + // a `code`, an `access_token` or a `client_secret` (Copilot). + const safeUrl = redactUrlQuery(url); const timer = deadlineMs === undefined ? undefined @@ -1273,7 +1279,7 @@ export function createRemoteApp( deadlineFired = true; controller.abort( new Error( - `proxied request to ${url} timed out after ${deadlineMs}ms`, + `proxied request to ${safeUrl} timed out after ${deadlineMs}ms`, ), ); }, deadlineMs); @@ -1338,7 +1344,7 @@ export function createRemoteApp( { error: msg, code: OAUTH_TIMEOUT_WIRE_CODE, - url, + url: safeUrl, timeoutMs: deadlineMs, }, 504, From 78f82475c494fa9a120cdfbc5db83bf7c7dadcc3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 08:19:57 -0400 Subject: [PATCH 21/23] fix: stop the drain and release both tee branches on timeout (#2319 round 24) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Losing the race rejected the caller but did not stop the read. Because clone() tees the body, a drain still running kept pulling bytes AND kept buffering them for the unread original branch — unbounded memory on exactly the signal-ignoring body the race exists to contain. The clone is now drained through an explicit reader rather than clone().arrayBuffer(), precisely so the losing side has something it can cancel: arrayBuffer() locks the stream to an internal reader nothing else can reach. On any throw both branches are torn down, which is safe because that path never hands the response to a caller. The cancels are deliberately not awaited: cancelling one branch of a tee can itself wait on the other, which would hold the timeout open for as long as the stall it is meant to end. Each carries its own catch, so a stream already cancelled, errored or locked elsewhere does not mask the timeout the caller actually needs — covered by a test whose stream throws from cancel(). Verified by mutation: removing the teardown makes the new test fail. 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 | 72 +++++++++++++++++++ core/auth/requestTimeout.ts | 55 +++++++++++++- 2 files changed, 126 insertions(+), 1 deletion(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index c7cb609f..5f5b0465 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -298,6 +298,78 @@ describe("withOAuthRequestTimeout", () => { expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); }); + it("releases both tee branches when the deadline wins", async () => { + vi.useFakeTimers(); + // Losing the race rejects the caller; on its own it does not stop the read. + // A drain still running would keep pulling bytes *and* keep buffering them + // for the unread original branch — unbounded memory on exactly the + // signal-ignoring body the race exists to contain. Observed through the + // stream's own `cancel`, which is what tearing the branches down calls. + let cancelled = false; + const stalled = new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode('{"issuer":')); + }, + // Never resolves: headers arrived, the body never finishes. + pull() { + return new Promise(() => {}); + }, + cancel() { + cancelled = true; + }, + }); + const wrapped = withOAuthRequestTimeout( + vi + .fn() + .mockResolvedValue( + new Response(stalled, { headers: { "content-type": "text/plain" } }), + ), + 1000, + ); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + // The cancels are deliberately not awaited — cancelling one branch of a tee + // can wait on the other — so let them settle before observing. + for (let i = 0; i < 20; i++) await Promise.resolve(); + expect(cancelled).toBe(true); + }); + + it("still reports the timeout when tearing the branches down fails", async () => { + vi.useFakeTimers(); + // Cancelling a stream that is already errored, locked elsewhere, or whose + // source rejects is not a failure here — the cleanup runs on a path that is + // already rejecting with something the caller cares about far more. + const hostile = new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode("x")); + }, + pull() { + return new Promise(() => {}); + }, + cancel() { + throw new Error("cancel blew up"); + }, + }); + const wrapped = withOAuthRequestTimeout( + vi + .fn() + .mockResolvedValue( + new Response(hostile, { headers: { "content-type": "text/plain" } }), + ), + 1000, + ); + + const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err); + await vi.advanceTimersByTimeAsync(1000); + + expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError); + // And no unhandled rejection escapes from the cleanup. + for (let i = 0; i < 20; i++) await Promise.resolve(); + }); + it("hands back the original response, not a copy", async () => { const original = new Response('{"issuer":"https://as.example.com"}', { status: 201, diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index ba06e53f..7740b58c 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -319,6 +319,44 @@ function requestHeadersOf( return new Headers(); } +/** + * Pull a reader to completion, discarding what it yields. An absent reader — a + * null-body response — is already complete. + * + * Read through an explicit reader rather than `clone().arrayBuffer()` so the + * losing side of the race has something it can cancel: `arrayBuffer()` locks + * the stream to an internal reader nothing else can reach. + */ +async function drainToEnd( + reader: ReadableStreamDefaultReader | undefined, +): Promise { + if (!reader) return; + for (;;) { + const { done } = await reader.read(); + if (done) return; + } +} + +/** + * Tear down both branches of the tee. + * + * Deliberately **not awaited**, and the `void`s say which of the documented + * cases this is: the callee owns its failures (each carries its own `catch`), + * and the caller genuinely cannot await — cancelling one branch of a tee can + * itself wait on the other, which would hold the timeout open for exactly as + * long as the stall it is meant to end. Best-effort on each besides: a stream + * already cancelled, errored or locked elsewhere is not a failure here, and + * this runs on a path that is already rejecting with something the caller cares + * about more. + */ +function cancelBoth( + reader: ReadableStreamDefaultReader | undefined, + response: Response | undefined, +): void { + void reader?.cancel().catch(() => {}); + void response?.body?.cancel().catch(() => {}); +} + /** The request URL, whatever form `fetch`'s first argument took. */ function requestUrlOf(input: RequestInfo | URL): string { if (typeof input === "string") return input; @@ -466,11 +504,16 @@ export function withOAuthRequestTimeout( const nextInit: RequestInit = { ...init, signal }; REQUEST_DEADLINES.set(nextInit, budget); + // Held so the losing path can tear both tee branches down — see below. + let settled: Response | undefined; + let drainReader: ReadableStreamDefaultReader | undefined; + try { const response = await Promise.race([ fetchFn(input, nextInit), abandoned, ]); + settled = response; // `fetch` resolves once the response *headers* arrive, so stopping here // would leave the body unbounded: an authorization server can send // headers, or half a JSON document, and then stall, and the caller's @@ -496,9 +539,19 @@ export function withOAuthRequestTimeout( // `content-length` wrong, so those had to be stripped — a rebuilt // response was observably not the original in at least four ways. Not // rebuilding removes the whole class. - await Promise.race([response.clone().arrayBuffer(), abandoned]); + const copy = response.clone(); + drainReader = copy.body?.getReader(); + await Promise.race([drainToEnd(drainReader), abandoned]); return response; } catch (err) { + // ⚠️ Losing the race rejects the caller; on its own it does not stop the + // read. `clone()` tees the body, so a drain still running would keep + // pulling bytes *and* keep buffering them for the unread original branch + // — unbounded memory on exactly the signal-ignoring body the race exists + // to contain (Copilot). Both branches are cancelled here, which is safe + // precisely because this path throws: the caller never receives the + // response, so nothing is left to read it. + cancelBoth(drainReader, settled); // `controller.abort()` above can reject the underlying fetch *before* // `reject` runs, in which case the race settles with undici's // `AbortError` instead of ours. Which of the two wins is an ordering From b4c02865e4cca03feb5b28e97f0b54f335192f7d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 08:33:18 -0400 Subject: [PATCH 22/23] fix: do not await the release on a cancellation path (#2319 round 25) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ReadableStream.cancel()` adopts the underlying source's cancel promise, which is permitted never to settle — so awaiting `releaseBody` while propagating a cancellation can hang the very thing it was meant to end. The five exceptional exits in the compat probe loop now start the release without waiting on it: the release is a courtesy to the connection pool, and the caller's abort or timeout must reach it regardless. The two non-exceptional awaits are unchanged. Same class as the tee teardown in round 24, one file over. Also moves the `redactUrlQuery` import above DEFAULT_OAUTH_REQUEST_TIMEOUT_MS's JSDoc, which it had split from the constant — so hover and API tooling see the doc again. Verified by mutation: awaiting the release makes the new test time out. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../core/auth/oidcDiscoveryCompat.test.ts | 30 +++++++++++++++++++ core/auth/oidcDiscoveryCompat.ts | 26 ++++++++++++---- core/auth/requestTimeout.ts | 4 +-- 3 files changed, 53 insertions(+), 7 deletions(-) diff --git a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts index 14096711..c9ea94b9 100644 --- a/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts +++ b/clients/web/src/test/core/auth/oidcDiscoveryCompat.test.ts @@ -578,6 +578,36 @@ describe("probe cancellation (#2319)", () => { expect(cancels).toHaveBeenCalled(); }); + it("does not wait on a cancel that never settles", async () => { + // ⚠️ `ReadableStream.cancel()` adopts the underlying source's cancel + // promise, which is permitted never to settle — so awaiting the release on + // a path that is propagating a cancellation would hang the very thing it + // was meant to end. + const discovery = new Response( + new ReadableStream({ + start(ctrl) { + ctrl.enqueue(new TextEncoder().encode("not found")); + }, + cancel() { + return new Promise(() => {}); + }, + }), + { status: 404 }, + ); + const inner = vi.fn((input) => + String(input) === RFC8414 + ? Promise.resolve(discovery) + : new Promise(() => {}), + ); + const wrapped = withRfc8414OidcCompat(inner); + const reason = new Error("gone"); + + // Would hang if the release were awaited; the assertion is that it settles. + await expect( + wrapped(RFC8414, { signal: AbortSignal.abort(reason) }), + ).rejects.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/core/auth/oidcDiscoveryCompat.ts b/core/auth/oidcDiscoveryCompat.ts index d6ca43d3..ce0ace87 100644 --- a/core/auth/oidcDiscoveryCompat.ts +++ b/core/auth/oidcDiscoveryCompat.ts @@ -170,6 +170,22 @@ async function releaseBody(response: Response): Promise { await response.body?.cancel().catch(() => {}); } +/** + * The same release, started but not waited on. + * + * ⚠️ `ReadableStream.cancel()` adopts the underlying source's cancel promise, + * which is permitted never to settle — so awaiting it on a path that is + * *propagating a cancellation* can hang the very thing that was meant to end + * (Copilot). Every exceptional exit below uses this: the release is a courtesy + * to the connection pool, and the caller's abort or timeout must reach it + * regardless. The `void` is the documented case where the callee owns its + * failures — `releaseBody` swallows its own — and the caller genuinely cannot + * await. + */ +function releaseBodyDetached(response: Response): void { + void releaseBody(response); +} + /** * Whether a parsed body is RFC 8414 authorization-server metadata that is *not* * a valid OpenID provider document — the exact shape the upstream schema @@ -280,7 +296,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // exported wrapper, where no timeout wrapper is underneath to have // already drained it. if (callerSignal?.aborted) { - await releaseBody(response); + releaseBodyDetached(response); throw callerSignal.reason; } @@ -299,7 +315,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // stall the SDK would have hit itself, unbounded. Everything else — a // CORS rejection, DNS, a reset — still falls back as documented above. if (err instanceof OAuthRequestTimeoutError) { - await releaseBody(response); + releaseBodyDetached(response); throw err; } // Likewise a caller that gave up: substituting the preceding 404 for @@ -308,7 +324,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // here and at the two sites below, rather than whatever shape the // underlying layer happened to reject with. if (callerSignal?.aborted) { - await releaseBody(response); + releaseBodyDetached(response); throw callerSignal.reason; } return response; @@ -325,7 +341,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // response either, and `continue` would otherwise carry on probing for // a caller that has stopped waiting. if (callerSignal?.aborted) { - await releaseBody(response); + releaseBodyDetached(response); throw callerSignal.reason; } if (continuesDiscovery(probe.status)) continue; @@ -352,7 +368,7 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch { // that merely will not parse still falls through, which is the case // this catch exists for. if (callerSignal?.aborted) { - await releaseBody(response); + releaseBodyDetached(response); throw callerSignal.reason; } return response; diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 7740b58c..7b771919 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -56,6 +56,8 @@ * shape, as `revokeToken`. */ +import { redactUrlQuery } from "../mcp/fetchTracking.js"; + /** * 30 seconds. Deliberately generous: discovery against a slow or cold-starting * authorization server is legitimate, and a bound that fires on a server that @@ -63,8 +65,6 @@ * halves the 60s the SDK handshake incidentally provides, and it is the only * bound at all on the legs that sit outside an SDK request. */ -import { redactUrlQuery } from "../mcp/fetchTracking.js"; - export const DEFAULT_OAUTH_REQUEST_TIMEOUT_MS = 30_000; /** From cf856c740cfd2c410a45c4be58ae5ee56934ea60 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 08:47:01 -0400 Subject: [PATCH 23/23] fix: do not await the discarded stream's cancel in /api/fetch (#2319 round 26) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third and last site of the same class. `ReadableStream.cancel()` adopts the underlying source's cancel promise, which is permitted never to settle, so awaiting it kept the handler pending forever — and on an unbounded request there is no timer to release it either. The cancel is started and the handler returns, so the JSON response and the `finally` complete regardless. The `void` carries the justification for which of AGENTS.md's permitted cases it is: the callee owns its failures through its own `catch`, and the caller cannot await. Test: a `/stream-hostile` upstream that sends event-stream headers and one frame and then never closes, asserting the route still answers 200 with no body. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../mcp/remote/server-extra-coverage.test.ts | 24 +++++++++++++++++++ core/mcp/remote/node/server.ts | 9 ++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 7d2fea1f..7314d514 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -353,6 +353,14 @@ describe("server.ts supplemental coverage", () => { }); const srv = createServer((req, res) => { upstreamHits.push(req.url ?? ""); + if (req.url === "/stream-hostile") { + // Headers, one frame, and a connection whose teardown the route must + // not wait on: `ReadableStream.cancel()` adopts the source's cancel + // promise, which may never settle. + res.writeHead(200, { "Content-Type": "text/event-stream" }); + res.write("data: hi\n\n"); + return; + } if (req.url === "/stream-open") { // Event-stream headers, one frame, and then nothing — the shape that // used to leave a socket open for good, because the route classifies @@ -568,6 +576,22 @@ describe("server.ts supplemental coverage", () => { expect(await settled).toBe("aborted by us"); }); + it("answers a discarded stream without waiting on its cancellation", async () => { + // ⚠️ The route starts the cancel and returns. Awaiting it would keep the + // handler pending on a source that never settles its cancel promise — + // and on an unbounded request there is no timer to release it either. + const res = await fetch(`${h.baseUrl}/api/fetch`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: `${targetUrl}/stream-hostile` }), + }); + + expect(res.status).toBe(200); + const payload = (await res.json()) as { status: number; body?: string }; + expect(payload.status).toBe(200); + expect(payload.body).toBeUndefined(); + }); + it("routes the outbound request through HTTP_PROXY (#2067)", async () => { // /api/fetch is the browser's ONLY way out to the network — the web // client's `environment.fetch` is `createRemoteFetch()`, which forwards diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index ccdc7a8b..15179eb1 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1323,7 +1323,14 @@ export function createRemoteApp( // its own in this configuration — so this is belt and braces rather // than a demonstrated leak, and it is kept because relying on that is // an implementation detail of the fetch beneath us, not a contract. - await res.body?.cancel().catch(() => {}); + // + // ⚠️ Started, not awaited. `ReadableStream.cancel()` adopts the + // underlying source's cancel promise, which is permitted never to + // settle — awaiting it would keep this handler pending forever, and on + // an unbounded request there is no timer to release it either + // (Copilot). The `void` is the documented case where the callee owns + // its failures, via the `catch` below, and the caller cannot await. + void res.body?.cancel().catch(() => {}); } return c.json({