modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 4

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

Browse source 
* 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>
This commit is contained in:
Cliff Hall 2026-07-27 15:06:01 -04:00 • committed by GitHub
commit 01df9d81bb
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
1 changed files with 906 additions and 8 deletions
Download patch file Download diff file Expand all files Collapse all files

129
core/mcp/inspectorClient.ts
View file

@ -186,6 +186,7 @@ import type {
import {
AuthRecoveryRequiredError,
EMA_STEP_UP_PENDING_URL,
findNestedAuthError,
isAuthChallengeError,
isConnectAuthRecoveryError,
parseAuthChallengeFromError,
@ -534,6 +535,22 @@ export class InspectorClient extends InspectorClientEventTarget {
private transportConfig: MCPServerConfig;
/** null until first transport is built; then true for in-process OAuth runners. */
private directAuthRecoveryActive: boolean | null = null;
/**
* Nesting depth of in-flight silent auth recoveries. `withDirectAuthRecovery`
* bounds retries per call (`attempt >= 1`), but a satisfied outcome recovers
* by running a *nested* `connect()`, which starts its own recovery with a
* fresh counter — so a server that keeps answering 401 with freshly refreshed
* tokens would recurse without bound. Counted across the nesting boundary and
* capped at {@link InspectorClient.MAX_NESTED_AUTH_RECOVERIES}.
*/
private authRecoveryDepth = 0;
/**
* Cap on nested silent recoveries. One is the normal case (challenge →
* refresh → reconnect); a couple more absorb a legitimately re-challenged
* reconnect. Beyond that the credentials are not fixing the challenge, so the
* challenge is surfaced to the caller instead of recovered again.
*/
private static readonly MAX_NESTED_AUTH_RECOVERIES = 3;
/**
* Opt-in from {@link InspectorClientOptions.directAuthRecovery}: when true and
* the live transport is direct (not {@link RemoteClientTransport}), RPCs use
@ -1710,7 +1727,29 @@ export class InspectorClient extends InspectorClientEventTarget {
this.directAuthRecoveryActive !== false &&
this.isHttpOAuthConfig() &&
oauthManager &&
transportOptions.authProvider
// No stored tokens means no authProvider (see above), and then a 401 on
// the era-negotiation probe reaches the SDK as a raw `SdkHttpError`.
// The probe's classifier ignores the HTTP status — it only looks for a
// JSON-RPC error body — so it verdicts "not a modern server", and pin
// ("modern") mode rethrows that as ERA_NEGOTIATION_FAILED with the 401
// discarded entirely: no status, not even a cause. Intercepting makes
// the 401 a typed AuthChallengeError, which survives the probe as
// `data.cause` for `findNestedAuthError` to recover (#1805).
//
// WORKAROUND (#1807, upstream modelcontextprotocol/typescript-sdk#2561):
// remove this clause once the SDK classifies a probe 401/403 as
// auth-required. `findNestedAuthError` is the permanent fix; the
// `|| this.probesProtocolEra()` clause below exists only to compensate
// for that upstream gap and should be deleted with it.
//
// Known, accepted side effect of turning intercept on with no stored
// tokens: `parseAuthChallengeFromResponse` treats 403 as a challenge
// too, so a probe answered 403 for a *non-auth* reason (a gateway
// rejecting the unknown `server/discover` method, say) now starts OAuth
// discovery instead of letting "auto" fall back to the legacy
// `initialize`. The outcome is a surfaced `oauthError`, not a hang, and
// it goes away with this clause.
(transportOptions.authProvider || this.probesProtocolEra())
) {
transportOptions.interceptAuthChallenges = true;
}
@ -1772,8 +1811,45 @@ export class InspectorClient extends InspectorClientEventTarget {
// Promise.race. On timeout, tear the transport down so the next
// connect() starts clean and the upstream socket isn't left hanging.
const connectTimeoutMs = this.serverSettings?.connectionTimeout ?? 0;
const connectPromise = this.client.connect(this.transport);
const runConnect = async (): Promise<void> => {
// Unwrap here — the earliest point — so an auth error the SDK's
// era-negotiation probe buried in its cause chain is surfaced before
// anything downstream inspects it: `withDirectAuthRecovery` (whose
// `isAuthChallengeError` check is shallow), the outer catch's
// `isConnectAuthRecoveryError` status guard, and every client's connect
// error handling. See {@link findNestedAuthError} (#1805).
const connectPromise = this.client
.connect(this.transport)
.catch((err: unknown) => {
throw findNestedAuthError(err) ?? err;
});
// Set when a satisfied auth recovery already completed a full connect()
// underneath us — see the short-circuit in `runConnect`.
let recoveredByNestedConnect = false;
const runConnect = async (attempt: number): Promise<void> => {
if (attempt > 0) {
// The *retry* leg of `withDirectAuthRecovery`: the challenge was
// satisfied silently (e.g. a refresh token), so
// `reconnectAfterAuthRecovery()` has already run a complete
// `connect()` — handshake, server info, `connect` event and all.
// `connectPromise` is created once, outside this closure, so
// re-awaiting it here would rethrow the *original* rejection and
// reject a `connect()` whose client is in fact connected. Short-
// circuit instead, and let the caller skip the post-connect block the
// nested connect already ran (dispatching a second `connect` event
// would re-trigger every list-state manager's refresh).
//
// Keyed off the leg rather than live status: an `onclose` landing
// between the nested connect and here would flip status off
// "connected" and fall through to the rejected `connectPromise`,
// reporting the stale handshake 401 as the reason the session died.
recoveredByNestedConnect = true;
if (this.status !== "connected") {
throw new Error(
"Connection closed during authorization recovery, after re-authorizing successfully",
);
}
return;
}
if (connectTimeoutMs > 0) {
connectPromise.catch(() => {});
let timer: ReturnType<typeof setTimeout> | undefined;
@ -1806,6 +1882,10 @@ export class InspectorClient extends InspectorClientEventTarget {
}
throw err;
}
if (recoveredByNestedConnect) {
// The nested connect() from the auth recovery did all of the below.
return;
}
this.status = "connected";
this.dispatchTypedEvent("statusChange", this.status);
@ -5263,13 +5343,34 @@ export class InspectorClient extends InspectorClientEventTarget {
return this.directAuthRecovery && this.directAuthRecoveryActive === true;
}
/**
* True when connect() sends the SDK's `server/discover` negotiation probe —
* i.e. `protocolEra` is "auto", or "modern" (`{ pin: MODERN_PROTOCOL_VERSION }`).
* Legacy is the default for an absent `mode`, matching the SDK.
*
* `versionNegotiation` is a public option, so a caller can pin a revision the
* repo's own {@link eraToVersionNegotiation} never produces (it only ever
* pins {@link MODERN_PROTOCOL_VERSION}). That is still "probing" and
* deliberately reports true: per the SDK, *any* `{ pin }` sends the
* connect-time `server/discover` — "the connect-time `server/discover` must
* offer it. No fallback" — so the pinned revision doesn't change whether the
* probe (and hence the buried-401 problem) happens. Only "legacy" skips it.
*/
private probesProtocolEra(): boolean {
const mode = this.versionNegotiation.mode;
return mode !== undefined && mode !== "legacy";
}
private async withDirectAuthRecovery<T>(
operation: () => Promise<T>,
operation: (attempt: number) => Promise<T>,
context?: { method?: string; toolName?: string },
attempt = 0,
): Promise<T> {
try {
return await operation();
// `attempt` is passed through so an operation can tell the first leg from
// the post-recovery retry leg — `connect()` needs that distinction, and
// live connection status is a racy proxy for it.
return await operation(attempt);
} catch (err) {
if (attempt >= 1 || !this.usesDirectAuthRecovery()) {
throw err;
@ -5277,6 +5378,13 @@ export class InspectorClient extends InspectorClientEventTarget {
if (!isAuthChallengeError(err)) {
throw err;
}
if (
this.authRecoveryDepth >= InspectorClient.MAX_NESTED_AUTH_RECOVERIES
) {
// Refreshed credentials are not satisfying the server: recovering again
// would just re-enter the nested connect() below. Surface the challenge.
throw err;
}
const challenge = parseAuthChallengeFromError(err, context);
/* v8 ignore next 3 -- defensive: parseAuthChallengeFromError shares isAuthChallengeError's checks, so it always returns a truthy challenge once that guard passes */
if (!challenge) {
@ -5295,7 +5403,12 @@ export class InspectorClient extends InspectorClientEventTarget {
if (this.activeToolCallAbortController) {
this.activeToolCallAbortController = undefined;
}
await this.reconnectAfterAuthRecovery();
this.authRecoveryDepth += 1;
try {
await this.reconnectAfterAuthRecovery();
} finally {
this.authRecoveryDepth -= 1;
}
this.dispatchTypedEvent("authChallengeRecovered", { challenge });
return this.withDirectAuthRecovery(operation, context, attempt + 1);
}
@ -5318,11 +5431,11 @@ export class InspectorClient extends InspectorClientEventTarget {
}
private async invokeMcpClient<T>(
operation: () => Promise<T>,
operation: (attempt: number) => Promise<T>,
context?: { method?: string; toolName?: string },
): Promise<T> {
if (!this.usesDirectAuthRecovery()) {
return operation();
return operation(0);
}
return this.withDirectAuthRecovery(operation, context);
}