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

fix: address Copilot review round 1 on #2319

Browse source 
Three findings, all real:

- The deadline stopped at the response headers. `fetch` resolves once
  headers arrive, so a server that sent headers — or half a JSON
  document — and then stalled left the caller's `response.json()`
  hanging with nothing watching it. The body is now buffered under the
  same race and the response rebuilt around it, preserving `url`,
  `redirected` and `type`, which have no constructor option, and
  passing `null` for a null-body status.

- The caller's signal was only forwarded, not raced. On the browser's
  `createRemoteFetch` path the signal is dropped in flight, so an abort
  left the promise pending for the whole budget rather than the outer
  cancellation winning as documented. It is raced now, with the
  caller's own `reason` preserved, and the listener removed in
  `finally` since one caller signal outlives several requests.

- A `Request` carries its signal on the input, not in `init`, so always
  passing our own `signal` in `init` silently overrode it. The caller
  signal is now derived from the `Request` when `init` has no `signal`
  key — and an explicit `null` there still wins, per the fetch spec.

Eight new tests cover each: the stalled body, caller cancellation
against a signal-ignoring fetch, an already-aborted signal, the
Request-embedded signal, the explicit-null override, listener removal,
the rebuilt response's metadata and a null-body status.

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 02:24:30 -04:00
commit e89ea4cf19
2 changed files with 260 additions and 16 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -17,12 +17,18 @@ afterEach(() => {
});
describe("withOAuthRequestTimeout", () => {
it("passes a prompt response straight through", async () => {
const ok = new Response("{}", { status: 200 });
const inner = vi.fn<typeof fetch>().mockResolvedValue(ok);
it("passes a prompt response through", async () => {
// Not the same object: the body is buffered under the deadline (see the
// stalled-body case below) and the response rebuilt around it.
const inner = vi
.fn<typeof fetch>()
.mockResolvedValue(new Response('{"ok":true}', { status: 200 }));
const wrapped = withOAuthRequestTimeout(inner, 1000);
await expect(wrapped(URL_UNDER_TEST)).resolves.toBe(ok);
const response = await wrapped(URL_UNDER_TEST);
expect(response.status).toBe(200);
await expect(response.json()).resolves.toEqual({ ok: true });
expect(inner).toHaveBeenCalledTimes(1);
});
@ -103,7 +109,7 @@ describe("withOAuthRequestTimeout", () => {
expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError);
});
it("preserves a caller-supplied signal, which still wins on its own", async () => {
it("forwards a caller-supplied signal to the inner fetch", async () => {
const inner: typeof fetch = (_input, init) =>
new Promise<Response>((_resolve, reject) => {
init?.signal?.addEventListener("abort", () =>
@ -119,6 +125,139 @@ describe("withOAuthRequestTimeout", () => {
await expect(pending).rejects.toThrow("caller cancelled");
});
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.
const wrapped = withOAuthRequestTimeout(neverSettles, 60_000);
const caller = new AbortController();
const pending = wrapped(URL_UNDER_TEST, { signal: caller.signal });
const reason = new Error("caller gave up");
caller.abort(reason);
await expect(pending).rejects.toBe(reason);
});
it("rejects immediately when the caller's signal is already aborted", async () => {
const wrapped = withOAuthRequestTimeout(neverSettles, 60_000);
const reason = new Error("already gone");
await expect(
wrapped(URL_UNDER_TEST, { signal: AbortSignal.abort(reason) }),
).rejects.toBe(reason);
});
it("honours the signal embedded in a Request input", async () => {
// `init.signal` is absent here, so the Request's own signal is the caller's
// — reading `init` alone would silently override it.
const wrapped = withOAuthRequestTimeout(neverSettles, 60_000);
const caller = new AbortController();
const pending = wrapped(
new Request(URL_UNDER_TEST, { signal: caller.signal }),
);
const reason = new Error("request cancelled");
caller.abort(reason);
await expect(pending).rejects.toBe(reason);
});
it("lets an explicit null init.signal override a Request's own", async () => {
vi.useFakeTimers();
// Per the fetch spec a present `signal` key wins, `null` included.
const wrapped = withOAuthRequestTimeout(neverSettles, 1000);
const caller = new AbortController();
const pending = wrapped(
new Request(URL_UNDER_TEST, { signal: caller.signal }),
{ signal: null },
);
const assertion = pending.catch((err: unknown) => err);
caller.abort(new Error("ignored"));
await vi.advanceTimersByTimeAsync(1000);
expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError);
});
it("removes its abort listener once the request settles", async () => {
const caller = new AbortController();
const removeSpy = vi.spyOn(caller.signal, "removeEventListener");
const wrapped = withOAuthRequestTimeout(
vi.fn<typeof fetch>().mockResolvedValue(new Response("{}")),
60_000,
);
await wrapped(URL_UNDER_TEST, { signal: caller.signal });
// A caller signal outlives one request — one connect attempt makes several
// — so a listener left behind per request would accumulate on it.
expect(removeSpy).toHaveBeenCalledWith("abort", expect.any(Function));
});
it("bounds a stalled response body, not just the headers", async () => {
vi.useFakeTimers();
// `fetch` resolves on headers. A server that sends them and then stalls
// would leave the caller's `response.json()` hanging unwatched.
const stalledBody: typeof fetch = () =>
Promise.resolve(
new Response(
new ReadableStream<Uint8Array>({
start(ctrl) {
ctrl.enqueue(new TextEncoder().encode('{"issuer":'));
},
}),
{ headers: { "content-type": "application/json" } },
),
);
const wrapped = withOAuthRequestTimeout(stalledBody, 1000);
const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
await vi.advanceTimersByTimeAsync(1000);
expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError);
});
it("returns a readable response whose metadata survives the rebuild", async () => {
const original = new Response('{"issuer":"https://as.example.com"}', {
status: 201,
statusText: "Created",
headers: { "content-type": "application/json", "x-probe": "kept" },
});
const wrapped = withOAuthRequestTimeout(
vi.fn<typeof fetch>().mockResolvedValue(original),
1000,
);
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.url).toBe(original.url);
expect(response.redirected).toBe(original.redirected);
expect(response.type).toBe(original.type);
await expect(response.json()).resolves.toEqual({
issuer: "https://as.example.com",
});
});
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(
vi
.fn<typeof fetch>()
.mockResolvedValue(new Response(null, { status: 204 })),
1000,
);
const response = await wrapped(URL_UNDER_TEST);
expect(response.status).toBe(204);
expect(response.body).toBeNull();
});
it("propagates a non-timeout failure unchanged", async () => {
const boom = new TypeError("network error");
const inner = vi.fn<typeof fetch>().mockRejectedValue(boom);
@ -173,7 +312,7 @@ describe("withOAuthRequestTimeout", () => {
vi.useFakeTimers();
const clearSpy = vi.spyOn(globalThis, "clearTimeout");
const wrapped = withOAuthRequestTimeout(
vi.fn<typeof fetch>().mockResolvedValue(new Response(null)),
vi.fn<typeof fetch>().mockResolvedValue(new Response("{}")),
1000,
);

125
core/auth/requestTimeout.ts
View file

@ -22,6 +22,13 @@
* redirect is about to end the attempt. A stalled fetch inside that window is
* a silent, unbounded spinner rather than a surfaced failure.
*
* The deadline covers the **whole exchange**, not just the headers: `fetch`
* 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.
*
* 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
* timeout and surfaces after 60s as `Request timed out`. This module makes the
@ -77,6 +84,63 @@ function requestUrlOf(input: RequestInfo | URL): string {
return input.url;
}
/**
* The caller's own `AbortSignal`, if it supplied one.
*
* A `Request` carries its signal on the *input*, not in `init`, and this
* wrapper always passes a `signal` of its own in `init` — so reading `init`
* alone would silently override the embedded one and change ordinary `fetch`
* cancellation semantics for `withOAuthRequestTimeout(new Request(url, { signal }))`
* (Copilot).
*
* `init.signal` still wins whenever the key is *present*, per the fetch spec:
* an explicit `null` there means "no signal" even when `input` is a `Request`
* that has one.
*/
function callerSignalOf(
input: RequestInfo | URL,
init: RequestInit | undefined,
): AbortSignal | undefined {
if (init && "signal" in init) return init.signal ?? undefined;
if (typeof input !== "string" && !(input instanceof URL)) return input.signal;
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.
*
* `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"`.
*/
function rebuildResponse(response: Response, body: ArrayBuffer): Response {
const rebuilt = new Response(
NULL_BODY_STATUSES.has(response.status) ? null : body,
{
status: response.status,
statusText: response.statusText,
headers: response.headers,
},
);
for (const key of ["url", "redirected", "type"] as const) {
Object.defineProperty(rebuilt, key, {
value: response[key],
enumerable: true,
configurable: true,
});
}
return rebuilt;
}
/**
* Wrap a `fetch` so every call through it is bounded by `timeoutMs`.
*
@ -87,9 +151,14 @@ function requestUrlOf(input: RequestInfo | URL): string {
* `buildEffectiveAuthFetch` rather than wrapping `this.fetchFn`, which the
* transport is handed directly.
*
* A caller-supplied `init.signal` is preserved — the two are composed with
* `AbortSignal.any`, so an outer cancellation still wins and this wrapper only
* ever *adds* a reason to give up.
* A caller's own signal is preserved — whether it arrived in `init` or embedded
* in a `Request` — and is both forwarded to the inner fetch (composed with
* `AbortSignal.any`) and raced here, so an outer cancellation still wins even on
* 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.
*/
export function withOAuthRequestTimeout(
fetchFn: typeof fetch,
@ -106,16 +175,18 @@ export function withOAuthRequestTimeout(
return async (input, init) => {
const url = requestUrlOf(input);
const controller = new AbortController();
// `init.signal` is `AbortSignal | null | undefined`; only an actual signal
// is worth composing.
const callerSignal = init?.signal ?? undefined;
const callerSignal = callerSignalOf(input, init);
const signal = callerSignal
? AbortSignal.any([controller.signal, callerSignal])
: controller.signal;
let timer: ReturnType<typeof setTimeout> | undefined;
let timedOut = false;
const deadline = new Promise<never>((_resolve, reject) => {
let onCallerAbort: (() => void) | undefined;
// The losing side of every race below: it rejects when the budget runs out,
// and when the caller cancels.
const abandoned = new Promise<never>((_resolve, reject) => {
timer = setTimeout(() => {
timedOut = true;
// Abort first so a direct fetch is actually cancelled rather than left
@ -124,20 +195,48 @@ export function withOAuthRequestTimeout(
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
// (Copilot). The caller's own `reason` is preserved, so it still sees its
// abort rather than a substituted error.
if (callerSignal) {
if (callerSignal.aborted) {
reject(callerSignal.reason);
} else {
onCallerAbort = () => reject(callerSignal.reason);
callerSignal.addEventListener("abort", onCallerAbort, { once: true });
}
}
});
try {
return await Promise.race([
const response = await Promise.race([
fetchFn(input, { ...init, signal }),
deadline,
abandoned,
]);
// `fetch` resolves once the response *headers* arrive, so stopping here
// would leave the body unbounded: an authorization server can send
// headers, or half a JSON document, and then stall, and the caller's
// `response.json()` would hang with nothing watching it (Copilot). Every
// response on this path is a small, finite document — metadata,
// a registration, a token — so buffering it under the same deadline
// bounds the whole exchange. This is the other reason the wrapper must
// never be applied to the transport fetch, whose bodies are streams that
// are supposed to stay open.
const body = await Promise.race([response.arrayBuffer(), abandoned]);
return rebuildResponse(response, body);
} catch (err) {
// `controller.abort()` above can reject the underlying fetch *before*
// `reject` runs, in which case the race settles with undici's
// `AbortError` instead of ours. Which of the two wins is an ordering
// detail of the fetch implementation, so normalize on the flag rather
// than on the error that surfaced: a timeout must always be reported as
// one, naming the endpoint.
// one, naming the endpoint. A caller-driven abort leaves the flag false
// and so passes through with its own reason intact.
if (timedOut) throw new OAuthRequestTimeoutError(url, budget);
throw err;
} finally {
@ -145,6 +244,12 @@ export function withOAuthRequestTimeout(
is always assigned by the time this runs; the guard exists only because
TypeScript cannot see that. */
if (timer !== undefined) clearTimeout(timer);
// Drop the listener even though it is `once`: a caller signal can outlive
// this request (one `AbortController` per connect attempt, several
// requests through it), and a listener per request would accumulate on it.
if (onCallerAbort && callerSignal) {
callerSignal.removeEventListener("abort", onCallerAbort);
}
}
};
}