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 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 75b354b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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: |
| them). The required media types are always present; additional types are kept for | ||
| proxy/gateway routing. | ||
|
|
||
| #### Redirect (3xx) responses on MCP requests |
There was a problem hiding this comment.
🟡 The new '#### Redirect (3xx) responses on MCP requests' subsection is inserted mid-way through the 'HTTP & headers' section, so the pre-existing hostHeaderValidation/localhostHostValidation and Content-Type/415 paragraphs that follow it (before '### Errors') are now structurally nested under the redirect subsection in any rendered TOC/outline, despite being unrelated to redirects. Move the new subsection to the end of the 'HTTP & headers' section (after those paragraphs), or give the trailing paragraphs their own heading.
Extended reasoning...
What the issue is
The migration guide's ### HTTP & headers section (line 782 of docs/migration/upgrade-to-v2.md) previously contained only flat paragraphs — no #### subsections. This PR inserts the new #### Redirect (3xx) responses on MCP requests heading at line 814, in the middle of that section. In Markdown's implicit outline model, a section extends until the next heading of the same or higher level — and the next heading after line 814 is ### Errors at line 880. Everything in between is therefore now a structural child of the redirect subsection.
What gets misfiled
Two pre-existing, unrelated blocks fall inside that span:
- The
hostHeaderValidation()/localhostHostValidation()moved-to-@modelcontextprotocol/expressparagraph (line 862). - The Content-Type / 415 media-type validation paragraph (lines 868–878).
Neither has anything to do with redirects, but any hierarchical rendering — a generated table of contents, a docs-site outline/sidebar, section-scoped anchors or search extraction — now files them under "Redirect (3xx) responses on MCP requests".
Step-by-step proof
- Before this PR, the heading sequence was:
### HTTP & headers(782) →### Errors; the host-header and Content-Type paragraphs were direct children of### HTTP & headers. - This PR inserts
#### Redirect (3xx) responses on MCP requestsat line 814, above those paragraphs. - A TOC generator walking the heading tree now produces:
HTTP & headers→Redirect (3xx) responses on MCP requests→ (host-header paragraph, Content-Type paragraph) →Errors. - A reader who clicks the "Redirect (3xx)" TOC entry (or extracts that subsection programmatically) finds host-header and media-type migration notes filed under redirects.
Why nothing prevents it
Markdown lint rules and the docs pipeline don't enforce topical grouping of heading spans, so the misfiled structure renders without any error.
How to fix
Either move the new #### Redirect (3xx) responses on MCP requests block to the end of the HTTP & headers section (immediately before ### Errors, after line 878), or give the two trailing paragraphs their own #### heading (e.g. #### Host validation & media types) so they are no longer implicit children of the redirect subsection.
Impact
Cosmetic/structural only — no behavior, code, or example is affected. This should not block the merge; it's a one-move fix.
The client transports built one requestInit-merging fetch and used it for both MCP traffic and the requests issued by the OAuth authorization flow, so headers configured for the MCP connection rode along on every protected-resource metadata, authorization-server metadata, token, and client registration request — including ones targeting a different origin than the MCP server. Connection-level requestInit headers now apply only to MCP requests. The authorization flow uses a fetch built from the custom fetch option and a new oauthRequestInit transport option, which configures those requests explicitly.
The token-exchange and client-registration POSTs followed HTTP redirects, re-sending the request body to whatever target the Location header named. Token responses are terminal (RFC 6749 §5): these requests are now issued with redirect: 'manual', and a 3xx answer (or a redirect the runtime filtered as opaque) rejects with an error instead of being re-sent.
4206f5b to
a9bb8c9
Compare
StreamableHTTPClientTransport and SSEClientTransport issue their MCP
requests (POST sends, SSE stream GETs, session DELETE) with
redirect: 'manual' and apply RFC 9110 redirect semantics themselves:
GET redirects are followed for up to 3 hops, with the connection's
configured headers scoped to the endpoint's origin (a cross-origin hop
carries only Accept, MCP-Protocol-Version and Last-Event-ID);
POST/DELETE requests are never re-sent to a Location target and surface
the redirect as SdkHttpError with the new code
ClientHttpRedirectNotFollowed. A redirect response can no longer supply
the mcp-session-id the transport adopts. The new redirectPolicy
transport option ('manual' | 'follow', default 'manual') restores
delegation to the fetch implementation for deployments behind
redirecting infrastructure and for browser runtimes.
a9bb8c9 to
359728a
Compare
| this._last401Response = undefined; | ||
| this._eventSource?.close(); | ||
| this._authProvider.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._fetchWithInit }).then( | ||
| this._authProvider.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._authFetch }).then( |
There was a problem hiding this comment.
🟡 The public JSDoc on UnauthorizedContext.fetchFn (packages/client/src/client/auth.ts:56) still says the fetch function is "configured with the transport's requestInit", but after this PR every onUnauthorized call site in both transports passes _authFetch, which deliberately merges oauthRequestInit and never requestInit. Update the doc to reference oauthRequestInit so AuthProvider authors don't expect their transport requestInit headers (e.g. gateway API keys) on requests made through ctx.fetchFn.
Extended reasoning...
What the issue is
This PR changes both client transports to stop applying the connection-level requestInit to OAuth requests. The private field that used to be _fetchWithInit = createFetchWithInit(opts?.fetch, opts?.requestInit) is now _authFetch = createFetchWithInit(opts?.fetch, opts?.oauthRequestInit), carrying the inline comment "deliberately merges oauthRequestInit, never requestInit". Every AuthProvider.onUnauthorized({ response, serverUrl, fetchFn }) call site was updated to pass this._authFetch — sse.ts:251 and sse.ts:405, plus the equivalent hunks in streamableHttp.ts (the _startOrAuthSse and _send 401 paths).
However, the JSDoc on the public interface that types that argument was not updated. packages/client/src/client/auth.ts:56 (untouched by this PR) still reads:
/** Fetch function configured with the transport's `requestInit`, for making auth requests. */
fetchFn: FetchLike;Why it matters
UnauthorizedContext is public API — it is the context every custom AuthProvider.onUnauthorized implementation receives. An author reading this doc would expect their transport requestInit headers (e.g. a gateway API key required to reach well-known/token endpoints) to ride on requests made through ctx.fetchFn. That is exactly the behavior this PR deliberately removes: the changeset and the migration guide's new bullet both state "Transport requestInit headers stay off OAuth requests" and point users at the new oauthRequestInit option instead. The stale doc promises the opposite of the shipped behavior and would send a confused integrator down the wrong debugging path.
Step-by-step proof
- A user configures
new StreamableHTTPClientTransport(url, { requestInit: { headers: { 'X-Gateway-Key': 'k' } }, authProvider })with a customAuthProviderwhoseonUnauthorizedcallsctx.fetchFn(wellKnownUrl). - Reading the
UnauthorizedContext.fetchFnJSDoc, they expectX-Gateway-Keyon that request. - At runtime, the transport passes
this._authFetch = createFetchWithInit(opts?.fetch, opts?.oauthRequestInit)(streamableHttp.ts constructor);oauthRequestInitis unset, so the request goes out with noX-Gateway-Key— the PR's own header-isolation tests (does not apply requestInit headers to authorization requests) pin exactly this. - The gateway rejects the metadata request; the user is left debugging a discrepancy the docs told them couldn't happen.
Why existing code doesn't prevent it
Nothing type-checks JSDoc prose against behavior; the PR's tests assert the new behavior but the interface comment lives in a file section the diff didn't touch.
How to fix
One-line doc update in auth.ts:56, e.g.: /** Fetch function configured with the transport's \oauthRequestInit`, for making auth requests. Transport `requestInit` headers are deliberately not applied. */`
Severity
Nit — docs-only, nothing breaks at runtime, but it should be fixed alongside this PR since the PR itself changed the documented behavior.
| this._redirectPolicy = opts?.redirectPolicy ?? 'manual'; | ||
| this._authFetch = createFetchWithInit(opts?.fetch, opts?.oauthRequestInit); |
There was a problem hiding this comment.
🔴 In browsers, fetchWithCorsRetry's preflight-sidestep retry (fetchFn(url, {}) at auth.ts:1570) silently re-acquires the oauthRequestInit headers because _authFetch = createFetchWithInit(fetch, opts?.oauthRequestInit) (here and sse.ts:173) merges baseInit.headers into any call whose init has no headers — so the 'simple request' retry still triggers the same CORS preflight, fails identically, and OAuth discovery falls through every candidate URL with a misleading error. This defeats the CORS fallback exactly when the new option is used the way the migration guide recommends (gateway headers on well-known endpoints) against an AS that doesn't CORS-allowlist the header; the PR fixed this same merge mechanism for requestInit (and pinned the un-merged-retry invariant in the new test 'keeps requestInit headers off the retried simple discovery request') but reintroduced it for oauthRequestInit. Fix by letting the bare retry bypass the merge — e.g. have createFetchWithInit treat an explicit headers-free init ({}) as un-merged, or thread the raw fetch for the retry.
Extended reasoning...
What the bug is
createFetchWithInit (packages/core-internal/src/shared/transport.ts:42) merges headers as:
headers: init?.headers ? { ...normalizeHeaders(baseInit.headers), ...normalizeHeaders(init.headers) } : baseInit.headersWhen the per-call init has no headers property, the base headers are applied wholesale. This PR wires the new option into both transports as _authFetch = createFetchWithInit(opts?.fetch, opts?.oauthRequestInit) (streamableHttp.ts:370, sse.ts:173), and _authFetch is the fetchFn handed to auth(), resolveAuthorizationCallbackParams, and every onUnauthorized site — i.e. to all OAuth discovery requests.
The code path that triggers it
fetchWithCorsRetry (packages/client/src/client/auth.ts:1559–1582) exists precisely to survive browser CORS preflight rejections on discovery requests: when the initial fetch (which carries MCP-Protocol-Version) throws a TypeError and CORS_IS_POSSIBLE is true, it retries with fetchFn(url, {}) — its comment says 'Retry as a simple request: if that succeeds, we've sidestepped the preflight.' But {} has no headers, so through _authFetch the retry goes out carrying the full oauthRequestInit header set. It is not a simple request; it preflights on the same custom header, throws the same TypeError, and fetchWithCorsRetry returns undefined — discoverMetadataWithFallback / discoverAuthorizationServerMetadata then fall through every candidate URL until the whole authorization flow fails with misleading errors ('Resource server does not implement OAuth 2.0 Protected Resource Metadata.' etc.).
Step-by-step proof
- Browser runtime (
CORS_IS_POSSIBLE = trueonly inshimsBrowser.ts); transport configured withoauthRequestInit: { headers: { 'X-Auth-Gateway': 'v' } }— the exact usage the migration guide recommends for 'gateway headers on well-known endpoints'. - Discovery calls
tryMetadataDiscovery→fetchWithCorsRetry(url, { 'MCP-Protocol-Version': ... }, _authFetch). The AS's/.well-knownendpoint doesn't listX-Auth-Gateway(orMCP-Protocol-Version) inAccess-Control-Allow-Headers→ preflight rejected →TypeError. - Retry:
fetchFn(url, {}). InsidecreateFetchWithInit,init?.headersisundefined→ mergedheaders = baseInit.headers = { 'X-Auth-Gateway': 'v' }. - The 'simple' retry preflights on
X-Auth-Gateway, rejected again →TypeError→fetchWithCorsRetryreturnsundefined. shouldAttemptFallbackseesundefined→ next candidate URL → identical failure → discovery exhausts all URLs → browser OAuth flow hard-fails. Without the merge, step 3's bare retry would have succeeded (no custom headers, no preflight) — as it does today for theMCP-Protocol-Versionheader whenoauthRequestInitis unset.
Why existing safeguards don't catch it
The PR's own new test 'keeps requestInit headers off the retried simple discovery request' pins exactly this invariant — its comment says 'The retry must go through the same un-merged fetch, so connection-level requestInit headers must not reappear on it' — and asserts the retry reaches the underlying fetch with init.headers === undefined. But it only exercises the oauthRequestInit-unset case. Pre-PR, requestInit had this same flaw via _fetchWithInit; the PR fixed it for requestInit by removing the merge, then reintroduced the identical mechanism for the new option. Setting oauthRequestInit violates the pinned invariant on day one.
Impact and fix
Impact is confined to browsers using the new option against an AS whose well-known endpoints don't CORS-allowlist the configured header (typical for third-party AS deployments — the user's gateway may need the header on its token endpoint while discovery targets the AS directly). No existing configuration regresses, but the brand-new option ships broken in one of its primary documented use cases, and the fallback designed for this exact situation is silently defeated. Note that even reading header retention as intentional ('the user asked for these headers on OAuth requests'), the current behavior re-issues an identical failing request and swallows the error — the fallback is defeated either way. Fix directions: make createFetchWithInit distinguish 'no headers specified' from an explicitly header-free init (so fetchFn(url, {}) or fetchFn(url, { headers: {} }) stays bare), thread the raw un-merged fetch to fetchWithCorsRetry for the retry leg, or apply oauthRequestInit at the request sites (as the token/registration POSTs do) rather than via a merged wrapper — and extend the existing isolation test to cover the oauthRequestInit-set case.
| if (response.type === 'opaqueredirect' || (response.status >= 300 && response.status < 400)) { | ||
| throw new Error( | ||
| `Token endpoint responded with a redirect (HTTP ${response.status || 'filtered by the runtime'}); token responses are terminal` | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 The four new redirect guards in the OAuth paths (executeTokenRequest and registerClient in auth.ts, requestJwtAuthorizationGrant and exchangeJwtAuthGrant in crossAppAccess.ts) throw without consuming the response body, unlike transportRedirect.ts in this same PR, which explicitly drains the redirect response before erroring. Under Node's undici an unconsumed body pins the keep-alive socket until GC; add await response.text?.().catch(() => {}); before each of the four throws to mirror the drain discipline used everywhere else in these files.
Extended reasoning...
What the bug is
This PR adds redirect: 'manual' plus a redirect guard to four OAuth request sites: executeTokenRequest (auth.ts:2098), registerClient (auth.ts:~2381), requestJwtAuthorizationGrant (crossAppAccess.ts:~159), and exchangeJwtAuthGrant (crossAppAccess.ts:~293). Each guard throws immediately when response.type === 'opaqueredirect' || (300 <= status < 400) — without first consuming the response body.
Why this is inconsistent with the PR's own discipline
The same PR's transportRedirect.ts handles the identical situation with an explicit drain and a comment explaining why:
// Release the redirect response before erroring or following.
await response.text?.().catch(() => {});And every other exit path in the four touched functions consumes the body before throwing: parseErrorResponse reads response.text(); the discovery helpers in auth.ts drain via await response.text?.().catch(() => {}) at lines 1535, 1693, and 1820; registerClient's !ok path reads await response.text() into RegistrationRejectedError; and the crossAppAccess !ok paths call await response.json().catch(() => ({})). The new redirect guards are the only exits in these functions that leave the body unconsumed.
Impact
Under Node's undici, a fetch Response whose body is never read or cancelled keeps the underlying keep-alive connection/stream pinned until the GC finalizer reclaims it (undici warns about this pattern). 3xx responses from misconfigured proxies routinely carry small HTML bodies, so the stream is real, not empty. That said, this is a one-shot terminal error path — the auth flow is failing anyway — and the socket is eventually reclaimed, so nothing user-visible breaks. In browser runtimes the opaqueredirect body is null, making the drain a no-op there.
Step-by-step proof
- A client calls
auth()against an authorization server whose token endpoint sits behind a proxy misconfigured to answer307 Temporary Redirectwith an HTML body. executeTokenRequestissues the POST withredirect: 'manual'; undici returns the 307 response with a readable body stream.- The guard at auth.ts:2098 matches (
status >= 300 && status < 400) and throwsToken endpoint responded with a redirect...— the body stream is never read or cancelled. - The keep-alive socket backing that response stays pinned until GC finalization; under load or repeated auth retries, several sockets can accumulate before a GC cycle runs.
- Compare the followed-redirect path in
transportRedirect.ts, which drains before every throw/follow and never exhibits this.
Why nothing prevents it
Nothing in fetch or undici forces body consumption; the leak only surfaces as an undici warning or lingering sockets, which no test asserts on. The PR's new tests mock text: async () => '' but never verify it was called.
How to fix
Add one line before each of the four throws, mirroring transportRedirect.ts and the sibling paths in the same files:
await response.text?.().catch(() => {});This is a hygiene/consistency fix on a rare terminal error path, not a merge blocker — hence nit severity.
Both client HTTP transports now build one module-internal object that owns the endpoint URL, fetch, requestInit, oauthRequestInit, and redirect policy, and issue every data-plane request and OAuth fetch through it instead of holding those as class fields. Also folds the four duplicated token/registration redirect guards into one assertNotRedirected helper with unchanged messages.
| if (method !== 'GET') { | ||
| throw new SdkHttpError( | ||
| SdkErrorCode.ClientHttpRedirectNotFollowed, | ||
| `Server answered ${method} with a redirect (HTTP ${status}${location ? ` to ${location}` : ''}); ` + | ||
| `${method} requests are not re-sent to redirect targets — point the transport at the new endpoint or set redirectPolicy: 'follow'`, | ||
| { status, statusText } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 The version-negotiation probe's send-error normalization (normalizeReply, versionNegotiation.ts:320-325) maps every SdkHttpError — including the new ClientHttpRedirectNotFollowed one, whose data has no text field — to { kind: 'http-error', status, body: undefined }, which classifyHttpError classifies as the conservative { kind: 'legacy' } era verdict. In pin or modern-only negotiation modes against a redirecting front (or any browser opaqueredirect under the default 'manual' policy), the deliberately remedy-naming redirect error is swallowed and replaced by a wrong EraNegotiationFailed diagnosis; route error.code === SdkErrorCode.ClientHttpRedirectNotFollowed to the network-error/typed-error row before the generic SdkHttpError branch.
Extended reasoning...
What the bug is
This PR makes MCP POSTs terminal on 3xx: under the default redirectPolicy: 'manual', a redirect answer to a POST throws SdkHttpError with the new code SdkErrorCode.ClientHttpRedirectNotFollowed and data = { status, statusText } — deliberately no text field (transportRedirect.ts:56-63, 194-201). That error now flows into a pre-existing consumer the PR didn't update: the connect-time version-negotiation probe. ProbeWindow.exchange sends its server/discover request through transport.send() and turns a rejection into a send-error reply; normalizeReply (versionNegotiation.ts:320-325) checks error instanceof SdkHttpError first and maps it to { kind: 'http-error', status, body: error.data.text } — body is undefined for the redirect error since there is no data.text.
The code path that triggers it
classifyHttpError (probeClassifier.ts:210-218) finds no parseable JSON-RPC body (it has no status-based filtering for 3xx or 0) and returns the conservative { kind: 'legacy' } era verdict. So a transport/config redirect refusal is treated as protocol-era evidence — "this is a pre-2026 server." This contradicts the classifier module's own documented contract: "a network outage is a typed connect error, never an era verdict" (probeClassifier.ts:8-9, and the { kind: 'error' } row is annotated "Typed connect error — never converted to an era verdict"). A redirect the transport refuses to follow is in exactly that transport-failure category — and in browsers the "status" is the literal 0 from an opaqueredirect, not even a real server HTTP status.
Step-by-step proof
- Client configures
versionNegotiation: { pin: '2026-07-28' }and points the transport at anhttp://URL whose server 308s tohttps://(the PR description's own breaking-change shape Better validation of data coming over the wire #1). - Connect runs the
server/discoverprobe →transport.send()POSTs →fetchWithRedirectPolicysees the 308, throwsSdkHttpError(ClientHttpRedirectNotFollowed, ..., { status: 308, statusText }). normalizeReplyhits theinstanceof SdkHttpErrorbranch →{ kind: 'http-error', status: 308, body: undefined }.classifyHttpError:parseJsonRpcErrorBody(undefined)→undefined→{ kind: 'legacy' }.negotiateEra(versionNegotiation.ts:424-429): pin mode + legacy verdict → throwsSdkError(EraNegotiationFailed, 'Version negotiation failed: the server did not offer pinned protocol version 2026-07-28 via server/discover (no fallback in pin mode)'). The server may support that version perfectly; the redirect error whose message was specifically written to name the remedies (point the transport at the final URL, or setredirectPolicy: 'follow') never reaches the user. Modern-only clients (fallbackAvailable: false) hit the equally wrong "the server gave no modern evidence..." message at lines 431-439. Browser runtimes amplify this: under the default'manual'policy every redirect answer is anopaqueredirect(status 0), so any redirecting deployment reproduces it for probe-enabled clients.
Why nothing prevents it
versionNegotiation.ts is untouched by this PR — its SdkHttpError branch predates the new error code, and no test exercises the probe against a redirect answer. Notably, the PR is aware this error is terminal-and-informative elsewhere: sse.ts special-cases exactly this code so the EventSource doesn't schedule reconnects — this consumer was just missed.
Impact and severity
Filed as a nit rather than blocking: version negotiation is opt-in (the default mode is legacy and sends no probe), the affected deployments are already intentionally broken by this PR (every subsequent POST to the redirecting endpoint fails the same way, so the connection fails by design either way), and in default auto-with-fallback mode the real redirect error does eventually surface from the doomed legacy initialize — costing one wasted round trip. The concrete harm is degraded diagnostics in pin/modern-only modes: a factually wrong EraNegotiationFailed message that sends the user down a protocol-version debugging trail instead of the redirect remedy the PR crafted.
How to fix
One small branch in normalizeReply (versionNegotiation.ts:320-325): check error.code === SdkErrorCode.ClientHttpRedirectNotFollowed before the generic SdkHttpError branch and return it as a network-error/typed-error row (e.g. { kind: 'network-error', error }), so the redirect error propagates as the typed connect error the classifier contract prescribes.
| } catch (error) { | ||
| if (error instanceof SdkHttpError && error.code === SdkErrorCode.ClientHttpRedirectNotFollowed) { | ||
| // Terminal, not transient: close so the EventSource does not | ||
| // schedule reconnects against the same redirecting endpoint. | ||
| reject(error); | ||
| this.onerror?.(error); | ||
| void this.close(); | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
🟡 In the new terminal-redirect catch block, the user-supplied this.onerror?.(error) is invoked before void this.close() with no try/finally, so a throwing onerror callback skips close() — the EventSource readyState stays CONNECTING and the eventsource package schedules reconnects against the same redirecting endpoint, re-entering this path each time: exactly the unbounded reconnect loop this block's comment says it exists to prevent (and the transport is left half-open — _abortController never aborted, onclose never fired). Fix: call void this.close() before this.onerror?.(error) (close is idempotent), or wrap the callback in try/finally.
Extended reasoning...
What the bug is
The catch block added in this PR to the EventSource custom fetch in _startOrAuth (packages/client/src/client/sse.ts:213-221) handles the terminal ClientHttpRedirectNotFollowed error:
} catch (error) {
if (error instanceof SdkHttpError && error.code === SdkErrorCode.ClientHttpRedirectNotFollowed) {
// Terminal, not transient: close so the EventSource does not
// schedule reconnects against the same redirecting endpoint.
reject(error);
this.onerror?.(error); // user-supplied callback, may throw
void this.close(); // skipped if onerror throws
}
throw error;
}The block's correctness contract — stated in its own comment and pinned by this PR's test 'fails start() with the clear error naming redirectPolicy on a runtime-filtered redirect of the stream GET' (fetchMock called exactly once, onclose called) — is that this.close() runs before the rethrow reaches the eventsource package. That package's fetch-failure path (#onFetchError → #scheduleReconnect) only skips the reconnect and error-event dispatch when readyState === CLOSED. In the happy path close() runs synchronously through this._eventSource?.close(), so the invariant holds.
The failure path
this.onerror is a user-supplied callback invoked at line 218, before close() at line 219, with no try/finally. If the host's onerror throws, execution leaves the catch block at line 218: neither void this.close() nor the throw error at line 221 runs.
Step-by-step proof
- Host configures
transport.onerror = (e) => { throw wrapError(e); }(an error handler that escalates, or one that itself has a bug) and connects to an endpoint that answers the stream GET with a redirect the policy rejects (e.g. an opaque redirect in a filtering runtime). eventSourceGetthrowsSdkHttpErrorwithClientHttpRedirectNotFollowed; the catch matches;reject(error)settles thestart()promise (correctly).this.onerror?.(error)at line 218 throws. Execution exits the catch — line 219'svoid this.close()never runs.this._eventSource.close()is never called, so readyState stays CONNECTING;_abortControlleris never aborted;onclosenever fires.- The user's thrown error propagates as the custom fetch's rejection. The eventsource package's
#onFetchErrorseesreadyState !== CLOSED, dispatches anerrorevent (which routes back into the transport's_eventSource.onerror→ wraps intoSseError→ calls the throwingonerroragain, now inside an EventTarget dispatch — surfacing as an uncaught exception in Node), and schedules a reconnect against the same redirecting endpoint. - Each reconnect re-issues the GET, gets the same terminal redirect, re-enters this catch (
rejectis now a no-op), throws inonerroragain, skipsclose()again — an unbounded request loop against the redirecting endpoint, the precise outcome the block exists to prevent, plus a permanently half-open transport.
Why existing code doesn't prevent it
Nothing constrains user onerror callbacks to be non-throwing, and the PR's new tests only exercise a vi.fn() onerror that never throws. This matches the repository's recurring async/lifecycle pattern (#1735, #1763): in close/shutdown paths, user-supplied callbacks must be wrapped so a throw cannot skip the remaining teardown. The pre-existing sibling onerror-before-close() call sites in this file don't have this dependency — their correctness doesn't hinge on statements after the callback — but this new block's terminal-teardown contract does.
How to fix
One-line reorder (close is idempotent and doesn't depend on onerror having run):
reject(error);
void this.close();
this.onerror?.(error);or wrap the callback:
reject(error);
try {
this.onerror?.(error);
} finally {
void this.close();
}Severity
Nit: the failure only triggers when the host's own onerror handler throws — well-behaved integrations see fully correct behavior (reject(error) already ran, so start() rejects properly, and the PR's pinned single-fetch/onclose behavior holds). But since the block's entire purpose is to guarantee teardown on this terminal error, the one-line hardening is worth doing in this PR.
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 preserves the previous following behavior where deployments need it.How Has This Been Tested?
New transport-level tests cover the header scoping and the redirect matrix (method × origin × policy, including runtimes that filter redirect responses); 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/DELETEare no longer followed (this includes anhttp://transport URL whose server upgrades tohttps://— a scheme change is a different origin). 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