diff --git a/README.md b/README.md index 702e5d49..257c15b5 100644 --- a/README.md +++ b/README.md @@ -330,6 +330,8 @@ The default `/.well-known/oauth-protected-resource` route is deliberately left u Add the server, click **Connect**, and watch the Inspector's first protected-resource metadata request in the Network tab: it must go to `/custom/protected-resource`. On the broken build the challenge's `resource_metadata` was parsed and then dropped before the SDK's `auth()` ever saw it ([#2071](https://github.com/modelcontextprotocol/inspector/issues/2071)), so discovery probed locations derived from the MCP server URL, 404'd, and authorization failed for any server that puts the document somewhere other than the well-known path. +The same server is worth running against `--cli` / `--tui`, which reach it by a different route: with no stored token in the legacy era the Inspector connects with no auth provider (so the SDK cannot open a browser before the callback server is listening), the 401 surfaces as the SDK's headerless `UnauthorizedError`, and the client calls `authenticate()` with no challenge in hand. The transport therefore *observes* every 401/403 passively, so the advertised URL is still available on that path. + The value now rides the normalized `AuthChallenge` as a string — it has to be serializable, because the web client's challenge crosses the remote-backend boundary as JSON — and is converted to a `URL` at the OAuth boundary, where it is handed to `auth()` as `resourceMetadataUrl` and to the CIMD pre-registration probe, which runs *before* `auth()` and would otherwise do its own default-location discovery. A malformed value is ignored rather than surfaced, matching the SDK's own `WWW-Authenticate` parser: discovery falls back to the default locations instead of failing the whole authorization on a bad header. The callback leg needs nothing extra — SDK `auth()` persists the URL in its discovery state, so it survives both the web full-page redirect and the CLI/TUI loopback callback. #### Logging, both eras diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 3d5fc320..22acdb58 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -1777,6 +1777,61 @@ describe("OAuthManager", () => { ); }); + it("uses the last observed challenge on the plain authenticate() path", async () => { + // The legacy first-authorization path: no challenge is passed in, so the + // one the transport observed is the only source (#2071). + mockedMcpAuth.mockResolvedValue("REDIRECT"); + const params = createMockParams(); + const manager = new OAuthManager(params); + const captureSpy = vi + .spyOn( + (await import("@inspector/core/auth/providers.js")) + .BaseOAuthClientProvider.prototype, + "getCapturedAuthUrl", + ) + .mockReturnValue(new URL("https://auth.example.com/authorize?state=x")); + + manager.noteObservedAuthChallenge({ + reason: "unauthorized", + resourceMetadataUrl: METADATA_URL, + }); + await manager.authenticate(); + + expect(mockedMcpAuth).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + resourceMetadataUrl: new URL(METADATA_URL), + }), + ); + captureSpy.mockRestore(); + }); + + it("clears the observed URL when a later challenge advertises none", async () => { + mockedMcpAuth.mockResolvedValue("REDIRECT"); + const params = createMockParams(); + const manager = new OAuthManager(params); + const captureSpy = vi + .spyOn( + (await import("@inspector/core/auth/providers.js")) + .BaseOAuthClientProvider.prototype, + "getCapturedAuthUrl", + ) + .mockReturnValue(new URL("https://auth.example.com/authorize?state=x")); + + manager.noteObservedAuthChallenge({ + reason: "unauthorized", + resourceMetadataUrl: METADATA_URL, + }); + manager.noteObservedAuthChallenge({ reason: "unauthorized" }); + await manager.authenticate(); + + expect(mockedMcpAuth).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ resourceMetadataUrl: undefined }), + ); + captureSpy.mockRestore(); + }); + it("forwards it on the forced step-up reauthorization leg too", async () => { mockedMcpAuth.mockResolvedValue("AUTHORIZED"); const params = createMockParams(); diff --git a/clients/web/src/test/integration/mcp/oauth-resource-metadata-challenge.test.ts b/clients/web/src/test/integration/mcp/oauth-resource-metadata-challenge.test.ts index 2a9f9f84..6f46ba28 100644 --- a/clients/web/src/test/integration/mcp/oauth-resource-metadata-challenge.test.ts +++ b/clients/web/src/test/integration/mcp/oauth-resource-metadata-challenge.test.ts @@ -10,8 +10,10 @@ * plumbing is load-bearing rather than merely present. */ -import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { describe, it, expect, beforeAll, afterAll, vi } from "vitest"; import { fileURLToPath } from "node:url"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { discoverOAuthProtectedResourceMetadata } from "@modelcontextprotocol/client"; import { TestServerHttp, @@ -26,8 +28,19 @@ import { challengeResourceMetadataUrl, parseAuthChallengeFromResponse, } from "@inspector/core/auth/challenge.js"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { + ConsoleNavigation, + type RedirectUrlProvider, +} from "@inspector/core/auth/providers.js"; +import { NodeOAuthStorage } from "@inspector/core/auth/node/storage-node.js"; +import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; const METADATA_PATH = "/custom/protected-resource"; +const STATIC_CLIENT_ID = "test-2071-resource-metadata"; +const STATIC_CLIENT_SECRET = "test-2071-secret"; +const REDIRECT_URL = "http://localhost:3000/oauth/callback"; describe("OAuth challenge resource_metadata (RFC 9728)", () => { let mcpServer: TestServerHttp | null = null; @@ -40,6 +53,16 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => { ...createOAuthTestServerConfig({ requireAuth: true, resourceMetadataPath: METADATA_PATH, + // Static rather than DCR: this suite is about *where the metadata + // document is found*, so registration should not be a second thing + // that can fail. + staticClients: [ + { + clientId: STATIC_CLIENT_ID, + clientSecret: STATIC_CLIENT_SECRET, + redirectUris: [REDIRECT_URL], + }, + ], }), }); const port = await mcpServer.start(); @@ -89,6 +112,68 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => { ).rejects.toThrow(); }); + it("drives first-time legacy authorization off the advertised URL (#2071)", async () => { + // The gap the challenge-driven plumbing alone does not close: with no + // stored token in the default legacy era, `InspectorClient` builds the + // transport with no `authProvider` and no challenge interception, so the + // 401 reaches the client as the SDK's headerless `UnauthorizedError` and + // the CLI/TUI runner calls `authenticate()` with nothing in hand. The + // observed-challenge path is what carries the URL across that gap. + const requested: string[] = []; + const spyFetch = vi.fn( + async (input: RequestInfo | URL, init?: RequestInit) => { + requested.push(String(input instanceof Request ? input.url : input)); + return fetch(input as RequestInfo, init); + }, + ) as unknown as typeof fetch; + + const redirectUrlProvider: RedirectUrlProvider = { + getRedirectUrl: () => REDIRECT_URL, + }; + + const client = new InspectorClient( + { + type: "streamable-http", + url: `${serverUrl}/mcp`, + } as MCPServerConfig, + { + environment: { + transport: createTransportNode, + fetch: spyFetch, + oauth: { + storage: new NodeOAuthStorage( + join(tmpdir(), `mcp-oauth-${process.pid}-2071.json`), + ), + navigation: new ConsoleNavigation(), + redirectUrlProvider, + }, + }, + oauth: { + clientId: STATIC_CLIENT_ID, + clientSecret: STATIC_CLIENT_SECRET, + scope: "mcp", + }, + }, + ); + + await expect(client.connect()).rejects.toThrow(); + + // Reaching an authorization URL at all means discovery resolved — which it + // can only have done through the advertised path, since the well-known one + // 404s on this server. + const authUrl = await client.authenticate(); + expect(authUrl?.href).toContain("/oauth/authorize"); + + expect(requested).toContain(`${serverUrl}${METADATA_PATH}`); + expect( + requested.filter((url) => + url.includes("/.well-known/oauth-protected-resource"), + ), + ).toEqual([]); + + await client.disconnect(); + }, 30_000); + it("keeps the showcase config valid and carrying the custom path", () => { const configPath = fileURLToPath( new URL( diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index fb0ad80e..a9d54947 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -1933,6 +1933,16 @@ export class InspectorClient extends InspectorClientEventTarget { ...(this.serverSettings && { settings: this.serverSettings }), }; if (this.isHttpOAuthConfig() && oauthManager) { + // Record every 401/403 the transport sees, whatever else happens to + // it. The legacy first-authorization path deliberately runs with no + // authProvider and no challenge interception (see below), so the SDK + // raises a headerless `UnauthorizedError` and the client calls + // `authenticate()` with nothing in hand — this is the only place the + // challenge's RFC 9728 `resource_metadata` still exists (#2071). + const manager = oauthManager; + transportOptions.onAuthChallengeObserved = (challenge) => { + manager.noteObservedAuthChallenge(challenge); + }; if (oauthManager.isEnterpriseManaged()) { await oauthManager.trySilentEnterpriseManagedAuth(); const provider = await oauthManager.createOAuthProviderForTransport(); diff --git a/core/mcp/node/authChallengeFetch.ts b/core/mcp/node/authChallengeFetch.ts index af5dcb2c..0ce757ba 100644 --- a/core/mcp/node/authChallengeFetch.ts +++ b/core/mcp/node/authChallengeFetch.ts @@ -2,6 +2,7 @@ import { AuthChallengeError, parseAuthChallengeFromResponse, } from "../../auth/challenge.js"; +import type { AuthChallenge } from "../../auth/challenge.js"; /** * Wrap fetch so MCP HTTP 401/403 responses become {@link AuthChallengeError} @@ -33,3 +34,37 @@ export function createAuthChallengeInterceptFetch( ); }; } + +/** + * Wrap fetch so every MCP HTTP 401/403 is *reported* — control flow untouched, + * response returned as-is. + * + * This is the passive sibling of {@link createAuthChallengeInterceptFetch}, + * and it exists for the one case that one cannot cover: a first-time + * authorization in the default legacy era. There, `InspectorClient` + * deliberately builds the transport with no `authProvider` (so the SDK does + * not open a browser before the callback server is listening) and therefore + * leaves interception off, so the CLI/TUI runner catches a plain + * `UnauthorizedError` and calls `authenticate()` itself. That SDK error + * carries no headers, so the challenge's RFC 9728 `resource_metadata` would be + * lost before discovery ever starts (#2071) — the observer keeps it. + * + * Only the status line and headers are read; the body is left untouched for + * the transport (or the interceptor layered above this one) to consume. + */ +export function createAuthChallengeObserverFetch( + baseFetch: typeof fetch, + onChallenge: (challenge: AuthChallenge) => void, +): typeof fetch { + return async (input, init) => { + const response = await baseFetch(input, init); + if (response.status === 401 || response.status === 403) { + const challenge = parseAuthChallengeFromResponse(response); + /* v8 ignore next 3 -- parseAuthChallengeFromResponse only returns undefined for non-401/403, which the status guard above already excludes */ + if (challenge) { + onChallenge(challenge); + } + } + return response; + }; +} diff --git a/core/mcp/node/transport.ts b/core/mcp/node/transport.ts index ba943da3..b03a048f 100644 --- a/core/mcp/node/transport.ts +++ b/core/mcp/node/transport.ts @@ -12,7 +12,10 @@ import { StdioClientTransport } from "@modelcontextprotocol/client/stdio"; import { SSEClientTransport } from "@modelcontextprotocol/client"; import { StreamableHTTPClientTransport } from "@modelcontextprotocol/client"; import { createFetchTracker } from "../fetchTracking.js"; -import { createAuthChallengeInterceptFetch } from "./authChallengeFetch.js"; +import { + createAuthChallengeInterceptFetch, + createAuthChallengeObserverFetch, +} from "./authChallengeFetch.js"; import type { Dispatcher } from "undici"; /** Node's `RequestInit` plus the undici-specific `dispatcher` option. */ @@ -110,14 +113,21 @@ export function createTransportNode( authProvider, settings, interceptAuthChallenges = false, + onAuthChallengeObserved, } = options; // Proxy-wrap first so the auth-challenge interceptor observes responses from // the (optionally proxied) network call. const baseFetch = withProxyDispatcher(optionsFetchFn ?? globalThis.fetch); - const fetchWithOptionalAuthIntercept = interceptAuthChallenges - ? createAuthChallengeInterceptFetch(baseFetch) + // The observer sits *under* the interceptor so it still reports the + // challenge on the path where interception throws — and, more to the point, + // on the legacy first-auth path where interception is off entirely. + const observedFetch = onAuthChallengeObserved + ? createAuthChallengeObserverFetch(baseFetch, onAuthChallengeObserved) : baseFetch; + const fetchWithOptionalAuthIntercept = interceptAuthChallenges + ? createAuthChallengeInterceptFetch(observedFetch) + : observedFetch; if (serverType === "stdio") { const stdioConfig = config as StdioServerConfig; diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 6f032437..e4fe8830 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -82,6 +82,12 @@ export class OAuthManager { private oauthFlowState: OAuthFlowState | null = null; /** SEP-2350 union scope pending until interactive step-up completes. */ private pendingAuthorizationScope: string | undefined; + + /** + * The RFC 9728 metadata URL from the last 401/403 the transport observed. + * See {@link noteObservedAuthChallenge}. + */ + private observedResourceMetadataUrl: URL | undefined; private authChallengeMutex: Promise = Promise.resolve(); constructor(params: OAuthManagerParams) { @@ -263,6 +269,20 @@ export class OAuthManager { return authorizationUrl; } + /** + * Record a challenge the transport saw but did not raise as one. + * + * The legacy first-authorization path connects with no `authProvider` and no + * challenge interception, so the client calls {@link authenticate} off a + * headerless SDK `UnauthorizedError` — this is what carries the advertised + * RFC 9728 metadata URL across that gap (#2071). Latest-wins: every + * challenge comes from the same server, so the most recent one is what that + * server currently says, including when it says nothing. + */ + noteObservedAuthChallenge(challenge: AuthChallenge): void { + this.observedResourceMetadataUrl = challengeResourceMetadataUrl(challenge); + } + async authenticate(): Promise { if (this.isEnterpriseManaged()) { return this.authenticateEnterpriseManaged(); @@ -273,16 +293,23 @@ export class OAuthManager { provider.clearCapturedAuthUrl(); + // No challenge is passed to this entry point — it is reached from a plain + // unauthorized error — so the last one the transport observed is the only + // source for the advertised metadata URL. + const resourceMetadataUrl = this.observedResourceMetadataUrl; + await ensureCimdClientRegistration({ serverUrl, provider, fetchFn: this.params.effectiveAuthFetch, + resourceMetadataUrl, }); const result = await mcpAuth(provider, { serverUrl, scope: provider.scope, fetchFn: this.params.effectiveAuthFetch, + resourceMetadataUrl, }); if (result === "AUTHORIZED") { diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 79475bc4..b2d66daf 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -34,6 +34,7 @@ import type { RedirectUrlProvider, } from "../auth/providers.js"; import type { OAuthStorage } from "../auth/storage.js"; +import type { AuthChallenge } from "../auth/challenge.js"; // Stdio transport config export interface StdioServerConfig { @@ -867,6 +868,15 @@ export interface CreateTransportOptions { * become {@link AuthChallengeError} before the SDK calls `auth()` on a frozen provider. */ interceptAuthChallenges?: boolean; + + /** + * Called for every HTTP 401/403 the transport sees, without altering control + * flow. Unlike {@link interceptAuthChallenges} this never throws, so it also + * covers the legacy first-time-authorization path where interception is + * deliberately off and the SDK's headerless `UnauthorizedError` would + * otherwise lose the challenge's `resource_metadata` (#2071). + */ + onAuthChallengeObserved?: (challenge: AuthChallenge) => void; } export interface CreateTransportResult { diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index 631f704b..c5375236 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -2914,7 +2914,10 @@ export function createOAuthTestServerConfig(options: { mode: "combined", requireAuth: options.requireAuth ?? false, scopesSupported: options.scopesSupported ?? ["mcp"], - ...(options.resourceMetadataPath + // `!== undefined`, not truthiness: an explicitly supplied `""` is + // invalid, and dropping it here would silently fall back to the + // well-known route instead of reporting the bad fixture (Copilot). + ...(options.resourceMetadataPath !== undefined ? { resourceMetadataPath: options.resourceMetadataPath } : {}), staticClients: options.staticClients,