Merge pull request #2324 from modelcontextprotocol/v2/feat/2319-oauth-timeouts
feat: bound every OAuth-path request with a deadline
This commit is contained in:
Cliff Hall
GitHub
commit
fcfc9ba359
13 changed files with 2444 additions and 29 deletions
|
|
@ -53,6 +53,7 @@ import {
|
|||
getAuthorizationServerUrlCandidates,
|
||||
} from "@inspector/core/auth/discovery.js";
|
||||
import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js";
|
||||
import { withOAuthRequestTimeout } from "@inspector/core/auth/requestTimeout.js";
|
||||
import { writeStoreFile } from "@inspector/core/storage/store-io.js";
|
||||
import {
|
||||
refreshAuthorization,
|
||||
|
|
@ -353,7 +354,14 @@ export async function refreshStoredAuthToken(
|
|||
// else puts a proxy under it, and a server reachable only through
|
||||
// `HTTPS_PROXY` would otherwise be probed directly. The same fetch is handed
|
||||
// to the token request below, so neither leg bypasses the proxy (Copilot).
|
||||
const storedAuthFetch = withRfc8414OidcCompat(createProxyFetch() ?? fetch);
|
||||
// #2319: this path runs outside `InspectorClient`, so nothing else bounds it
|
||||
// — the discovery and the token request below would otherwise hang forever
|
||||
// against an authorization server that accepts the connection and never
|
||||
// answers. Innermost, so the compat wrapper's own probe requests inherit the
|
||||
// deadline too.
|
||||
const storedAuthFetch = withRfc8414OidcCompat(
|
||||
withOAuthRequestTimeout(createProxyFetch() ?? fetch),
|
||||
);
|
||||
const discover: typeof discoverAuthorizationServerMetadata =
|
||||
deps.discover ??
|
||||
((authorizationServerUrl, options) =>
|
||||
|
|
|
|||
|
|
@ -439,3 +439,186 @@ describe("withRfc8414OidcCompat", () => {
|
|||
expect(base).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
});
|
||||
|
||||
describe("probe cancellation (#2319)", () => {
|
||||
const RFC8414 =
|
||||
"https://as.example.com/.well-known/oauth-authorization-server/tenant";
|
||||
|
||||
/** 404 on the RFC 8414 leg, then never answers the OIDC probe. */
|
||||
function stallingProbeFetch() {
|
||||
return vi.fn<typeof fetch>((input, init) => {
|
||||
if (String(input) === RFC8414) {
|
||||
return Promise.resolve(new Response(null, { status: 404 }));
|
||||
}
|
||||
return new Promise<Response>((_resolve, reject) => {
|
||||
// A real `fetch` rejects immediately on a signal that is already
|
||||
// aborted, so the double is only useful if it does the same.
|
||||
if (init?.signal?.aborted) {
|
||||
reject(init.signal.reason as Error);
|
||||
return;
|
||||
}
|
||||
init?.signal?.addEventListener("abort", () =>
|
||||
reject(init.signal?.reason as Error),
|
||||
);
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
it("carries the caller's signal into the probe", async () => {
|
||||
const inner = stallingProbeFetch();
|
||||
const wrapped = withRfc8414OidcCompat(inner);
|
||||
const caller = new AbortController();
|
||||
|
||||
const settled = wrapped(RFC8414, { signal: caller.signal }).then(
|
||||
(r) => r,
|
||||
(e: unknown) => e,
|
||||
);
|
||||
// Wait until the probe is actually in flight, so the abort exercises
|
||||
// cancellation rather than the pre-aborted short-circuit.
|
||||
await vi.waitFor(() => {
|
||||
expect(inner).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
const reason = new Error("caller gave up");
|
||||
caller.abort(reason);
|
||||
|
||||
// Not the preceding 404: substituting it would report a discovery failure
|
||||
// for a request the caller deliberately cancelled.
|
||||
expect(await settled).toBe(reason);
|
||||
expect((inner.mock.calls[1][1] as RequestInit).signal).toBe(caller.signal);
|
||||
});
|
||||
|
||||
it("does not issue a probe the caller already cancelled", async () => {
|
||||
const inner = stallingProbeFetch();
|
||||
const wrapped = withRfc8414OidcCompat(inner);
|
||||
const reason = new Error("already gone");
|
||||
|
||||
const settled = wrapped(RFC8414, {
|
||||
signal: AbortSignal.abort(reason),
|
||||
}).then(
|
||||
(r) => r,
|
||||
(e: unknown) => e,
|
||||
);
|
||||
|
||||
expect(await settled).toBe(reason);
|
||||
// Only the RFC 8414 leg, which the caller did make.
|
||||
expect(inner).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("rethrows an abort that lands while the probe body is pending", async () => {
|
||||
// `fetch` resolves on headers, so a late abort rejects `probe.text()`
|
||||
// rather than the fetch — a separate catch, which used to answer with the
|
||||
// preceding 404.
|
||||
const caller = new AbortController();
|
||||
const inner = vi.fn<typeof fetch>((input, init) => {
|
||||
if (String(input) === RFC8414) {
|
||||
return Promise.resolve(new Response(null, { status: 404 }));
|
||||
}
|
||||
// Headers now, body never.
|
||||
return Promise.resolve(
|
||||
new Response(
|
||||
new ReadableStream<Uint8Array>({
|
||||
start(ctrl) {
|
||||
ctrl.enqueue(new TextEncoder().encode('{"issuer":'));
|
||||
init?.signal?.addEventListener("abort", () =>
|
||||
ctrl.error(init.signal?.reason as Error),
|
||||
);
|
||||
},
|
||||
}),
|
||||
{ headers: { "content-type": "application/json" } },
|
||||
),
|
||||
);
|
||||
});
|
||||
const wrapped = withRfc8414OidcCompat(inner);
|
||||
|
||||
const settled = wrapped(RFC8414, { signal: caller.signal }).then(
|
||||
(r) => r,
|
||||
(e: unknown) => e,
|
||||
);
|
||||
await vi.waitFor(() => {
|
||||
expect(inner).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
const reason = new Error("caller gave up mid-body");
|
||||
caller.abort(reason);
|
||||
|
||||
expect(await settled).toBe(reason);
|
||||
});
|
||||
|
||||
it("releases the discarded discovery response on an exceptional exit", async () => {
|
||||
// Nobody receives `response` when the loop throws, and an unread body holds
|
||||
// its connection open on Node/undici — the same reason the successful
|
||||
// substitution path releases it.
|
||||
const cancels = vi.fn<() => Promise<void>>().mockResolvedValue(undefined);
|
||||
const discovery = new Response(
|
||||
new ReadableStream<Uint8Array>({
|
||||
start(ctrl) {
|
||||
ctrl.enqueue(new TextEncoder().encode("not found"));
|
||||
},
|
||||
}),
|
||||
{ status: 404 },
|
||||
);
|
||||
// Spy on the body's cancel, which is what `releaseBody` calls.
|
||||
const body = discovery.body;
|
||||
if (body) vi.spyOn(body, "cancel").mockImplementation(cancels);
|
||||
|
||||
const inner = vi.fn<typeof fetch>((input) =>
|
||||
String(input) === RFC8414
|
||||
? Promise.resolve(discovery)
|
||||
: new Promise<Response>(() => {}),
|
||||
);
|
||||
const wrapped = withRfc8414OidcCompat(inner);
|
||||
|
||||
const settled = wrapped(RFC8414, {
|
||||
signal: AbortSignal.abort(new Error("gone")),
|
||||
}).then(
|
||||
() => "resolved",
|
||||
() => "threw",
|
||||
);
|
||||
|
||||
expect(await settled).toBe("threw");
|
||||
expect(cancels).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does not wait on a cancel that never settles", async () => {
|
||||
// ⚠️ `ReadableStream.cancel()` adopts the underlying source's cancel
|
||||
// promise, which is permitted never to settle — so awaiting the release on
|
||||
// a path that is propagating a cancellation would hang the very thing it
|
||||
// was meant to end.
|
||||
const discovery = new Response(
|
||||
new ReadableStream<Uint8Array>({
|
||||
start(ctrl) {
|
||||
ctrl.enqueue(new TextEncoder().encode("not found"));
|
||||
},
|
||||
cancel() {
|
||||
return new Promise<void>(() => {});
|
||||
},
|
||||
}),
|
||||
{ status: 404 },
|
||||
);
|
||||
const inner = vi.fn<typeof fetch>((input) =>
|
||||
String(input) === RFC8414
|
||||
? Promise.resolve(discovery)
|
||||
: new Promise<Response>(() => {}),
|
||||
);
|
||||
const wrapped = withRfc8414OidcCompat(inner);
|
||||
const reason = new Error("gone");
|
||||
|
||||
// Would hang if the release were awaited; the assertion is that it settles.
|
||||
await expect(
|
||||
wrapped(RFC8414, { signal: AbortSignal.abort(reason) }),
|
||||
).rejects.toBe(reason);
|
||||
});
|
||||
|
||||
it("still falls back to the original response on an ordinary probe failure", async () => {
|
||||
const inner = vi.fn<typeof fetch>((input) => {
|
||||
if (String(input) === RFC8414) {
|
||||
return Promise.resolve(new Response(null, { status: 404 }));
|
||||
}
|
||||
return Promise.reject(new TypeError("network error"));
|
||||
});
|
||||
const wrapped = withRfc8414OidcCompat(inner);
|
||||
|
||||
const response = await wrapped(RFC8414);
|
||||
|
||||
expect(response.status).toBe(404);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
895
clients/web/src/test/core/auth/requestTimeout.test.ts
Normal file
895
clients/web/src/test/core/auth/requestTimeout.test.ts
Normal file
|
|
@ -0,0 +1,895 @@
|
|||
/**
|
||||
* Unit coverage for `withOAuthRequestTimeout` (#2319) — the deadline every
|
||||
* OAuth-path request runs under.
|
||||
*
|
||||
* What is pinned here, and why each case exists rather than being obvious:
|
||||
*
|
||||
* - **The bound itself**, including the *body*: `fetch` resolves on headers, so
|
||||
* a server that sends them and then stalls has to be caught by the buffering
|
||||
* race, not by the fetch promise.
|
||||
* - **The response's identity.** The bound drains a *clone* and hands back the
|
||||
* original, so the cases assert identity, that the body is still readable
|
||||
* after the drain, and that a caller's own `clone()` keeps its metadata —
|
||||
* the property a rebuilt response could not have, since `url` / `redirected`
|
||||
* / `type` are internal slots.
|
||||
* - **Cancellation semantics.** The caller's signal is forwarded *and* raced,
|
||||
* so the tests distinguish the two: one uses a fetch that honours the signal,
|
||||
* another uses one that ignores it entirely.
|
||||
* - **Composition with `withRfc8414OidcCompat`**, written as a contrast between
|
||||
* the two orderings. It is the ordering that is under test, not one
|
||||
* arrangement's behaviour, so the wrong one is asserted to misattribute.
|
||||
*
|
||||
* Fake timers throughout: every budget here is exercised by advancing the clock
|
||||
* rather than by waiting, so the suite costs milliseconds.
|
||||
*/
|
||||
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||
import {
|
||||
DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
|
||||
OAuthRequestTimeoutError,
|
||||
deadlineForRequestInit,
|
||||
exemptMcpEndpoint,
|
||||
withOAuthRequestTimeout,
|
||||
} from "@inspector/core/auth/requestTimeout.js";
|
||||
import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js";
|
||||
|
||||
const URL_UNDER_TEST =
|
||||
"https://as.example.com/.well-known/oauth-authorization-server";
|
||||
|
||||
/** A fetch that never settles — the wedged authorization server of #2319. */
|
||||
const neverSettles: typeof fetch = () => new Promise<Response>(() => {});
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
describe("withOAuthRequestTimeout", () => {
|
||||
it("passes a prompt response through", async () => {
|
||||
// The same object: the body is drained through a clone under the deadline
|
||||
// (see the stalled-body case below), so nothing about the response the
|
||||
// caller receives changes.
|
||||
const inner = vi
|
||||
.fn<typeof fetch>()
|
||||
.mockResolvedValue(new Response('{"ok":true}', { status: 200 }));
|
||||
const wrapped = withOAuthRequestTimeout(inner, 1000);
|
||||
|
||||
const response = await wrapped(URL_UNDER_TEST);
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
await expect(response.json()).resolves.toEqual({ ok: true });
|
||||
expect(inner).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("rejects a stalled request with an error naming the endpoint", async () => {
|
||||
vi.useFakeTimers();
|
||||
const wrapped = withOAuthRequestTimeout(neverSettles, 1000);
|
||||
|
||||
const pending = wrapped(URL_UNDER_TEST);
|
||||
const assertion = expect(pending).rejects.toThrow(
|
||||
`OAuth request to ${URL_UNDER_TEST} timed out after 1000ms`,
|
||||
);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
await assertion;
|
||||
});
|
||||
|
||||
it("reports the timeout as OAuthRequestTimeoutError carrying url and budget", async () => {
|
||||
vi.useFakeTimers();
|
||||
const wrapped = withOAuthRequestTimeout(neverSettles, 1000);
|
||||
|
||||
const pending = wrapped(new URL(URL_UNDER_TEST));
|
||||
const assertion = pending.catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
const err = await assertion;
|
||||
|
||||
expect(err).toBeInstanceOf(OAuthRequestTimeoutError);
|
||||
const timeout = err as OAuthRequestTimeoutError;
|
||||
expect(timeout.name).toBe("OAuthRequestTimeoutError");
|
||||
expect(timeout.url).toBe(URL_UNDER_TEST);
|
||||
expect(timeout.timeoutMs).toBe(1000);
|
||||
});
|
||||
|
||||
it("redacts sensitive query values from the endpoint it names", async () => {
|
||||
vi.useFakeTimers();
|
||||
// This message is recorded verbatim by `createFetchTracker` into the
|
||||
// Network log and the persisted session, and an OAuth endpoint's query
|
||||
// string can carry a `code`, an `access_token` or a `client_secret` —
|
||||
// which `fetchTracking` already redacts everywhere else it records a URL.
|
||||
const sensitive =
|
||||
"https://as.example.com/token?code=abc123&client_secret=shhh&state=keepme";
|
||||
const wrapped = withOAuthRequestTimeout(neverSettles, 1000);
|
||||
|
||||
const assertion = wrapped(sensitive).catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
const err = (await assertion) as OAuthRequestTimeoutError;
|
||||
|
||||
for (const secret of ["abc123", "shhh"]) {
|
||||
expect(err.message).not.toContain(secret);
|
||||
expect(err.url).not.toContain(secret);
|
||||
}
|
||||
// Still identifiable, which was the point of naming the endpoint: the path
|
||||
// and every non-sensitive parameter survive. Read through `URL`, since
|
||||
// `URLSearchParams.toString()` percent-encodes the redaction marker.
|
||||
const redacted = new URL(err.url);
|
||||
expect(redacted.origin + redacted.pathname).toBe(
|
||||
"https://as.example.com/token",
|
||||
);
|
||||
expect(redacted.searchParams.get("state")).toBe("keepme");
|
||||
expect(redacted.searchParams.get("code")).toBe("[REDACTED]");
|
||||
expect(redacted.searchParams.get("client_secret")).toBe("[REDACTED]");
|
||||
});
|
||||
|
||||
it("takes the URL off a Request object too", async () => {
|
||||
vi.useFakeTimers();
|
||||
const wrapped = withOAuthRequestTimeout(neverSettles, 1000);
|
||||
|
||||
const pending = wrapped(new Request(URL_UNDER_TEST, { method: "POST" }));
|
||||
const assertion = pending.catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
|
||||
expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({
|
||||
url: URL_UNDER_TEST,
|
||||
});
|
||||
});
|
||||
|
||||
it("aborts the underlying fetch rather than leaving it running detached", async () => {
|
||||
vi.useFakeTimers();
|
||||
let seen: AbortSignal | undefined;
|
||||
const inner: typeof fetch = (_input, init) => {
|
||||
seen = init?.signal ?? undefined;
|
||||
return new Promise<Response>((_resolve, reject) => {
|
||||
init?.signal?.addEventListener("abort", () =>
|
||||
reject(new Error("aborted by signal")),
|
||||
);
|
||||
});
|
||||
};
|
||||
const wrapped = withOAuthRequestTimeout(inner, 1000);
|
||||
|
||||
const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
|
||||
expect(seen?.aborted).toBe(true);
|
||||
// The underlying rejection can win the race against our own; either way the
|
||||
// caller must be told it was a timeout, and which endpoint stalled.
|
||||
expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError);
|
||||
});
|
||||
|
||||
it("still enforces the deadline when the fetch ignores the signal", async () => {
|
||||
vi.useFakeTimers();
|
||||
// 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);
|
||||
|
||||
const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
|
||||
expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError);
|
||||
});
|
||||
|
||||
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", () =>
|
||||
reject(new Error("caller cancelled")),
|
||||
);
|
||||
});
|
||||
const wrapped = withOAuthRequestTimeout(inner, 60_000);
|
||||
|
||||
const caller = new AbortController();
|
||||
const pending = wrapped(URL_UNDER_TEST, { signal: caller.signal });
|
||||
caller.abort();
|
||||
|
||||
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 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();
|
||||
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 an already-aborted signal without sending the request", async () => {
|
||||
const inner = vi.fn<typeof fetch>(neverSettles);
|
||||
const wrapped = withOAuthRequestTimeout(inner, 60_000);
|
||||
const reason = new Error("already gone");
|
||||
|
||||
await expect(
|
||||
wrapped(URL_UNDER_TEST, { signal: AbortSignal.abort(reason) }),
|
||||
).rejects.toBe(reason);
|
||||
// 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();
|
||||
});
|
||||
|
||||
it("treats an undefined init.signal as absent, keeping a Request's own", async () => {
|
||||
// `RequestInit` is a WebIDL dictionary: a member present as `undefined` is
|
||||
// converted as absent. `{ ...base, signal: undefined }` is what a spread
|
||||
// over an options object with no signal produces.
|
||||
const wrapped = withOAuthRequestTimeout(neverSettles, 60_000);
|
||||
|
||||
const caller = new AbortController();
|
||||
const pending = wrapped(
|
||||
new Request(URL_UNDER_TEST, { signal: caller.signal }),
|
||||
{ signal: undefined },
|
||||
);
|
||||
const reason = new Error("request cancelled");
|
||||
caller.abort(reason);
|
||||
|
||||
await expect(pending).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("releases both tee branches when the deadline wins", async () => {
|
||||
vi.useFakeTimers();
|
||||
// Losing the race rejects the caller; on its own it does not stop the read.
|
||||
// A drain still running would keep pulling bytes *and* keep buffering them
|
||||
// for the unread original branch — unbounded memory on exactly the
|
||||
// signal-ignoring body the race exists to contain. Observed through the
|
||||
// stream's own `cancel`, which is what tearing the branches down calls.
|
||||
let cancelled = false;
|
||||
const stalled = new ReadableStream<Uint8Array>({
|
||||
start(ctrl) {
|
||||
ctrl.enqueue(new TextEncoder().encode('{"issuer":'));
|
||||
},
|
||||
// Never resolves: headers arrived, the body never finishes.
|
||||
pull() {
|
||||
return new Promise<void>(() => {});
|
||||
},
|
||||
cancel() {
|
||||
cancelled = true;
|
||||
},
|
||||
});
|
||||
const wrapped = withOAuthRequestTimeout(
|
||||
vi
|
||||
.fn<typeof fetch>()
|
||||
.mockResolvedValue(
|
||||
new Response(stalled, { headers: { "content-type": "text/plain" } }),
|
||||
),
|
||||
1000,
|
||||
);
|
||||
|
||||
const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
|
||||
expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError);
|
||||
// The cancels are deliberately not awaited — cancelling one branch of a tee
|
||||
// can wait on the other — so let them settle before observing.
|
||||
for (let i = 0; i < 20; i++) await Promise.resolve();
|
||||
expect(cancelled).toBe(true);
|
||||
});
|
||||
|
||||
it("still reports the timeout when tearing the branches down fails", async () => {
|
||||
vi.useFakeTimers();
|
||||
// Cancelling a stream that is already errored, locked elsewhere, or whose
|
||||
// source rejects is not a failure here — the cleanup runs on a path that is
|
||||
// already rejecting with something the caller cares about far more.
|
||||
const hostile = new ReadableStream<Uint8Array>({
|
||||
start(ctrl) {
|
||||
ctrl.enqueue(new TextEncoder().encode("x"));
|
||||
},
|
||||
pull() {
|
||||
return new Promise<void>(() => {});
|
||||
},
|
||||
cancel() {
|
||||
throw new Error("cancel blew up");
|
||||
},
|
||||
});
|
||||
const wrapped = withOAuthRequestTimeout(
|
||||
vi
|
||||
.fn<typeof fetch>()
|
||||
.mockResolvedValue(
|
||||
new Response(hostile, { headers: { "content-type": "text/plain" } }),
|
||||
),
|
||||
1000,
|
||||
);
|
||||
|
||||
const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
|
||||
expect((await assertion) as Error).toBeInstanceOf(OAuthRequestTimeoutError);
|
||||
// And no unhandled rejection escapes from the cleanup.
|
||||
for (let i = 0; i < 20; i++) await Promise.resolve();
|
||||
});
|
||||
|
||||
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" },
|
||||
});
|
||||
const wrapped = withOAuthRequestTimeout(
|
||||
vi.fn<typeof fetch>().mockResolvedValue(original),
|
||||
1000,
|
||||
);
|
||||
|
||||
const response = await wrapped(URL_UNDER_TEST);
|
||||
|
||||
// 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("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("hello")),
|
||||
1000,
|
||||
);
|
||||
|
||||
const response = await wrapped(URL_UNDER_TEST);
|
||||
|
||||
expect(response.bodyUsed).toBe(false);
|
||||
await expect(response.text()).resolves.toBe("hello");
|
||||
});
|
||||
|
||||
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(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();
|
||||
});
|
||||
|
||||
it("propagates a non-timeout failure unchanged", async () => {
|
||||
const boom = new TypeError("network error");
|
||||
const inner = vi.fn<typeof fetch>().mockRejectedValue(boom);
|
||||
const wrapped = withOAuthRequestTimeout(inner, 1000);
|
||||
|
||||
await expect(wrapped(URL_UNDER_TEST)).rejects.toBe(boom);
|
||||
});
|
||||
|
||||
it("rounds a fractional budget, which is reported to the caller", async () => {
|
||||
vi.useFakeTimers();
|
||||
// `setTimeout` would accept the fraction and truncate it. The rounding is
|
||||
// for the budget's *reported* form: a `performance.now()` subtraction would
|
||||
// otherwise name "1000.4000000953674ms" in the message and expose a
|
||||
// non-integer `timeoutMs`.
|
||||
const wrapped = withOAuthRequestTimeout(neverSettles, 1000.4);
|
||||
|
||||
const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
|
||||
expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({
|
||||
timeoutMs: 1000,
|
||||
});
|
||||
});
|
||||
|
||||
it("falls back to the default on a non-finite budget", async () => {
|
||||
vi.useFakeTimers();
|
||||
// `NaN` survives `Math.max(0, Math.round(NaN))` and `setTimeout(fn, NaN)`
|
||||
// fires immediately, so without this every OAuth request under a budget
|
||||
// that came out of bad arithmetic would fail at once with "timed out after
|
||||
// NaNms".
|
||||
for (const bad of [NaN, Infinity, -Infinity]) {
|
||||
const wrapped = withOAuthRequestTimeout(neverSettles, bad);
|
||||
const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
|
||||
|
||||
await vi.advanceTimersByTimeAsync(DEFAULT_OAUTH_REQUEST_TIMEOUT_MS - 1);
|
||||
expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe(
|
||||
"pending",
|
||||
);
|
||||
|
||||
await vi.advanceTimersByTimeAsync(1);
|
||||
expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({
|
||||
timeoutMs: DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
it("clamps an over-large budget to what setTimeout can schedule", async () => {
|
||||
vi.useFakeTimers();
|
||||
// Past 2**31-1 the delay overflows a 32-bit signed int and Node falls back
|
||||
// to 1ms — an immediate timeout, the opposite of what was asked for.
|
||||
const wrapped = withOAuthRequestTimeout(neverSettles, 2 ** 40);
|
||||
|
||||
const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe(
|
||||
"pending",
|
||||
);
|
||||
|
||||
await vi.advanceTimersByTimeAsync(2_147_483_647);
|
||||
expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({
|
||||
timeoutMs: 2_147_483_647,
|
||||
});
|
||||
});
|
||||
|
||||
it("clamps a negative budget to zero rather than throwing", async () => {
|
||||
vi.useFakeTimers();
|
||||
const wrapped = withOAuthRequestTimeout(neverSettles, -1);
|
||||
|
||||
const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(0);
|
||||
|
||||
expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({
|
||||
timeoutMs: 0,
|
||||
});
|
||||
});
|
||||
|
||||
it("defaults to a generous budget a slow authorization server can meet", async () => {
|
||||
vi.useFakeTimers();
|
||||
const wrapped = withOAuthRequestTimeout(neverSettles);
|
||||
|
||||
const assertion = wrapped(URL_UNDER_TEST).catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(DEFAULT_OAUTH_REQUEST_TIMEOUT_MS - 1);
|
||||
expect(await Promise.race([assertion, Promise.resolve("pending")])).toBe(
|
||||
"pending",
|
||||
);
|
||||
|
||||
await vi.advanceTimersByTimeAsync(1);
|
||||
expect((await assertion) as OAuthRequestTimeoutError).toMatchObject({
|
||||
timeoutMs: DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
|
||||
});
|
||||
});
|
||||
|
||||
describe("the deadline stamped on the init (#2319 proxy hop)", () => {
|
||||
// How the budget reaches `/api/fetch` without travelling upstream: a header
|
||||
// would be copied verbatim into the payload and re-sent to the
|
||||
// authorization server, so the carrier is a WeakMap keyed on the init.
|
||||
it("stamps the budget on the init it hands down", async () => {
|
||||
const inner = vi.fn<typeof fetch>().mockResolvedValue(new Response("{}"));
|
||||
const wrapped = withOAuthRequestTimeout(inner, 1234);
|
||||
|
||||
await wrapped(URL_UNDER_TEST);
|
||||
|
||||
expect(deadlineForRequestInit(inner.mock.calls[0][1])).toBe(1234);
|
||||
});
|
||||
|
||||
it("stamps the rounded budget, matching what is reported", async () => {
|
||||
const inner = vi.fn<typeof fetch>().mockResolvedValue(new Response("{}"));
|
||||
const wrapped = withOAuthRequestTimeout(inner, 1000.4);
|
||||
|
||||
await wrapped(URL_UNDER_TEST);
|
||||
|
||||
expect(deadlineForRequestInit(inner.mock.calls[0][1])).toBe(1000);
|
||||
});
|
||||
|
||||
it("stamps nothing on an exempt request", async () => {
|
||||
// The route must apply no deadline to MCP traffic, and it decides that
|
||||
// from the absence of a budget in the envelope.
|
||||
const inner = vi.fn<typeof fetch>().mockResolvedValue(new Response("{}"));
|
||||
const wrapped = withOAuthRequestTimeout(inner, 1234, () => true);
|
||||
|
||||
await wrapped(URL_UNDER_TEST, { method: "POST" });
|
||||
|
||||
expect(deadlineForRequestInit(inner.mock.calls[0][1])).toBeUndefined();
|
||||
});
|
||||
|
||||
it("reads nothing off an init that never went through the wrapper", () => {
|
||||
expect(deadlineForRequestInit(undefined)).toBeUndefined();
|
||||
expect(deadlineForRequestInit({})).toBeUndefined();
|
||||
expect(deadlineForRequestInit({ method: "GET" })).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe("exemptMcpEndpoint (the transport chain's mixed traffic)", () => {
|
||||
const SERVER = "https://srv.example.com/mcp";
|
||||
|
||||
/** Most cases are plain JSON-RPC traffic, which carries no form media type. */
|
||||
const JSON_HEADERS = new Headers({ "content-type": "application/json" });
|
||||
/** The token family: RFC 6749 §4.1.3 and RFC 7009 §2.1 are form-encoded. */
|
||||
const FORM_HEADERS = new Headers({
|
||||
"content-type": "application/x-www-form-urlencoded;charset=UTF-8",
|
||||
});
|
||||
|
||||
/** Streamable HTTP: every MCP request goes to the configured URL. */
|
||||
const streamable = (serverUrl: string) =>
|
||||
exemptMcpEndpoint(
|
||||
() => serverUrl,
|
||||
() => false,
|
||||
);
|
||||
/** Legacy SSE: the message endpoint arrives inside the stream. */
|
||||
const sse = (serverUrl: string) =>
|
||||
exemptMcpEndpoint(
|
||||
() => serverUrl,
|
||||
() => true,
|
||||
);
|
||||
|
||||
it("exempts the MCP endpoint itself, whatever method or query", () => {
|
||||
const isExempt = exemptMcpEndpoint(() => SERVER);
|
||||
|
||||
expect(isExempt(SERVER, JSON_HEADERS)).toBe(true);
|
||||
expect(isExempt(`${SERVER}?sessionId=abc`, JSON_HEADERS)).toBe(true);
|
||||
expect(isExempt(`${SERVER}#frag`, JSON_HEADERS)).toBe(true);
|
||||
});
|
||||
|
||||
it("never exempts a form-encoded request, whatever URL it uses", () => {
|
||||
// `oauthTokenUrl` takes any absolute URL, including the MCP endpoint's
|
||||
// own — at which point a URL rule would exempt the transport-internal
|
||||
// refresh, the one request this change exists to bound. The token family
|
||||
// is form-encoded and MCP never is, so the media type settles it first.
|
||||
for (const isExempt of [
|
||||
streamable(SERVER),
|
||||
sse(SERVER),
|
||||
exemptMcpEndpoint(() => SERVER),
|
||||
]) {
|
||||
expect(isExempt(SERVER, FORM_HEADERS)).toBe(false);
|
||||
}
|
||||
// And a missing server URL no longer fails open past it either.
|
||||
expect(exemptMcpEndpoint(() => undefined)(SERVER, FORM_HEADERS)).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it("bounds same-origin OAuth work on a Streamable HTTP connection", () => {
|
||||
// The narrow rule, available because the endpoint is the configured URL.
|
||||
const isExempt = streamable(SERVER);
|
||||
|
||||
expect(isExempt(SERVER, JSON_HEADERS)).toBe(true);
|
||||
expect(
|
||||
isExempt(
|
||||
"https://srv.example.com/.well-known/oauth-protected-resource/mcp",
|
||||
JSON_HEADERS,
|
||||
),
|
||||
).toBe(false);
|
||||
expect(isExempt("https://srv.example.com/token", JSON_HEADERS)).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it("widens to the origin on legacy SSE, whose message path is unknowable", () => {
|
||||
// The cost of the wider rule, asserted rather than implied: same-origin
|
||||
// OAuth is exempt here, and falls back to the SDK's per-request timeout.
|
||||
const isExempt = sse("https://srv.example.com/sse");
|
||||
|
||||
expect(
|
||||
isExempt(
|
||||
"https://srv.example.com/.well-known/openid-configuration",
|
||||
JSON_HEADERS,
|
||||
),
|
||||
).toBe(true);
|
||||
expect(isExempt("https://as.example.com/token", JSON_HEADERS)).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it("falls back to the origin rule when the transport is unknown", () => {
|
||||
// Fails open on this uncertainty like every other: a path rule applied to
|
||||
// a connection that turns out to be SSE severs real MCP traffic.
|
||||
const noHint = exemptMcpEndpoint(() => SERVER);
|
||||
const throws = exemptMcpEndpoint(
|
||||
() => SERVER,
|
||||
() => {
|
||||
throw new Error("not yet connected");
|
||||
},
|
||||
);
|
||||
|
||||
expect(noHint("https://srv.example.com/token", JSON_HEADERS)).toBe(true);
|
||||
expect(throws("https://srv.example.com/token", JSON_HEADERS)).toBe(true);
|
||||
});
|
||||
|
||||
it("exempts legacy SSE's separate message endpoint", () => {
|
||||
// The case a path rule gets wrong: `SSEClientTransport` opens the
|
||||
// configured URL and is handed a *different* pathname to POST every
|
||||
// JSON-RPC message to. Bounding those would sever a slow tool call over
|
||||
// SSE and report it as an OAuth timeout. The SDK enforces that the
|
||||
// message endpoint shares the stream URL's origin, which is what makes
|
||||
// an origin rule exact rather than approximate.
|
||||
const isExempt = exemptMcpEndpoint(() => "https://srv.example.com/sse");
|
||||
|
||||
expect(
|
||||
isExempt(
|
||||
"https://srv.example.com/messages?sessionId=abc",
|
||||
JSON_HEADERS,
|
||||
),
|
||||
).toBe(true);
|
||||
expect(isExempt("https://srv.example.com/", JSON_HEADERS)).toBe(true);
|
||||
});
|
||||
|
||||
it("bounds OAuth work on a different origin", () => {
|
||||
const isExempt = exemptMcpEndpoint(() => SERVER);
|
||||
|
||||
expect(isExempt("https://as.example.com/token", JSON_HEADERS)).toBe(
|
||||
false,
|
||||
);
|
||||
expect(
|
||||
isExempt(
|
||||
"https://as.example.com/.well-known/openid-configuration",
|
||||
JSON_HEADERS,
|
||||
),
|
||||
).toBe(false);
|
||||
// Origin is scheme + host + port, so any of the three differing bounds it.
|
||||
expect(isExempt("http://srv.example.com/token", JSON_HEADERS)).toBe(
|
||||
false,
|
||||
);
|
||||
expect(isExempt("https://srv.example.com:8443/token", JSON_HEADERS)).toBe(
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
it("fails open when the server URL is unknown or unparseable", () => {
|
||||
// The two errors are not symmetric: bounding what should not be bounded
|
||||
// severs a long-running tool call, while failing to bound leaves the
|
||||
// SDK's own per-request timeout as the backstop it already was.
|
||||
expect(
|
||||
exemptMcpEndpoint(() => undefined)(
|
||||
"https://as.example.com/token",
|
||||
JSON_HEADERS,
|
||||
),
|
||||
).toBe(true);
|
||||
expect(
|
||||
exemptMcpEndpoint(() => "")(
|
||||
"https://as.example.com/token",
|
||||
JSON_HEADERS,
|
||||
),
|
||||
).toBe(true);
|
||||
expect(exemptMcpEndpoint(() => "not a url")(SERVER, JSON_HEADERS)).toBe(
|
||||
true,
|
||||
);
|
||||
expect(exemptMcpEndpoint(() => SERVER)("not a url", JSON_HEADERS)).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
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);
|
||||
|
||||
expect(isExempt("https://other.example.com/mcp", JSON_HEADERS)).toBe(
|
||||
false,
|
||||
);
|
||||
current = "https://other.example.com/mcp";
|
||||
expect(isExempt("https://other.example.com/mcp", JSON_HEADERS)).toBe(
|
||||
true,
|
||||
);
|
||||
expect(isExempt(SERVER, JSON_HEADERS)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
it("passes an exempt request through with no deadline and no buffering", async () => {
|
||||
vi.useFakeTimers();
|
||||
// Untouched means untouched: no signal of ours, and the body is left as a
|
||||
// live stream rather than buffered — which is what an SSE response needs.
|
||||
const body = new ReadableStream<Uint8Array>({
|
||||
start(ctrl) {
|
||||
ctrl.enqueue(new TextEncoder().encode("data: hi\n\n"));
|
||||
},
|
||||
});
|
||||
const original = new Response(body, {
|
||||
headers: { "content-type": "text/event-stream" },
|
||||
});
|
||||
const inner = vi.fn<typeof fetch>().mockResolvedValue(original);
|
||||
const wrapped = withOAuthRequestTimeout(inner, 1000, () => true);
|
||||
|
||||
const response = await wrapped(URL_UNDER_TEST);
|
||||
|
||||
expect(response).toBe(original);
|
||||
expect(
|
||||
(inner.mock.calls[0][1] as RequestInit | undefined)?.signal,
|
||||
).toBeUndefined();
|
||||
// Nothing is armed, so advancing past the budget cannot sever it.
|
||||
await vi.advanceTimersByTimeAsync(5000);
|
||||
expect(response.bodyUsed).toBe(false);
|
||||
});
|
||||
|
||||
describe("composed under withRfc8414OidcCompat (#2319 ordering)", () => {
|
||||
// `withRfc8414OidcCompat` re-fetches an OIDC candidate after a failed RFC
|
||||
// 8414 discovery, so where the deadline sits relative to it decides which
|
||||
// request is being timed. `InspectorClient` and the CLI both put it
|
||||
// innermost; these two cases are why.
|
||||
const RFC8414 =
|
||||
"https://as.example.com/.well-known/oauth-authorization-server/tenant";
|
||||
// A path-suffixed RFC 8414 candidate has two OIDC siblings, and the compat
|
||||
// wrapper tries both.
|
||||
const PROBES = [
|
||||
"https://as.example.com/.well-known/openid-configuration/tenant",
|
||||
"https://as.example.com/tenant/.well-known/openid-configuration",
|
||||
];
|
||||
|
||||
/** Answers the RFC 8414 leg with a prompt 404, then stalls on the probe. */
|
||||
function stallingProbeFetch() {
|
||||
return vi.fn<typeof fetch>((input) => {
|
||||
if (String(input) === RFC8414) {
|
||||
return Promise.resolve(new Response(null, { status: 404 }));
|
||||
}
|
||||
return new Promise<Response>(() => {});
|
||||
});
|
||||
}
|
||||
|
||||
it("outside, a stalled probe is misreported under the preceding URL", async () => {
|
||||
vi.useFakeTimers();
|
||||
const inner = stallingProbeFetch();
|
||||
const wrapped = withOAuthRequestTimeout(
|
||||
withRfc8414OidcCompat(inner),
|
||||
1000,
|
||||
);
|
||||
|
||||
const assertion = wrapped(RFC8414).catch((err: unknown) => err);
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
|
||||
const err = (await assertion) as OAuthRequestTimeoutError;
|
||||
expect(err).toBeInstanceOf(OAuthRequestTimeoutError);
|
||||
// The endpoint that stalled was PROBE. One budget covers both legs, and
|
||||
// the error names the request the caller made rather than the one that
|
||||
// hung — exactly the diagnostic this change exists to provide.
|
||||
expect(err.url).toBe(RFC8414);
|
||||
});
|
||||
|
||||
it("innermost, the probe's own timeout escapes and names the probe", async () => {
|
||||
vi.useFakeTimers();
|
||||
const inner = stallingProbeFetch();
|
||||
const wrapped = withRfc8414OidcCompat(
|
||||
withOAuthRequestTimeout(inner, 1000),
|
||||
);
|
||||
|
||||
const assertion = wrapped(RFC8414).catch((err: unknown) => err);
|
||||
// The probe is timed from when it starts, on a budget of its own — not on
|
||||
// whatever an outer one had left after the RFC 8414 leg.
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
|
||||
const err = (await assertion) as OAuthRequestTimeoutError;
|
||||
// The compat wrapper is inert on an ordinary probe failure, but a deadline
|
||||
// the Inspector imposed escapes it, so the caller is told which endpoint
|
||||
// stalled rather than being handed the preceding 404.
|
||||
expect(err).toBeInstanceOf(OAuthRequestTimeoutError);
|
||||
expect(err.url).toBe(PROBES[0]);
|
||||
expect(inner).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
});
|
||||
|
||||
it("clears its timer once the request settles", async () => {
|
||||
vi.useFakeTimers();
|
||||
const clearSpy = vi.spyOn(globalThis, "clearTimeout");
|
||||
const wrapped = withOAuthRequestTimeout(
|
||||
vi.fn<typeof fetch>().mockResolvedValue(new Response("{}")),
|
||||
1000,
|
||||
);
|
||||
|
||||
await wrapped(URL_UNDER_TEST);
|
||||
|
||||
expect(clearSpy).toHaveBeenCalled();
|
||||
// Nothing is left to fire, so no unhandled rejection can surface later.
|
||||
await vi.advanceTimersByTimeAsync(5000);
|
||||
});
|
||||
});
|
||||
|
|
@ -393,11 +393,11 @@ describe("revokeToken", () => {
|
|||
});
|
||||
});
|
||||
|
||||
// The web fetch is `createRemoteFetch`, which re-issues the call as a POST to
|
||||
// `/api/fetch` and drops `init.signal`; the backend's outbound fetch gets no
|
||||
// signal either. A signal-only bound is therefore inert on exactly the path
|
||||
// the timeout exists for, so the deadline has to hold against a fetch that
|
||||
// ignores the signal entirely.
|
||||
// The signal reaches every path since #2319 — `createRemoteFetch` forwards it
|
||||
// onto the `/api/fetch` hop and that route composes it into its outbound
|
||||
// fetch — but a signal-only bound is still only as good as the fetch beneath
|
||||
// it. A `fetchFn` that ignores `AbortSignal`, or a wedged backend, would
|
||||
// leave a signal-only deadline inert, so it has to hold without one.
|
||||
it("gives up on a fetch that never settles and ignores the signal", async () => {
|
||||
const outcome = await revokeToken({
|
||||
endpoint: REVOKE_URL,
|
||||
|
|
|
|||
|
|
@ -1,5 +1,10 @@
|
|||
import { describe, it, expect, vi } from "vitest";
|
||||
import { createRemoteFetch } from "@inspector/core/mcp/remote/createRemoteFetch.js";
|
||||
import {
|
||||
OAUTH_TIMEOUT_WIRE_CODE,
|
||||
OAuthRequestTimeoutError,
|
||||
withOAuthRequestTimeout,
|
||||
} from "@inspector/core/auth/requestTimeout.js";
|
||||
|
||||
function ok(body: object): Response {
|
||||
return new Response(JSON.stringify(body), {
|
||||
|
|
@ -168,4 +173,225 @@ describe("createRemoteFetch", () => {
|
|||
expect(res.headers.get("x-test")).toBe("yes");
|
||||
expect(await res.text()).toBe("hello");
|
||||
});
|
||||
|
||||
describe("cancellation forwarding (#2319)", () => {
|
||||
// Without this the caller's abort settled only its own promise: the POST
|
||||
// stayed in flight, the backend never saw its request cancelled, and its
|
||||
// outbound fetch to the authorization server ran on detached.
|
||||
function capture() {
|
||||
const fetchFn = vi
|
||||
.fn<typeof fetch>()
|
||||
.mockResolvedValue(ok(standardRemoteBody));
|
||||
const remoteFetch = createRemoteFetch({
|
||||
baseUrl: "http://remote.example",
|
||||
// A contextually typed forwarding function rather than a double cast:
|
||||
// the mock stays inspectable and nothing bypasses the type system.
|
||||
fetchFn: (input, init) => fetchFn(input, init),
|
||||
});
|
||||
return { fetchFn, remoteFetch };
|
||||
}
|
||||
|
||||
function signalOf(fetchFn: ReturnType<typeof vi.fn<typeof fetch>>) {
|
||||
return (fetchFn.mock.calls[0][1] as RequestInit).signal;
|
||||
}
|
||||
|
||||
it("forwards init.signal onto the proxy hop", async () => {
|
||||
const { fetchFn, remoteFetch } = capture();
|
||||
const caller = new AbortController();
|
||||
|
||||
await remoteFetch("http://upstream.example/", { signal: caller.signal });
|
||||
|
||||
expect(signalOf(fetchFn)).toBe(caller.signal);
|
||||
});
|
||||
|
||||
it("forwards a Request's own signal when init has none", async () => {
|
||||
const { fetchFn, remoteFetch } = capture();
|
||||
const caller = new AbortController();
|
||||
// Compared against `request.signal`, not `caller.signal`: the Fetch
|
||||
// standard gives `new Request(url, { signal })` a *dependent* signal, so
|
||||
// the two are distinct objects in a spec-compliant runtime. Asserting
|
||||
// against the controller's would encode non-standard identity and could
|
||||
// pass or fail on the host rather than on the behaviour (Copilot).
|
||||
const request = new Request("http://upstream.example/", {
|
||||
signal: caller.signal,
|
||||
});
|
||||
|
||||
await remoteFetch(request);
|
||||
|
||||
expect(signalOf(fetchFn)).toBe(request.signal);
|
||||
});
|
||||
|
||||
it("treats an undefined init.signal as absent, keeping the Request's", async () => {
|
||||
// WebIDL dictionary conversion: a member present as `undefined` is absent.
|
||||
const { fetchFn, remoteFetch } = capture();
|
||||
const caller = new AbortController();
|
||||
const request = new Request("http://upstream.example/", {
|
||||
signal: caller.signal,
|
||||
});
|
||||
|
||||
await remoteFetch(request, { signal: undefined });
|
||||
|
||||
// Against `request.signal` for the dependent-signal reason above.
|
||||
expect(signalOf(fetchFn)).toBe(request.signal);
|
||||
});
|
||||
|
||||
it("honours an explicit null init.signal as no signal", async () => {
|
||||
const { fetchFn, remoteFetch } = capture();
|
||||
const caller = new AbortController();
|
||||
|
||||
await remoteFetch(
|
||||
new Request("http://upstream.example/", { signal: caller.signal }),
|
||||
{ signal: null },
|
||||
);
|
||||
|
||||
expect(signalOf(fetchFn)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("sends no signal when the caller supplied none", async () => {
|
||||
const { fetchFn, remoteFetch } = capture();
|
||||
|
||||
await remoteFetch("http://upstream.example/");
|
||||
|
||||
expect(signalOf(fetchFn)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe("proxy-side deadline (#2319)", () => {
|
||||
function remoteReturning(status: number, body: string) {
|
||||
const fetchFn = vi.fn<typeof fetch>().mockResolvedValue(
|
||||
new Response(body, {
|
||||
status,
|
||||
headers: { "content-type": "application/json" },
|
||||
}),
|
||||
);
|
||||
return createRemoteFetch({
|
||||
baseUrl: "http://remote.example",
|
||||
// Forwarding function, not a double cast — see `capture` above.
|
||||
fetchFn: (input, init) => fetchFn(input, init),
|
||||
});
|
||||
}
|
||||
|
||||
it("rebuilds an OAuthRequestTimeoutError from the route's marker", async () => {
|
||||
// The proxy boundary in isolation: whichever end's deadline fires, a
|
||||
// timeout that comes back as an ordinary error response must still
|
||||
// reconstruct as a typed one. Both ends run the same budget in
|
||||
// production, so this path is the backend winning the race — a
|
||||
// backgrounded tab throttling `setTimeout` is the plausible case — and
|
||||
// which one fired must not decide whether the error names its endpoint.
|
||||
const remoteFetch = remoteReturning(
|
||||
504,
|
||||
JSON.stringify({
|
||||
error: "proxied request to https://as.example.com/x timed out",
|
||||
code: OAUTH_TIMEOUT_WIRE_CODE,
|
||||
url: "https://as.example.com/x",
|
||||
timeoutMs: 30000,
|
||||
}),
|
||||
);
|
||||
|
||||
const err = await remoteFetch("https://as.example.com/x").catch(
|
||||
(e: unknown) => e,
|
||||
);
|
||||
|
||||
expect(err).toBeInstanceOf(OAuthRequestTimeoutError);
|
||||
const timeout = err as OAuthRequestTimeoutError;
|
||||
expect(timeout.url).toBe("https://as.example.com/x");
|
||||
expect(timeout.timeoutMs).toBe(30000);
|
||||
});
|
||||
|
||||
it("leaves an ordinary error response as a plain Error", async () => {
|
||||
const remoteFetch = remoteReturning(
|
||||
500,
|
||||
JSON.stringify({ error: "connect ECONNREFUSED" }),
|
||||
);
|
||||
|
||||
const err = await remoteFetch("https://as.example.com/x").catch(
|
||||
(e: unknown) => e,
|
||||
);
|
||||
|
||||
expect(err).not.toBeInstanceOf(OAuthRequestTimeoutError);
|
||||
expect((err as Error).message).toMatch(/Remote fetch failed \(500\)/);
|
||||
});
|
||||
|
||||
it("ignores a non-JSON error body rather than throwing on the parse", async () => {
|
||||
const remoteFetch = remoteReturning(502, "<html>bad gateway</html>");
|
||||
|
||||
const err = await remoteFetch("https://as.example.com/x").catch(
|
||||
(e: unknown) => e,
|
||||
);
|
||||
|
||||
expect((err as Error).message).toMatch(/Remote fetch failed \(502\)/);
|
||||
});
|
||||
|
||||
it("ignores a marker whose fields are the wrong shape", async () => {
|
||||
// An upstream could serve JSON of its own through a failing proxy; the
|
||||
// guard checks the field types, not just the code.
|
||||
const remoteFetch = remoteReturning(
|
||||
504,
|
||||
JSON.stringify({ code: OAUTH_TIMEOUT_WIRE_CODE, url: 5 }),
|
||||
);
|
||||
|
||||
const err = await remoteFetch("https://as.example.com/x").catch(
|
||||
(e: unknown) => e,
|
||||
);
|
||||
|
||||
expect(err).not.toBeInstanceOf(OAuthRequestTimeoutError);
|
||||
});
|
||||
});
|
||||
|
||||
describe("carrying the caller's deadline to the route (#2319)", () => {
|
||||
function envelopeOf(fetchFn: ReturnType<typeof vi.fn<typeof fetch>>) {
|
||||
const init = fetchFn.mock.calls[0][1] as RequestInit;
|
||||
return JSON.parse(init.body as string) as Record<string, unknown>;
|
||||
}
|
||||
|
||||
it("puts a bounded call's budget in the envelope, not in the headers", async () => {
|
||||
const fetchFn = vi
|
||||
.fn<typeof fetch>()
|
||||
.mockResolvedValue(ok(standardRemoteBody));
|
||||
const remoteFetch = createRemoteFetch({
|
||||
baseUrl: "http://remote.example",
|
||||
fetchFn: (input, init) => fetchFn(input, init),
|
||||
});
|
||||
// Composed the way `InspectorClient` composes it: the wrapper outside,
|
||||
// the remote fetch beneath.
|
||||
const bounded = withOAuthRequestTimeout(remoteFetch, 7000);
|
||||
|
||||
await bounded("http://upstream.example/token");
|
||||
|
||||
const envelope = envelopeOf(fetchFn);
|
||||
expect(envelope.timeoutMs).toBe(7000);
|
||||
// Not a header: `headers` is re-sent verbatim to the upstream server, so
|
||||
// a marker there would leak the Inspector's internals to a third party.
|
||||
expect(JSON.stringify(envelope.headers)).not.toContain("7000");
|
||||
});
|
||||
|
||||
it("omits the budget for an exempt call, so the route bounds nothing", async () => {
|
||||
const fetchFn = vi
|
||||
.fn<typeof fetch>()
|
||||
.mockResolvedValue(ok(standardRemoteBody));
|
||||
const remoteFetch = createRemoteFetch({
|
||||
baseUrl: "http://remote.example",
|
||||
fetchFn: (input, init) => fetchFn(input, init),
|
||||
});
|
||||
const bounded = withOAuthRequestTimeout(remoteFetch, 7000, () => true);
|
||||
|
||||
await bounded("http://upstream.example/mcp");
|
||||
|
||||
expect(envelopeOf(fetchFn)).not.toHaveProperty("timeoutMs");
|
||||
});
|
||||
|
||||
it("omits the budget for a direct call that no wrapper bounded", async () => {
|
||||
const fetchFn = vi
|
||||
.fn<typeof fetch>()
|
||||
.mockResolvedValue(ok(standardRemoteBody));
|
||||
const remoteFetch = createRemoteFetch({
|
||||
baseUrl: "http://remote.example",
|
||||
fetchFn: (input, init) => fetchFn(input, init),
|
||||
});
|
||||
|
||||
await remoteFetch("http://upstream.example/mcp");
|
||||
|
||||
expect(envelopeOf(fetchFn)).not.toHaveProperty("timeoutMs");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -300,6 +300,12 @@ 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[];
|
||||
/** Resolves when the never-ending `/stream-open` response is closed. */
|
||||
let openStreamClosed: Promise<void>;
|
||||
|
||||
// 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 +342,43 @@ 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 = [];
|
||||
let markStreamClosed: () => void = () => {};
|
||||
openStreamClosed = new Promise<void>((resolve) => {
|
||||
markStreamClosed = resolve;
|
||||
});
|
||||
const srv = createServer((req, res) => {
|
||||
upstreamHits.push(req.url ?? "");
|
||||
if (req.url === "/stream-hostile") {
|
||||
// Headers, one frame, and a connection whose teardown the route must
|
||||
// not wait on: `ReadableStream.cancel()` adopts the source's cancel
|
||||
// promise, which may never settle.
|
||||
res.writeHead(200, { "Content-Type": "text/event-stream" });
|
||||
res.write("data: hi\n\n");
|
||||
return;
|
||||
}
|
||||
if (req.url === "/stream-open") {
|
||||
// Event-stream headers, one frame, and then nothing — the shape that
|
||||
// used to leave a socket open for good, because the route classifies
|
||||
// it as a stream, returns JSON without the body, and clears its
|
||||
// deadline on the way out.
|
||||
res.writeHead(200, { "Content-Type": "text/event-stream" });
|
||||
res.write("data: hi\n\n");
|
||||
res.on("close", () => markStreamClosed());
|
||||
return;
|
||||
}
|
||||
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 +423,175 @@ 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("cancels a discarded stream that sends headers and never ends (#2319)", async () => {
|
||||
// The route answers with JSON and no body, so nobody downstream owns the
|
||||
// upstream stream and nothing will ever read it; the route cancels it
|
||||
// explicitly rather than relying on that being reclaimed for it.
|
||||
//
|
||||
// ⚠️ This pins the OUTCOME, not the mechanism: measured, it still passes
|
||||
// with the explicit `cancel()` removed, because undici reclaims an unread
|
||||
// response body here on its own. So the cancel is defensive — it makes
|
||||
// the release explicit and independent of that behaviour — and this test
|
||||
// will catch the route starting to hold the stream open, not the cancel
|
||||
// being deleted. Said plainly rather than left to imply a stronger claim.
|
||||
const res = await fetch(`${h.baseUrl}/api/fetch`, {
|
||||
method: "POST",
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify({ url: `${targetUrl}/stream-open` }),
|
||||
});
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
const payload = (await res.json()) as { status: number; body?: string };
|
||||
expect(payload.status).toBe(200);
|
||||
expect(payload.body).toBeUndefined();
|
||||
// The observable contract: the route does not leave the upstream stream
|
||||
// running after it has answered. (Per the qualification above, this does
|
||||
// not isolate the explicit `cancel()` as the cause.)
|
||||
await openStreamClosed;
|
||||
});
|
||||
|
||||
it("answers 504 with a typed marker when the request's deadline fires (#2319)", async () => {
|
||||
// The caller's budget travels in the envelope, so this costs milliseconds
|
||||
// rather than the production thirty seconds. What it pins is typed-marker
|
||||
// preservation when the *backend* wins the client-side race — both ends
|
||||
// run the same budget, and a backgrounded tab throttling `setTimeout` is
|
||||
// the plausible way the server's fires first. The error has to survive
|
||||
// the hop as something an `instanceof OAuthRequestTimeoutError` check can
|
||||
// still recognize, whichever end produced it.
|
||||
const res = await fetch(`${h.baseUrl}/api/fetch`, {
|
||||
method: "POST",
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify({ url: `${targetUrl}/stall`, timeoutMs: 120 }),
|
||||
});
|
||||
|
||||
expect(res.status).toBe(504);
|
||||
const payload = (await res.json()) as {
|
||||
error: string;
|
||||
code: string;
|
||||
url: string;
|
||||
timeoutMs: number;
|
||||
};
|
||||
expect(payload.code).toBe("oauth_request_timeout");
|
||||
expect(payload.url).toBe(`${targetUrl}/stall`);
|
||||
expect(payload.timeoutMs).toBe(120);
|
||||
expect(payload.error).toContain("timed out after 120ms");
|
||||
// And the upstream is released rather than left running.
|
||||
await stallClosed;
|
||||
});
|
||||
|
||||
it("applies no deadline to a request that carries none (#2319)", async () => {
|
||||
// ⚠️ The regression this guards: an unconditional timer here would abort
|
||||
// a Streamable HTTP tool call that legitimately withholds its response
|
||||
// headers — reintroducing on the backend exactly what `exemptMcpEndpoint`
|
||||
// prevents on the client, and reporting it as an OAuth timeout besides.
|
||||
//
|
||||
// ⚠️ What it does NOT catch, stated so nobody reads more into it: the
|
||||
// wait below is 400ms, so an unconditional timer restored at the
|
||||
// production 30s budget would still leave the request pending here and
|
||||
// the test would pass (Copilot). Catching that needs either a ~31s wait
|
||||
// on every CI run or a route-level deadline knob existing only for the
|
||||
// test — and a second source of truth for the deadline is the defect
|
||||
// round 13 removed. Measured, this fails against an unconditional timer
|
||||
// at any budget shorter than the wait.
|
||||
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 settled = pending.then(
|
||||
(r) => `responded ${r.status}`,
|
||||
() => "aborted by us",
|
||||
);
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(upstreamHits).toContain("/stall");
|
||||
});
|
||||
// Well past any budget the route might have applied on its own. Nothing
|
||||
// should have answered: an unbounded request is still in flight.
|
||||
await new Promise((r) => setTimeout(r, 400));
|
||||
expect(
|
||||
await Promise.race([settled, Promise.resolve("still pending")]),
|
||||
).toBe("still pending");
|
||||
|
||||
caller.abort();
|
||||
expect(await settled).toBe("aborted by us");
|
||||
});
|
||||
|
||||
it("ignores a non-numeric deadline rather than trusting it (#2319)", async () => {
|
||||
const caller = new AbortController();
|
||||
const pending = fetch(`${h.baseUrl}/api/fetch`, {
|
||||
method: "POST",
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify({
|
||||
url: `${targetUrl}/stall`,
|
||||
timeoutMs: "soon",
|
||||
}),
|
||||
signal: caller.signal,
|
||||
});
|
||||
const settled = pending.then(
|
||||
(r) => `responded ${r.status}`,
|
||||
() => "aborted by us",
|
||||
);
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(upstreamHits).toContain("/stall");
|
||||
});
|
||||
await new Promise((r) => setTimeout(r, 300));
|
||||
expect(
|
||||
await Promise.race([settled, Promise.resolve("still pending")]),
|
||||
).toBe("still pending");
|
||||
|
||||
caller.abort();
|
||||
expect(await settled).toBe("aborted by us");
|
||||
});
|
||||
|
||||
it("answers a discarded stream without waiting on its cancellation", async () => {
|
||||
// ⚠️ The route starts the cancel and returns. Awaiting it would keep the
|
||||
// handler pending on a source that never settles its cancel promise —
|
||||
// and on an unbounded request there is no timer to release it either.
|
||||
const res = await fetch(`${h.baseUrl}/api/fetch`, {
|
||||
method: "POST",
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify({ url: `${targetUrl}/stream-hostile` }),
|
||||
});
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
const payload = (await res.json()) as { status: number; body?: string };
|
||||
expect(payload.status).toBe(200);
|
||||
expect(payload.body).toBeUndefined();
|
||||
});
|
||||
|
||||
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
|
||||
|
|
|
|||
|
|
@ -51,6 +51,18 @@ export {
|
|||
withRfc8414OidcCompat,
|
||||
} from "./oidcDiscoveryCompat.js";
|
||||
|
||||
// OAuth-path request deadline (#2319)
|
||||
export {
|
||||
DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
|
||||
OAUTH_TIMEOUT_WIRE_CODE,
|
||||
OAuthRequestTimeoutError,
|
||||
deadlineForRequestInit,
|
||||
exemptMcpEndpoint,
|
||||
isOAuthRequestTimeoutWire,
|
||||
withOAuthRequestTimeout,
|
||||
} from "./requestTimeout.js";
|
||||
export type { OAuthRequestTimeoutWire } from "./requestTimeout.js";
|
||||
|
||||
// Storage
|
||||
export type {
|
||||
OAuthStorage,
|
||||
|
|
|
|||
|
|
@ -90,6 +90,7 @@
|
|||
* wiring in `core/mcp/inspectorClient.ts`.
|
||||
*/
|
||||
|
||||
import { callerSignalOf, OAuthRequestTimeoutError } from "./requestTimeout.js";
|
||||
import {
|
||||
OAuthMetadataSchema,
|
||||
OpenIdProviderDiscoveryMetadataSchema,
|
||||
|
|
@ -169,6 +170,22 @@ async function releaseBody(response: Response): Promise<void> {
|
|||
await response.body?.cancel().catch(() => {});
|
||||
}
|
||||
|
||||
/**
|
||||
* The same release, started but not waited on.
|
||||
*
|
||||
* ⚠️ `ReadableStream.cancel()` adopts the underlying source's cancel promise,
|
||||
* which is permitted never to settle — so awaiting it on a path that is
|
||||
* *propagating a cancellation* can hang the very thing that was meant to end
|
||||
* (Copilot). Every exceptional exit below uses this: the release is a courtesy
|
||||
* to the connection pool, and the caller's abort or timeout must reach it
|
||||
* regardless. The `void` is the documented case where the callee owns its
|
||||
* failures — `releaseBody` swallows its own — and the caller genuinely cannot
|
||||
* await.
|
||||
*/
|
||||
function releaseBodyDetached(response: Response): void {
|
||||
void releaseBody(response);
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a parsed body is RFC 8414 authorization-server metadata that is *not*
|
||||
* a valid OpenID provider document — the exact shape the upstream schema
|
||||
|
|
@ -248,6 +265,13 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
|
|||
if (candidates.length === 0) return response;
|
||||
|
||||
const headers = discoveryHeaders(input, init);
|
||||
// The probe is a request the caller never made, issued on its behalf — so
|
||||
// it has to be cancellable by the caller too. Without the signal, aborting
|
||||
// after the initial 404 left the probe running to its own deadline and the
|
||||
// caller was handed a timeout in place of its own abort (Copilot). Same
|
||||
// precedence rule as everywhere else, hence the shared helper rather than a
|
||||
// second copy of it.
|
||||
const callerSignal = callerSignalOf(input, init);
|
||||
// The loop advances to the next candidate only where the SDK's own loop
|
||||
// would. Anywhere else it hands the original response back and lets
|
||||
// discovery run its normal course, because *promoting a later candidate
|
||||
|
|
@ -259,10 +283,50 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
|
|||
// those, returning `response` leaves the SDK to make the same request and
|
||||
// reach the same verdict it always would.
|
||||
for (const candidate of candidates) {
|
||||
// Don't issue a probe the caller has already given up on. A real `fetch`
|
||||
// rejects immediately on a pre-aborted signal, so this is mostly about
|
||||
// not sending the request at all — the same reasoning as the
|
||||
// short-circuit in `withOAuthRequestTimeout`.
|
||||
//
|
||||
// Every exceptional exit below releases `response` first. On a normal
|
||||
// return it is the value the caller gets and reads; on a throw nobody
|
||||
// will ever read it, and an unread body holds its connection open on
|
||||
// Node/undici — the same reason the successful-substitution path
|
||||
// releases it (Copilot). Matters most for a standalone user of this
|
||||
// exported wrapper, where no timeout wrapper is underneath to have
|
||||
// already drained it.
|
||||
if (callerSignal?.aborted) {
|
||||
releaseBodyDetached(response);
|
||||
throw callerSignal.reason;
|
||||
}
|
||||
|
||||
let probe: Response;
|
||||
try {
|
||||
probe = await fetchFn(candidate, { headers });
|
||||
} catch {
|
||||
probe = await fetchFn(candidate, { headers, signal: callerSignal });
|
||||
} catch (err) {
|
||||
// A deadline the Inspector imposed is *our* error, not the server's
|
||||
// answer, so it escapes rather than being folded back into the original
|
||||
// response (#2319, Copilot). Swallowing it would hand the caller the
|
||||
// preceding 404 and discard the one thing the deadline adds — the name
|
||||
// of the endpoint that stalled — leaving discovery to fail later under
|
||||
// a message that points at the wrong URL. There is no flow this can
|
||||
// break that was not already broken: every candidate this loop probes
|
||||
// is one the SDK's own `buildDiscoveryUrls` emits, so a stall here is a
|
||||
// stall the SDK would have hit itself, unbounded. Everything else — a
|
||||
// CORS rejection, DNS, a reset — still falls back as documented above.
|
||||
if (err instanceof OAuthRequestTimeoutError) {
|
||||
releaseBodyDetached(response);
|
||||
throw err;
|
||||
}
|
||||
// Likewise a caller that gave up: substituting the preceding 404 for
|
||||
// its own abort would report a discovery failure for a request it
|
||||
// deliberately cancelled. The caller's own `reason` is what surfaces,
|
||||
// here and at the two sites below, rather than whatever shape the
|
||||
// underlying layer happened to reject with.
|
||||
if (callerSignal?.aborted) {
|
||||
releaseBodyDetached(response);
|
||||
throw callerSignal.reason;
|
||||
}
|
||||
return response;
|
||||
}
|
||||
if (!probe.ok) {
|
||||
|
|
@ -272,6 +336,14 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
|
|||
// candidate exhaust the origin's pool (Copilot). Same discipline as
|
||||
// `core/mcp/node/authChallengeFetch.ts`.
|
||||
await releaseBody(probe);
|
||||
// Rechecked after the release, which awaits: an abort that lands while
|
||||
// the body is being discarded must not be answered with the preceding
|
||||
// response either, and `continue` would otherwise carry on probing for
|
||||
// a caller that has stopped waiting.
|
||||
if (callerSignal?.aborted) {
|
||||
releaseBodyDetached(response);
|
||||
throw callerSignal.reason;
|
||||
}
|
||||
if (continuesDiscovery(probe.status)) continue;
|
||||
return response;
|
||||
}
|
||||
|
|
@ -288,6 +360,17 @@ export function withRfc8414OidcCompat(fetchFn: typeof fetch): typeof fetch {
|
|||
body = await probe.text();
|
||||
parsed = JSON.parse(body);
|
||||
} catch {
|
||||
// `fetch` resolves on headers, so the abort can land here rather than
|
||||
// in the catch above: the probe's headers arrived, the caller gave up,
|
||||
// and `probe.text()` rejects on the partial body. Substituting the
|
||||
// preceding 404 would hand a standalone user of this wrapper a
|
||||
// discovery failure in place of its own abort reason (Copilot). A body
|
||||
// that merely will not parse still falls through, which is the case
|
||||
// this catch exists for.
|
||||
if (callerSignal?.aborted) {
|
||||
releaseBodyDetached(response);
|
||||
throw callerSignal.reason;
|
||||
}
|
||||
return response;
|
||||
}
|
||||
if (!isRfc8414OnlyMetadata(parsed)) {
|
||||
|
|
|
|||
577
core/auth/requestTimeout.ts
Normal file
577
core/auth/requestTimeout.ts
Normal file
|
|
@ -0,0 +1,577 @@
|
|||
/**
|
||||
* A bound on every network call the Inspector makes on the OAuth path (#2319).
|
||||
*
|
||||
* ## Why this exists
|
||||
*
|
||||
* Protected-resource-metadata discovery, authorization-server metadata
|
||||
* discovery, dynamic client registration, the token exchange and the refresh
|
||||
* all went out with no `AbortSignal` at all. Token revocation was the single
|
||||
* exception — `revocation.ts` has carried its own deadline since #2144, and it
|
||||
* is the pattern this module generalizes.
|
||||
*
|
||||
* An unbounded fetch matters more here than it usually would, because of where
|
||||
* this work runs and what the UI does while it runs:
|
||||
*
|
||||
* - It runs **server-side**, in the Inspector's own Node process, proxied
|
||||
* through the backend. A stall is therefore invisible in browser devtools —
|
||||
* #2188's reporter correctly observed that no request left the browser at
|
||||
* all while a Node-side socket sat established and idle.
|
||||
* - Connect-time auth errors deliberately hold the connection status at
|
||||
* `"connecting"` rather than moving it to `"error"`
|
||||
* (`isConnectAuthRecoveryError`, `challenge.ts`), on the theory that a
|
||||
* 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 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
|
||||
* timeout and surfaces after 60s as `Request timed out`. This module makes the
|
||||
* bound deliberate, applies it to the stalls that sit *outside* an SDK request
|
||||
* too, and — the point of naming the URL in the error — says *which* endpoint
|
||||
* stalled instead of blaming the handshake.
|
||||
*
|
||||
* ## The signal, the proxy hop, and why the race is still here
|
||||
*
|
||||
* 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`.
|
||||
*/
|
||||
|
||||
import { redactUrlQuery } from "../mcp/fetchTracking.js";
|
||||
|
||||
/**
|
||||
* 30 seconds. Deliberately generous: discovery against a slow or cold-starting
|
||||
* authorization server is legitimate, and a bound that fires on a server that
|
||||
* was going to answer is worse than the unbounded wait it replaced. It still
|
||||
* halves the 60s the SDK handshake incidentally provides, and it is the only
|
||||
* bound at all on the legs that sit outside an SDK request.
|
||||
*/
|
||||
export const DEFAULT_OAUTH_REQUEST_TIMEOUT_MS = 30_000;
|
||||
|
||||
/**
|
||||
* Raised when an OAuth-path request outlives its budget.
|
||||
*
|
||||
* Carries the URL because "discovery timed out" without one is only marginally
|
||||
* better than a spinner: the OAuth path makes several requests to several
|
||||
* hosts, and which of them stalled is the whole diagnostic.
|
||||
*/
|
||||
export class OAuthRequestTimeoutError extends Error {
|
||||
/**
|
||||
* The endpoint that stalled, with sensitive query values redacted.
|
||||
*
|
||||
* Redacted in the constructor rather than at each display site, so every
|
||||
* consumer is covered by construction: this error's message is recorded
|
||||
* verbatim by `createFetchTracker` into the Network log and the persisted
|
||||
* session, and an OAuth endpoint can carry a `code`, an `access_token` or a
|
||||
* `client_secret` in its query string — which `fetchTracking` already
|
||||
* deliberately redacts everywhere else it records a URL (Copilot). The path
|
||||
* and every non-sensitive parameter survive, so the endpoint stays
|
||||
* identifiable, which was the point of naming it.
|
||||
*/
|
||||
readonly url: string;
|
||||
readonly timeoutMs: number;
|
||||
|
||||
constructor(url: string, timeoutMs: number) {
|
||||
const safeUrl = redactUrlQuery(url);
|
||||
super(`OAuth request to ${safeUrl} timed out after ${timeoutMs}ms`);
|
||||
this.name = "OAuthRequestTimeoutError";
|
||||
this.url = safeUrl;
|
||||
this.timeoutMs = timeoutMs;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The largest delay `setTimeout` can actually schedule. Past `2 ** 31 - 1` the
|
||||
* delay overflows a 32-bit signed integer and Node falls back to **1ms**, so an
|
||||
* `Infinity` or an over-large budget produces an *immediate* timeout — the exact
|
||||
* opposite of what the caller asked for (Copilot).
|
||||
*/
|
||||
const MAX_TIMER_DELAY_MS = 2_147_483_647;
|
||||
|
||||
/**
|
||||
* Coerce a caller's budget into something `setTimeout` can honour and the error
|
||||
* can honestly report.
|
||||
*
|
||||
* - Non-finite (`NaN`, `±Infinity`) falls back to the default. `NaN` survives
|
||||
* `Math.max(0, Math.round(...))` and `setTimeout(fn, NaN)` fires immediately,
|
||||
* so a caller whose arithmetic produced a `NaN` would otherwise see every
|
||||
* OAuth request fail at once with "timed out after NaNms".
|
||||
* - Above `MAX_TIMER_DELAY_MS` it clamps rather than overflowing to 1ms.
|
||||
* - Negative clamps to 0, which is a real budget (fire on the next tick), not
|
||||
* an error.
|
||||
*
|
||||
* Rounded because the budget is *reported* — see the note at the call site.
|
||||
*/
|
||||
function normalizeBudget(timeoutMs: number): number {
|
||||
if (!Number.isFinite(timeoutMs)) return DEFAULT_OAUTH_REQUEST_TIMEOUT_MS;
|
||||
return Math.min(MAX_TIMER_DELAY_MS, Math.max(0, Math.round(timeoutMs)));
|
||||
}
|
||||
|
||||
/**
|
||||
* How a proxy-side deadline is carried back to the browser (#2319).
|
||||
*
|
||||
* `/api/fetch` serializes its failures as a JSON error response, and
|
||||
* `createRemoteFetch` turns any non-OK answer into a plain `Error` — so a
|
||||
* timeout enforced by the *backend* would arrive as an untyped error and the
|
||||
* `instanceof OAuthRequestTimeoutError` checks downstream would all be false
|
||||
* (Copilot). Both ends run the same budget, so the client's race usually
|
||||
* settles first; this matters when the backend's timer wins anyway — most
|
||||
* plausibly a backgrounded tab, where the browser throttles `setTimeout` while
|
||||
* the server's fires on schedule. Which of two equal deadlines happened to fire
|
||||
* must not decide whether the error names its endpoint. So the route stamps
|
||||
* this marker and `createRemoteFetch` reconstructs the typed error from it.
|
||||
*/
|
||||
export const OAUTH_TIMEOUT_WIRE_CODE = "oauth_request_timeout";
|
||||
|
||||
/** The body `/api/fetch` returns when its own deadline fired. */
|
||||
export interface OAuthRequestTimeoutWire {
|
||||
code: typeof OAUTH_TIMEOUT_WIRE_CODE;
|
||||
url: string;
|
||||
timeoutMs: number;
|
||||
}
|
||||
|
||||
/** Whether a parsed `/api/fetch` error body is a proxy-side deadline. */
|
||||
export function isOAuthRequestTimeoutWire(
|
||||
value: unknown,
|
||||
): value is OAuthRequestTimeoutWire {
|
||||
if (typeof value !== "object" || value === null) return false;
|
||||
const candidate = value as Record<string, unknown>;
|
||||
return (
|
||||
candidate.code === OAUTH_TIMEOUT_WIRE_CODE &&
|
||||
typeof candidate.url === "string" &&
|
||||
typeof candidate.timeoutMs === "number"
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Build the `isExempt` predicate for a chain that carries MCP traffic as well as
|
||||
* OAuth work: it returns true for the MCP endpoint itself.
|
||||
*
|
||||
* ⚠️ **Fails open, deliberately.** The two errors are not symmetric. Bounding a
|
||||
* request that should not be bounded severs a long-running tool call or an SSE
|
||||
* stream — a severe, user-visible regression. Failing to bound one leaves the
|
||||
* SDK's own per-request timeout as the backstop it has always been. So anything
|
||||
* uncertain — no server URL yet, a URL that will not parse, either side — is
|
||||
* treated as the MCP endpoint and exempted.
|
||||
*
|
||||
* The comparison is **as narrow as the transport allows**, which is not the same
|
||||
* rule for both of them:
|
||||
*
|
||||
* - **Streamable HTTP** sends every MCP request to the configured URL, so
|
||||
* `origin + pathname` is exact. Same-origin OAuth work — a protected-resource
|
||||
* document, or an authorization server deployed beside the resource — is a
|
||||
* different path and stays bounded, which is what #2319 asks for (Copilot).
|
||||
* - **Legacy SSE** opens the configured URL (`/sse`), is handed a *separate*
|
||||
* message endpoint by the server, and POSTs every JSON-RPC message to that
|
||||
* second pathname. We never see that URL — it arrives inside the stream, in
|
||||
* the SDK's own parser — so a path rule there bounds and body-buffers real
|
||||
* MCP traffic, severing a slow tool call at 30s and reporting it as an
|
||||
* `OAuthRequestTimeoutError`. Origin is the narrowest rule available, and it
|
||||
* is exact rather than approximate: the SDK *enforces* that the message
|
||||
* endpoint shares the stream URL's origin and rejects one that does not.
|
||||
*
|
||||
* So the residual gap is confined to legacy SSE against a server that also
|
||||
* hosts its own OAuth endpoints, where transport-internal OAuth falls back to
|
||||
* the SDK's per-request timeout — where it was before #2319. The Inspector's
|
||||
* own discovery and token requests to those URLs are bounded regardless, on the
|
||||
* OAuth chain.
|
||||
*
|
||||
* ⚠️ **Fails open** on every uncertainty, and the uncertainties differ by rule:
|
||||
* no server URL, an unparseable URL on either side, or *not knowing which
|
||||
* transport this is* all resolve to exempt — the last one by falling back to
|
||||
* the wider origin rule.
|
||||
*/
|
||||
/**
|
||||
* Read the transport hint, treating "absent" and "threw" alike as the wide
|
||||
* case. A path rule applied to a connection that turns out to be legacy SSE
|
||||
* severs real MCP traffic, so every uncertainty resolves toward the origin rule.
|
||||
*/
|
||||
function readNegotiatesEndpoint(hint: (() => boolean) | undefined): boolean {
|
||||
if (!hint) return true;
|
||||
try {
|
||||
return hint();
|
||||
} catch {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The media type every OAuth token-family request uses — the token exchange,
|
||||
* the refresh, and revocation are all `application/x-www-form-urlencoded` per
|
||||
* RFC 6749 §4.1.3 and RFC 7009 §2.1. MCP is JSON-RPC over `application/json`
|
||||
* and never uses it, which is what makes this a safe discriminator rather than
|
||||
* a heuristic.
|
||||
*/
|
||||
const OAUTH_FORM_MEDIA_TYPE = "application/x-www-form-urlencoded";
|
||||
|
||||
export function exemptMcpEndpoint(
|
||||
getServerUrl: () => string | undefined,
|
||||
/**
|
||||
* Whether the message endpoint is negotiated rather than configured — true
|
||||
* for legacy SSE. Read per call, since the transport can change between
|
||||
* connects. Absent, or throwing, is treated as `true`.
|
||||
*/
|
||||
negotiatesEndpoint?: () => boolean,
|
||||
): (url: string, headers: Headers) => boolean {
|
||||
return (url, headers) => {
|
||||
// A URL match is not by itself proof of MCP traffic. `oauthTokenUrl` takes
|
||||
// any absolute URL, so a user may point it at the MCP endpoint's own URL,
|
||||
// and the transport-internal refresh then travels this chain to a path the
|
||||
// rules below would exempt — leaving the one request this whole change
|
||||
// exists to bound running unbounded (Copilot). The token family is
|
||||
// form-encoded and MCP never is, so the media type settles it before any
|
||||
// URL comparison happens.
|
||||
if (
|
||||
(headers.get("content-type") ?? "").split(";")[0].trim().toLowerCase() ===
|
||||
OAUTH_FORM_MEDIA_TYPE
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// `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 {
|
||||
const a = new URL(url);
|
||||
const b = new URL(serverUrl);
|
||||
if (a.origin !== b.origin) return false;
|
||||
return negotiated || a.pathname === b.pathname;
|
||||
} catch {
|
||||
return true;
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* The deadline a wrapped call is running under, keyed by the exact `RequestInit`
|
||||
* the wrapper handed down (#2319).
|
||||
*
|
||||
* This is how the budget crosses the proxy hop without travelling *upstream*.
|
||||
* `/api/fetch` must not time every request it serves — `createRemoteFetch` also
|
||||
* carries MCP traffic, and a Streamable HTTP tool call can legitimately withhold
|
||||
* its response headers for minutes — so the route needs to know which requests
|
||||
* are bounded and which are not (Copilot). A header would be the obvious
|
||||
* carrier and is the wrong one: `serializeRequest` copies request headers
|
||||
* verbatim into the payload and the route re-sends them to the authorization
|
||||
* server, so a marker header would leak to a third party.
|
||||
*
|
||||
* A `WeakMap` keyed on the init object has neither problem. It is invisible to
|
||||
* serialization, it needs no cast onto `RequestInit`, and it is collected with
|
||||
* the object — the wrapper builds a fresh init per call, so there is one entry
|
||||
* per in-flight request and nothing accumulates.
|
||||
*/
|
||||
const REQUEST_DEADLINES = new WeakMap<object, number>();
|
||||
|
||||
/**
|
||||
* The deadline `withOAuthRequestTimeout` stamped on this init, if any.
|
||||
*
|
||||
* `undefined` means the request is not bounded by this wrapper and must not be
|
||||
* bounded by anything downstream either — an exempt MCP request reads as
|
||||
* `undefined` because the exempt path never builds an init of its own.
|
||||
*/
|
||||
export function deadlineForRequestInit(
|
||||
init: RequestInit | undefined,
|
||||
): number | undefined {
|
||||
return init ? REQUEST_DEADLINES.get(init) : undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* The request's headers, whatever form `fetch`'s arguments took. `init` wins
|
||||
* over a `Request`'s own, matching `fetch`.
|
||||
*/
|
||||
function requestHeadersOf(
|
||||
input: RequestInfo | URL,
|
||||
init: RequestInit | undefined,
|
||||
): Headers {
|
||||
if (init?.headers) return new Headers(init.headers);
|
||||
if (typeof input !== "string" && !(input instanceof URL)) {
|
||||
return new Headers(input.headers);
|
||||
}
|
||||
return new Headers();
|
||||
}
|
||||
|
||||
/**
|
||||
* Pull a reader to completion, discarding what it yields. An absent reader — a
|
||||
* null-body response — is already complete.
|
||||
*
|
||||
* Read through an explicit reader rather than `clone().arrayBuffer()` so the
|
||||
* losing side of the race has something it can cancel: `arrayBuffer()` locks
|
||||
* the stream to an internal reader nothing else can reach.
|
||||
*/
|
||||
async function drainToEnd(
|
||||
reader: ReadableStreamDefaultReader<Uint8Array> | undefined,
|
||||
): Promise<void> {
|
||||
if (!reader) return;
|
||||
for (;;) {
|
||||
const { done } = await reader.read();
|
||||
if (done) return;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Tear down both branches of the tee.
|
||||
*
|
||||
* Deliberately **not awaited**, and the `void`s say which of the documented
|
||||
* cases this is: the callee owns its failures (each carries its own `catch`),
|
||||
* and the caller genuinely cannot await — cancelling one branch of a tee can
|
||||
* itself wait on the other, which would hold the timeout open for exactly as
|
||||
* long as the stall it is meant to end. Best-effort on each besides: a stream
|
||||
* already cancelled, errored or locked elsewhere is not a failure here, and
|
||||
* this runs on a path that is already rejecting with something the caller cares
|
||||
* about more.
|
||||
*/
|
||||
function cancelBoth(
|
||||
reader: ReadableStreamDefaultReader<Uint8Array> | undefined,
|
||||
response: Response | undefined,
|
||||
): void {
|
||||
void reader?.cancel().catch(() => {});
|
||||
void response?.body?.cancel().catch(() => {});
|
||||
}
|
||||
|
||||
/** The request URL, whatever form `fetch`'s first argument took. */
|
||||
function requestUrlOf(input: RequestInfo | URL): string {
|
||||
if (typeof input === "string") return input;
|
||||
if (input instanceof URL) return input.href;
|
||||
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).
|
||||
*
|
||||
* An `init.signal` still wins, but only when it is not `undefined`. `RequestInit`
|
||||
* is a WebIDL dictionary, where a member present with the value `undefined` is
|
||||
* converted as *absent* — so `{ ...base, signal: undefined }`, which is what a
|
||||
* spread over an options object that had no signal produces, must inherit the
|
||||
* `Request`'s signal rather than clear it (Copilot). An explicit `null` is the
|
||||
* genuine "no signal" override and is honoured as one.
|
||||
*/
|
||||
export function callerSignalOf(
|
||||
input: RequestInfo | URL,
|
||||
init: RequestInit | undefined,
|
||||
): AbortSignal | undefined {
|
||||
const explicit = init?.signal;
|
||||
if (explicit !== undefined) return explicit ?? undefined;
|
||||
if (typeof input !== "string" && !(input instanceof URL)) return input.signal;
|
||||
return undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* Wrap a `fetch` so every call through it is bounded by `timeoutMs`.
|
||||
*
|
||||
* 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` receives the request's URL **and headers**, because a URL alone
|
||||
* cannot always tell the two kinds of traffic apart — see `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
|
||||
* OAuth work and the MCP traffic itself, and the MCP traffic must not be
|
||||
* timed — a Streamable HTTP POST for a long-running tool call legitimately
|
||||
* withholds its response headers for minutes, and an SSE body stays open
|
||||
* indefinitely. So `InspectorClient` exempts the MCP endpoint by URL and bounds
|
||||
* everything else on that chain, and **fails open**: if the server URL cannot
|
||||
* be determined or parsed, everything is exempt. Bounding a request that should
|
||||
* not be bounded breaks a working tool call; failing to bound one leaves the
|
||||
* SDK's own per-request timeout as the backstop it already was.
|
||||
*
|
||||
* 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 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,
|
||||
timeoutMs: number = DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
|
||||
isExempt?: (url: string, headers: Headers) => boolean,
|
||||
): typeof fetch {
|
||||
// Whole milliseconds. Not for the reason `revocation.ts:253` gives — that one
|
||||
// calls `AbortSignal.timeout`, which really does throw `ERR_OUT_OF_RANGE` on
|
||||
// a fractional delay (verified on Node 26) and so would fail before the
|
||||
// request was sent. This wrapper drives its deadline with `setTimeout` and an
|
||||
// `AbortController`, and `setTimeout` accepts a fractional delay and
|
||||
// truncates it, so nothing here would throw (Copilot).
|
||||
//
|
||||
// The real reason is that the budget is *reported*: it is interpolated into
|
||||
// the timeout message and exposed as `OAuthRequestTimeoutError.timeoutMs`. A
|
||||
// caller whose budget came from a `performance.now()` subtraction would
|
||||
// otherwise produce "timed out after 1000.4000000953674ms" and a non-integer
|
||||
// public field. Rounding rather than flooring so a caller's own
|
||||
// whole-millisecond timeout survives the trip through that clock and is still
|
||||
// the number the message names.
|
||||
const budget = normalizeBudget(timeoutMs);
|
||||
|
||||
return async (input, init) => {
|
||||
const url = requestUrlOf(input);
|
||||
// An exempt request is passed through completely untouched — no deadline,
|
||||
// no signal of ours, and crucially no body buffering, since the responses
|
||||
// this exists for are streams that must stay open (see `isExempt` above).
|
||||
if (isExempt?.(url, requestHeadersOf(input, init))) {
|
||||
return fetchFn(input, init);
|
||||
}
|
||||
|
||||
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 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
|
||||
? AbortSignal.any([controller.signal, callerSignal])
|
||||
: controller.signal;
|
||||
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
let timedOut = false;
|
||||
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, 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 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.
|
||||
if (callerSignal) {
|
||||
onCallerAbort = () => reject(callerSignal.reason);
|
||||
callerSignal.addEventListener("abort", onCallerAbort, { once: true });
|
||||
}
|
||||
});
|
||||
|
||||
// Built once and stamped, so `createRemoteFetch` can read the budget off it
|
||||
// and tell `/api/fetch` this particular request is bounded.
|
||||
const nextInit: RequestInit = { ...init, signal };
|
||||
REQUEST_DEADLINES.set(nextInit, budget);
|
||||
|
||||
// Held so the losing path can tear both tee branches down — see below.
|
||||
let settled: Response | undefined;
|
||||
let drainReader: ReadableStreamDefaultReader<Uint8Array> | undefined;
|
||||
|
||||
try {
|
||||
const response = await Promise.race([
|
||||
fetchFn(input, nextInit),
|
||||
abandoned,
|
||||
]);
|
||||
settled = response;
|
||||
// `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. It is also the second reason `isExempt` is
|
||||
// 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.
|
||||
// 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.
|
||||
const copy = response.clone();
|
||||
drainReader = copy.body?.getReader();
|
||||
await Promise.race([drainToEnd(drainReader), abandoned]);
|
||||
return response;
|
||||
} catch (err) {
|
||||
// ⚠️ Losing the race rejects the caller; on its own it does not stop the
|
||||
// read. `clone()` tees the body, so a drain still running would keep
|
||||
// pulling bytes *and* keep buffering them for the unread original branch
|
||||
// — unbounded memory on exactly the signal-ignoring body the race exists
|
||||
// to contain (Copilot). Both branches are cancelled here, which is safe
|
||||
// precisely because this path throws: the caller never receives the
|
||||
// response, so nothing is left to read it.
|
||||
cancelBoth(drainReader, settled);
|
||||
// `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. 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 {
|
||||
/* v8 ignore next -- the Promise executor runs synchronously, so `timer`
|
||||
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);
|
||||
}
|
||||
}
|
||||
};
|
||||
}
|
||||
|
|
@ -268,14 +268,15 @@ export async function revokeToken(
|
|||
// outcome — and every caller has already cleared its local state by now, so
|
||||
// a rejection would break the documented best-effort guarantee.
|
||||
const { url, init } = buildRevocationRequest(params);
|
||||
// The signal alone is not enough to bound this. In the browser the 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 a wedged authorization server would hold the teardown
|
||||
// open indefinitely on exactly the path the timeout exists for. 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.
|
||||
// The signal cancels; the race is what guarantees the deadline. Since #2319
|
||||
// the signal reaches every path — `createRemoteFetch` forwards it onto the
|
||||
// `/api/fetch` hop and that route composes it into its outbound fetch, so a
|
||||
// browser-side abort now tears down the backend's request to the
|
||||
// authorization server as well as the direct-fetch paths (CLI, TUI,
|
||||
// backend) it always did. The race is still not redundant: it bounds a
|
||||
// `fetchFn` that ignores `AbortSignal` altogether, and a backend that is
|
||||
// itself wedged. Same reasoning, and the same shape, as
|
||||
// `withOAuthRequestTimeout`.
|
||||
const response = await withDeadline(
|
||||
params.fetchFn(url, { ...init, signal: AbortSignal.timeout(timeoutMs) }),
|
||||
timeoutMs,
|
||||
|
|
@ -307,11 +308,12 @@ export async function revokeToken(
|
|||
/**
|
||||
* Reject with a timeout error if `promise` has not settled within `timeoutMs`.
|
||||
*
|
||||
* The underlying request is abandoned rather than cancelled — nothing here can
|
||||
* cancel a fetch the proxy already stripped the signal from. That is the right
|
||||
* trade for this caller: the point is that the *teardown* proceeds, and the
|
||||
* response, if it ever arrives, is a revocation we no longer need to wait for.
|
||||
* The timer is cleared on the settled path so a caller is never held awake by it.
|
||||
* This abandons the promise; cancelling the request is the caller's signal's job
|
||||
* (see `revokeToken`, where the two are used together). Abandonment alone is an
|
||||
* acceptable floor for this caller even where the signal is ignored: the point
|
||||
* is that the *teardown* proceeds, and the response, if it ever arrives, is a
|
||||
* revocation we no longer need to wait for. The timer is cleared on the settled
|
||||
* path so a caller is never held awake by it.
|
||||
*/
|
||||
async function withDeadline<T>(
|
||||
promise: Promise<T>,
|
||||
|
|
|
|||
|
|
@ -256,6 +256,11 @@ import {
|
|||
} from "../auth/challenge.js";
|
||||
import { withOAuthEndpointOverrides } from "../auth/endpointOverrides.js";
|
||||
import { withRfc8414OidcCompat } from "../auth/oidcDiscoveryCompat.js";
|
||||
import {
|
||||
DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
|
||||
exemptMcpEndpoint,
|
||||
withOAuthRequestTimeout,
|
||||
} from "../auth/requestTimeout.js";
|
||||
import type { TokenRevocationOutcome } from "../auth/revocation.js";
|
||||
import type { OAuthTokens } from "@modelcontextprotocol/client";
|
||||
import { silentLogger, type InspectorLogger } from "../logging/logger.js";
|
||||
|
|
@ -810,8 +815,9 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
// `this.fetchFn` directly. The overrides are read lazily — `oauthManager` is
|
||||
// created a few lines below, and `setOAuthConfig` can change them later — so
|
||||
// the wrapper is inert until a server actually configures one.
|
||||
this.fetchFn = withOAuthEndpointOverrides(this.fetchFn ?? fetch, () =>
|
||||
this.oauthManager?.getEndpointOverrides(),
|
||||
const withOverrides = withOAuthEndpointOverrides(
|
||||
this.fetchFn ?? fetch,
|
||||
() => this.oauthManager?.getEndpointOverrides(),
|
||||
);
|
||||
// #2172: recover discovery when a plain OAuth 2.0 authorization server
|
||||
// publishes RFC 8414 metadata at `/.well-known/openid-configuration`, which
|
||||
|
|
@ -822,8 +828,40 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
// still hit the upstream failure. The substituted response is stamped with
|
||||
// `COMPAT_SOURCE_HEADER` so a captured entry names the URL its body came
|
||||
// from rather than appearing to be a 200 from the RFC 8414 path.
|
||||
this.fetchFn = withRfc8414OidcCompat(this.fetchFn);
|
||||
this.effectiveAuthFetch = this.buildEffectiveAuthFetch();
|
||||
// The transport chain is bounded too, but only for the requests on it that
|
||||
// are *not* MCP traffic — the OAuth work the SDK runs from inside the
|
||||
// transport on the 401/refresh path, which otherwise waits out the SDK's
|
||||
// incidental per-request timeout and reports a bare `Request timed out`
|
||||
// naming no endpoint (Copilot). The MCP endpoint itself is exempt, and the
|
||||
// predicate fails open: a long-running tool call withholding its response
|
||||
// headers for minutes, or an SSE body that stays open, must never be timed
|
||||
// here. Deadline innermost, for the same reason as the auth chain below.
|
||||
this.fetchFn = withRfc8414OidcCompat(
|
||||
withOAuthRequestTimeout(
|
||||
withOverrides,
|
||||
DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
|
||||
exemptMcpEndpoint(
|
||||
() => this.getServerUrl(),
|
||||
// Legacy SSE negotiates its message endpoint inside the stream, so
|
||||
// its path is unknowable here and the exemption has to widen to the
|
||||
// origin. Streamable HTTP sends everything to the configured URL, so
|
||||
// the path rule is exact there and same-origin OAuth stays bounded.
|
||||
() => this.transportConfig?.type === "sse",
|
||||
),
|
||||
),
|
||||
);
|
||||
// #2319: the auth chain is composed separately so the deadline sits
|
||||
// *inside* the compat wrapper. Reusing `this.fetchFn` as the base would put
|
||||
// it outside, and then a stalled OIDC probe would be reported under the
|
||||
// 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`. 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)),
|
||||
);
|
||||
|
||||
this.sessionId = options.sessionId;
|
||||
|
||||
|
|
@ -990,8 +1028,22 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
);
|
||||
}
|
||||
|
||||
private buildEffectiveAuthFetch(): typeof fetch {
|
||||
const base = this.fetchFn ?? fetch;
|
||||
/**
|
||||
* @param base - the composed OAuth-path fetch, deadline innermost (#2319).
|
||||
*
|
||||
* 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
|
||||
// they're inspectable in the Network tab. Token-exchange responses carry
|
||||
// `access_token` / `refresh_token`; the Network UI masks those (and other
|
||||
|
|
|
|||
|
|
@ -21,6 +21,12 @@
|
|||
* the web client against a strict modern server, same as from the CLI/TUI.
|
||||
*/
|
||||
|
||||
import {
|
||||
deadlineForRequestInit,
|
||||
isOAuthRequestTimeoutWire,
|
||||
OAuthRequestTimeoutError,
|
||||
} from "../../auth/requestTimeout.js";
|
||||
|
||||
export interface RemoteFetchOptions {
|
||||
/** Base URL of the remote server (e.g. http://localhost:3000) */
|
||||
baseUrl: string;
|
||||
|
|
@ -133,14 +139,68 @@ export function createRemoteFetch(options: RemoteFetchOptions): typeof fetch {
|
|||
reqHeaders["x-mcp-remote-auth"] = `Bearer ${options.authToken}`;
|
||||
}
|
||||
|
||||
// #2319: forward the caller's cancellation onto the proxy hop. Without it
|
||||
// an abort — including the OAuth deadline's own — settled only the caller's
|
||||
// promise: the POST stayed in flight, so the backend never saw its request
|
||||
// cancelled and its outbound fetch to the authorization server was left
|
||||
// running detached, holding a handler and a socket for as long as the
|
||||
// server cared to stall (Copilot). `init.signal` wins when present and not
|
||||
// `undefined` (a WebIDL dictionary member present as `undefined` converts
|
||||
// as absent), otherwise a `Request` carries its own.
|
||||
const explicitSignal = init?.signal;
|
||||
const signal =
|
||||
explicitSignal !== undefined
|
||||
? (explicitSignal ?? undefined)
|
||||
: input instanceof Request
|
||||
? input.signal
|
||||
: undefined;
|
||||
|
||||
// #2319: tell the route whether *this* request is bounded, and by how much.
|
||||
// The route serves MCP traffic as well as OAuth work, and a Streamable HTTP
|
||||
// tool call can legitimately withhold its response headers for minutes — so
|
||||
// a timer there must be per-request rather than unconditional (Copilot). An
|
||||
// exempt request carries no deadline and the route applies none.
|
||||
//
|
||||
// In the JSON envelope rather than a header: `headers` is re-sent verbatim
|
||||
// to the upstream server, so a marker header would leak to a third party.
|
||||
const timeoutMs = deadlineForRequestInit(init);
|
||||
|
||||
const res = await fetchFn(`${baseUrl}/api/fetch`, {
|
||||
method: "POST",
|
||||
headers: reqHeaders,
|
||||
body: JSON.stringify({ url, method, headers, body }),
|
||||
body: JSON.stringify({
|
||||
url,
|
||||
method,
|
||||
headers,
|
||||
body,
|
||||
...(timeoutMs !== undefined && { timeoutMs }),
|
||||
}),
|
||||
signal,
|
||||
});
|
||||
|
||||
if (!res.ok) {
|
||||
const text = await res.text();
|
||||
// A deadline the backend enforced arrives as an ordinary error response;
|
||||
// rebuild the typed error from its marker so the `instanceof` checks
|
||||
// downstream — most importantly the one that lets a stalled probe escape
|
||||
// `withRfc8414OidcCompat` — still see a timeout for what it is (#2319,
|
||||
// Copilot).
|
||||
//
|
||||
// Both ends run the same budget, so the client's race usually settles
|
||||
// first and this path is not taken. It matters when the backend's timer
|
||||
// wins anyway — most plausibly a backgrounded tab, where the browser
|
||||
// throttles `setTimeout` while the server's fires on schedule. Without
|
||||
// this, which of two equal deadlines happened to fire would decide
|
||||
// whether the error carried its endpoint.
|
||||
let parsed: unknown;
|
||||
try {
|
||||
parsed = JSON.parse(text);
|
||||
} catch {
|
||||
parsed = undefined;
|
||||
}
|
||||
if (isOAuthRequestTimeoutWire(parsed)) {
|
||||
throw new OAuthRequestTimeoutError(parsed.url, parsed.timeoutMs);
|
||||
}
|
||||
throw new Error(`Remote fetch failed (${res.status}): ${text}`);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -28,6 +28,8 @@ import { bodyLimit } from "hono/body-limit";
|
|||
import { watch as chokidarWatch, type FSWatcher } from "chokidar";
|
||||
import { createTransportNode } from "../../node/transport.js";
|
||||
import { createProxyFetch } from "../../node/proxyFetch.js";
|
||||
import { OAUTH_TIMEOUT_WIRE_CODE } from "../../../auth/requestTimeout.js";
|
||||
import { redactUrlQuery } from "../../fetchTracking.js";
|
||||
import type {
|
||||
RemoteConnectRequest,
|
||||
RemoteSendRequest,
|
||||
|
|
@ -1204,6 +1206,8 @@ export function createRemoteApp(
|
|||
method?: string;
|
||||
headers?: Record<string, string>;
|
||||
body?: string;
|
||||
/** Per-request deadline, set only by a bounded OAuth call (#2319). */
|
||||
timeoutMs?: number;
|
||||
};
|
||||
try {
|
||||
body = (await c.req.json()) as typeof body;
|
||||
|
|
@ -1211,11 +1215,75 @@ export function createRemoteApp(
|
|||
return c.json({ error: "Invalid JSON body" }, 400);
|
||||
}
|
||||
|
||||
const { url, method = "GET", headers = {}, body: reqBody } = body;
|
||||
const {
|
||||
url,
|
||||
method = "GET",
|
||||
headers = {},
|
||||
body: reqBody,
|
||||
timeoutMs: requestedTimeoutMs,
|
||||
} = body;
|
||||
if (!url) {
|
||||
return c.json({ error: "Missing url" }, 400);
|
||||
}
|
||||
|
||||
// #2319: bound the outbound call, and release it when the browser gives up.
|
||||
// This hop is where a client-side deadline used to stop being enforceable:
|
||||
// the browser abandons its promise, but the request it made is served here,
|
||||
// and an outbound fetch with neither a signal nor a timeout holds a handler
|
||||
// and an authorization-server socket open indefinitely — once per timed-out
|
||||
// retry (Copilot). `c.req.raw.signal` is the propagated cancellation, now
|
||||
// that `createRemoteFetch` forwards it; the timer is the backstop for a
|
||||
// client that vanishes without aborting, and it matches the client-side
|
||||
// budget so the two agree on when this is hopeless.
|
||||
const controller = new AbortController();
|
||||
// Set by the timer below and read in the `catch`. The fetch rejects with
|
||||
// the signal's abort reason, but which of several aborts won is not worth
|
||||
// inferring from the error — the flag says it directly, the same
|
||||
// normalization `withOAuthRequestTimeout` uses.
|
||||
let deadlineFired = false;
|
||||
const clientSignal: AbortSignal | undefined = c.req.raw.signal;
|
||||
const signal = clientSignal
|
||||
? AbortSignal.any([controller.signal, clientSignal])
|
||||
: controller.signal;
|
||||
// Cleared in `finally`, before the handler returns. Safe because by then
|
||||
// the body has been either read or explicitly cancelled — this route never
|
||||
// hands a live stream to its caller, so there is nothing left for the timer
|
||||
// to protect and nothing it could sever.
|
||||
// ⚠️ Only the caller decides whether this request is bounded, and by how
|
||||
// much. This route is the browser's way out to the network for MCP traffic
|
||||
// as well as OAuth work, so an unconditional timer here would abort a
|
||||
// Streamable HTTP tool call that legitimately withholds its response
|
||||
// headers for longer than the budget — reintroducing on the backend exactly
|
||||
// the regression `exemptMcpEndpoint` exists to prevent on the client, and
|
||||
// reporting it as an OAuth timeout besides (Copilot). No deadline in the
|
||||
// envelope means no timer at all.
|
||||
// Clamped to what `setTimeout` can schedule: past 2**31-1 the delay
|
||||
// overflows and Node falls back to 1ms, turning an over-large budget into
|
||||
// an immediate timeout. Non-finite is ignored outright rather than
|
||||
// defaulted — an envelope carrying `NaN` is a malformed request, and the
|
||||
// safe reading of a malformed deadline is "no deadline".
|
||||
const deadlineMs =
|
||||
typeof requestedTimeoutMs === "number" &&
|
||||
Number.isFinite(requestedTimeoutMs)
|
||||
? Math.min(2_147_483_647, Math.max(0, Math.round(requestedTimeoutMs)))
|
||||
: undefined;
|
||||
// Redacted for the same reason `OAuthRequestTimeoutError` redacts: this
|
||||
// message and this URL are handed back to the browser, recorded in the
|
||||
// Network log and persisted, and an OAuth endpoint's query string can carry
|
||||
// a `code`, an `access_token` or a `client_secret` (Copilot).
|
||||
const safeUrl = redactUrlQuery(url);
|
||||
const timer =
|
||||
deadlineMs === undefined
|
||||
? undefined
|
||||
: setTimeout(() => {
|
||||
deadlineFired = true;
|
||||
controller.abort(
|
||||
new Error(
|
||||
`proxied request to ${safeUrl} timed out after ${deadlineMs}ms`,
|
||||
),
|
||||
);
|
||||
}, deadlineMs);
|
||||
|
||||
try {
|
||||
// Proxy-aware, not the bare global. This route is the browser's ONLY way
|
||||
// out to the network: the web client's `environment.fetch` is
|
||||
|
|
@ -1228,6 +1296,7 @@ export function createRemoteApp(
|
|||
method,
|
||||
headers: new Headers(headers),
|
||||
body: reqBody,
|
||||
signal,
|
||||
});
|
||||
|
||||
const resHeaders: Record<string, string> = {};
|
||||
|
|
@ -1242,6 +1311,26 @@ export function createRemoteApp(
|
|||
let resBody: string | undefined;
|
||||
if (!isStream && res.body) {
|
||||
resBody = await res.text();
|
||||
} else if (isStream) {
|
||||
// This route does not return the stream — it answers with JSON and no
|
||||
// body — so nobody downstream owns it and nothing will ever read it.
|
||||
// Left un-cancelled, an OAuth endpoint that sends event-stream or
|
||||
// NDJSON headers and never closes would hold the upstream socket open
|
||||
// for good, since the `finally` below clears the only deadline once
|
||||
// this handler returns (Copilot). Best-effort, like `releaseBody` in
|
||||
// `oidcDiscoveryCompat`: a body already consumed, locked or absent is
|
||||
// not an error here. Measured, undici does reclaim an unread body on
|
||||
// its own in this configuration — so this is belt and braces rather
|
||||
// than a demonstrated leak, and it is kept because relying on that is
|
||||
// an implementation detail of the fetch beneath us, not a contract.
|
||||
//
|
||||
// ⚠️ Started, not awaited. `ReadableStream.cancel()` adopts the
|
||||
// underlying source's cancel promise, which is permitted never to
|
||||
// settle — awaiting it would keep this handler pending forever, and on
|
||||
// an unbounded request there is no timer to release it either
|
||||
// (Copilot). The `void` is the documented case where the callee owns
|
||||
// its failures, via the `catch` below, and the caller cannot await.
|
||||
void res.body?.cancel().catch(() => {});
|
||||
}
|
||||
|
||||
return c.json({
|
||||
|
|
@ -1253,7 +1342,24 @@ export function createRemoteApp(
|
|||
});
|
||||
} catch (err) {
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
if (deadlineFired) {
|
||||
// Stamped so the browser can rebuild an `OAuthRequestTimeoutError`
|
||||
// rather than receiving a plain `Error` that no `instanceof` check
|
||||
// downstream can recognize (Copilot). 504 because that is what this
|
||||
// is — the gateway gave up on the upstream, not a fault in the route.
|
||||
return c.json(
|
||||
{
|
||||
error: msg,
|
||||
code: OAUTH_TIMEOUT_WIRE_CODE,
|
||||
url: safeUrl,
|
||||
timeoutMs: deadlineMs,
|
||||
},
|
||||
504,
|
||||
);
|
||||
}
|
||||
return c.json({ error: msg }, 500);
|
||||
} finally {
|
||||
if (timer !== undefined) clearTimeout(timer);
|
||||
}
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue