fix(client): treat HTTP 401/403 on the negotiation probe as auth failures, not legacy evidence - #2564
Conversation
🦋 Changeset detectedLatest commit: 105e007 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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}
There was a problem hiding this comment.
Beyond the inline findings, I also verified two adjacent concerns and ruled them out: (1) ranking the new 401/403 row ahead of the JSON-RPC body parse in classifyHttpError does not reroute the deployed legacy first-contact shapes — both wire-real -32000 bodies arrive as HTTP 400 and still classify legacy per the fixture corpus; (2) moving the auth-flow check ahead of the generic SdkHttpError branch in normalizeReply only redirects the ClientHttpAuthentication/ClientHttpForbidden codes — all other SdkHttpError codes still reach the http-error row unchanged (plus the new statusText).
Extended reasoning...
Bugs were found and posted inline (the missed OAuthError in the send-error disjunction, plus a docs-consistency nit), so this note is informational only. I independently confirmed the inline bug's premise: OAuthError is branded mcp.OAuthError and is a sibling of OAuthClientFlowError, and auth()'s refresh catch (auth.ts:1327-1331) re-throws it raw for codes like invalid_grant, so it falls through the new disjunction to the generic network-error wrap, contradicting the changeset's 'always propagates unchanged' claim. Separately I checked the two structural risks of the diff — classifier row ordering vs the deployed 400/-32000 legacy shapes, and the normalizeReply branch reordering — and both are behavior-preserving for non-auth statuses.
a22fe96 to
a915712
Compare
There was a problem hiding this comment.
Beyond the two inline nits, one behavior-change candidate was examined and ruled out this run: a bare 403 with no auth evidence (e.g. a WAF/proxy denying the probe's 2026-only headers) now hard-fails instead of falling back to legacy — this is the PR's stated design (auth status is never era evidence), is covered by the bare-403 fixture row, and matches what a non-probing client surfaces for the same server, so it was judged intentional, not a regression.
Extended reasoning...
Bugs were found (two nits: an untyped-auth-flow-error gap that is strictly narrower than main's behavior with only the changeset prose overstating, and a stale example comment in host.ts), so the inline comments already signal the review outcome. This note only records the one additional candidate a finder raised and a verifier refuted — the bare-403 legacy-fallback-removal concern — so a later pass or human reviewer does not re-derive it from scratch. No approval: the PR touches the client's auth-flow error classification and version-negotiation engine, which warrants a human look regardless of the nit-level findings.
a915712 to
bc4ce30
Compare
There was a problem hiding this comment.
Beyond the inline findings, this pass also examined whether classifyHttpError's new 403 arm misbehaves for challenge-less 403s from non-auth middleboxes (WAF/API gateway) — ruled out: the typed SdkHttpError(EraNegotiationFailed) carrying the status is the intended outcome there too (auth status is never era evidence, and it matches what a non-probing client surfaces for the same server), so no separate challenge-gated branch is needed.
Extended reasoning...
This run's three findings are posted inline (the rethrowTypedAuthFlowError cross-mode rebranding of user-provider-callback errors, and two doc-accuracy nits on the with-provider 403 sentence and the troubleshooting.md message-tail enumeration). Separately, a finder raised the concern that the new 401/403 classifier row fires on challenge-less 403s from middleboxes that are not auth layers; verification concluded this is intentional and consistent — the PR's design principle is that a 403 rejecting the probe is never era evidence regardless of its origin, and the resulting typed error names the status exactly as a legacy-mode (non-probing) client would surface it. Recording it here so a later pass does not re-explore it. All findings from the two prior review rounds were verified as addressed at HEAD (OAuthError in the isAuthFlowError disjunction, the insufficient_scope doc qualification, rethrowTypedAuthFlowError for untyped DCR escapes, and the host.ts unwrap cleanup).
…are never legacy evidence A 401 (without an authProvider) or 403 rejection of the connect-time server/discover probe fell into the classifier's conservative legacy fallback: auto mode sent a doomed legacy initialize, and pin mode reported "the server did not offer pinned protocol version ..." for a server that was never asked. Auth status is not era evidence — and neither is a 5xx from a mid-deploy server, which took the same fallback. classifyHttpError now has explicit 401/403 and 5xx rows ahead of the JSON-RPC body parse: typed SdkHttpErrors carrying the status, reason phrase, and response text — ClientHttpAuthentication/ClientHttpForbidden for the auth walls (deliberately not EraNegotiationFailed, so era-recovery flows keyed on that code can never persist a verdict for an unauthorized exchange) and EraNegotiationFailed for 5xx. The legacy fallback now fires only on the 4xx shapes the spec licenses. Auth-flow provenance is recorded at the throw boundary, never reconstructed from error types: the HTTP transports stamp every error escaping an auth seam (the token() read, onUnauthorized invocations including custom callbacks, the 403 step-up flow, and their own auth-failure constructions) with a global-registry symbol, and the probe routes stamped errors as auth outcomes, propagating them unchanged with identity intact — a TypeError from a DCR sub-fetch or a custom callback can no longer fall into the probe's browser CORS heuristic and read as legacy-era evidence. Fixes #2561
df555bb to
27c2eb0
Compare
fb58c4f to
105e007
Compare
| With an `authProvider`, the transport's auth flow runs first and whatever | ||
| escapes it propagates unchanged, identity intact: the HTTP transports stamp | ||
| errors at their auth seams (the `token()` read, `onUnauthorized` including | ||
| custom callbacks, the 403 step-up flow, and their own auth-failure | ||
| constructions), so `UnauthorizedError` for `finishAuth()`, the flow's typed | ||
| failures (`OAuthError`, `InsufficientScopeError`, the | ||
| 401-after-re-authentication diagnostic), and even an untyped `TypeError` | ||
| thrown inside the flow all reach the caller as thrown — never rewrapped, | ||
| never consumed by the probe's browser CORS heuristic as legacy-era evidence. |
There was a problem hiding this comment.
🟡 The last paragraph's universal claim — auth-flow errors are "never rewrapped, never consumed by the probe's browser CORS heuristic as legacy-era evidence" (echoed in docs/protocol-versions.md) — is contradicted by markAuthSeamEscape's own documented carve-outs: a frozen/sealed error leaves defineProperty throwing and the error unstamped, so a frozen TypeError escaping a user auth callback in a browser still takes the CORS row and yields a legacy verdict (resurrecting the false pin-mode diagnostic), and a primitive throw cannot carry the stamp and gets rewrapped as SdkError(EraNegotiationFailed). Fix: qualify this sentence and the docs/protocol-versions.md one (frozen/sealed objects and primitive throws cannot carry the provenance stamp), or back the stamp with a WeakSet fallback when defineProperty throws — WeakSet.add works on frozen objects and preserves identity.
Extended reasoning...
The claim vs. the mechanism's own carve-outs. The changeset's last paragraph (lines 20–28) states that everything escaping a stamped auth seam — "even an untyped TypeError thrown inside the flow" — reaches the caller "as thrown — never rewrapped, never consumed by the probe's browser CORS heuristic as legacy-era evidence." docs/protocol-versions.md makes the same universal claim ("even an untyped crash inside a callback all propagate unchanged"). But packages/client/src/client/authSeam.ts deliberately documents two exceptions in its own JSDoc: "A frozen/sealed object is returned unstamped rather than replaced: identity outranks provenance. Primitive throws cannot carry the stamp." markAuthSeamEscape stamps via Object.defineProperty inside a try/catch whose catch swallows the failure and returns the error unstamped — verified empirically: Object.freeze(new TypeError(...)), Object.seal(...), Object.preventExtensions(...), and a thrown string all come back with isAuthSeamEscape === false.\n\nThe code path that re-opens the exact bug the PR eliminates. For the frozen case, in a browser: (1) a StreamableHTTPClientTransport with an authProvider whose custom onUnauthorized (or token()) throws a frozen TypeError — e.g. a SES/hardened environment or a library that freezes its error singletons; (2) the probe POST gets a readable 401 → _send's 401 branch → the onUnauthorized catch calls markAuthSeamEscape, which silently fails to stamp; (3) normalizeReply (versionNegotiation.ts:385-407): isAuthSeamEscape is false, name is 'TypeError' not 'UnauthorizedError', not SdkHttpError → network-error row; (4) classifyNetworkError (probeClassifier.ts): environment === 'browser' && isOpaqueFetchTypeError(error) — a frozen TypeError is still instanceof TypeError — → { kind: 'legacy' }. The auth-flow crash is consumed as legacy-era evidence.\n\nStep-by-step consequence in pin mode. (1) Client configured versionNegotiation: { mode: { pin: '2026-07-28' } }, browser, provider whose callback throws Object.freeze(new TypeError('re-auth failed')). (2) Probe 401 → callback throws → stamp attempt fails silently. (3) Legacy verdict reaches negotiateEra's pin arm → connect() rejects with SdkError(EraNegotiationFailed, 'Version negotiation failed: the server did not offer pinned protocol version 2026-07-28 via server/discover (no fallback in pin mode)') — byte-for-byte the false diagnostic this PR's motivation quotes as the bug being fixed. In auto mode the doomed legacy initialize goes on the wire and the whole auth flow re-runs (second discovery/DCR round, second consent side effects).\n\nThe primitive case contradicts the other half of the sentence. throw 'oops' from a user callback cannot be stamped, matches no auth row in normalizeReply, and lands in classifyNetworkError's wrap: SdkError(EraNegotiationFailed, 'Version negotiation probe failed: oops', { cause }) — contradicting "never rewrapped" / "reach the caller as thrown" (though at least not a legacy verdict).\n\nWhy existing coverage misses it. The PR's tests only exercise extensible Error instances: probeAuthSeam.test.ts B12 uses a plain TypeError, and the versionNegotiation.test.ts seam rows pre-stamp extensible errors — the carve-out path is untested. This is also not the earlier-reported allowlist gap (the bare extensible TypeError from the DCR sub-fetch, which the seam stamps now fix): it is the residual escape hatch the new mechanism itself defines, intentionally per the code comments, while the changeset and docs deny it exists — the repo's "Documentation & Changesets" recurring catch (prose promising behavior the diff doesn't back).\n\nFix. Either (a) qualify the changeset sentence and the docs/protocol-versions.md sentence with one clause — e.g. "unless the thrown value is frozen/sealed or a primitive, which cannot carry the provenance stamp" — or (b) close the frozen-object hole with a module-level WeakSet fallback in markAuthSeamEscape when defineProperty throws (WeakSet.prototype.add works on frozen objects and preserves identity; the Symbol.for cross-bundle rationale is unaffected since stamper and reader live in the same @modelcontextprotocol/client copy in the common case). Severity is nit because the trigger — a frozen/sealed error or primitive throw escaping a user auth callback, in a browser, under a probing negotiation mode — is exotic, the identity-over-provenance tradeoff is defensible, and nothing breaks in realistic usage.
| // HTTP-rejected probes carry their JSON-RPC error in the response body — classify it like an in-band error. | ||
| const rpcError = parseJsonRpcErrorBody(outcome.body); | ||
| if (rpcError !== undefined) { | ||
| return classifyRpcError(rpcError, context); | ||
| } | ||
| // Unparseable or unrecognized HTTP rejection: conservative legacy fallback. | ||
| // Unparseable or unrecognized 4xx rejection: conservative legacy fallback. | ||
| // With the auth and 5xx rows above, this row's legacy set now equals the | ||
| // spec-licensed set exactly — the 4xx a deployed 2025 server answers a | ||
| // request it does not recognize. | ||
| return { kind: 'legacy' }; |
There was a problem hiding this comment.
🟡 The new fallthrough comment claims this row's legacy set "now equals the spec-licensed set exactly" (and the changeset says the legacy fallback "now fires only on the 4xx shapes the spec licenses"), but 429 (rate limiter), 408 (proxy timeout), and 407 (proxy auth) still land on this legacy row — they are infrastructure statuses a deployed 2025 server never emits for an unrecognized method, exactly the class the new 5xx row was added to exclude, and 407 is an auth status, contradicting the PR's headline invariant verbatim. A modern server behind a standard rate limiter whose probe gets 429 is still silently demoted to legacy in auto mode (and in pin mode still produces the false "did not offer pinned protocol version" diagnostic this PR's motivation quotes). Behavior is unchanged from main — the PR strictly narrows the false-legacy set — so this is not blocking, but consider a typed row for 429/408 (like 5xx), routing 407 with the auth statuses, or at minimum softening the exactness claim in the comment and changeset.
Extended reasoning...
The claim vs. the code. The rewritten classifyHttpError (packages/client/src/client/probeClassifier.ts:246-300) has three rows — 401/403 → typed auth failure, >= 500 → typed SdkHttpError(EraNegotiationFailed), and a final fallthrough whose new comment (lines 296-299) states: "With the auth and 5xx rows above, this row's legacy set now equals the spec-licensed set exactly — the 4xx a deployed 2025 server answers a request it does not recognize." The changeset makes the same claim ("the legacy fallback now fires only on the 4xx shapes the spec licenses"). Both are false under the PR's own definition: 429, 408, and 407 are in the row's legacy set, but they are never what a 2025 server answers an unrecognized method with — deployed servers answer 400/-32000 (pinned byte-exact by this repo's own wire-real integration fixtures). They come from infrastructure — a rate limiter or API gateway (429), a proxy timeout (408), a proxy auth wall (407) — exactly the class the 5xx row's comment says is "never era evidence". And 407 is an auth status, so the changeset's headline invariant "Auth status is never era evidence" is contradicted verbatim: 407 still selects the legacy era.\n\nStep-by-step proof (429, the realistic trigger). (1) A modern (2026-07-28) MCP server sits behind a standard rate limiter (Cloudflare, AWS API Gateway, nginx limit_req); the client connects with versionNegotiation: { mode: 'auto' }. (2) A reconnect storm or per-IP burst makes the probe POST answer 429 with the limiter's HTML/JSON error page. (3) In StreamableHTTPClientTransport._send, 429 hits no auth branch — not the provider-gated 401 path, not a 403 insufficient_scope challenge, not the modern-400 in-band case — so it falls to the generic throw new SdkHttpError(ClientHttpNotImplemented, ..., { status: 429, text }) with no markAuthSeamEscape stamp (no auth seam ran). (4) normalizeReply (versionNegotiation.ts:385-412): not a stamped auth escape, not UnauthorizedError → the SdkHttpError row → { kind: 'http-error', status: 429 }. (5) classifyHttpError: not 401/403, not >= 500, and the limiter's body fails parseJsonRpcErrorBody → { kind: 'legacy' }. (6a) Auto mode: the client sends the legacy initialize; if the limiter's token bucket has refilled (probe and initialize are separate requests), the modern server — which serves legacy initialize for backward compat — is silently pinned to the legacy era for the whole session, with 2026-era surfaces (subscriptions/listen, server/discover capabilities) silently unavailable; a host persisting the verdict via PriorDiscovery then caches legacy "silently forever" (that type's own JSDoc warning). (6b) Pin mode: the legacy verdict makes negotiateEra throw the byte-for-byte "the server did not offer pinned protocol version ... via server/discover (no fallback in pin mode)" diagnostic — the exact factually-wrong message this PR's own motivation quotes as the bug being fixed, now produced by a transient rate limit.\n\nWhy nothing else catches it. The 5xx row is status >= 500 only; the JSON-RPC body parse cannot rescue these statuses (429/408/407 bodies are infrastructure pages, not JSON-RPC); no auth seam runs, so no stamp applies; and the new fixture corpus (probeFixtureCorpus.test.ts) covers only 400/401/403/500/502/503 — no transient-4xx row is pinned, so the gap is invisible to the test suite.\n\nWhy this belongs to this PR despite the behavior predating it. The PR rewrote this exact function, applied the rationale "infrastructure failure is never era evidence" to 5xx while leaving 429/408 (statuses emitted by the same infrastructure class), and shipped new prose asserting completeness. That is a partial migration inside the very function being fixed — the repo's REVIEW.md Completeness catch ("when a PR replaces a pattern, grep for surviving instances of the old form") — and the exactness comment/changeset are prose the diff doesn't back — the Documentation & Changesets catch ("read added .changeset/*.md text and new inline comments against the implementation in the same diff").\n\nWhy this is a nit, not blocking. Pre-PR classifyHttpError sent all unrecognized HTTP rejections — including 429/408/407 and 5xx — to legacy, so this PR strictly narrows the false-legacy set and regresses nothing; merging as-is makes nothing worse at runtime. Mitigating factors verifiers noted: 407/408 triggers are exotic in fetch-based clients, and in auto mode a rate-limited server frequently 429s the follow-up initialize too, narrowing the silent-demotion window. One verifier argued for blocking on the ground that the changeset affirmatively tells consumers this misclassification class is fixed; the majority view (which I adopt) is that the introduced defect is overclaiming prose plus an incomplete extension, not a regression — the prose is what's wrong, not newly-shipped code behavior.\n\nFix. Either (a) add a typed row for 429 (and 408) parallel to the 5xx row — SdkHttpError(EraNegotiationFailed) is arguably right since retry is the remedy — and route 407 with the 401/403 auth statuses per the PR's own invariant, plus fixture rows pinning them; or (b) at minimum, soften the comment (e.g. "…now approximates the spec-licensed set; transient/proxy 4xx such as 429/408/407 still fall through conservatively") and the changeset's "fires only on the 4xx shapes the spec licenses" sentence so the shipped prose matches the shipped classifier.
|
|
||
| Replace the persisted blob with the fresh `getDiscoverResult()` and the rest of the fleet recovers on its next read. | ||
|
|
||
| The `EraNegotiationFailed` filter above is deliberate and safe against auth walls: a `401`/`403` rejecting the probe carries `ClientHttpAuthentication`/`ClientHttpForbidden` instead, so an unauthorized exchange re-throws out of this recovery path and can never be persisted as an era verdict. |
There was a problem hiding this comment.
🟡 The absolute claim added at docs/advanced/gateway.md:154 — that a 401/403 rejecting the probe always carries ClientHttpAuthentication/ClientHttpForbidden and "can never be persisted as an era verdict" — is false over the legacy SSEClientTransport: its _send has no typed HTTP rejection for a providerless 401 (and no 403 branch at all), so the plain Error it throws is classified as a network failure and wrapped as SdkError(EraNegotiationFailed), which DOES match the gateway recipe's EraNegotiationFailed-keyed filter. The PR's own docs/protocol-versions.md paragraph already concedes exactly this SSE caveat, contradicting gateway.md and the changeset headline. Fix: qualify the gateway.md sentence (and the changeset) with the same Streamable-HTTP-only caveat protocol-versions.md carries.
Extended reasoning...
The claim. docs/advanced/gateway.md:154 (added by this PR) states: "a 401/403 rejecting the probe carries ClientHttpAuthentication/ClientHttpForbidden instead, so an unauthorized exchange re-throws out of this recovery path and can never be persisted as an era verdict." The changeset .changeset/probe-auth-status-not-era-evidence.md makes the same unqualified headline claim ("a 401 or 403 rejection of the server/discover probe now surfaces as … an SdkHttpError with code ClientHttpAuthentication (401) or ClientHttpForbidden (403)").
Why it's false over the legacy SSE transport. In packages/client/src/client/sse.ts, SSEClientTransport._send's only auth branch is gated on response.status === 401 && this._authProvider, and there is no 403 branch at all. A providerless 401 — or any 403 — on the probe POST falls through to the generic throw new Error(Error POSTing to endpoint (HTTP ${response.status}): ${text}). That plain Error carries no auth-seam stamp (markAuthSeamEscape is only applied to the token() read, onUnauthorized escapes, and the typed UnauthorizedError/SdkHttpError throws), is not an UnauthorizedError, and is not an SdkHttpError.
The classification chain. normalizeReply's send-error row in packages/client/src/client/versionNegotiation.ts: every clause of the auth disjunction misses (no stamp, wrong class, wrong name), and the SdkHttpError branch misses too — so the error lands in { kind: 'network-error' }. classifyNetworkError in packages/client/src/client/probeClassifier.ts then wraps it as SdkError(SdkErrorCode.EraNegotiationFailed, 'Version negotiation probe failed: …'). That is precisely the code the gateway recipe's recovery filter keys on — the exact outcome gateway.md:154 says "can never" happen.
Step-by-step proof. (1) A client uses versionNegotiation: { mode: 'auto' } with an SSEClientTransport and no authProvider against a server whose GET stream opens but whose POST endpoint sits behind an auth wall (returns 403). detectProbeTransportKind returns 'http' for SSE (no stderr/pid accessors), so the probing modes run on it. (2) The probe POST gets the 403; _send has no 403 branch, so it throws the plain Error('Error POSTing to endpoint (HTTP 403): …'). (3) normalizeReply → network-error → classifyNetworkError → SdkError(EraNegotiationFailed). (4) A gateway following the recipe's error.code === SdkErrorCode.EraNegotiationFailed filter enters the recovery path for an unauthorized exchange — contradicting the sentence's "re-throws out of this recovery path".
The PR is internally inconsistent about this. Its own docs/protocol-versions.md paragraph (same diff) explicitly concedes the gap: "the legacy SSE client transport reports a non-2xx POST as a generic error, so over SSE a probe rejection surfaces as the generic Version negotiation probe failed connect error instead." gateway.md:154 and the changeset omit that qualification while stating the guarantee absolutely.
Why this is a nit, not blocking. No code misbehaves — this is prose-only. SSE is deprecated, every gateway.md example uses StreamableHTTPClientTransport, and even in the SSE scenario the ultimate hazard does not materialize: the recovery branch's re-probe fails against the same auth wall, so connect() rejects before getDiscoverResult() can be read and no wrong verdict is persisted. This matches the repo's Documentation & Changesets recurring catch (prose promising behavior the diff doesn't back).
Fix. Add the same one-clause transport qualification protocol-versions.md already carries to both gateway.md:154 and the changeset headline — e.g. "…carries ClientHttpAuthentication/ClientHttpForbidden instead (on the Streamable HTTP transport; the legacy SSE client transport surfaces an auth-walled POST as the generic EraNegotiationFailed probe failure)…".
| this._authProvider.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._fetchWithInit }).then( | ||
| // onUnauthorized succeeded → retry fresh. Its onerror handles its own onerror?.() + reject. | ||
| () => this._startOrAuth().then(resolve, reject), | ||
| // onUnauthorized failed → not yet reported. | ||
| error => { | ||
| this.onerror?.(error); | ||
| // onUnauthorized failed → not yet reported. Auth-seam | ||
| // stamp: covers the SDK's OAuth flow and custom | ||
| // callbacks alike. | ||
| (error: unknown) => { | ||
| markAuthSeamEscape(error); | ||
| this.onerror?.(error as Error); | ||
| reject(error); | ||
| } | ||
| ); | ||
| return; | ||
| } | ||
| const error = new UnauthorizedError(); | ||
| const error = markAuthSeamEscape(new UnauthorizedError()); | ||
| reject(error); |
There was a problem hiding this comment.
🟣 Pre-existing issue (the recursive call is unchanged from main, this PR only stamps the adjacent failure arm): SSEClientTransport._startOrAuth retries a 401 by recursing into itself with no isAuthRetry cap, so an authProvider whose onUnauthorized resolves while the server keeps answering 401 (e.g. { token: async () => 'stale', onUnauthorized: async () => {} } — the exact shape this PR's integration tests use over streamableHttp — or an OAuth refresh that succeeds at the AS while the RS keeps rejecting) makes start()/connect() loop forever with a full auth-flow re-run per cycle. All three sibling 401 paths (sse._send, streamableHttp._send, streamableHttp._startOrAuthSse) gate on !isAuthRetry and throw the stamped SdkHttpError(ClientHttpAuthentication, 'Server returned 401 after re-authentication') on the second 401; threading the same flag through _startOrAuth closes the one remaining unbounded path.
Extended reasoning...
The bug. _startOrAuth's onerror 401 arm handles a resolving onUnauthorized by recursing: () => this._startOrAuth().then(resolve, reject) (packages/client/src/client/sse.ts:222). Unlike the other three 401-retry paths in the HTTP client transports, this method takes no isAuthRetry flag or retry counter. The fresh EventSource's fetch wrapper re-captures _last401Response on every 401 response (sse.ts:200-207), so the guard at line 216 (this._authProvider.onUnauthorized && this._last401Response) is true again on every iteration — while onUnauthorized keeps resolving, the recursion never terminates.\n\nWhy onUnauthorized can keep resolving while the server keeps 401ing. Two realistic shapes: (1) the trivial custom provider { token: async () => 'stale', onUnauthorized: async () => {} } — byte-for-byte the provider shape this PR's own integration test uses over streamableHttp (test/integration/test/client/versionNegotiation.test.ts, the "onUnauthorized re-auth that does not help" leg) — resolves unconditionally; (2) an OAuthClientProvider whose adapter path handleOAuthUnauthorized → auth() resolves whenever the refresh succeeds at the authorization server, while the resource server still rejects the refreshed token (audience mismatch, RS-side revocation). In case (2) every loop iteration runs a full auth() refresh round-trip against the AS before reopening the stream — unbounded network churn against two servers, and connect() never settles.\n\nStep-by-step proof. (1) new SSEClientTransport(url, { authProvider: { token: async () => 'stale', onUnauthorized: async () => {} } }); server answers every GET 401. (2) start() → _startOrAuth() → EventSource fetch wrapper sees 401, sets _last401Response (sse.ts:200). (3) onerror fires with event.code === 401; guard at line 216 passes (onUnauthorized set, _last401Response set); _last401Response is cleared, the EventSource closed, onUnauthorized awaited — it resolves. (4) Line 222 recurses into _startOrAuth(): a fresh EventSource, whose fetch wrapper re-sets _last401Response on the next 401. (5) Steps 2–4 repeat forever; the outer promise never settles, so start(), Client.connect(), and — under versionNegotiation over SSE — ProbeWindow.open() all hang. Note probe.timeoutMs bounds only exchange(), not start(), so the negotiation probe path has no timeout at all here.\n\nWhy this is the lone inconsistent path. The other three 401 handlers are all bounded: sse._send(message, isAuthRetry) (sse.ts:375-397), streamableHttp._send, and streamableHttp._startOrAuthSse each gate the onUnauthorized retry on !isAuthRetry and, on the second 401, throw the stamped SdkHttpError(SdkErrorCode.ClientHttpAuthentication, 'Server returned 401 after re-authentication'). _startOrAuth is the only one with no cap. It also contradicts two pieces of prose: the authProvider JSDoc in this same file (sse.ts:74-77: "the request is retried once. If the retry also gets 401 … UnauthorizedError is thrown"), and this PR's changeset/docs claim that "the 401-after-re-authentication diagnostic … reach[es] the caller as thrown" — on this seam the diagnostic is never constructed, because the PR stamps this handler's failure arm (sse.ts:225-231) while the success arm loops.\n\nWhy pre-existing. The recursive success arm () => this._startOrAuth().then(resolve, reject) is byte-identical on main — this PR only adds markAuthSeamEscape stamps to the adjacent lines — so nothing regresses, and the SSE transport is deprecated. But the PR edits exactly this handler and ships the bounded-retry diagnostic as its headline contract, so per the repo's Completeness convention ("when a PR replaces a pattern … flag every leftover site") this is the natural moment to close the one remaining unbounded path.\n\nFix. Thread an isAuthRetry flag through _startOrAuth like the other three paths: on the retry's 401, skip onUnauthorized and throw the stamped SdkHttpError(ClientHttpAuthentication, 'Server returned 401 after re-authentication') (matching sse._send's shape), so the SSE start path finally produces the typed diagnostic the JSDoc and changeset promise instead of hanging.
1 participant
The connect-time version-negotiation probe classified HTTP 401/403 rejections (and 5xx failures) as legacy-era evidence.
Motivation and Context
A client using
versionNegotiation: { mode: 'auto' }(or a pin) against an OAuth-protected server, with noauthProviderconfigured, gets a wrong-era diagnosis instead of the auth challenge: the probe's 401 falls into the classifier's conservative legacy fallback, so the client sends a doomed legacyinitialize, and in pin modeconnect()rejects with "the server did not offer pinned protocol version … via server/discover" — factually wrong, since the server refused at the auth layer and was never asked anything. A 403 denial and a 5xx (mid-deploy proxy, crashed backend) took the same path. Auth status — and server failure — is never era evidence.Fix, in three parts:
classifyHttpErrorgets explicit rows ahead of the JSON-RPC body parse: 401/403 reject typed asSdkHttpErrorwith codeClientHttpAuthentication/ClientHttpForbiddencarrying the status, reason phrase, and response text. The codes are deliberately notEraNegotiationFailed, so era-recovery flows keyed on that code (the gateway guide's cached-verdict recipe) can never persist a legacy verdict for an unauthorized exchange. A 5xx rejects typed asSdkHttpError(EraNegotiationFailed)— the legacy fallback now fires only on the 4xx shapes the spec licenses.token()read,onUnauthorizedinvocations including custom callbacks, the 403 step-up flow, and their own auth-failure constructions) with a global-registry symbol, and the probe routes stamped errors as auth outcomes, propagating them unchanged with identity intact. ATypeErrorfrom a DCR sub-fetch or a custom callback can no longer fall into the probe's browser CORS heuristic and read as legacy-era evidence.authProvidernothing changes for the happy path:UnauthorizedErrorstill propagates forfinishAuth(), and the flow's typed failures (OAuthError,InsufficientScopeError, the 401-after-re-authentication diagnostic) reach the caller as thrown.Fixes #2561
How Has This Been Tested?
TypeErrorinjected at each stamped seam (DCR POST,token()read, customonUnauthorized, user provider callback insideauth()) propagates identity-preserved as an auth outcome, never a legacy verdict, in node and browser environments; a control error outside the seams keeps the typed network wrap; a non-probe-path callback error propagates raw.onUnauthorizedre-auth legs), asserting noinitializeever reaches the wire.mode: 'auto'— probe 401'd,UnauthorizedErrorpropagated,finishAuth, reconnect re-probes with the token, the legacy rejection supplies the era evidence,initializeandtools/callsucceed (wire order asserted).check:all/test:all(unit + integration + e2e) / examples matrix pass.Breaking Changes
None.
connect()against a protected or failing server already rejected — it now rejects at the probe with the status named and the right code, without first putting a doomedinitializeon the wire.Types of changes
Checklist
Additional context
docs/protocol-versions.mddocuments the auth-status and 5xx behavior next to the existing probe-failure rows (scoped to the transports that surface typed HTTP rejections),docs/troubleshooting.mdcarries the two new message strings with their codes, and the gateway guide notes why itsEraNegotiationFailedfilter is safe against auth walls. A changeset (patch,@modelcontextprotocol/client) is included. The newauthSeammodule is internal (not exported); the stamp usesSymbol.forso it survives duplicated SDK copies in one process.