diff --git a/clients/web/src/test/core/auth/challenge.test.ts b/clients/web/src/test/core/auth/challenge.test.ts index bafacc01a..ed2525761 100644 --- a/clients/web/src/test/core/auth/challenge.test.ts +++ b/clients/web/src/test/core/auth/challenge.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import { AuthChallengeError, AuthRecoveryRequiredError, + findNestedAuthError, isAuthChallengeError, isConnectAuthRecoveryError, parseAuthChallengeFromError, @@ -391,3 +392,89 @@ describe("isConnectAuthRecoveryError", () => { ).toBe(false); }); }); + +/** + * The SDK's era-negotiation probe (protocolEra "auto"/"modern") reports a failed + * `server/discover` as `SdkError(ERA_NEGOTIATION_FAILED)` and moves the real + * error to `data.cause`, hiding the auth signal connect-time recovery matches on + * (#1805). These cover the recovery walk over both link names. + */ +describe("findNestedAuthError", () => { + const authorizationUrl = new URL("https://as.example/authorize"); + const recoveryRequired = () => + new AuthRecoveryRequiredError(authorizationUrl, { reason: "unauthorized" }); + + it("recovers an AuthRecoveryRequiredError from `data.cause` (the SDK probe wrapper)", () => { + const nested = recoveryRequired(); + const wrapper = new Error( + "Version negotiation probe failed: Interactive auth recovery required", + ) as Error & { data?: { cause?: unknown } }; + wrapper.data = { cause: nested }; + + expect(findNestedAuthError(wrapper)).toBe(nested); + }); + + it("recovers an AuthChallengeError from `data.cause` (direct transport, intercepted 401)", () => { + const nested = new AuthChallengeError({ reason: "token_expired" }, 401); + const wrapper = new Error("Version negotiation probe failed") as Error & { + data?: { cause?: unknown }; + }; + wrapper.data = { cause: nested }; + + expect(findNestedAuthError(wrapper)).toBe(nested); + }); + + it("follows the native `cause` link", () => { + const nested = recoveryRequired(); + expect(findNestedAuthError(new Error("outer", { cause: nested }))).toBe( + nested, + ); + }); + + it("walks more than one level and prefers the native `cause` branch", () => { + const nested = recoveryRequired(); + const middle = new Error("middle") as Error & { + data?: { cause?: unknown }; + }; + middle.data = { cause: nested }; + + expect(findNestedAuthError(new Error("outer", { cause: middle }))).toBe( + nested, + ); + }); + + it("returns the error itself when it is already a typed auth error", () => { + const err = recoveryRequired(); + expect(findNestedAuthError(err)).toBe(err); + }); + + it("returns undefined when no auth error is in the chain", () => { + const plain = new Error("outer", { cause: new Error("inner") }) as Error & { + data?: { cause?: unknown }; + }; + plain.data = { cause: new Error("also not auth") }; + + expect(findNestedAuthError(plain)).toBeUndefined(); + }); + + it("returns undefined for non-object errors and a non-object `data`", () => { + expect(findNestedAuthError(undefined)).toBeUndefined(); + expect(findNestedAuthError(null)).toBeUndefined(); + expect(findNestedAuthError("failed (401)")).toBeUndefined(); + const stringData = new Error("outer") as Error & { data?: unknown }; + stringData.data = "not an object"; + expect(findNestedAuthError(stringData)).toBeUndefined(); + const nullData = new Error("outer") as Error & { data?: unknown }; + nullData.data = null; + expect(findNestedAuthError(nullData)).toBeUndefined(); + }); + + it("terminates on a cyclic cause chain", () => { + const a = new Error("a") as Error & { cause?: unknown }; + const b = new Error("b") as Error & { cause?: unknown }; + a.cause = b; + b.cause = a; + + expect(findNestedAuthError(a)).toBeUndefined(); + }); +}); diff --git a/clients/web/src/test/core/mcp/inspectorClient-era-probe-auth.test.ts b/clients/web/src/test/core/mcp/inspectorClient-era-probe-auth.test.ts new file mode 100644 index 000000000..b3b987482 --- /dev/null +++ b/clients/web/src/test/core/mcp/inspectorClient-era-probe-auth.test.ts @@ -0,0 +1,143 @@ +import { describe, it, expect } from "vitest"; +import { + AuthChallengeError, + AuthRecoveryRequiredError, +} from "@inspector/core/auth/challenge.js"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js"; +import type { JSONRPCMessage, Transport } from "@modelcontextprotocol/client"; + +/** + * Connecting with `protocolEra: "auto" | "modern"` sends the SDK's + * `server/discover` negotiation probe first, and the probe's classifier reports + * whatever the transport threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the + * original error moved to `data.cause`. That buried the auth signals every + * client's connect-error handling matches on, so an OAuth-protected server that + * authorized fine on the legacy era produced a dead-end "Version negotiation + * probe failed" instead of starting authorization (#1805). + * + * `connect()` unwraps the rejection, so these assert the *type* that reaches the + * caller. The live counterpart (a real modern server answering 401) is + * `src/test/integration/mcp/inspectorClient-modern-era-oauth.test.ts`. + */ +describe("InspectorClient connect() era-probe auth unwrapping (#1805)", () => { + /** + * Minimal transport whose `send` rejects — which is what the probe's + * `server/discover` exchange hits. The remote path rejects with + * `AuthRecoveryRequiredError` (after the backend intercepted the 401 and + * `handleAuthChallenge` returned `interactive`); a direct transport with + * challenge interception rejects with `AuthChallengeError`. + */ + class RejectingTransport implements Transport { + onclose?: () => void; + onerror?: (error: Error) => void; + onmessage?: (message: JSONRPCMessage) => void; + + private readonly rejection: Error; + + // A parameter property would trip `erasableSyntaxOnly`. + constructor(rejection: Error) { + this.rejection = rejection; + } + + async start(): Promise {} + + async send(): Promise { + throw this.rejection; + } + + async close(): Promise { + this.onclose?.(); + } + } + + function makeClient( + rejection: Error, + era: "legacy" | "auto" | "modern", + ): InspectorClient { + return new InspectorClient( + { type: "streamable-http", url: "https://mcp.example/mcp" }, + { + environment: { + transport: () => ({ transport: new RejectingTransport(rejection) }), + }, + versionNegotiation: eraToVersionNegotiation(era), + }, + ); + } + + const recoveryRequired = () => + new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { + reason: "unauthorized", + }); + + for (const era of ["auto", "modern"] as const) { + it(`surfaces AuthRecoveryRequiredError from the probe wrapper on the "${era}" era`, async () => { + const rejection = recoveryRequired(); + const client = makeClient(rejection, era); + + await expect(client.connect()).rejects.toBe(rejection); + }); + + it(`surfaces AuthChallengeError from the probe wrapper on the "${era}" era`, async () => { + const rejection = new AuthChallengeError( + { reason: "token_expired" }, + 401, + ); + const client = makeClient(rejection, era); + + // The direct-recovery retry is off for this client, so the challenge + // itself reaches the caller rather than a recovery outcome. + await expect(client.connect()).rejects.toBe(rejection); + }); + + it(`leaves a non-auth probe failure untouched on the "${era}" era`, async () => { + const client = makeClient(new Error("ECONNREFUSED"), era); + + // No auth error in the chain: the SDK's typed negotiation error stands, so + // callers still report a plain connection failure. + await expect(client.connect()).rejects.toThrow( + /Version negotiation|ECONNREFUSED/, + ); + }); + } + + it("passes an unwrapped legacy-era rejection through unchanged", async () => { + // Legacy sends no probe, so nothing wraps the error — the baseline the + // probing eras now match. + const rejection = recoveryRequired(); + const client = makeClient(rejection, "legacy"); + + await expect(client.connect()).rejects.toBe(rejection); + }); +}); + +describe("InspectorClient probesProtocolEra (#1805)", () => { + function probesFor( + versionNegotiation: + | { mode?: "legacy" | "auto" | { pin: string } } + | undefined, + ): boolean { + const client = new InspectorClient( + { type: "streamable-http", url: "https://mcp.example/mcp" }, + { + environment: { transport: () => ({}) as never }, + ...(versionNegotiation ? { versionNegotiation } : {}), + }, + ); + return ( + client as unknown as { probesProtocolEra: () => boolean } + ).probesProtocolEra(); + } + + it("is true for the probing eras and false for legacy", () => { + expect(probesFor(eraToVersionNegotiation("auto"))).toBe(true); + expect(probesFor(eraToVersionNegotiation("modern"))).toBe(true); + expect(probesFor(eraToVersionNegotiation("legacy"))).toBe(false); + }); + + it("treats an absent mode and an absent option as legacy (the SDK default)", () => { + expect(probesFor({})).toBe(false); + expect(probesFor(undefined)).toBe(false); + }); +}); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-modern-era-oauth.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-modern-era-oauth.test.ts new file mode 100644 index 000000000..d7a912744 --- /dev/null +++ b/clients/web/src/test/integration/mcp/inspectorClient-modern-era-oauth.test.ts @@ -0,0 +1,188 @@ +/** + * Live coverage of an OAuth-protected connect on the probing eras (#1805). + * + * A modern (2026-07-28) server behind `requireAuth` answers the SDK's + * `server/discover` negotiation probe with 401 — the probe runs before anything + * else, so it, not `initialize`, is where authorization first surfaces. The + * probe's classifier reports the failure as `SdkError(ERA_NEGOTIATION_FAILED)` + * with the real error moved to `data.cause`, which used to bury the auth signal + * and leave `connect()` rejecting with a dead-end "Version negotiation probe + * failed" instead of starting the authorization flow (the same server authorized + * fine on `protocolEra: "legacy"`). + * + * Complements `inspectorClient-modern-era.test.ts` (modern era, no auth) and + * `inspectorClient-oauth-direct-mid-session-e2e.test.ts` (auth, legacy era). + */ + +import { describe, it, expect, beforeEach, afterEach, afterAll } from "vitest"; +import { rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { NodeOAuthStorage } from "@inspector/core/auth/node/storage-node.js"; +import { + TestServerHttp, + waitForOAuthWellKnown, + getDefaultServerConfig, + createOAuthTestServerConfig, + clearOAuthTestData, +} from "@modelcontextprotocol/inspector-test-server"; +import { + AuthRecoveryRequiredError, + isConnectAuthRecoveryError, +} from "@inspector/core/auth/challenge.js"; +import { + eraToVersionNegotiation, + MODERN_PROTOCOL_VERSION, +} from "@inspector/core/mcp/types.js"; +import { + createOAuthClientConfig, + completeOAuthAuthorization, +} from "../helpers/oauth-client-fixtures.js"; +import { ConsoleNavigation } from "@inspector/core/auth/providers.js"; +import type { InspectorClientOptions } from "@inspector/core/mcp/inspectorClient.js"; +import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; + +const oauthTestStatePath = join( + tmpdir(), + `mcp-oauth-${process.pid}-modern-era-oauth.json`, +); + +const testRedirectUrl = "http://localhost:3000/oauth/callback"; +const staticClientId = "test-modern-era-oauth"; +const staticClientSecret = "test-secret-modern-era-oauth"; + +describe("OAuth connect on the probing eras (#1805)", () => { + let mcpServer: TestServerHttp | null = null; + let client: InspectorClient | null = null; + + beforeEach(() => { + clearOAuthTestData(); + }); + + afterEach(async () => { + if (client) { + await client.disconnect().catch(() => {}); + client = null; + } + if (mcpServer) { + await mcpServer.stop(); + mcpServer = null; + } + }, 30_000); + + afterAll(() => { + try { + rmSync(oauthTestStatePath, { force: true }); + } catch { + // ignore + } + }); + + /** Modern-era MCP server that requires a Bearer token on /mcp. */ + async function startProtectedModernServer(): Promise { + const started = new TestServerHttp({ + ...getDefaultServerConfig(), + serverType: "streamable-http" as const, + // Modern (2026-07-28) serving: the bearer middleware guards this leg too, + // so the negotiation probe itself is answered 401. + modern: {}, + ...createOAuthTestServerConfig({ + requireAuth: true, + supportRefreshTokens: true, + staticClients: [ + { + clientId: staticClientId, + clientSecret: staticClientSecret, + redirectUris: [testRedirectUrl], + }, + ], + }), + }); + const port = await started.start(); + mcpServer = started; + const serverUrl = `http://localhost:${port}`; + await waitForOAuthWellKnown(serverUrl); + return serverUrl; + } + + function createClient( + serverUrl: string, + era: "auto" | "modern", + ): InspectorClient { + const oauthConfig = { + ...createOAuthClientConfig({ + mode: "static", + clientId: staticClientId, + clientSecret: staticClientSecret, + redirectUrl: testRedirectUrl, + }), + storage: new NodeOAuthStorage(oauthTestStatePath), + }; + + const clientConfig: InspectorClientOptions = { + environment: { + transport: createTransportNode, + oauth: { + storage: oauthConfig.storage, + navigation: new ConsoleNavigation(), + redirectUrlProvider: oauthConfig.redirectUrlProvider, + }, + }, + // The CLI/TUI shape (see core/client/runner.ts). + directAuthRecovery: true, + versionNegotiation: eraToVersionNegotiation(era), + oauth: { + clientId: oauthConfig.clientId, + clientSecret: oauthConfig.clientSecret, + clientMetadataUrl: oauthConfig.clientMetadataUrl, + scope: oauthConfig.scope, + }, + }; + + const created = new InspectorClient( + { type: "streamable-http", url: `${serverUrl}/mcp` } as MCPServerConfig, + clientConfig, + ); + client = created; + return created; + } + + for (const era of ["auto", "modern"] as const) { + it(`surfaces a recoverable auth error (not a negotiation failure) on the "${era}" era with no stored tokens`, async () => { + const serverUrl = await startProtectedModernServer(); + const connecting = createClient(serverUrl, era).connect(); + + // Recoverable: the caller can drive the OAuth redirect from this error. + // Before the fix the probe's wrapper reached here instead, so every + // client fell through to a generic "failed to connect" report. + await expect(connecting).rejects.toBeInstanceOf( + AuthRecoveryRequiredError, + ); + const error = await connecting.catch((err: unknown) => err); + expect(isConnectAuthRecoveryError(error)).toBe(true); + expect((error as Error).message).not.toMatch(/version negotiation/i); + expect( + (error as AuthRecoveryRequiredError).authorizationUrl, + ).toBeInstanceOf(URL); + }, 30_000); + + it(`connects on the "${era}" era once authorization completes`, async () => { + const serverUrl = await startProtectedModernServer(); + const authorizing = createClient(serverUrl, era); + + const authUrl = await authorizing.authenticate(); + if (!authUrl) throw new Error("Expected an authorization URL"); + const { code, iss } = await completeOAuthAuthorization(authUrl); + await authorizing.completeOAuthFlow(code, iss); + await authorizing.connect(); + + // With a token the probe is answered, so the modern era is reached — the + // outcome the negotiation failure was masking. + expect(authorizing.getProtocolEra()).toBe("modern"); + expect(authorizing.getProtocolVersion()).toBe(MODERN_PROTOCOL_VERSION); + expect((await authorizing.listTools()).tools.length).toBeGreaterThan(0); + }, 30_000); + } +}); diff --git a/core/auth/challenge.ts b/core/auth/challenge.ts index c30074e6e..09d9ba2ef 100644 --- a/core/auth/challenge.ts +++ b/core/auth/challenge.ts @@ -103,6 +103,62 @@ export function isConnectAuthRecoveryError(err: unknown): boolean { return isUnauthorizedError(err); } +/** + * Recover a typed auth error that another error is carrying in its cause chain. + * + * Under `protocolEra: auto|modern` the SDK sends a `server/discover` negotiation + * probe before anything else, and its classifier reports whatever the transport + * threw as `SdkError(ERA_NEGOTIATION_FAILED)` with the original error moved to + * `data.cause`. That buries the two connect-time auth signals recovery keys off + * — {@link AuthRecoveryRequiredError} on the remote path (it carries the + * authorization URL and is matched with `instanceof`), and + * {@link AuthChallengeError} on a direct transport — so a modern-era connect + * against an OAuth server reported "Version negotiation probe failed" instead of + * starting authorization (#1805). + * + * Walks `cause` and `data.cause` (the same two links {@link isUnauthorizedError} + * follows for a nested 401) and returns the first such error. Deliberately not + * gated on the SDK's error code or message: any wrapper that keeps the cause + * chain intact should surface the same signal, so this survives SDK rewording. + */ +export function findNestedAuthError( + err: unknown, +): AuthRecoveryRequiredError | AuthChallengeError | undefined { + return findNestedAuthErrorDeep(err, new Set()); +} + +function findNestedAuthErrorDeep( + err: unknown, + seen: Set, +): AuthRecoveryRequiredError | AuthChallengeError | undefined { + if (err === null || typeof err !== "object" || seen.has(err)) { + return undefined; + } + seen.add(err); + + if ( + err instanceof AuthRecoveryRequiredError || + err instanceof AuthChallengeError + ) { + return err; + } + + const nested = findNestedAuthErrorDeep( + (err as { cause?: unknown }).cause, + seen, + ); + if (nested) { + return nested; + } + + const data = (err as { data?: unknown }).data; + if (data !== null && typeof data === "object") { + return findNestedAuthErrorDeep((data as { cause?: unknown }).cause, seen); + } + + return undefined; +} + export interface WwwAuthenticateBearerParams { error?: string; scope?: string; diff --git a/core/auth/index.ts b/core/auth/index.ts index cf82840d0..bf4a098f8 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -78,6 +78,7 @@ export { unionAuthorizationScopes, isAuthChallengeError, isConnectAuthRecoveryError, + findNestedAuthError, EMA_STEP_UP_PENDING_URL, } from "./challenge.js"; diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 8a4d6705c..6b96395de 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -185,6 +185,7 @@ import type { import { AuthRecoveryRequiredError, EMA_STEP_UP_PENDING_URL, + findNestedAuthError, isAuthChallengeError, isConnectAuthRecoveryError, parseAuthChallengeFromError, @@ -1114,7 +1115,15 @@ 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). + (transportOptions.authProvider || this.probesProtocolEra()) ) { transportOptions.interceptAuthChallenges = true; } @@ -1170,7 +1179,17 @@ 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); + // 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; + }); const runConnect = async (): Promise => { if (connectTimeoutMs > 0) { connectPromise.catch(() => {}); @@ -4740,6 +4759,16 @@ 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 }`). Legacy is the default + * for an absent `mode`, matching the SDK. + */ + private probesProtocolEra(): boolean { + const mode = this.versionNegotiation.mode; + return mode !== undefined && mode !== "legacy"; + } + private async withDirectAuthRecovery( operation: () => Promise, context?: { method?: string; toolName?: string },