fix: narrow the exemption per transport (#2319 round 18)

Round 17 widened exemptMcpEndpoint to the origin for everything, which
exempted same-origin OAuth work as well as MCP traffic — on the very
401/discovery path #2319 exists to bound.

The rule is now as narrow as each transport allows. Streamable HTTP
sends every MCP request to the configured URL, so origin + pathname is
exact there and same-origin OAuth stays bounded. Legacy SSE negotiates
its message endpoint inside the stream, where we never see it, so the
origin rule is the narrowest available — and it is exact rather than
approximate, since the SDK enforces that the message endpoint shares
the stream URL's origin.

The residual gap is therefore confined to legacy SSE against a server
hosting its own OAuth endpoints, and it is stated in the docblock and
pinned by a test rather than left implicit. Neither alternative was
reachable: the SDK takes a single fetch for all network requests, so
there is no separate auth seam to route through, and the negotiated SSE
endpoint is parsed inside the SDK's own stream handler.

Fails open on every uncertainty, including not knowing the transport —
a path rule applied to what turns out to be SSE severs real MCP
traffic, so an absent or throwing hint falls back to the origin rule.

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 06:40:07 -04:00
commit 42339ff4a4
3 changed files with 109 additions and 34 deletions

View file

@ -506,6 +506,58 @@ describe("withOAuthRequestTimeout", () => {
expect(isExempt(`${SERVER}#frag`)).toBe(true);
});
/** 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("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(
"https://srv.example.com/.well-known/oauth-protected-resource/mcp",
),
).toBe(false);
expect(isExempt("https://srv.example.com/token")).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"),
).toBe(true);
expect(isExempt("https://as.example.com/token")).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")).toBe(true);
expect(throws("https://srv.example.com/token")).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
@ -533,21 +585,6 @@ describe("withOAuthRequestTimeout", () => {
expect(isExempt("https://srv.example.com:8443/token")).toBe(false);
});
it("exempts a same-origin OAuth endpoint, which is the cost of the rule", () => {
// Stated as a test rather than left implicit: protected-resource metadata
// lives on the resource's own origin, so it is exempt here. The OAuth
// chain still bounds the Inspector's own requests to it; only the SDK's
// transport-internal OAuth against a same-origin server falls back to the
// SDK's per-request timeout.
const isExempt = exemptMcpEndpoint(() => SERVER);
expect(
isExempt(
"https://srv.example.com/.well-known/oauth-protected-resource/mcp",
),
).toBe(true);
});
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

View file

@ -157,34 +157,65 @@ export function isOAuthRequestTimeoutWire(
* uncertain — no server URL yet, a URL that will not parse, either side — is
* treated as the MCP endpoint and exempted.
*
* Compared on **origin alone**, not origin + pathname. Path matching looks more
* precise and is wrong: 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 — so a path rule bounds and body-buffers real MCP
* traffic, and a slow tool call over SSE is severed at 30s and reported as an
* `OAuthRequestTimeoutError` (Copilot).
* The comparison is **as narrow as the transport allows**, which is not the same
* rule for both of them:
*
* Origin is not a loose approximation of the right rule; it is exact, because
* the SDK *enforces* that the SSE message endpoint shares the stream URL's
* origin and rejects one that does not. Every MCP request this chain carries is
* therefore same-origin with the configured URL, by construction.
* - **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.
*
* What it costs: an OAuth endpoint hosted on the MCP server's own origin — the
* protected-resource metadata document, or an authorization server deployed
* beside the resource — is exempt here and so unbounded on this chain. That is
* the fail-open direction again, and the loss is narrow: the OAuth *chain*
* bounds the Inspector's own discovery and token requests to those same URLs,
* so only the SDK's transport-internal OAuth against a same-origin server falls
* back to the SDK's per-request timeout, which is where it was before #2319.
* 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;
}
}
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) => boolean {
return (url) => {
const serverUrl = getServerUrl();
if (!serverUrl) return true;
const negotiated = readNegotiatesEndpoint(negotiatesEndpoint);
try {
return new URL(url).origin === new URL(serverUrl).origin;
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;
}

View file

@ -840,7 +840,14 @@ export class InspectorClient extends InspectorClientEventTarget {
withOAuthRequestTimeout(
withOverrides,
DEFAULT_OAUTH_REQUEST_TIMEOUT_MS,
exemptMcpEndpoint(() => this.getServerUrl()),
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