-
Notifications
You must be signed in to change notification settings - Fork 2k
fix(client): treat HTTP 401/403 on the negotiation probe as auth failures, not legacy evidence #2564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+909
−91
Merged
fix(client): treat HTTP 401/403 on the negotiation probe as auth failures, not legacy evidence #2564
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': patch | ||
| --- | ||
|
|
||
| The version-negotiation probe no longer misclassifies auth-protected or | ||
| failing servers as legacy. Auth status is never era evidence: a 401 or 403 | ||
| rejection of the `server/discover` probe now surfaces as a typed | ||
| authorization failure — an `SdkHttpError` with code `ClientHttpAuthentication` | ||
| (401) or `ClientHttpForbidden` (403), carrying the HTTP status, reason | ||
| phrase, and response text — instead of triggering the legacy `initialize` | ||
| fallback (which put a doomed `initialize` on the wire) or, under `pin` mode, | ||
| the false "server did not offer pinned protocol version" diagnostic. The | ||
| codes are deliberately not `EraNegotiationFailed`, so era-recovery flows | ||
| keyed on that code cannot persist a verdict for an unauthorized exchange. A | ||
| 5xx rejecting the probe is a server failure and now also rejects typed | ||
| (`SdkHttpError(EraNegotiationFailed)`) instead of demoting a mid-deploy | ||
| modern server to legacy — the legacy fallback now fires only on the 4xx | ||
| shapes the spec licenses. | ||
|
|
||
| With an `authProvider`, a `401` (and a `403` `insufficient_scope` challenge) runs the transport's auth flow first — a plain `403` rejects the same as without a provider — 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /** | ||
| * Auth-seam provenance stamp (internal — not part of the public API). | ||
| * | ||
| * Errors escaping a transport auth seam — the `authProvider.token()` read, | ||
| * an `onUnauthorized` invocation (SDK adapter or custom user callback), the | ||
| * 403 step-up flow, the transport's own auth-failure constructions — are | ||
| * stamped at the throw boundary. The version-negotiation probe routes stamped | ||
| * errors as auth outcomes; provenance is recorded where it is known, never | ||
| * reconstructed from error types downstream. | ||
| * | ||
| * `Symbol.for` uses the global symbol registry, so the stamp survives a | ||
| * duplicated SDK copy in one process (bundler double-install, version skew) | ||
| * by design: both copies resolve the same symbol. | ||
| */ | ||
| const AUTH_SEAM = Symbol.for('mcp.authSeamEscape'); | ||
|
|
||
| /** | ||
| * Stamp `error` as an auth-seam escape and return it — identity-preserving | ||
| * (the same object flows on, `instanceof` and `.cause` chains intact). A | ||
| * frozen/sealed object is returned unstamped rather than replaced: identity | ||
| * outranks provenance. Primitive throws cannot carry the stamp. | ||
| */ | ||
| export function markAuthSeamEscape<T>(error: T): T { | ||
| if ((typeof error === 'object' && error !== null) || typeof error === 'function') { | ||
| try { | ||
| Object.defineProperty(error, AUTH_SEAM, { value: true, configurable: true }); | ||
| } catch { | ||
| // Frozen/sealed: leave unstamped. | ||
| } | ||
| } | ||
| return error; | ||
| } | ||
|
|
||
| /** Whether `error` escaped through a transport auth seam. */ | ||
| export function isAuthSeamEscape(error: unknown): boolean { | ||
| return ( | ||
| ((typeof error === 'object' && error !== null) || typeof error === 'function') && | ||
| (error as Record<PropertyKey, unknown>)[AUTH_SEAM] === true | ||
| ); | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The absolute claim added at docs/advanced/gateway.md:154 — that a 401/403 rejecting the probe always carries
ClientHttpAuthentication/ClientHttpForbiddenand "can never be persisted as an era verdict" — is false over the legacySSEClientTransport: its_sendhas no typed HTTP rejection for a providerless 401 (and no 403 branch at all), so the plainErrorit throws is classified as a network failure and wrapped asSdkError(EraNegotiationFailed), which DOES match the gateway recipe'sEraNegotiationFailed-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: "a401/403rejecting the probe carriesClientHttpAuthentication/ClientHttpForbiddeninstead, 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.mdmakes the same unqualified headline claim ("a 401 or 403 rejection of theserver/discoverprobe now surfaces as … anSdkHttpErrorwith codeClientHttpAuthentication(401) orClientHttpForbidden(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 onresponse.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 genericthrow new Error(Error POSTing to endpoint (HTTP ${response.status}): ${text}). That plainErrorcarries no auth-seam stamp (markAuthSeamEscapeis only applied to thetoken()read,onUnauthorizedescapes, and the typedUnauthorizedError/SdkHttpErrorthrows), is not anUnauthorizedError, and is not anSdkHttpError.The classification chain.
normalizeReply'ssend-errorrow inpackages/client/src/client/versionNegotiation.ts: every clause of the auth disjunction misses (no stamp, wrong class, wrong name), and theSdkHttpErrorbranch misses too — so the error lands in{ kind: 'network-error' }.classifyNetworkErrorinpackages/client/src/client/probeClassifier.tsthen wraps it asSdkError(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 anSSEClientTransportand noauthProvideragainst a server whose GET stream opens but whose POST endpoint sits behind an auth wall (returns 403).detectProbeTransportKindreturns'http'for SSE (nostderr/pidaccessors), so the probing modes run on it. (2) The probe POST gets the 403;_sendhas no 403 branch, so it throws the plainError('Error POSTing to endpoint (HTTP 403): …'). (3)normalizeReply→network-error→classifyNetworkError→SdkError(EraNegotiationFailed). (4) A gateway following the recipe'serror.code === SdkErrorCode.EraNegotiationFailedfilter 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.mdparagraph (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 genericVersion negotiation probe failedconnect 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, soconnect()rejects beforegetDiscoverResult()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/ClientHttpForbiddeninstead (on the Streamable HTTP transport; the legacy SSE client transport surfaces an auth-walled POST as the genericEraNegotiationFailedprobe failure)…".