From a8cbc506ab16824c59dd9c5b51de4b53221b504a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 10 Sep 2026 22:49:36 -0400 Subject: [PATCH] fix: address Copilot review round 1 on #2306 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validate `oauth.clientMetadataPath` the way the other two metadata paths are validated — in `loadConfig` for the JSON route and again at server setup for a `ServerConfig` built in code. This path is not merely advertised: it becomes the served document's own `client_id`, so an off-origin or query-bearing value would publish a client id this server cannot honour. Derive that `client_id` from `req.originalUrl` rather than from the registered route, so a document fetched with a query string answers with a `client_id` byte-identical to the URL it was fetched from — the equality CIMD turns on. Correct the docs' fixed-port warning: this fixture configures no `issuerUrl` and derives the `client_id` from the request, which is exactly why the integration test can drive it on a harness-chosen port. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UWGH2ytPzSPxnp4cibaV3Z Signed-off-by: cliffhall --- .../mcp/oauth-cimd-fixture.test.ts | 56 +++++++++++++++++++ docs/test-servers.md | 9 +-- test-servers/src/composable-test-server.ts | 11 +++- test-servers/src/load-config.ts | 11 ++++ test-servers/src/test-server-oauth.ts | 31 +++++++++- 5 files changed, 111 insertions(+), 7 deletions(-) diff --git a/clients/web/src/test/integration/mcp/oauth-cimd-fixture.test.ts b/clients/web/src/test/integration/mcp/oauth-cimd-fixture.test.ts index e5621830..759d1d2a 100644 --- a/clients/web/src/test/integration/mcp/oauth-cimd-fixture.test.ts +++ b/clients/web/src/test/integration/mcp/oauth-cimd-fixture.test.ts @@ -1,4 +1,6 @@ import { describe, it, expect, afterEach } from "vitest"; +import fs from "node:fs"; +import os from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { @@ -119,6 +121,60 @@ describe("CIMD showcase fixture (#2242)", () => { expect(doc.token_endpoint_auth_method).toBe("none"); }); + it("preserves a query-bearing document URL in the client_id it publishes", async () => { + const started = await startShowcase(); + const documentUrl = `${originOf(started)}/client-metadata.json?profile=a`; + + const res = await fetch(documentUrl); + expect(res.ok).toBe(true); + const doc = await res.json(); + + // CIMD turns on the document's `client_id` being the URL it was fetched + // from. Answering `?profile=a` with the bare route would publish a + // document that fails that equality for a client id the server just + // served (Copilot). + expect(doc.client_id).toBe(documentUrl); + }); + + it("rejects a clientMetadataPath that would publish a foreign client_id", () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "cimd-config-")); + const badPath = path.join(dir, "bad-cimd.json"); + const base = JSON.parse(fs.readFileSync(configPath, "utf8")); + fs.writeFileSync( + badPath, + JSON.stringify({ + ...base, + oauth: { ...base.oauth, clientMetadataPath: "//other-host/doc" }, + }), + ); + + try { + // The path is not merely advertised: it becomes the document's own + // `client_id`, so an off-origin value publishes a client id naming a + // host this server does not serve. + expect(() => loadConfig(badPath)).toThrow(/clientMetadataPath/); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("rejects the same path built programmatically rather than from JSON", async () => { + const resolved = resolveConfig(loadConfig(configPath)); + const started = createTestServerHttp({ + ...resolved, + oauth: { ...resolved.oauth!, clientMetadataPath: "/doc?version=1" }, + serverInfo: createTestServerInfo("oauth-cimd-badpath-test", "1.0.0"), + port: undefined, + }); + server = started; + + // `loadConfig` covers the JSON route only, so the server-setup check is + // what catches a `ServerConfig` assembled in code — the same split the + // two existing metadata paths have. + await expect(started.start()).rejects.toThrow(/clientMetadataPath/); + server = null; + }); + it("does not serve the document when CIMD is switched off", async () => { const resolved = resolveConfig(loadConfig(configPath)); const started = createTestServerHttp({ diff --git a/docs/test-servers.md b/docs/test-servers.md index 2b257590..55afa07a 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -584,10 +584,11 @@ against this list. The shipped fixture lists **6274** (the default), **6330** an other port the flow fails with `Invalid redirect_uri`, which reads like a CIMD problem and is not one. Add your port to the config rather than debugging the registration path. -⚠️ **The same fixed-`issuerUrl` hazard as the fixture above applies**, for the same reason: the -`client_id` this server publishes is derived from its issuer URL, so a server that walked to another -port on `EADDRINUSE` publishes a `client_id` pointing at whatever process holds 8092. Check with -`lsof -nP -iTCP:8092 -sTCP:LISTEN` before believing a failure. +This fixture has **no** fixed-`issuerUrl` hazard, unlike several of the ones above: it configures no +`issuerUrl`, and the document's `client_id` is derived from the request it was fetched over — query +string included — so a server that walked to another port on `EADDRINUSE` still publishes a +`client_id` equal to the URL you fetched, and the integration test drives it on a harness-chosen port +for exactly that reason. The fixed-port dependency that *does* bite is `redirect_uris`, above. ## Revoking tokens on clear (RFC 7009) diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 2bdf78e9..2c094fc9 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -759,7 +759,16 @@ export interface ServerConfig { scope?: string; }; - /** Where to serve `clientMetadata` (default `/client-metadata.json`). */ + /** + * Where to serve `clientMetadata` (default `/client-metadata.json`). + * + * Must be origin-relative with no query or fragment, and is validated as + * such — both by `loadConfig` and again at server setup for a config built + * in code. Unlike the other metadata paths this one is not merely + * advertised: it becomes the document's own `client_id`, so an off-origin + * or query-bearing value would publish a client id this server cannot + * honour (Copilot). + */ clientMetadataPath?: string; /** diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 2b4f2e83..865fdaab 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -48,6 +48,11 @@ export interface ConfigFileOAuth { clientName?: string; scope?: string; }; + /** + * Where to serve `clientMetadata` (default `/client-metadata.json`); + * validated as an origin-relative path, since it becomes the document's own + * `client_id`. See `composable-test-server.ts`. + */ clientMetadataPath?: string; tokenExpirationSeconds?: number; supportRefreshTokens?: boolean; @@ -260,6 +265,12 @@ function validateConfig( `Invalid config in ${filePath}: oauth.asMetadataPath must be an origin-relative path (e.g. "/.well-known/openid-configuration") — a value such as "//host/doc" would move the document off this server entirely`, ); } + const cimdPath = oauth.clientMetadataPath; + if (cimdPath !== undefined && !isOriginRelativePath(cimdPath)) { + throw new Error( + `Invalid config in ${filePath}: oauth.clientMetadataPath must be an origin-relative path (e.g. "/client-metadata.json") — this path becomes the document's own client_id, so a value such as "//host/doc" would publish a client id naming a host this server does not serve`, + ); + } if (transportType === "stdio" && oauth.enabled === true) { throw new Error( `Invalid config in ${filePath}: oauth requires streamable-http or sse transport`, diff --git a/test-servers/src/test-server-oauth.ts b/test-servers/src/test-server-oauth.ts index 1ca7fa2d..19c716ca 100644 --- a/test-servers/src/test-server-oauth.ts +++ b/test-servers/src/test-server-oauth.ts @@ -101,6 +101,27 @@ function asMetadataPath(config: OAuthConfig): string { return path; } +/** + * Where the CIMD client metadata document is served from, validated the same + * way the two metadata paths above are — and for a sharper reason than either. + * This path is not merely advertised: it becomes the document's own + * `client_id`, so a value such as `//other-host/doc` would publish a client id + * naming a host this server does not control, and `/doc?version=1` would + * publish one that cannot reach the route Express registered (Copilot). + */ +function clientMetadataPath(config: OAuthConfig): string { + const path = config.clientMetadataPath; + if (path === undefined) { + return "/client-metadata.json"; + } + if (!isOriginRelativePath(path)) { + throw new Error( + `oauth.clientMetadataPath must be an origin-relative path (got ${JSON.stringify(path)})`, + ); + } + return path; +} + /** * The `WWW-Authenticate` challenge sent with every 401. * @@ -294,15 +315,21 @@ function setupMetadataEndpoints( // nothing. if (config.supportCIMD && config.clientMetadata) { const doc = config.clientMetadata; - const metadataPath = config.clientMetadataPath ?? "/client-metadata.json"; + const metadataPath = clientMetadataPath(config); app.get(metadataPath, (req: Request, res: Response) => { // Derived from the request rather than from `issuerUrl`, so the // document's own `client_id` always equals the URL it was fetched from // — which is what CIMD requires, and what stays true if the server // walked to another port on EADDRINUSE. + // + // `originalUrl` rather than the registered route, so a client id that + // carries a query string (`/client-metadata.json?profile=a`) still gets + // a document whose `client_id` is byte-identical to the URL that was + // fetched. Answering with the bare route instead would hand back a + // document that fails the very equality CIMD turns on (Copilot). const requestBaseUrl = `${req.protocol}://${req.get("host")}`; res.json({ - client_id: new URL(metadataPath, requestBaseUrl).href, + client_id: new URL(req.originalUrl, requestBaseUrl).href, client_name: doc.clientName ?? "MCP Inspector (CIMD test fixture)", redirect_uris: doc.redirectUris, // CIMD clients are public and authenticate with nothing; the server's