From 2bb0b6245c52f22ce32ac52df1eaa87acde89ebc Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 00:34:27 -0400 Subject: [PATCH] fix(oauth): walk the candidates in the CLI refresh path too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 1. `refreshStoredAuthToken` consumes the same fallback and made a single SDK discovery call, so preserving the path there — without the walk the other two consumers got — would have regressed exactly the case the walk exists to keep: a path-hosted MCP server whose authorization metadata is published at the domain root. It injects its own discovery function as a test seam, so the walk is now also exposed as `discoverAuthorizationServerMetadataFromCandidates(candidates, discover)`, which takes that function and reports *which* candidate answered — the CLI needs the winning URL, since it is the base the token request is made against, not just the metadata. Also drops the `as unknown as typeof fetch` from the new test in favour of `vi.fn()`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- AGENTS.md | 12 ++- clients/cli/__tests__/stored-auth.test.ts | 91 +++++++++++++++++++ clients/cli/src/cli.ts | 32 +++++-- .../test/integration/auth/discovery.test.ts | 4 +- core/auth/discovery.ts | 48 +++++++--- 5 files changed, 163 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 46fb431b..e854e9de 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,7 +90,17 @@ v2/main/ │ │ # what keeps a server that merely lives │ │ # under a path while publishing metadata │ │ # at the root working, so do not drop it -│ │ # in favour of a straight swap — #2110; +│ │ # in favour of a straight swap. EVERY +│ │ # consumer of the fallback must walk: +│ │ # discoverScopes, the CIMD probe, and +│ │ # the CLI's refreshStoredAuthToken, +│ │ # which is why the walk is also exposed +│ │ # as ...FromCandidates(candidates, +│ │ # discover) — the CLI injects its own +│ │ # discovery function as a test seam and +│ │ # needs to know WHICH candidate +│ │ # answered, since that is the base its +│ │ # token request is made against — #2110; │ │ # scopes.ts SEP-2350 scope union, oauthUx.ts │ │ # shared copy, mcpAuth.ts force-reauthorization, │ │ # issuerBinding.ts SEP-2352 callback-leg failure diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index 1b9b5768..3fb5c0b1 100644 --- a/clients/cli/__tests__/stored-auth.test.ts +++ b/clients/cli/__tests__/stored-auth.test.ts @@ -158,6 +158,97 @@ describe("refreshStoredAuthToken", () => { } }); + // #2110: with no stored metadata the MCP server URL stands in as the + // authorization server. A path-hosted server has two plausible answers, and + // both must keep working — the path-scoped URL for a server that publishes + // its metadata under its own path, the bare origin for one that merely lives + // under a path. The candidate that answers is also the base the token request + // is made against, so the walk has to report *which* one it was. + it("prefers the path-scoped authorization server for a path-hosted MCP server", async () => { + const path = writeOAuthFixture({ + [SERVER]: { + tokens: { refresh_token: "old-refresh", token_type: "Bearer" }, + clientInformation: { client_id: "cid" }, + }, + }); + try { + const refresh = vi.fn().mockResolvedValue(freshTokens); + const pathMetadata = { + issuer: "https://api.example/mcp", + token_endpoint: "https://api.example/mcp/token", + }; + const discover = vi.fn().mockResolvedValue(pathMetadata); + + await refreshStoredAuthToken(SERVER, path, { refresh, discover }); + + expect(discover).toHaveBeenCalledTimes(1); + expect(discover).toHaveBeenCalledWith(new URL("https://api.example/mcp")); + const [authServerUrl, opts] = refresh.mock.calls[0]!; + expect(authServerUrl).toEqual(new URL("https://api.example/mcp")); + expect(opts.metadata).toEqual(pathMetadata); + } finally { + rmSync(path, { force: true }); + } + }); + + it("falls back to the origin when a path-hosted server publishes metadata at the root", async () => { + const path = writeOAuthFixture({ + [SERVER]: { + tokens: { refresh_token: "old-refresh", token_type: "Bearer" }, + clientInformation: { client_id: "cid" }, + }, + }); + try { + const refresh = vi.fn().mockResolvedValue(freshTokens); + const rootMetadata = { + issuer: "https://api.example", + token_endpoint: "https://api.example/token", + }; + const discover = vi + .fn() + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce(rootMetadata); + + const token = await refreshStoredAuthToken(SERVER, path, { + refresh, + discover, + }); + + expect(token).toBe("refreshed-access-token"); + expect(discover).toHaveBeenNthCalledWith( + 2, + new URL("https://api.example/"), + ); + // The candidate that answered is the one the token request targets. + const [authServerUrl, opts] = refresh.mock.calls[0]!; + expect(authServerUrl).toEqual(new URL("https://api.example/")); + expect(opts.metadata).toEqual(rootMetadata); + } finally { + rmSync(path, { force: true }); + } + }); + + it("keeps the path-scoped candidate as the token-request base when no candidate answers", async () => { + const path = writeOAuthFixture({ + [SERVER]: { + tokens: { refresh_token: "old-refresh", token_type: "Bearer" }, + clientInformation: { client_id: "cid" }, + }, + }); + try { + const refresh = vi.fn().mockResolvedValue(freshTokens); + const discover = vi.fn().mockResolvedValue(undefined); + + await refreshStoredAuthToken(SERVER, path, { refresh, discover }); + + expect(discover).toHaveBeenCalledTimes(2); + const [authServerUrl] = refresh.mock.calls[0]!; + expect(authServerUrl).toEqual(new URL("https://api.example/mcp")); + } finally { + rmSync(path, { force: true }); + } + }); + it("uses the distinct no_client_information code when client info is missing", async () => { const path = writeOAuthFixture({ [SERVER]: { diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index c80cd1ab..af4bed16 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -47,7 +47,11 @@ import { serializeOAuthPersistBlob, type OAuthPersistSnapshot, } from "@inspector/core/auth/oauth-persist.js"; -import { getAuthorizationServerUrl } from "@inspector/core/auth/discovery.js"; +import { + discoverAuthorizationServerMetadataFromCandidates, + getAuthorizationServerUrl, + getAuthorizationServerUrlCandidates, +} from "@inspector/core/auth/discovery.js"; import { writeStoreFile } from "@inspector/core/storage/store-io.js"; import { refreshAuthorization, @@ -347,11 +351,27 @@ export async function refreshStoredAuthToken( ); } - const authServerUrl = found.state.serverMetadata?.issuer - ? new URL(found.state.serverMetadata.issuer) - : getAuthorizationServerUrl(serverUrl); - const metadata = - found.state.serverMetadata ?? (await discover(authServerUrl)) ?? undefined; + // With stored metadata the issuer settles it. Without, the MCP server URL + // stands in as the authorization server — and a path-hosted server has two + // plausible answers, so walk them rather than committing to the path-scoped + // one: a server that merely lives under a path while publishing its metadata + // at the domain root must keep working (#2110). The candidate that *answered* + // becomes `authServerUrl`, since it is also the base the token request below + // is made against. + let authServerUrl: URL; + let metadata = found.state.serverMetadata ?? undefined; + if (found.state.serverMetadata?.issuer) { + authServerUrl = new URL(found.state.serverMetadata.issuer); + } else { + const discovered = await discoverAuthorizationServerMetadataFromCandidates( + getAuthorizationServerUrlCandidates(serverUrl), + discover, + ); + authServerUrl = + discovered?.authorizationServerUrl ?? + getAuthorizationServerUrl(serverUrl); + metadata = discovered?.metadata; + } let tokens: OAuthTokens; try { diff --git a/clients/web/src/test/integration/auth/discovery.test.ts b/clients/web/src/test/integration/auth/discovery.test.ts index cfe58bab..213e9677 100644 --- a/clients/web/src/test/integration/auth/discovery.test.ts +++ b/clients/web/src/test/integration/auth/discovery.test.ts @@ -473,12 +473,12 @@ describe("path-hosted MCP servers (#2110)", () => { vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue( undefined, ); - const fetchFn = vi.fn(); + const fetchFn = vi.fn(); await discoverAuthorizationServerMetadataForServer( pathServerUrl, undefined, - fetchFn as unknown as typeof fetch, + fetchFn, ); expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith( diff --git a/core/auth/discovery.ts b/core/auth/discovery.ts index 23f5fed4..789c1303 100644 --- a/core/auth/discovery.ts +++ b/core/auth/discovery.ts @@ -74,33 +74,35 @@ export function getAuthorizationServerUrlCandidates( return [primary, new URL("/", primary)]; } +export interface DiscoveredAuthorizationServer { + /** The candidate that answered — not necessarily the first one tried. */ + authorizationServerUrl: URL; + metadata: NonNullable; +} + /** - * Discovers authorization server metadata for an MCP server, walking the - * candidates above until one answers. + * Walks `candidates` until one answers, and reports **which** one did. * * A candidate that fails — a 5xx, or an RFC 8414 §3.3 issuer mismatch, both of * which the SDK raises rather than returning `undefined` — is not fatal while * another candidate remains. If every candidate fails, the first error is * rethrown, so a caller still sees the original diagnosis rather than a bare * `undefined`. + * + * `discover` is injected rather than closed over so a caller that already owns + * a discovery function — the CLI's `refreshStoredAuthToken`, which takes one as + * a test seam — can walk the same candidates without giving that seam up. */ -export async function discoverAuthorizationServerMetadataForServer( - serverUrl: string, - resourceMetadata?: OAuthProtectedResourceMetadata | null, - fetchFn?: typeof fetch, -): Promise { - const candidates = getAuthorizationServerUrlCandidates( - serverUrl, - resourceMetadata, - ); +export async function discoverAuthorizationServerMetadataFromCandidates( + candidates: URL[], + discover: (url: URL) => Promise, +): Promise { let firstError: unknown; let sawError = false; for (const candidate of candidates) { try { - const metadata = await discoverAuthorizationServerMetadata(candidate, { - fetchFn, - }); - if (metadata) return metadata; + const metadata = await discover(candidate); + if (metadata) return { authorizationServerUrl: candidate, metadata }; } catch (err) { if (!sawError) { firstError = err; @@ -112,6 +114,22 @@ export async function discoverAuthorizationServerMetadataForServer( return undefined; } +/** + * Discovers authorization server metadata for an MCP server, walking the + * candidates above until one answers. + */ +export async function discoverAuthorizationServerMetadataForServer( + serverUrl: string, + resourceMetadata?: OAuthProtectedResourceMetadata | null, + fetchFn?: typeof fetch, +): Promise { + const found = await discoverAuthorizationServerMetadataFromCandidates( + getAuthorizationServerUrlCandidates(serverUrl, resourceMetadata), + (candidate) => discoverAuthorizationServerMetadata(candidate, { fetchFn }), + ); + return found?.metadata; +} + /** * Discovers OAuth scopes from server metadata, with preference for resource metadata scopes * @param serverUrl - The MCP server URL