From e234343702386248c8b9d20f3d19214326f76a8c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 06:06:36 -0400 Subject: [PATCH] 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