fix: address Copilot review round 1 on #2306

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UWGH2ytPzSPxnp4cibaV3Z
Signed-off-by: cliffhall <cliff@futurescale.com>

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({

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)
 

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;
 
     /**

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`,

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