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

fix: sanitize rebuilt headers, refresh stale docs, cover the route (#2319 round 7)

Browse source 
Three findings, all real.

`arrayBuffer()` hands back the decoded entity body, so the rebuilt
response was carrying the upstream `content-encoding` and the
compressed `content-length` — a gzipped metadata document came back as
plain JSON bytes still claiming gzip and the wrong size, visible in the
Network capture. Both headers are deleted now, as `responseWithBody` in
endpointOverrides.ts already did for the same reason.

The "why a race, and not just the signal" rationale described a world
this PR had since changed: the signal does now reach the backend's
outbound fetch. The module doc and three implementation comments are
rewritten to say what the race is actually still for — a fetchFn that
ignores AbortSignal, and a backend that is itself wedged — rather than
a proxy hop that no longer drops it.

The backend cancellation path had only mock-level coverage. A
route-level test drives /api/fetch at an upstream that accepts and never
answers, aborts the browser side, and asserts the upstream observes its
connection closing. Verified by mutation: dropping `signal` from the
route's outbound fetch makes it time out rather than pass.

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 03:43:57 -04:00
commit f3e4023e35
3 changed files with 128 additions and 32 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -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<Response>(() => {});
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<typeof fetch>().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(

47
clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts
View file

@ -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<void>;
/** 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<void>((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

65
core/auth/requestTimeout.ts
View file

@ -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<never>((_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.