diff --git a/clients/web/src/test/core/auth/cimd.test.ts b/clients/web/src/test/core/auth/cimd.test.ts index caf2e30e..63bc64a5 100644 --- a/clients/web/src/test/core/auth/cimd.test.ts +++ b/clients/web/src/test/core/auth/cimd.test.ts @@ -105,24 +105,21 @@ describe("ensureCimdClientRegistration", () => { const metadataUrl = new URL( "http://127.0.0.1:9999/custom/protected-resource", ); - // `ensureCimdClientRegistration` runs protected-resource discovery through - // the SDK's default fetch, so the advertised URL is observable only on the - // global — the AS-metadata leg is the one that takes `fetchFn`. - const globalFetch = vi.fn(async (input: RequestInfo | URL) => - String(input) === metadataUrl.href - ? new Response( - JSON.stringify({ - resource: SERVER_URL, - authorization_servers: ["http://127.0.0.1:9999"], - }), - ) - : new Response(null, { status: 404 }), - ); - vi.stubGlobal("fetch", globalFetch); - - const fetchFn = vi.fn( - async () => - new Response( + // Both discovery legs must run through the configured fetch — on web that + // is the backend proxy, so a leg that fell back to the global would fail + // on CORS and be swallowed. + const fetchFn = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input); + if (url === metadataUrl.href) { + return new Response( + JSON.stringify({ + resource: SERVER_URL, + authorization_servers: ["http://127.0.0.1:9999"], + }), + ); + } + if (url.includes("/.well-known/oauth-authorization-server")) { + return new Response( JSON.stringify({ issuer: "http://127.0.0.1:9999", authorization_endpoint: "http://127.0.0.1:9999/oauth/authorize", @@ -130,8 +127,14 @@ describe("ensureCimdClientRegistration", () => { response_types_supported: ["code"], client_id_metadata_document_supported: true, }), - ), - ); + ); + } + return new Response(null, { status: 404 }); + }); + // Nothing may reach the network directly; a call here means a leg fell + // back to the global fetch. + const globalFetch = vi.fn(); + vi.stubGlobal("fetch", globalFetch); await ensureCimdClientRegistration({ serverUrl: SERVER_URL, @@ -140,10 +143,14 @@ describe("ensureCimdClientRegistration", () => { resourceMetadataUrl: metadataUrl, }); - expect(globalFetch).toHaveBeenCalled(); - for (const [input] of globalFetch.mock.calls) { - expect(String(input)).toBe(metadataUrl.href); - } + expect(globalFetch).not.toHaveBeenCalled(); + const requested = fetchFn.mock.calls.map(([input]) => String(input)); + expect(requested).toContain(metadataUrl.href); + expect( + requested.filter((url) => + url.includes("/.well-known/oauth-protected-resource"), + ), + ).toEqual([]); expect(storage.saveClientInformation).toHaveBeenCalledWith( SERVER_URL, { client_id: METADATA_URL }, 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 909ac187..bda34f61 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 @@ -17,6 +17,7 @@ import { TestServerHttp, getDefaultServerConfig, createOAuthTestServerConfig, + isOriginRelativePath, loadConfig, resolveConfig, waitForOAuthWellKnown, @@ -103,4 +104,20 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => { resourceMetadataPath: METADATA_PATH, }); }); + + it.each([ + ["/custom/protected-resource", true], + ["/a", true], + // Both re-point the origin when resolved against the request base while + // Express still registers the route locally, so the server would advertise + // a document it does not serve (Copilot). + ["//other-host/doc", false], + ["/\\other-host/doc", false], + ["https://other-host/doc", false], + ["custom/protected-resource", false], + ["/has a space", false], + ["", false], + ])("origin-relative path check: %j -> %s", (value, expected) => { + expect(isOriginRelativePath(value)).toBe(expected); + }); }); diff --git a/core/auth/cimd.ts b/core/auth/cimd.ts index 1b140d2b..d8ccc8bf 100644 --- a/core/auth/cimd.ts +++ b/core/auth/cimd.ts @@ -38,6 +38,12 @@ export async function ensureCimdClientRegistration(params: { resourceMetadata = await discoverOAuthProtectedResourceMetadata( params.serverUrl, { resourceMetadataUrl: params.resourceMetadataUrl }, + // The same fetch the AS-metadata leg below uses. On web that is + // `createRemoteFetch`, which proxies through the backend to sidestep + // CORS — on the global `fetch` this leg would fail in the browser, be + // swallowed by the catch, and leave CIMD probing the wrong + // authorization server (Copilot). + params.fetchFn, ); } catch { resourceMetadata = undefined; diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index c791eef2..ca1f75ef 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -6,6 +6,7 @@ import { readFileSync } from "fs"; import path from "path"; import YAML from "yaml"; +import { isOriginRelativePath } from "./test-server-oauth.js"; export interface PresetRef { preset: string; @@ -203,12 +204,9 @@ function validateConfig( } } const metadataPath = oauth.resourceMetadataPath; - if ( - metadataPath !== undefined && - (typeof metadataPath !== "string" || !metadataPath.startsWith("/")) - ) { + if (metadataPath !== undefined && !isOriginRelativePath(metadataPath)) { throw new Error( - `Invalid config in ${filePath}: oauth.resourceMetadataPath must be an absolute path starting with "/"`, + `Invalid config in ${filePath}: oauth.resourceMetadataPath must be an origin-relative path (e.g. "/custom/protected-resource") — a value such as "//host/doc" would advertise a document the server does not serve`, ); } if (transportType === "stdio" && oauth.enabled === true) { diff --git a/test-servers/src/test-server-oauth.ts b/test-servers/src/test-server-oauth.ts index 946f7030..71280861 100644 --- a/test-servers/src/test-server-oauth.ts +++ b/test-servers/src/test-server-oauth.ts @@ -27,6 +27,54 @@ export function getOAuthMode( return config.mode ?? "combined"; } +const PATH_VALIDATION_BASE = "http://config.invalid"; + +/** + * True for a path that resolves under its own origin — the only shape safe to + * use as both an Express route and a `resource_metadata` value. + * + * A leading-slash check is not enough: `//other-host/doc` and `/\other-host/doc` + * both re-point the origin when resolved against the request base (the URL + * parser folds a backslash into a slash for special schemes), while Express + * still registers the route locally — so the server would advertise a document + * it does not serve (Copilot). Comparing the resolved href against the literal + * also rejects anything the parser would rewrite (spaces, unescaped + * characters), which an Express route would not match either. + */ +export function isOriginRelativePath(value: unknown): value is string { + if (typeof value !== "string" || !value.startsWith("/")) { + return false; + } + try { + const resolved = new URL(value, PATH_VALIDATION_BASE); + return ( + resolved.origin === PATH_VALIDATION_BASE && + resolved.href === `${PATH_VALIDATION_BASE}${value}` + ); + } catch { + return false; + } +} + +/** + * The configured metadata path, validated. Throws at server-setup time rather + * than serving a route that contradicts the challenge — the JSON-config path + * is validated earlier by `load-config`, so this covers a `ServerConfig` + * built programmatically. + */ +function resourceMetadataPath(config: OAuthConfig): string | undefined { + const path = config.resourceMetadataPath; + if (path === undefined) { + return undefined; + } + if (!isOriginRelativePath(path)) { + throw new Error( + `oauth.resourceMetadataPath must be an origin-relative path (got ${JSON.stringify(path)})`, + ); + } + return path; +} + /** * The `WWW-Authenticate` challenge sent with every 401. * @@ -36,7 +84,7 @@ export function getOAuthMode( * `Bearer` challenge keeps the existing fixtures byte-identical. */ function bearerChallenge(config: OAuthConfig, req: Request): string { - const path = config.resourceMetadataPath; + const path = resourceMetadataPath(config); if (!path) { return "Bearer"; } @@ -206,7 +254,7 @@ function setupMetadataEndpoints( // a client that ignores the advertised `resource_metadata` URL gets a 404 // — see the field's doc comment. app.get( - config.resourceMetadataPath ?? "/.well-known/oauth-protected-resource", + resourceMetadataPath(config) ?? "/.well-known/oauth-protected-resource", (req: Request, res: Response) => { const requestBaseUrl = `${req.protocol}://${req.get("host")}`; const resourceUrl = config.resource ?? new URL("/", requestBaseUrl).href;