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); } });