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