fix(client): scope transport headers to MCP requests and handle redirects explicitly - #2465
fix(client): scope transport headers to MCP requests and handle redirects explicitly#2465felixweinberger wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: cebae09 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
a9bb8c9 to
359728a
Compare
75b354b to
3fd6ef1
Compare
There was a problem hiding this comment.
Beyond the new inline nit on the reconnect branch's duplicate onerror delivery, I re-checked all findings from earlier review rounds against the current head and they are addressed: the CORS sidestep retry now goes through unmergedFetch (for both requestInit and oauthRequestInit, with tests pinning each), assertNotRedirected drains the redirect body before throwing at all four OAuth sites, normalizeReply routes ClientHttpRedirectNotFollowed to the network-error row instead of a legacy era verdict, the SSE terminal-redirect catch wraps onerror in try/finally so a throwing callback cannot skip close(), the UnauthorizedContext.fetchFn JSDoc now names oauthRequestInit, and the migration-guide redirect subsection was moved to the end of the 'HTTP & headers' section.
Extended reasoning...
Bugs were found this run (one nit: the terminal-redirect branch in _scheduleReconnection reports the same SdkHttpError instance to onerror twice), so this is the informational ruled-out note only — not an approval. This PR is large, breaking, and touches security-sensitive OAuth header scoping and redirect handling, so it needs human review regardless; the note exists to record that the six previously-flagged issues were each verified fixed at the current head (commit 3fd6ef1) so a later pass need not re-derive them.
| this._startOrAuthSse(options).catch(error => { | ||
| if (this._abortController?.signal.aborted || options.requestSignal?.aborted) return; | ||
| if (error instanceof SdkHttpError && error.code === SdkErrorCode.ClientHttpRedirectNotFollowed) { | ||
| // Terminal, not transient: a redirect answer is deterministic, | ||
| // so surface the remedy-naming error instead of retrying into | ||
| // the same redirect until maxRetries. | ||
| this.onerror?.(error); | ||
| options.onRequestStreamEnd?.(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 The new terminal-redirect branch in _scheduleReconnection's reconnect closure calls this.onerror?.(error) with the exact same SdkHttpError instance that _startOrAuthSse's own catch block already reported (its isIntentionalAbort() guard is false on this path), so hosts see the identical ClientHttpRedirectNotFollowed error double-reported per redirect-terminated reconnect. Since _startOrAuthSse has already surfaced the error, the branch can drop the this.onerror?.(error) call and just fire options.onRequestStreamEnd?.() and return — matching the file's own "so we don't call onerror twice" discipline on the 401 paths.
Extended reasoning...
What happens
The reconnect closure in _scheduleReconnection (packages/client/src/client/streamableHttp.ts:675-684) handles the new terminal-redirect case by calling this.onerror?.(error) before firing options.onRequestStreamEnd?.() and returning. But by the time the .catch runs, _startOrAuthSse's own catch block (lines ~619-624) has already invoked this.onerror?.(error as Error) with the same SdkHttpError object and rethrown it. The inner report is only suppressed when isIntentionalAbort() is true — and on this path it cannot be: the reconnect closure returned early on both abort signals before calling _startOrAuthSse (line 674), and re-checks them again at the top of the .catch (line 676). So the host's onerror handler receives the identical terminal ClientHttpRedirectNotFollowed error twice per redirect-terminated reconnect.
Step-by-step proof
- A GET stream drops after a priming event;
_handleSseStreamcalls_scheduleReconnection, which schedules thereconnectclosure. reconnectfires: neitherthis._abortController.signalnoroptions.requestSignalis aborted, so it callsthis._startOrAuthSse(options).- The reconnect GET is answered with a 307 without a followable
Location→fetchWithRedirectPolicythrowsSdkHttpErrorwith codeClientHttpRedirectNotFollowed. _startOrAuthSse's catch:isIntentionalAbort()is false →this.onerror?.(error)fires (first delivery) → rethrow.- The reconnect closure's
.catchre-checks the abort signals (still false), matches the redirect code, and callsthis.onerror?.(error)again with the literally identical object (second delivery), thenonRequestStreamEnd+ return.
The PR's own test — 'stops reconnecting when the reconnect GET is answered with an unfollowable redirect' in packages/client/test/client/streamableHttp.test.ts — collects both deliveries into its errors array but only asserts via errors.find(...), so the duplicate goes unnoticed.
Why this is a nit, not blocking
- The onerror call count is unchanged from the pre-existing behavior: the generic fallback path (line 685) has always produced two calls per failed reconnect — the raw error from
_startOrAuthSse, then a wrappingFailed to reconnect SSE stream: ...Error. The new branch merely delivers the same instance twice instead of raw-then-wrapped, so no host regresses in error volume. - The identical-object double-report pattern also pre-exists elsewhere in the file:
_send's resumptionToken path and the 202-initialized path both dothis._startOrAuthSse(...).catch(error => this.onerror?.(error)), re-reporting the object the inner catch already surfaced. onerroris a non-fatal out-of-band reporting channel — nothing breaks; hosts that count/alert on transport errors at worst double-log the terminal redirect.
Why it's still worth fixing here
The duplicate call is new code added by this PR, and the same file explicitly treats double-onerror as a defect — the 401 retry paths carry // Purposely _not_ awaited, so we don't call onerror twice comments. The second delivery carries zero new information (unlike the fallback path's contextual wrap), so it's pure double-reporting.
Fix
In the terminal-redirect branch, drop the this.onerror?.(error) call — _startOrAuthSse has already surfaced the error — and keep just options.onRequestStreamEnd?.() and return. The test can then tighten to errors.filter(e => e instanceof SdkHttpError) having length 1 to pin the invariant.
3fd6ef1 to
0462948
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ects explicitly The client transports applied their configured requestInit headers to every request they issue and relied on the platform's implicit redirect handling, so connection-scoped headers rode along to OAuth endpoints and to redirect targets on other origins. - requestInit now applies only to MCP requests. The OAuth requests the authorization flow issues (metadata discovery, token, registration) are configured separately via the new oauthRequestInit option, and the CORS preflight-sidestep retry during discovery stays a bare simple request instead of re-acquiring merged headers. - MCP requests are issued under the new redirectPolicy option (default 'manual', bounded at 3 hops): GET redirects are followed, carrying only request-descriptive headers (Accept, MCP-Protocol-Version, Last-Event-ID) once a hop leaves the endpoint's origin; POST/DELETE follow a same-origin 307/308 by re-sending the identical request (keeps trailing-slash-redirecting framework mounts working), while a same-origin 301/302/303 (platforms re-send those as GET) or any cross-origin redirect surfaces as SdkHttpError with the new code ClientHttpRedirectNotFollowed; a redirect response can no longer supply the session id the transport adopts. 'follow' delegates to the fetch implementation; 'error' makes every redirect terminal. - Token and client registration responses are terminal (RFC 6749 §5, RFC 7591 §3.2): a 3xx answer rejects instead of re-sending the request, including the cross-app access token exchanges. - Terminal redirect errors stop SSE retry loops in both transports, and the version negotiation probe surfaces them instead of misreading a redirect answer as legacy-era evidence.
0462948 to
cebae09
Compare
| currentUrl = nextUrl; | ||
| leftOrigin ||= nextUrl.origin !== initialUrl.origin; | ||
| response = leftOrigin | ||
| ? await fetchFn(currentUrl, { | ||
| method: 'GET', | ||
| headers: args.crossOriginHeaders ?? new Headers(), | ||
| signal, | ||
| redirect: 'manual' | ||
| }) | ||
| : await fetchFn(currentUrl, { ...requestInit, method: 'GET', headers, signal, redirect: 'manual' }); | ||
| throwIfRedirectFiltered(response, 'GET'); |
There was a problem hiding this comment.
🔴 Cross-origin GET redirect hops are issued as fresh top-level calls through the user-supplied fetch (opts.fetch ?? fetch), so a credential-injecting fetch wrapper — the SDK-endorsed custom-fetch-adds-Authorization pattern the SSE tests demonstrate, or withOAuth composed as a transport fetch — re-adds Authorization onto the deliberately scrubbed hop and sends the bearer token to the foreign origin. Pre-PR, redirect following happened inside the terminal platform fetch, whose HTTP-redirect algorithm strips Authorization on cross-origin hops, so under the default redirectPolicy: 'manual' this is a security regression that contradicts the changeset's own invariant that Authorization "is not applied across origins". Consider routing cross-origin hops through an unwrapped fetch, refusing cross-origin GET follows under 'manual' (mirroring the POST rule), or at minimum documenting that credential-injecting fetch wrappers void the scrubbing guarantee.
Extended reasoning...
What the bug is
fetchWithRedirectPolicy scrubs connection-scoped headers when a followed GET hop leaves the endpoint's origin: the hop is issued with only crossOriginHeaders (Accept, MCP-Protocol-Version, Last-Event-ID) — transportRedirect.ts:146-156. But the scrubbing only controls the headers the transport passes. Each hop is a fresh top-level call through fetchFn, which is opts.fetch ?? fetch (createTransportHttp, lines ~217/241) — i.e. the user-supplied fetch. When that fetch is a credential-injecting wrapper, the wrapper re-decorates the scrubbed cross-origin hop before it hits the network:
withOAuth(packages/client/src/client/middleware.ts:43-51) setsheaders.set('Authorization', \Bearer ${tokens.access_token}`)**unconditionally on every request URL**, with no origin scoping. Composed asfetch: applyMiddlewares(fetch, withOAuth(provider))`, every scrubbed hop goes out with the bearer token attached.- The plainer custom-fetch-adds-Authorization pattern is SDK-endorsed: the pre-existing SSE test "uses custom fetch implementation from options" (packages/client/test/client/sse.test.ts:274-281) passes exactly such a fetch to the transport constructor, and the
fetchoption's JSDoc carries no caveat.
Why this is a regression introduced by this PR, not pre-existing
Pre-PR, redirect was unset (platform 'follow'), so the transport invoked fetchFn once: the wrapper decorated only the initial request, and redirect following happened inside the terminal platform fetch, where the Fetch spec's HTTP-redirect-fetch algorithm deletes Authorization when the redirect crosses origins (undici implements this — it additionally strips Cookie/Proxy-Authorization/Host; browsers do it per spec). On compliant runtimes the wrapper-injected token never reached the foreign origin. Post-PR, under the default redirectPolicy: 'manual', every hop is a separate initial request through the wrapper — the runtime sees an ordinary request, applies no stripping, and the wrapper re-adds the credential. The PR turned a runtime-enforced protection into a transport-level scrub that a common, documented fetch composition silently defeats — directly contradicting the changeset's and migration guide's invariant that "headers configured for the connection (requestInit headers, the auth provider's Authorization, the server-issued Mcp-Session-Id) are not applied across origins."
Step-by-step proof
new StreamableHTTPClientTransport(url, { fetch: myAuthFetch })wheremyAuthFetchinjectsAuthorization: Bearer <token>(the SSE-test pattern), orfetch: applyMiddlewares(fetch, withOAuth(provider, url)).- The GET that opens or resumes the SSE stream is answered
307 Location: https://other.example/mcp(an http→https upgrade of the same host counts too — origin is exact scheme+host+port). fetchWithRedirectPolicyfollows the GET hop:fetchFn(currentUrl, { method: 'GET', headers: crossOriginHeaders, signal, redirect: 'manual' })— the scrubbed header set, exactly as intended.- The wrapper's own logic sets
Authorization: Bearer <token>on top of the scrubbed headers; the request goes out toother.examplecarrying the credential. The redirect target is server/attacker-controlled, so this is precisely the token-exfiltration-via-redirect shape the Fetch spec's cross-originAuthorizationstripping (Add render-blocking boolean to fetch timing info whatwg/fetch#1449) exists to prevent. - Bonus symptom for
withOAuth: if the foreign origin answers 401,withOAuthderivesserverUrlfrom the request URL's origin whenbaseUrlis unset (middleware.ts:62) and runs itsauth()flow against the redirect target.
Why existing safeguards miss it
The PR's own test "follows a cross-origin GET redirect without the connection-configured headers" uses a bare fetchMock, so the fetch-wrapper layer is never exercised. The PR clearly anticipated this class of problem for its own wrapper — the new unmergedFetch machinery unwraps createFetchWithInit wrappers for the CORS-retry path — but fetchWithRedirectPolicy has no way to see through arbitrary user wrappers. Scope caveats that bound but don't defuse the finding: browsers throw on opaqueredirect before any hop is followed (so the leak is Node-only — but Node is the dominant client runtime); the built-in authProvider path is unaffected (its Authorization lives in the transport-derived headers that ARE scrubbed); and non-Authorization custom-wrapper headers already leaked pre-PR (runtimes strip only Authorization/Cookie-class headers). The Authorization delta is the security-relevant regression.
How to fix
Options, roughly in order of strength: (a) route cross-origin hops through the fully unwrapped platform fetch — the same idea as unmergedFetch, though it cannot see arbitrary user wrappers, so it would need to be paired with documentation; (b) refuse to follow cross-origin GET redirects under 'manual', mirroring the POST/DELETE rule, so leaving the origin requires an explicit redirectPolicy: 'follow' opt-in (this also aligns GET with the PR's stated origin-scoping philosophy); or (c) at minimum, document on the fetch option and in the migration guide that a credential-injecting fetch wrapper re-applies its credentials on every hop and voids the cross-origin scrubbing guarantee.
| if (status !== 307 && status !== 308) { | ||
| // Platforms re-send a 301/302/303-answered POST as GET | ||
| // (RFC 9110 §15.4), which would corrupt an MCP request. | ||
| const slashHint = differsOnlyByTrailingSlash(currentUrl, nextUrl) | ||
| ? ` — the target differs from the request URL only by a trailing slash; point the transport at ${nextUrl.href}` | ||
| : ` — point the transport at the endpoint's final URL, or set redirectPolicy: 'follow' to delegate to the fetch implementation`; | ||
| throw new SdkHttpError( | ||
| SdkErrorCode.ClientHttpRedirectNotFollowed, | ||
| `Server answered ${method} with a redirect (HTTP ${status} to ${location}); following it would re-send the request as GET${slashHint}`, | ||
| { status, statusText } | ||
| ); |
There was a problem hiding this comment.
🟡 The error message thrown for a DELETE answered with a same-origin 301/302 ("following it would re-send the request as GET") states a false rationale: the WHATWG Fetch spec rewrites the method to GET only for 301/302 + POST (or 303 + non-GET/HEAD), so every conformant fetch re-sends a 301/302-answered DELETE as DELETE — meaning v1 followed this hop correctly and the new rejection is stricter than the stated justification supports. Either reword the message/comment/migration-guide prose to scope the GET-rewrite rationale to POST (and 303), or treat same-origin 301/302 on DELETE like 307/308; note no test pins the DELETE×same-origin-301/302 cell.
Extended reasoning...
What the bug is
The non-307/308 branch of fetchWithRedirectPolicy (packages/client/src/client/transportRedirect.ts:125-135) rejects any same-origin 301/302/303 answer to a POST or DELETE with a message that interpolates the method:
Server answered ${method} with a redirect (HTTP ${status} to ${location}); following it would re-send the request as GET…
The inline comment says "Platforms re-send a 301/302/303-answered POST as GET (RFC 9110 §15.4), which would corrupt an MCP request", and the migration guide (docs/migration/upgrade-to-v2.md, the new "Redirect (3xx) responses on MCP requests" subsection) and the transport-redirect-policy changeset repeat the claim for POST/DELETE jointly: "a same-origin 301/302/303 — platforms re-send those as GET".
Why the claim is false for DELETE × 301/302
The WHATWG Fetch spec's HTTP-redirect fetch step rewrites the request method to GET only when "status is 301 or 302 and request's method is POST, or status is 303 and request's method is not GET or HEAD". undici implements this predicate verbatim, and browsers match it. RFC 9110 §15.4.2/§15.4.3 likewise scope the historical method-change note to POST→GET. So a DELETE answered with a same-origin 301 or 302 is re-sent as DELETE by every conformant fetch — only a 303 rewrites it. This was also verified empirically against Node's fetch (undici) during review: 301/302 preserve DELETE; only 303 rewrites it.
The code path that triggers it
A client whose session-terminating DELETE is answered by a same-origin 301/302 (e.g. a legacy permanent-move rule on the mount) hits terminateSession() → _http.mcpRequest('DELETE', …) → fetchWithRedirectPolicy. method !== 'GET', the origin matches, status !== 307 && status !== 308 → the branch throws ClientHttpRedirectNotFollowed with the GET-rewrite message.
Step-by-step proof
- Transport at
http://host/mcp; the front answersDELETE /mcpwith301 Location: /mcp/(same origin). - v1 (platform
redirect: 'follow'): fetch's HTTP-redirect step checks (301|302 ∧ POST) — false for DELETE — so it re-sendsDELETE /mcp/with the method preserved; the session terminates. - v2 (
redirectPolicy: 'manual'default):fetchWithRedirectPolicyreaches thestatus !== 307 && status !== 308branch and throws: "Server answered DELETE with a redirect (HTTP 301 to /mcp/); following it would re-send the request as GET — the target differs from the request URL only by a trailing slash; …". - The user debugging this is told the platform would corrupt the method — which it would not — and the session is never terminated server-side.
Why existing safeguards don't catch it
The new tests pin POST×301/302/303 and DELETE×308-cross-origin / DELETE×307-same-origin, but no test exercises DELETE × same-origin 301/302, so the wrong-rationale rejection ships unexercised.
Impact and how to fix
The rejection itself is documented, intentional behavior (the PR's matrix deliberately groups POST/DELETE × 301/302/303 as a typed error, matching python-sdk), so nothing undocumented breaks — the defect is the false justification in new user-facing text, the code comment, the migration guide, and the changeset, plus unnecessary strictness for one cell that would be exactly as safe to follow as 307/308 (same origin, method preserved, no body). Two fixes, either sufficient: (a) minimal — reword the error message/comment/docs so the GET-rewrite rationale is stated for POST (and 303) only, keeping the conservative rejection as a deliberate simplification; or (b) treat same-origin 301/302 on DELETE like 307/308 (re-send the identical request), matching both the Fetch spec and v1 behavior. In either case, add a test pinning the DELETE × same-origin-301/302 cell.
2 participants
The client transports applied their configured
requestInitheaders to every request they issue, and relied on the platform's implicit redirect handling. This scopes configured headers to MCP requests and makes redirect handling explicit.Motivation and Context
Aligns request handling with RFC 9110 redirect semantics and RFC 6749 §5 (token responses are terminal). Headers configured for the connection apply to that connection; authorization requests are configured separately via a new
oauthRequestInittransport option. AredirectPolicyoption covers deployments that need different redirect handling. The default redirect matrix matches the direction python-sdk is taking in modelcontextprotocol/python-sdk#3075.Default redirect matrix for MCP requests (
redirectPolicy: 'manual', bounded at 3 hops):GETAccept,MCP-Protocol-Version,Last-Event-ID)POST/DELETE, 307/308POST/DELETE, 301/302/303GET; message names a trailing-slash mismatch)Origin is an exact scheme+host+port match, so an
http://→https://upgrade is cross-origin.redirectPolicy: 'follow'delegates to the fetch implementation;redirectPolicy: 'error'treats every redirect answer as terminal.How Has This Been Tested?
New transport-level tests cover the header scoping and the redirect matrix (method × origin × status class × policy, including runtimes that filter redirect responses); a same-origin 307-fronted server (the trailing-slash framework-mount shape) negotiates and serves
tools/callend-to-end; full suite, conformance runs, and the examples e2e matrix pass.Breaking Changes
Three deployment shapes are affected (details and options in the migration guide):
POST/DELETEwith a same-origin301/302/303or with any cross-origin redirect (anhttp://transport URL whose server upgrades tohttps://is cross-origin — a scheme change is a different origin): those are no longer followed. A same-origin307/308front — the common trailing-slash mount shape — keeps working out of the box. Remedy: use the endpoint's final URL, or setredirectPolicy: 'follow'.requestInitheaders on authorization requests: those headers no longer reach OAuth requests. Remedy: setoauthRequestInit.Deployments with none of these shapes see identical behavior.
Types of changes
Checklist