From e71b8b9dfb57ec57c4d363e3addc7f9e1c0c36b2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 11 Sep 2026 07:10:00 -0400 Subject: [PATCH] fix: never exempt a form-encoded request (#2319 round 20) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall --- .../src/test/core/auth/requestTimeout.test.ts | 113 +++++++++++++----- .../mcp/remote/server-extra-coverage.test.ts | 9 ++ core/auth/requestTimeout.ts | 51 +++++++- 3 files changed, 142 insertions(+), 31 deletions(-) diff --git a/clients/web/src/test/core/auth/requestTimeout.test.ts b/clients/web/src/test/core/auth/requestTimeout.test.ts index 6cd232e7..3fb90a68 100644 --- a/clients/web/src/test/core/auth/requestTimeout.test.ts +++ b/clients/web/src/test/core/auth/requestTimeout.test.ts @@ -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); }); }); diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 96e3bd15..7d2fea1f 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -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", diff --git a/core/auth/requestTimeout.ts b/core/auth/requestTimeout.ts index 46943308..5a963fe8 100644 --- a/core/auth/requestTimeout.ts +++ b/core/auth/requestTimeout.ts @@ -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);