diff --git a/clients/web/src/test/core/mcp/node/authChallengeFetch.test.ts b/clients/web/src/test/core/mcp/node/authChallengeFetch.test.ts index f94b7241..4dbfc46c 100644 --- a/clients/web/src/test/core/mcp/node/authChallengeFetch.test.ts +++ b/clients/web/src/test/core/mcp/node/authChallengeFetch.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect, vi } from "vitest"; -import { createAuthChallengeInterceptFetch } from "@inspector/core/mcp/node/authChallengeFetch.js"; +import { + createAuthChallengeInterceptFetch, + createAuthChallengeObserverFetch, +} from "@inspector/core/mcp/node/authChallengeFetch.js"; import { AuthChallengeError } from "@inspector/core/auth/challenge.js"; describe("createAuthChallengeInterceptFetch", () => { @@ -84,3 +87,63 @@ describe("createAuthChallengeInterceptFetch", () => { ); }); }); + +describe("createAuthChallengeObserverFetch", () => { + const METADATA_URL = "http://127.0.0.1:3001/custom/protected-resource"; + + it("reports a challenge without altering the response or its body", async () => { + const baseFetch = vi.fn( + async () => + new Response("payload", { + status: 401, + headers: { + "WWW-Authenticate": `Bearer resource_metadata="${METADATA_URL}"`, + }, + }), + ); + const onChallenge = vi.fn(); + const fetchFn = createAuthChallengeObserverFetch(baseFetch, onChallenge); + + const res = await fetchFn("https://example.com/mcp"); + + expect(res.status).toBe(401); + // The body must survive for the transport (or the interceptor above). + expect(await res.text()).toBe("payload"); + expect(onChallenge).toHaveBeenCalledWith( + expect.objectContaining({ resourceMetadataUrl: METADATA_URL }), + ); + }); + + it("reports a 403 challenge too", async () => { + const baseFetch = vi.fn( + async () => + new Response(null, { + status: 403, + headers: { "WWW-Authenticate": 'Bearer error="insufficient_scope"' }, + }), + ); + const onChallenge = vi.fn(); + + await createAuthChallengeObserverFetch( + baseFetch, + onChallenge, + )("https://example.com/mcp"); + + expect(onChallenge).toHaveBeenCalledWith( + expect.objectContaining({ reason: "insufficient_scope" }), + ); + }); + + it("stays silent on a non-challenge response", async () => { + const baseFetch = vi.fn(async () => new Response("ok", { status: 200 })); + const onChallenge = vi.fn(); + + const res = await createAuthChallengeObserverFetch( + baseFetch, + onChallenge, + )("https://example.com/mcp"); + + expect(res.status).toBe(200); + expect(onChallenge).not.toHaveBeenCalled(); + }); +}); 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 6f46ba28..d9965b68 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 @@ -11,7 +11,9 @@ */ import { describe, it, expect, beforeAll, afterAll, vi } from "vitest"; +import { createServer } from "node:http"; import { fileURLToPath } from "node:url"; +import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { discoverOAuthProtectedResourceMetadata } from "@modelcontextprotocol/client"; @@ -28,6 +30,7 @@ import { challengeResourceMetadataUrl, parseAuthChallengeFromResponse, } from "@inspector/core/auth/challenge.js"; +import type { AuthChallenge } from "@inspector/core/auth/challenge.js"; import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; import { @@ -35,16 +38,18 @@ import { 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"; +/** Fixed, since this one is asserted against a server of its own. */ +const METADATA_URL_LITERAL = "http://127.0.0.1:3001/custom/protected-resource"; describe("OAuth challenge resource_metadata (RFC 9728)", () => { let mcpServer: TestServerHttp | null = null; let serverUrl = ""; + let storageDir = ""; beforeAll(async () => { mcpServer = new TestServerHttp({ @@ -67,12 +72,17 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => { }); const port = await mcpServer.start(); serverUrl = `http://localhost:${port}`; + storageDir = mkdtempSync(join(tmpdir(), "mcp-inspector-2071-")); await waitForOAuthWellKnown(serverUrl); }, 30_000); afterAll(async () => { await mcpServer?.stop(); mcpServer = null; + if (storageDir) { + rmSync(storageDir, { recursive: true, force: true }); + storageDir = ""; + } }, 30_000); it("advertises the non-default metadata URL on the 401 challenge", async () => { @@ -120,30 +130,27 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => { // 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 spyFetch: typeof fetch = async (input, init) => { + requested.push(input instanceof Request ? input.url : String(input)); + return fetch(input, init); + }; const redirectUrlProvider: RedirectUrlProvider = { getRedirectUrl: () => REDIRECT_URL, }; const client = new InspectorClient( - { - type: "streamable-http", - url: `${serverUrl}/mcp`, - } as MCPServerConfig, + { type: "streamable-http", url: `${serverUrl}/mcp` }, { environment: { transport: createTransportNode, fetch: spyFetch, oauth: { - storage: new NodeOAuthStorage( - join(tmpdir(), `mcp-oauth-${process.pid}-2071.json`), - ), + // A fresh path per run. SDK `auth()` persists its discovery + // state — the advertised metadata URL included — so a reused file + // would let a watch-mode rerun satisfy discovery from the cache + // and pass even with the observer plumbing removed (Copilot). + storage: new NodeOAuthStorage(join(storageDir, "oauth.json")), navigation: new ConsoleNavigation(), redirectUrlProvider, }, @@ -174,6 +181,52 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => { await client.disconnect(); }, 30_000); + it("observes the SSE challenge through a caller-supplied eventSourceInit.fetch", async () => { + // That explicit fetch is deliberately not proxy- or intercept-wrapped, so + // it used to bypass the observer entirely and a first-time legacy SSE + // authorization lost the advertised URL the same way (Copilot). + const sseServer = createServer((_req, res) => { + res.writeHead(401, { + "WWW-Authenticate": `Bearer resource_metadata="${METADATA_URL_LITERAL}"`, + }); + res.end(); + }); + await new Promise((resolve) => sseServer.listen(0, resolve)); + const address = sseServer.address(); + /* v8 ignore next 3 -- listen(0) on a TCP socket always yields an AddressInfo */ + if (address === null || typeof address === "string") { + throw new Error("expected a TCP address"); + } + + const explicitFetch = vi.fn((input, init) => + fetch(input, init), + ); + const observed: AuthChallenge[] = []; + + try { + const { transport } = createTransportNode( + { + type: "sse", + url: `http://127.0.0.1:${address.port}/sse`, + eventSourceInit: { fetch: explicitFetch }, + }, + { + onAuthChallengeObserved: (challenge) => observed.push(challenge), + }, + ); + + await expect(transport.start()).rejects.toThrow(); + await transport.close().catch(() => {}); + } finally { + await new Promise((resolve) => sseServer.close(() => resolve())); + } + + expect(explicitFetch).toHaveBeenCalled(); + expect(observed).toContainEqual( + expect.objectContaining({ resourceMetadataUrl: METADATA_URL_LITERAL }), + ); + }, 30_000); + it("keeps the showcase config valid and carrying the custom path", () => { const configPath = fileURLToPath( new URL( diff --git a/core/mcp/node/transport.ts b/core/mcp/node/transport.ts index b03a048f..5521c1cd 100644 --- a/core/mcp/node/transport.ts +++ b/core/mcp/node/transport.ts @@ -119,15 +119,18 @@ export function createTransportNode( // Proxy-wrap first so the auth-challenge interceptor observes responses from // the (optionally proxied) network call. const baseFetch = withProxyDispatcher(optionsFetchFn ?? globalThis.fetch); + // Purely passive, so — unlike proxying and interception — it is safe to + // apply to any fetch, including a caller's explicit one. + const withChallengeObserver = (inner: typeof fetch): typeof fetch => + onAuthChallengeObserved + ? createAuthChallengeObserverFetch(inner, onAuthChallengeObserved) + : inner; // 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; + ? createAuthChallengeInterceptFetch(withChallengeObserver(baseFetch)) + : withChallengeObserver(baseFetch); if (serverType === "stdio") { const stdioConfig = config as StdioServerConfig; @@ -160,9 +163,15 @@ export function createTransportNode( // A caller-supplied eventSourceInit.fetch wins as-is (explicit fetch is not // re-wrapped for proxying); the default path uses fetchWithOptionalAuthIntercept // (the proxy-aware baseFetch plus the optional auth-challenge intercept). - const sseFetch = - (sseConfig.eventSourceInit?.fetch as typeof fetch) || - fetchWithOptionalAuthIntercept; + // The challenge observer is applied either way: it is passive, and without + // it a first-time legacy SSE authorization through an explicit fetch would + // lose the challenge's `resource_metadata` the same way (Copilot). + const configuredSseFetch = sseConfig.eventSourceInit?.fetch as + | typeof fetch + | undefined; + const sseFetch = configuredSseFetch + ? withChallengeObserver(configuredSseFetch) + : fetchWithOptionalAuthIntercept; const trackedFetch = onFetchRequest ? createFetchTracker(sseFetch, { trackRequest: onFetchRequest,