-
Notifications
You must be signed in to change notification settings - Fork 2.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
fix(client): treat HTTP 401/403 on the negotiation probe as auth failures, not legacy evidence #2564
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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`, 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. | ||
|
Check warning on line 28 in .changeset/probe-auth-status-not-era-evidence.md
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -151,6 +151,8 @@ | |
|
|
||
| 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. | ||
|
Check warning on line 154 in docs/advanced/gateway.md
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Extended reasoning...The claim. Why it's false over the legacy SSE transport. In The classification chain. Step-by-step proof. (1) A client uses The PR is internally inconsistent about this. Its own Why this is a nit, not blocking. No code misbehaves — this is prose-only. SSE is deprecated, every gateway.md example uses 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 |
||
|
|
||
| ## Skip the probe for a known-legacy server | ||
|
|
||
| When out-of-band metadata already says the server is pre-2026 — a registry entry, an earlier connection's outcome — an `'auto'`-mode probe is a round trip that fails on every single connect. Supply the negative verdict instead: `PriorDiscovery`'s `{ kind: 'legacy' }` arm skips the probe and goes straight to the `initialize` handshake. | ||
|
|
||
| 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 | ||
| ); | ||
| } |
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 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 leavesdefinePropertythrowing and the error unstamped, so a frozenTypeErrorescaping 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 asSdkError(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 aWeakSetfallback whendefinePropertythrows —WeakSet.addworks 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
TypeErrorthrown 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.mdmakes the same universal claim ("even an untyped crash inside a callback all propagate unchanged"). Butpackages/client/src/client/authSeam.tsdeliberately 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."markAuthSeamEscapestamps viaObject.definePropertyinside atry/catchwhose 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 withisAuthSeamEscape === false.\n\nThe code path that re-opens the exact bug the PR eliminates. For the frozen case, in a browser: (1) aStreamableHTTPClientTransportwith anauthProviderwhose customonUnauthorized(ortoken()) throws a frozenTypeError— 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 → theonUnauthorizedcatch callsmarkAuthSeamEscape, which silently fails to stamp; (3)normalizeReply(versionNegotiation.ts:385-407):isAuthSeamEscapeis false, name is'TypeError'not'UnauthorizedError', notSdkHttpError→network-errorrow; (4)classifyNetworkError(probeClassifier.ts):environment === 'browser' && isOpaqueFetchTypeError(error)— a frozenTypeErroris stillinstanceof TypeError— →{ kind: 'legacy' }. The auth-flow crash is consumed as legacy-era evidence.\n\nStep-by-step consequence in pin mode. (1) Client configuredversionNegotiation: { mode: { pin: '2026-07-28' } }, browser, provider whose callback throwsObject.freeze(new TypeError('re-auth failed')). (2) Probe 401 → callback throws → stamp attempt fails silently. (3) Legacy verdict reachesnegotiateEra's pin arm →connect()rejects withSdkError(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 legacyinitializegoes 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 innormalizeReply, and lands inclassifyNetworkError'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 extensibleErrorinstances:probeAuthSeam.test.tsB12 uses a plainTypeError, and theversionNegotiation.test.tsseam rows pre-stamp extensible errors — the carve-out path is untested. This is also not the earlier-reported allowlist gap (the bare extensibleTypeErrorfrom 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 thedocs/protocol-versions.mdsentence 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-levelWeakSetfallback inmarkAuthSeamEscapewhendefinePropertythrows (WeakSet.prototype.addworks on frozen objects and preserves identity; theSymbol.forcross-bundle rationale is unaffected since stamper and reader live in the same@modelcontextprotocol/clientcopy 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.