From 234d0a9ac43a35c43178867e60ec82c0ee6039ba Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 05:07:31 -0400 Subject: [PATCH] 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