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.