fix(mcp): fall back to the legacy handshake when server/discover is refused with 401/403 - #2434
Merged
Merged
Conversation
…l report Two independent harnesses for the report that a delivery read against an AdCP 3.1 seller on /v3/mcp arrives with neither Authorization nor x-adcp-auth on 13.0.0-rc.4 (works on 12.0.3). - v3-mcp-auth-header-regression.test.js: real MCP seller stub on /v3/mcp across both protocol eras, with request-signing idle and engaged, that rejects uncredentialed requests the way the reported seller does (HTTP 200 + JSON-RPC -32602). Asserts the credential and agent.headers on all four wire requests of the read: server/discover, tools/list, get_adcp_capabilities, get_media_buy_delivery. - v3-mcp-auth-header-outbound-spy.test.js: no-server counterpart. A transport.fetchFn records outbound headers and answers every request with the seller's rejection. Sees more of the discovery retry surface (8 requests, including the SSE fallback GET and the trailing-slash variant) but never reaches the tool calls, and bypasses the endpoint/era/connection caches because fetchFn forces the scoped-fetch path. Both PASS as committed. The reported configuration alone does not lose the credential on this tree, so this is the baseline to modify until it fails, not a reproduction. Each file's header documents its coverage and blind spots; the scenario table in the first is where a trigger drops in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`versionNegotiation: { mode: 'auto' }` makes the MCP client probe
`server/discover` before `initialize`. Its classifier (`classifyHttpError`)
treats 401/403 as the only probe outcome that never falls back to the legacy
era: 404, 405, an unrecognized JSON-RPC code and a 4xx with a non-JSON-RPC body
all yield a `legacy` verdict, and 5xx yields EraNegotiationFailed. A server that
rejects the modern discovery method while accepting the very same credential on
`initialize` was therefore unreachable, and the caller was told to "provide
auth_token in agent config" while holding a valid one.
Measured against a live seller: `server/discover` returns 401 while
`initialize` and `tools/list` both return 200 with one valid API key. The era
header is not involved; `MCP-Protocol-Version: 2026-07-28` returns 200 when the
method is `initialize`.
This is the 12.0.3 -> 13.0.0-rc.4 behaviour change. 12.0.3 carried MCP client
1.x, which had no `server/discover` probe, so it opened a legacy session first
and a later refusal was survivable. The `is401Error` throw sites in this file
are unchanged; what changed is that they moved onto the critical path.
`createNegotiatedClient` now retries once with `prior: { kind: 'legacy' }` on a
negotiation-time 401/403, the escape hatch the MCP SDK documents for a server
known to be legacy. Fixing the chokepoint rather than the individual catch
blocks matters: other call paths re-probe, and patching only
`probeModernMCPConnection` and `tryListModernMCPTools` left the delivery read
failing. If the credential really is bad, the legacy connect fails on its own
and surfaces the seller's 401, so this does not mask auth errors. It does
re-send the credential to the same origin over a different transport after a
refusal, which is the behaviour change to weigh in review.
Tests
- Add test/lib/mcp-negotiation-401-legacy-fallback.test.js (renamed and
reworked from an earlier draft): 8 cases over 3 controls, 2 cases that are red
without this fix (401 and 403 on `server/discover`), and 3 safety pins that a
rejected credential still fails loudly and the retry stays bounded.
- Delete test/lib/v3-mcp-auth-header-outbound-spy.test.js. It guarded header
attachment, which was never broken, and its own header listed its blind
spots: the canned rejection stopped it before any tool call and
`transport.fetchFn` bypassed the connection caches. The server-backed sibling
covers the same claim better.
- Rewrite the docstring of test/lib/v3-mcp-auth-header-regression.test.js. Its
credential-loss premise was wrong; the era x signing matrix is kept as
ordinary coverage and the header now says so.
Verified: 46 pass / 0 fail across the negotiation and auth suites, 5 / 0 on the
prepublish set. Red/green split confirmed by reverting the fix, rebuilding,
running, and restoring.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…edential Narrows the previous commit. Era negotiation is meant to be automatic, and it is for every probe refusal except 401/403. Those are carved out as terminal, and that carve-out is right in one case and wrong in another: - No credential presented: a probe 401 IS the server's challenge. The caller wants it, and `discoverMCPEndpoint` walks the WWW-Authenticate / RFC 9728 chain to report how to authenticate. Swapping that for a legacy retry that cannot succeed either would lose real information. - OAuth provider configured: a 401 is the provider's cue to refresh, so it has to reach the OAuth machinery untouched. - Static token/header credential already presented, and the server accepts it on other methods: the refusal says nothing about the credential, only about `server/discover`. This is the case where the documented automatic fallback should have run, and the only one we now retry. The retry is therefore gated on `presentedStaticCredential(...)`: an auth header on the probe and no auth provider. Also adds the case that pins the boundary: an uncredentialed client against a seller that 401s everything must still surface AuthenticationRequiredError rather than a legacy retry. Writing it caught a fault in the stub, where the uncredentialed branch outranked reject-everything so no 401 ever reached the client; reject-everything now means every request, credential or not. Longer term this belongs upstream. The MCP client's own documentation for `mode: 'auto'` says "definitive legacy signals (and anything unrecognized) fall back to the plain legacy `initialize` handshake", and a probe refusal from a server that accepts our credential elsewhere is unrecognized by any reasonable reading. If `classifyHttpError` grows the same credential-aware distinction, this retry becomes dead code and should be deleted. Verified: 9 cases in the guard file, 2 red without the fix and green with it, 47 pass / 0 fail across the negotiation and auth suites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The release flow is changesets-driven (`changeset version` -> `sync-version` -> `publish-adcp-release.ts`), so without this the fix would land with no version bump and no published build for consumers to pick up. Patch, not minor: this restores documented `mode: 'auto'` behaviour for a case the classifier carved out, rather than adding surface. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Line-joining only, no semantic change: `git diff -w` is identical to the plain diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The SDK-side probe logic is byte-identical between 12.0.3 (6d645e9) and rc.4 — the `is401Error(error) ... throw` branch has been there since 11.2.0 — so this cannot be asserted as the 12.0.3 -> rc.4 delta on our side. The MCP client moved 2.0.0-beta.4 -> 2.0.0 over the same window, which is where a behaviour change would have to live. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Narrow, well-tested legacy-fallback fix: when a server refuses the server/discover probe with 401/403 while holding a static credential it accepts elsewhere, createNegotiatedClient now retries once into the legacy initialize handshake (prior: { kind: 'legacy' }, skipProbe). Fallback is correctly gated to static credentials — OAuth and uncredentialed probes still propagate the 401 — and two new test files pin the boundary on the wire. Changeset present and correctly scoped patch; witness-not-translator and official-client rules unaffected.
Medium findings
- src/lib/protocols/mcp-modern.ts:350 — EraNegotiationFailed branch doesn't disarm the skipProbe retry bound; a specific malformed-legacy shape can re-enter the probe cycle.
Decision: 1 medium finding, no critical/high. gated_paths is false, so row 2 does not apply. high_risk is true but the reason is a (modified) file with only a single medium finding — row 5 requires a modified high-risk file AND a medium finding; that combination does fire escalation... rechecking: row 5 = high_risk true AND (modified) reason AND any medium finding → escalate.
Medium findings
- src/lib/protocols/mcp-modern.ts:350 — EraNegotiationFailed branch doesn't disarm the skipProbe retry bound
Review finding on #2434: on the `skipProbe` retry `prior` is the synthetic `{ kind: 'legacy' }` and therefore truthy, so an `EraNegotiationFailed` from that connect took the re-probe branch, which resets `skipProbe` to its default — re-probing, taking the same 401, and re-entering the legacy retry a second time per URL. Guard the re-probe with `!skipProbe`, matching the `prior && !skipProbe` shape the surrounding cache-eviction lines already use. Re-probing is only meaningful when `prior` was a stale cached *modern* discovery; after an explicit-legacy attempt fails there is nothing to renegotiate, and throwing surfaces the real error. Reachability, since it decides whether this is a fix or hardening: no server can drive it today — a 5xx legacy `initialize` surfaces as `CLIENT_HTTP_NOT_IMPLEMENTED` and a malformed body as `CLIENT_HTTP_UNEXPECTED_CONTENT`, both measured. But the branch is not dead: `_legacyHandshake` in @modelcontextprotocol/client raises `EraNegotiationFailed` when `supportedProtocolVersions` offers no pre-2026-07-28 version, and again for an unrecognized `prior` shape. Those turn on client construction and the library's error taxonomy, both of which moved in the 2.0.0-beta.4 -> 2.0.0 bump. Adds `failNonDiscoverWith` to the seller stub and a case driving the legacy retry to a non-auth (503) failure — the shape `rejectEverythingWith: 401` cannot express — asserting exactly one probe and at most one legacy attempt per candidate URL. It passes with and without the guard, so it is coverage pinning the observable bound, not a regression test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent review, prior medium resolved, no current findings.
What I checked:
- The prior approve's one outstanding medium (mcp-modern.ts:350 — EraNegotiationFailed branch not disarming the skipProbe retry bound) has been addressed: the re-probe branch in
createNegotiatedClientnow carries!skipProbe, making the explicit-legacy retry terminal instead of re-entering probe→legacy per URL. - Companion coverage test pins the observable bound (one server/discover, at most one legacy initialize per path).
- Changeset present (
.changeset/legacy-fallback-on-probe-auth-refusal.md), consistent with thesrc/lib/**wire-impact requirement. high_riskis true only via a(modified)match onsrc/lib/protocols/mcp-modern.ts; with no medium-or-higher finding on that file, the modification is presumed safe (row 5 does not fire).gated_pathsfalse; no no-auto-approve team match.
No new findings this run. Decision table: rows 1–8 do not fire → row 9 approve.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Why
A seller that answers
401/403to the modernserver/discoverversion-negotiation probe is unreachable, even when it accepts the very same credential oninitializeandtools/list.versionNegotiation: { mode: 'auto' }probesserver/discoverbeforeinitialize. Every other refusal — 404, 405, an unrecognized JSON-RPC code, a 4xx with a non-JSON-RPC body — yields alegacyverdict and falls back cleanly.401/403is the sole outcome the MCP client's classifier treats as terminal. That is correct when we hold no credential (the 401 is the server's challenge and the caller needs it), and wrong when we already presented one: the refusal is a verdict on the discovery method, not on the credential.The user-visible symptom is a false
AuthenticationRequiredErrortelling the caller to "provide auth_token in agent config" while it is already sending a valid one.Isolated before the fix, same seller and same key, one variable:
What Changed
createNegotiatedClientretries once withprior: { kind: 'legacy' }— the escape hatch the MCP SDK documents for a server known to be legacy — skipping the probe and going straight toinitialize.The retry is deliberately narrow, gated on
presentedStaticCredential:WWW-Authenticate/ RFC 9728 discovery walk is unaffected.skipProbebounds it to a single retry.Tests
test/lib/mcp-negotiation-401-legacy-fallback.test.js(9 cases): the two failing scenarios (401 and 403 onserver/discover), plus controls that pass with and without the fix, plus the boundaries that keep the retry honest — a credential rejected everywhere still fails loudly, an uncredentialed client still gets its auth challenge instead of a silent legacy retry, and the retry is asserted at most once on the wire.test/lib/v3-mcp-auth-header-regression.test.js(5 cases) is coverage kept from the original, wrong hypothesis: header attachment across the era × signing matrix, asserted on the wire. It passed before this change and passes after.Full suite: 12,803 pass, 0 fail, 7 skipped. Lint and
tsc --noEmitclean.Reviewer notes
/v3/mcpAdCP 3.1 seller. That hypothesis was disproved — the SDK attachesAuthorization/x-adcp-authon every hop. The bare requests in that report are most likelyprobeAgent401, which fires a deliberately uncredentialedtools/listafter a 401 to read theWWW-Authenticatechallenge. Whether this fix resolves that specific report is not established.@modelcontextprotocol/client, which moved2.0.0-beta.4→2.0.0over the same span (fix(mcp): upgrade modular SDK to 2.0.0 #2413).🤖 Generated with Claude Code