Skip to content

fix(core): start OAuth on a modern/auto-era connect (#1805) - #1806

Closed
rinormaloku wants to merge 1 commit into
modelcontextprotocol:v2/mainfrom
rinormaloku:fix/1805-era-probe-hides-auth-challenge
Closed

fix(core): start OAuth on a modern/auto-era connect (#1805)#1806
rinormaloku wants to merge 1 commit into
modelcontextprotocol:v2/mainfrom
rinormaloku:fix/1805-era-probe-hides-auth-challenge

Conversation

@rinormaloku

@rinormaloku rinormaloku commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Closes #1805

Connecting to an OAuth-protected HTTP server with Protocol Era = Auto or Modern never started the authorization flow. The same server authorized fine on Legacy, and the user saw a red toast instead of the OAuth redirect:

Failed to connect to "<server>": Version negotiation probe failed: Interactive auth recovery required

Root cause

Era auto/modern sends the SDK's server/discover negotiation probe before anything else, so the probe — not initialize — is where authorization first surfaces. Its classifier reports whatever the transport threw as SdkError(ERA_NEGOTIATION_FAILED), keeping the real error at data.cause. That buried both connect-time auth signals:

  • Remote path (web). The backend always sets interceptAuthChallenges: true, so a 401 becomes AuthChallengeErrorauth_challengeRemoteClientTransport rejects with AuthRecoveryRequiredError (carrying the authorization URL). Wrapped, it no longer matched err instanceof AuthRecoveryRequiredError in App.tsx, and isUnauthorizedError() couldn't see it either (no status/code). It also flipped connection status to error, which isConnectAuthRecoveryError exists to prevent.
  • Direct path (CLI/TUI). No stored tokens means no authProvider, so no interception, so the 401 reached the SDK as a raw SdkHttpError. The classifier ignores the HTTP status — it only looks for a JSON-RPC error body — and verdicts "not a modern server"; pin mode then rethrew that with the 401 discarded entirely: no status, not even a cause.

Reproduced against a live dual-era server answering 401 on both initialize and server/discover:

era error out of client.connect() recovery matched?
legacy AuthRecoveryRequiredError ✅ redirect
auto SdkError / ERA_NEGOTIATION_FAILED ❌ generic toast
modern (pin) SdkError / ERA_NEGOTIATION_FAILED ❌ generic toast

core/auth/utils.ts already anticipated this wrapper shape but only walked the chain for UnauthorizedError — not the two typed errors the Inspector's own paths produce.

Not caused by the 2026-07-28 authorization SEPs (SEP-2468 iss, SEP-837 application_type, SEP-2352 issuer binding, DCR deprecated in favour of CIMD): legacy-era authorization against the same server worked.

Changes

  • core/auth/challenge.tsfindNestedAuthError() walks cause / data.cause for a nested AuthRecoveryRequiredError / AuthChallengeError. Deliberately not gated on the SDK's error code or message, so it survives SDK rewording.
  • core/mcp/inspectorClient.tsconnect() unwraps the client.connect() rejection at the earliest point: ahead of withDirectAuthRecovery (whose isAuthChallengeError check is shallow), the outer status guard, and every client's connect-error handling. Web, CLI, and TUI are all fixed with no client-side change.
  • core/mcp/inspectorClient.ts — on the direct path, enable interceptAuthChallenges for the probing eras even with no stored tokens, so the 401 becomes a typed AuthChallengeError that survives the probe as data.cause instead of being silently reclassified.

After the fix, against the same live server: auto and modern reject with AuthRecoveryRequiredError carrying a real authorization URL.

Tests

  • clients/web/src/test/core/auth/challenge.test.ts — 8 cases over the walk (both link names, multi-level, already-typed, absent, non-object/null data, cyclic chain).
  • clients/web/src/test/core/mcp/inspectorClient-era-probe-auth.test.tsconnect() surfaces each typed error on auto/modern, leaves a non-auth probe failure untouched, and keeps the legacy baseline; plus probesProtocolEra().
  • clients/web/src/test/integration/mcp/inspectorClient-modern-era-oauth.test.ts — live modern-era server behind requireAuth: connect() rejects with a recoverable auth error (not a negotiation failure) on both probing eras, and connects reporting the modern era once authorization completes.

Each half of the fix is load-bearing — reverting either one fails the new integration tests. npm run ci is green (4,714 tests; per-file ≥90 coverage gate passed).

Follow-ups (not in this PR)

  • Upstream SDK: classifyHttpError should treat HTTP 401/403 on the probe as auth-required rather than absence of modern evidence, and pin mode should not drop the status.
  • A separate callback-leg failure exists on this path: when a pending authorization has no recorded discovery state, the SDK's SEP-2352 binding check throws AuthorizationServerMismatchError ("discoveryState was not available on the callback leg") and the Inspector dead-ends instead of restarting authorization. Clearing OAuth state works around it. Needs its own issue.

🤖 Generated with Claude Code

https://claude.ai/code/session_013SNjAqoEhUVVkr3UStR6om

…ocol#1805)

The SDK's `server/discover` negotiation probe runs before anything else on
`protocolEra: "auto" | "modern"`, and its classifier reports whatever the
transport threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the real error moved
to `data.cause`. That buried both connect-time auth signals: the remote path's
`AuthRecoveryRequiredError` (matched with `instanceof`, and carrying the
authorization URL) and a direct transport's `AuthChallengeError`. An
OAuth-protected server that authorized fine on the legacy era instead produced a
dead-end "Version negotiation probe failed" — no redirect, and the connection
status flipped to `error` despite `isConnectAuthRecoveryError` existing to
prevent exactly that.

- `findNestedAuthError()` walks `cause` / `data.cause` for either typed error.
  Not gated on the SDK's error code or message, so it survives rewording.
- `connect()` unwraps the rejection at the earliest point, ahead of
  `withDirectAuthRecovery` (whose `isAuthChallengeError` check is shallow), the
  outer status guard, and every client's connect error handling — so web, CLI,
  and TUI are all fixed without a client-side change.
- On the direct path, enable `interceptAuthChallenges` for the probing eras even
  with no stored tokens. Without an authProvider the 401 reaches the SDK as a raw
  `SdkHttpError`, and the classifier ignores the HTTP status (it only looks for a
  JSON-RPC error body) — so pin mode rethrew it with the 401 discarded entirely,
  no status and not even a cause for the walk to find.

Verified against a live dual-era server that answers 401 on both `initialize`
and `server/discover`: `auto` and `modern` now reject with
`AuthRecoveryRequiredError` carrying a real authorization URL. Each half of the
fix is load-bearing — reverting either fails the new integration tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013SNjAqoEhUVVkr3UStR6om
@cliffhall

cliffhall commented Jul 27, 2026

Copy link
Copy Markdown
Member

Amended — I originally asked for the authProvider || guard to be dropped so interception was unconditional. Withdrawn; see the reasoning below. The requested change is now smaller.

Verified this end to end (details in #1805) — the diagnosis is right, both halves of the fix are load-bearing, and I found no regressions across web (4728), TUI (282) and CLI (304).

The two halves have different lifespans

Worth separating, because it drives what I'm asking for:

Requested: mark the workaround as removable

Keep the (transportOptions.authProvider || this.probesProtocolEra()) guard exactly as you wrote it — scoping it to the probing eras is right, precisely because it's temporary. Please just make its lifespan explicit in the comment, so whoever picks up #1807 knows this block is the thing to delete rather than having to re-derive it:

// WORKAROUND (#1807, upstream modelcontextprotocol/typescript-sdk#2561):
// remove this clause once the SDK classifies a probe 401/403 as auth-required.

That's the whole ask. Two notes on why I'm no longer pushing for the unconditional version: making it unconditional widens the surface of the code we intend to delete, and the era-scoped form is self-documenting about which connects need the compensation. The behavioral asymmetry it creates is real — on the direct path with no stored tokens, legacy rejects with a raw SdkHttpError 401 while auto/modern run discovery + DCR and reject with AuthRecoveryRequiredError (I confirmed both against a live composable server) — but that asymmetry is a faithful reflection of the upstream gap, and it disappears when #2561 lands. Encoding it in a comment beats encoding it in a wider blast radius.

Not blocking

Before merge

CI hasn't run on this yet (external contributor — needs maintainer approval to trigger workflows). It needs to go green on the full ci chain, in particular the per-file ≥90 coverage gate, which I did not run locally.

@cliffhall

Copy link
Copy Markdown
Member

Superseded by #1809, which carries your commit unchanged (authorship preserved), rebased onto current v2/main, plus the one requested comment change marking the era-probe interception as a removable workaround (#1807 / modelcontextprotocol/typescript-sdk#2561).

I ran the full npm run ci chain locally on that branch — green end to end, including the per-file ≥90 coverage gate that hadn't been exercised here. Follow-ups are now filed as #1807 and #1808 and linked from the PR body.

Thanks for the thorough diagnosis and tests — closing this in favour of #1809.

@cliffhall cliffhall closed this Jul 27, 2026
cliffhall added a commit that referenced this pull request Jul 27, 2026
* fix(core): start OAuth on a modern/auto-era connect (#1805)

The SDK's `server/discover` negotiation probe runs before anything else on
`protocolEra: "auto" | "modern"`, and its classifier reports whatever the
transport threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the real error moved
to `data.cause`. That buried both connect-time auth signals: the remote path's
`AuthRecoveryRequiredError` (matched with `instanceof`, and carrying the
authorization URL) and a direct transport's `AuthChallengeError`. An
OAuth-protected server that authorized fine on the legacy era instead produced a
dead-end "Version negotiation probe failed" — no redirect, and the connection
status flipped to `error` despite `isConnectAuthRecoveryError` existing to
prevent exactly that.

- `findNestedAuthError()` walks `cause` / `data.cause` for either typed error.
  Not gated on the SDK's error code or message, so it survives rewording.
- `connect()` unwraps the rejection at the earliest point, ahead of
  `withDirectAuthRecovery` (whose `isAuthChallengeError` check is shallow), the
  outer status guard, and every client's connect error handling — so web, CLI,
  and TUI are all fixed without a client-side change.
- On the direct path, enable `interceptAuthChallenges` for the probing eras even
  with no stored tokens. Without an authProvider the 401 reaches the SDK as a raw
  `SdkHttpError`, and the classifier ignores the HTTP status (it only looks for a
  JSON-RPC error body) — so pin mode rethrew it with the 401 discarded entirely,
  no status and not even a cause for the walk to find.

Verified against a live dual-era server that answers 401 on both `initialize`
and `server/discover`: `auto` and `modern` now reject with
`AuthRecoveryRequiredError` carrying a real authorization URL. Each half of the
fix is load-bearing — reverting either fails the new integration tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013SNjAqoEhUVVkr3UStR6om

* docs(core): mark the era-probe challenge interception as a removable workaround

Per review on #1806: the `|| this.probesProtocolEra()` clause compensates for
an upstream SDK gap (modelcontextprotocol/typescript-sdk#2561, tracked as
#1807). Make its lifespan explicit so whoever picks up #1807 knows this block
is the thing to delete, rather than having to re-derive it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt

* fix(core): resolve connect() when auth recovery is satisfied silently

Review findings on #1809:

- connect() built its handshake promise once, outside the runConnect closure
  withDirectAuthRecovery retries. On a "satisfied" outcome the recovery runs a
  complete nested connect() via reconnectAfterAuthRecovery(), so the retry leg
  re-awaited the already-rejected promise and rejected a connect() whose client
  was in fact connected. Short-circuit the retry when already connected, and
  skip the post-connect block the nested connect already ran (a duplicate
  `connect` event re-triggers every list-state manager's refresh).
- Document the accepted 403 side effect of enabling interceptAuthChallenges
  with no stored tokens, and why every `{ pin }` counts as probing.
- Replace the private-method double cast in the era-probe test with an
  observable assertion on the transport options, and split the non-auth
  failure case per era instead of a message alternation that passed either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt

* fix(core): key the connect retry leg off attempt, bound nested recoveries

Second-pass review findings on #1809:

- The satisfied-recovery short-circuit keyed off live connection status, so an
  onclose landing between the nested connect() and the retry leg fell through
  to the rejected handshake promise and reported the stale 401 as the reason
  the session died. withDirectAuthRecovery now passes `attempt` to its
  operation, so connect() identifies the retry leg structurally and reports the
  teardown when the recovered session is already gone.
- Bound nested silent recoveries. The `attempt >= 1` guard is per-call and a
  satisfied outcome recovers via a nested connect() with a fresh counter, so a
  server that keeps rejecting freshly refreshed tokens recursed without bound.
  Counted across the nesting boundary, capped at 3.
- Strengthen the regression test (server info populated by the nested connect,
  no duplicate statusChange) and add the two missing cases: a failed reconnect
  still rejects, and the recursion bound terminates.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt

* test(core): cover the recovery teardown branch; fix a duplicated case

Third-pass review findings on #1809, both about tests added in the previous
commit:

- The retry leg's teardown error had no coverage, so a regression to the old
  status fall-through would have stayed green. Covered deterministically: the
  nested connect dispatches `connect` after setting "connected", so closing the
  live transport from that listener flips the status before the retry leg runs.
  Asserts the error identity, which is the point of the change.
- "still rejects when the reconnect underneath the recovery fails" used a
  challenging transport for every connect, so it walked the same bounded chain
  as the recursion-bound test rather than covering a non-auth reconnect
  failure, and its comment claimed an "error" status the auth path never
  reaches. Now fails the reconnect with a plain error and asserts both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt

---------

Co-authored-by: rinormaloku <rinormaloku37@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OAuth never starts on a modern/auto-era connect: the era-negotiation probe hides the auth challenge

2 participants