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

fix: never exempt a form-encoded request (#2319 round 20)

Browse source 
The exemption was URL-only, so an authorization server configured at
the MCP endpoint's own URL — `oauthTokenUrl` takes any absolute URL —
made the transport-internal refresh take the exempt branch and run with
no signal, no body buffering and no deadline. That is the one request
this change exists to bound.

`isExempt` now receives the request's headers as well as its URL, and
`exemptMcpEndpoint` refuses to exempt anything sent as
`application/x-www-form-urlencoded`, before any URL comparison. That is
a discriminator rather than a heuristic: the token exchange, the
refresh and revocation are all form-encoded per RFC 6749 §4.1.3 and RFC
7009 §2.1, and MCP is JSON-RPC over application/json and never is.

Also records, on the no-deadline route test, what it does not catch: at
a 400ms wait an unconditional timer restored at the production 30s
budget would still leave the request pending and the test would pass.
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.

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

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

@ -498,12 +498,11 @@ describe("withOAuthRequestTimeout", () => {
describe("exemptMcpEndpoint (the transport chain's mixed traffic)", () => {
const SERVER = "https://srv.example.com/mcp";
it("exempts the MCP endpoint itself, whatever method or query", () => {
const isExempt = exemptMcpEndpoint(() => SERVER);
expect(isExempt(SERVER)).toBe(true);
expect(isExempt(`${SERVER}?sessionId=abc`)).toBe(true);
expect(isExempt(`${SERVER}#frag`)).toBe(true);
/** 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. */
@ -519,17 +518,46 @@ describe("withOAuthRequestTimeout", () => {
() => 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)).toBe(true);
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")).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", () => {
@ -538,9 +566,14 @@ describe("withOAuthRequestTimeout", () => {
const isExempt = sse("https://srv.example.com/sse");
expect(
isExempt("https://srv.example.com/.well-known/openid-configuration"),
isExempt(
"https://srv.example.com/.well-known/openid-configuration",
JSON_HEADERS,
),
).toBe(true);
expect(isExempt("https://as.example.com/token")).toBe(false);
expect(isExempt("https://as.example.com/token", JSON_HEADERS)).toBe(
false,
);
});
it("falls back to the origin rule when the transport is unknown", () => {
@ -554,8 +587,8 @@ describe("withOAuthRequestTimeout", () => {
},
);
expect(noHint("https://srv.example.com/token")).toBe(true);
expect(throws("https://srv.example.com/token")).toBe(true);
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", () => {
@ -567,22 +600,34 @@ describe("withOAuthRequestTimeout", () => {
// an origin rule exact rather than approximate.
const isExempt = exemptMcpEndpoint(() => "https://srv.example.com/sse");
expect(isExempt("https://srv.example.com/messages?sessionId=abc")).toBe(
true,
);
expect(isExempt("https://srv.example.com/")).toBe(true);
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")).toBe(false);
expect(isExempt("https://as.example.com/token", JSON_HEADERS)).toBe(
false,
);
expect(
isExempt("https://as.example.com/.well-known/openid-configuration"),
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")).toBe(false);
expect(isExempt("https://srv.example.com:8443/token")).toBe(false);
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", () => {
@ -590,23 +635,37 @@ describe("withOAuthRequestTimeout", () => {
// 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"),
exemptMcpEndpoint(() => undefined)(
"https://as.example.com/token",
JSON_HEADERS,
),
).toBe(true);
expect(exemptMcpEndpoint(() => "")("https://as.example.com/token")).toBe(
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,
);
expect(exemptMcpEndpoint(() => "not a url")(SERVER)).toBe(true);
expect(exemptMcpEndpoint(() => SERVER)("not a url")).toBe(true);
});
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")).toBe(false);
expect(isExempt("https://other.example.com/mcp", JSON_HEADERS)).toBe(
false,
);
current = "https://other.example.com/mcp";
expect(isExempt("https://other.example.com/mcp")).toBe(true);
expect(isExempt(SERVER)).toBe(false);
expect(isExempt("https://other.example.com/mcp", JSON_HEADERS)).toBe(
true,
);
expect(isExempt(SERVER, JSON_HEADERS)).toBe(false);
});
});

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

@ -505,6 +505,15 @@ describe("server.ts supplemental coverage", () => {
// 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",

51
core/auth/requestTimeout.ts
View file

@ -198,6 +198,15 @@ function readNegotiatesEndpoint(hint: (() => boolean) | undefined): boolean {
}
}
/**
* 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,
/**
@ -206,8 +215,22 @@ export function exemptMcpEndpoint(
* connects. Absent, or throwing, is treated as `true`.
*/
negotiatesEndpoint?: () => boolean,
): (url: string) => boolean {
return (url) => {
): (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;
}
const serverUrl = getServerUrl();
if (!serverUrl) return true;
const negotiated = readNegotiatesEndpoint(negotiatesEndpoint);
@ -255,6 +278,21 @@ export function deadlineForRequestInit(
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();
}
/** The request URL, whatever form `fetch`'s first argument took. */
function requestUrlOf(input: RequestInfo | URL): string {
if (typeof input === "string") return input;
@ -359,6 +397,9 @@ function rebuildResponse(response: Response, body: ArrayBuffer): Response {
* 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
@ -382,7 +423,7 @@ function rebuildResponse(response: Response, body: ArrayBuffer): Response {
export function withOAuthRequestTimeout(
fetchFn: typeof fetch,
timeoutMs: number = DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
isExempt?: (url: string) => boolean,
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
@ -405,7 +446,9 @@ export function withOAuthRequestTimeout(
// 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)) return fetchFn(input, init);
if (isExempt?.(url, requestHeadersOf(input, init))) {
return fetchFn(input, init);
}
const controller = new AbortController();
const callerSignal = callerSignalOf(input, init);