modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: bound the body by draining a clone, not by rebuilding (#2319 round 21)

Browse source 
Rebuilding a Response around the buffered bytes could never be
faithful. `url`, `redirected` and `type` are internal slots, so
shadowing them as own properties satisfied a direct read and was lost
the moment a caller called clone() — which returns a fresh native
Response. The headers guard differed too, and the decoded bytes made
the inherited content-encoding and content-length wrong, so those had
to be stripped: four observable differences from the response fetch
actually produced.

The wrapper now drains `response.clone()` under the deadline and
returns the original. Cloning tees the body, so draining one branch
buffers the other: the bound is unchanged, the caller reads without a
second trip, and every native slot stays intact. rebuildResponse and
its null-body handling are gone, and with them the whole class.

Also: 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 — so the documented fail-open was reachable only until
that getter threw, at which point a configuration error propagated out
of a fetch. Caught, for the same reason readNegotiatesEndpoint catches.

Tests: response identity, the body still readable after the drain, a
re-clone keeping its metadata (the case a rebuild could not satisfy),
content-encoding preserved because nothing invalidates it now, a
null-body status passed through, and the throwing getter failing open
while still not slipping past the form-encoded check.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-11 07:25:56 -04:00
commit bb01c1e73b
2 changed files with 105 additions and 122 deletions
Download patch file Download diff file Expand all files Collapse all files

138
clients/web/src/test/core/auth/requestTimeout.test.ts
View file

@ -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<typeof fetch>().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<typeof fetch>().mockResolvedValue(
new Response(decoded, {
headers: {
"content-type": "application/json",
"content-encoding": "gzip",
"content-length": "42",
},
}),
),
vi.fn<typeof fetch>().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<typeof fetch>()
.mockResolvedValue(new Response(null, { status: 204 })),
vi.fn<typeof fetch>().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<typeof fetch>().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<typeof fetch>().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);

99
core/auth/requestTimeout.ts
View file

@ -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