Skip to content

fix(client): scope transport headers to MCP requests and handle redirects explicitly#2465

Open
felixweinberger wants to merge 5 commits into
mainfrom
fweinberger/oauth-header-isolation
Open

fix(client): scope transport headers to MCP requests and handle redirects explicitly#2465
felixweinberger wants to merge 5 commits into
mainfrom
fweinberger/oauth-header-isolation

Conversation

@felixweinberger

@felixweinberger felixweinberger commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

The client transports applied their configured requestInit headers 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 oauthRequestInit transport option. A redirectPolicy option 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):

  1. An MCP server reached through a redirecting front: redirects on POST/DELETE are no longer followed (this includes an http:// transport URL whose server upgrades to https:// — a scheme change is a different origin). Remedy: use the endpoint's final URL, or set redirectPolicy: 'follow'.
  2. A gateway that requires the transport's requestInit headers on authorization requests: those headers no longer reach OAuth requests. Remedy: set oauthRequestInit.
  3. An authorization server whose token or registration endpoint answers with a 307/308: the exchange now errors instead of re-sending the request. Remedy: use the endpoint's final URL (no client option, by design).

Deployments with none of these shapes see identical behavior.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@felixweinberger felixweinberger requested a review from a team as a code owner July 8, 2026 13:55
@changeset-bot

changeset-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 75b354b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@modelcontextprotocol/client Patch
@modelcontextprotocol/core-internal Patch

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

@pkg-pr-new

pkg-pr-new Bot commented Jul 8, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2465

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2465

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2465

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2465

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2465

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2465

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2465

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2465

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2465

commit: 75b354b

them). The required media types are always present; additional types are kept for
proxy/gateway routing.

#### Redirect (3xx) responses on MCP requests

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  1. The hostHeaderValidation() / localhostHostValidation() moved-to-@modelcontextprotocol/express paragraph (line 862).
  2. 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

  1. Before this PR, the heading sequence was: ### HTTP & headers (782) → ### Errors; the host-header and Content-Type paragraphs were direct children of ### HTTP & headers.
  2. This PR inserts #### Redirect (3xx) responses on MCP requests at line 814, above those paragraphs.
  3. A TOC generator walking the heading tree now produces: HTTP & headersRedirect (3xx) responses on MCP requests → (host-header paragraph, Content-Type paragraph) → Errors.
  4. 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.
@felixweinberger felixweinberger force-pushed the fweinberger/oauth-header-isolation branch from 4206f5b to a9bb8c9 Compare July 8, 2026 14:20
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.
@felixweinberger felixweinberger force-pushed the fweinberger/oauth-header-isolation branch from a9bb8c9 to 359728a Compare July 8, 2026 14:24
Comment thread packages/client/src/client/sse.ts Outdated
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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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._authFetchsse.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

  1. A user configures new StreamableHTTPClientTransport(url, { requestInit: { headers: { 'X-Gateway-Key': 'k' } }, authProvider }) with a custom AuthProvider whose onUnauthorized calls ctx.fetchFn(wellKnownUrl).
  2. Reading the UnauthorizedContext.fetchFn JSDoc, they expect X-Gateway-Key on that request.
  3. At runtime, the transport passes this._authFetch = createFetchWithInit(opts?.fetch, opts?.oauthRequestInit) (streamableHttp.ts constructor); oauthRequestInit is unset, so the request goes out with no X-Gateway-Key — the PR's own header-isolation tests (does not apply requestInit headers to authorization requests) pin exactly this.
  4. 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.

Comment on lines +369 to +370
this._redirectPolicy = opts?.redirectPolicy ?? 'manual';
this._authFetch = createFetchWithInit(opts?.fetch, opts?.oauthRequestInit);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.headers

When 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 undefineddiscoverMetadataWithFallback / 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

  1. Browser runtime (CORS_IS_POSSIBLE = true only in shimsBrowser.ts); transport configured with oauthRequestInit: { headers: { 'X-Auth-Gateway': 'v' } } — the exact usage the migration guide recommends for 'gateway headers on well-known endpoints'.
  2. Discovery calls tryMetadataDiscoveryfetchWithCorsRetry(url, { 'MCP-Protocol-Version': ... }, _authFetch). The AS's /.well-known endpoint doesn't list X-Auth-Gateway (or MCP-Protocol-Version) in Access-Control-Allow-Headers → preflight rejected → TypeError.
  3. Retry: fetchFn(url, {}). Inside createFetchWithInit, init?.headers is undefined → merged headers = baseInit.headers = { 'X-Auth-Gateway': 'v' }.
  4. The 'simple' retry preflights on X-Auth-Gateway, rejected again → TypeErrorfetchWithCorsRetry returns undefined.
  5. shouldAttemptFallback sees undefined → 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 the MCP-Protocol-Version header when oauthRequestInit is 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.

Comment thread packages/client/src/client/auth.ts Outdated
Comment on lines +2098 to +2103
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`
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  1. A client calls auth() against an authorization server whose token endpoint sits behind a proxy misconfigured to answer 307 Temporary Redirect with an HTML body.
  2. executeTokenRequest issues the POST with redirect: 'manual'; undici returns the 307 response with a readable body stream.
  3. The guard at auth.ts:2098 matches (status >= 300 && status < 400) and throws Token endpoint responded with a redirect... — the body stream is never read or cancelled.
  4. 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.
  5. 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.
Comment on lines +56 to +63
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 }
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  1. Client configures versionNegotiation: { pin: '2026-07-28' } and points the transport at an http:// URL whose server 308s to https:// (the PR description's own breaking-change shape Better validation of data coming over the wire #1).
  2. Connect runs the server/discover probe → transport.send() POSTs → fetchWithRedirectPolicy sees the 308, throws SdkHttpError(ClientHttpRedirectNotFollowed, ..., { status: 308, statusText }).
  3. normalizeReply hits the instanceof SdkHttpError branch → { kind: 'http-error', status: 308, body: undefined }.
  4. classifyHttpError: parseJsonRpcErrorBody(undefined)undefined{ kind: 'legacy' }.
  5. negotiateEra (versionNegotiation.ts:424-429): pin mode + legacy verdict → throws SdkError(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 set redirectPolicy: '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 an opaqueredirect (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.

Comment on lines +213 to +221
} 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  1. 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).
  2. eventSourceGet throws SdkHttpError with ClientHttpRedirectNotFollowed; the catch matches; reject(error) settles the start() promise (correctly).
  3. this.onerror?.(error) at line 218 throws. Execution exits the catch — line 219's void this.close() never runs. this._eventSource.close() is never called, so readyState stays CONNECTING; _abortController is never aborted; onclose never fires.
  4. The user's thrown error propagates as the custom fetch's rejection. The eventsource package's #onFetchError sees readyState !== CLOSED, dispatches an error event (which routes back into the transport's _eventSource.onerror → wraps into SseError → calls the throwing onerror again, now inside an EventTarget dispatch — surfacing as an uncaught exception in Node), and schedules a reconnect against the same redirecting endpoint.
  5. Each reconnect re-issues the GET, gets the same terminal redirect, re-enters this catch (reject is now a no-op), throws in onerror again, skips close() 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant