fix(hub)!: close the discovery status oracle and tool name collisions (#19)

The discovery route refuses plain-http loopback targets unless the host sets
allowLoopback, and reports an upstream non-2xx as a refusal without its
status. McpError carries that status. A catalog listing a tool name twice is
refused by tools/list, mcpTools and mcpServers, and a handle or server name
containing "." is refused so no tool falls under another server's grant.
mcpTools shares mcpServers' server-list check: an empty or repeated name, or
a non-https URL, is refused before any request.

BREAKING CHANGE: plain-http loopback discovery needs allowLoopback, and
handles or server names containing "." are refused.

Author: TheGreatAxios

README.md

@@ -70,7 +70,7 @@ Every client call takes `{ fetch, timeoutMs, signal, session }` as its last argu
 
 ### Grants
 
-Every call is checked as resource `tool:<name>`. The most specific match wins, and `deny` beats `ask` beats `allow`. `tool:<handle>.<tool>` grants one remote tool and `tool:<handle>.*` covers a whole server.
+Every call is checked as resource `tool:<name>`. The most specific match wins, and `deny` beats `ask` beats `allow`. `tool:<handle>.<tool>` grants one remote tool and `tool:<handle>.*` covers a whole server. A handle or server name may not contain `.`, and a catalog may not list a tool name twice, so no tool falls under another server's grant.
 
 List `"<handle>.<tool>"` in `allowWithoutAsk` to let an `allow` grant run that tool without approval. The list is ignored for tools flagged `destructiveHint: true`.
 
@@ -112,7 +112,7 @@ mountMcpDiscovery(mcpRoutes, {
 
 Mount `mcpRoutes` on the hub app under `/api/tenants/:tenantId`, behind the hub's auth and tenant middleware.
 
-`POST /api/tenants/:tenantId/mcp/discover` with `{ url, credentialId? }` returns `{ data: { serverInfo, tools } }`. `url` must be https (http only on loopback). `credentialId` names a tenant credential whose secret is sent as a bearer; a credential holding `MCP_NO_TOKEN_SENTINEL` from `@corbits/credential-http` sends no `authorization` header. Errors: 400 for a bad body or URL, 404 for an unknown credential or one that is not active or has expired, 422 when the server fails discovery, the secret is not a valid header value, or the request would leave the credential's origin. A 422 carries only this package's own messages; any other failure reads as a generic handshake error, so no response or `onError` text quotes the secret. `requireGrant` is the host's own grant middleware for this route.
+`POST /api/tenants/:tenantId/mcp/discover` with `{ url, credentialId? }` returns `{ data: { serverInfo, tools } }`. `url` must be https; plain-http loopback is refused unless the host sets `allowLoopback: true` for local development. `credentialId` names a tenant credential whose secret is sent as a bearer; a credential holding `MCP_NO_TOKEN_SENTINEL` from `@corbits/credential-http` sends no `authorization` header. Errors: 400 for a bad body or URL, 404 for an unknown credential or one that is not active or has expired, 422 when the server fails discovery, the secret is not a valid header value, or the request would leave the credential's origin. A 422 carries only this package's own messages and never the upstream HTTP status; any other failure reads as a generic handshake error, so no response or `onError` text quotes the secret. `requireGrant` is the host's own grant middleware for this route.
 
 When a secret is sent, the fetch is pinned to the origin of the credential's provider `apiBaseUrl`; with no credential or a keyless one, to the URL's origin. Redirects are always refused, so the secret never leaves that origin. Each discovery request times out after 30 seconds.
 
@@ -175,6 +175,7 @@ Grant the agent's principal `tool:deepwiki.*` for the whole server, or `tool:dee
 - Discovery with a credential pins to the credential's provider `apiBaseUrl` origin, not to the requested URL's origin. A credential with a secret whose provider has no `apiBaseUrl` is refused, and a URL on another origin needs `extraOrigins`. No origin is allowed by default.
 - `readCredentialSecret` is now `readCredential`, which returns `{ secret, origin? }`. `discoverMcpServer` takes `credential: { secret, origin? }` instead of `secret`.
 - `mcpInitialize` returns an `McpSession` (`protocolVersion`, `sessionId?`, `serverInfo?`) and sends `notifications/initialized`. Pass it as `{ session }` to `mcpListTools` and `mcpCallTool`; a stateful server needs it.
+- The discovery route refuses plain-http loopback URLs unless `allowLoopback` is set. Handles and server names containing `.`, and catalogs repeating a tool name, are refused. `mcpTools` refuses an empty or repeated server name, and a non-https URL, before any request.
 - `@intx/agent` and `@intx/harness` peers are `^0.4.0`.
 - Discovery rejects authorization-server metadata whose `issuer` differs from the one the protected resource names.
 

src/client.test.ts

@@ -352,3 +352,29 @@ describe("initialize hardening", () => {
     expect((failure as Error).message).toContain("within 50ms");
   });
 });
+
+describe("tools/list", () => {
+  test("a catalog with duplicate tool names is refused", async () => {
+    const server = Bun.serve({
+      port: 0,
+      async fetch(req) {
+        const parsed = RequestIdOnly(await req.json());
+        if (parsed instanceof type.errors)
+          return new Response("", { status: 400 });
+        const tool = { name: "dup", inputSchema: {} };
+        return Response.json({
+          jsonrpc: "2.0",
+          id: parsed.id,
+          result: { tools: [tool, tool] },
+        });
+      },
+    });
+    try {
+      await expect(mcpListTools(server.url.toString())).rejects.toThrow(
+        /duplicate tool names/,
+      );
+    } finally {
+      void server.stop(true);
+    }
+  });
+});

src/client.ts

@@ -51,9 +51,13 @@ const JsonRpcResponse = type({
 });
 
 export class McpError extends Error {
-  constructor(message: string) {
+  /** The HTTP status, when the server answered with a non-2xx one. */
+  readonly status: number | undefined;
+
+  constructor(message: string, status?: number) {
     super(message);
     this.name = "McpError";
+    this.status = status;
   }
 }
 
@@ -123,6 +127,7 @@ async function sendRequest(
       await response.body?.cancel();
       throw new McpError(
         `MCP server ${url} responded ${response.status} to ${method}`,
+        response.status,
       );
     }
 
@@ -188,6 +193,7 @@ async function sendNotification(
   if (!response.ok) {
     throw new McpError(
       `MCP server ${url} responded ${response.status} to ${method}`,
+      response.status,
     );
   }
 }
@@ -351,6 +357,12 @@ export async function mcpListTools(
       `MCP server ${url} sent a malformed tools/list result: ${parsed.summary}`,
     );
   }
+  const names = new Set(parsed.tools.map((tool) => tool.name));
+  if (names.size !== parsed.tools.length) {
+    throw new McpError(
+      `MCP server ${url} sent a tools/list result with duplicate tool names`,
+    );
+  }
   return parsed.tools;
 }
 

src/hub/discover.test.ts

@@ -29,6 +29,7 @@ function appWith(
     readonly status?: string;
     readonly expiresAt?: Date | null;
     readonly onError?: (error: unknown) => void;
+    readonly allowLoopback?: boolean;
   } = {},
 ): Hono<TenantEnv> {
   const app = new Hono<TenantEnv>();
@@ -74,6 +75,7 @@ function appWith(
     },
     extraOrigins: opts.extraOrigins,
     onError: opts.onError,
+    allowLoopback: opts.allowLoopback ?? true,
   } as unknown as MountMcpDiscoveryOpts;
   mountMcpDiscovery(app, mountOpts);
   return app;
@@ -337,4 +339,31 @@ describe("POST /mcp/discover", () => {
     expect(status).toBe(404);
     expect(handle.requestsSeen).toHaveLength(0);
   });
+
+  test("a plain-http loopback target is refused unless the host allows it", async () => {
+    handle = startTestMcpServer();
+    const { status, json } = await post(appWith({}, { allowLoopback: false }), {
+      url: handle.url,
+    });
+    expect(status).toBe(400);
+    expect(String(json["error"])).toContain("https");
+    expect(handle.requestsSeen).toHaveLength(0);
+  });
+
+  test("an upstream status is not echoed", async () => {
+    const server = Bun.serve({
+      port: 0,
+      fetch: () => new Response("<html>admin</html>", { status: 403 }),
+    });
+    try {
+      const { status, json } = await post(appWith({}), {
+        url: new URL("/admin", server.url).href,
+      });
+      expect(status).toBe(422);
+      expect(String(json["error"])).toEndWith("the server refused the request");
+      expect(String(json["error"])).not.toContain("403");
+    } finally {
+      await server.stop(true);
+    }
+  });
 });

src/hub/discover.ts

@@ -42,6 +42,11 @@ export type MountMcpDiscoveryOpts = {
    * origin. Passed through to `@corbits/credential-http`. Empty by default.
    */
   readonly extraOrigins?: Readonly<Record<string, readonly string[]>>;
+  /**
+   * Allow plain-http loopback targets, for local development only. Off by
+   * default so the route cannot be pointed at the hub's own ports.
+   */
+  readonly allowLoopback?: boolean;
   /** Reported when a discovery attempt fails; the caller only sees a message. */
   readonly onError?: (error: unknown, context: { url: string }) => void;
 };
@@ -205,6 +210,9 @@ export function mountMcpDiscovery(
         400,
       );
     }
+    if (target.protocol === "http:" && opts.allowLoopback !== true) {
+      return c.json({ error: "the MCP server URL must be https" }, 400);
+    }
 
     try {
       const found =
@@ -229,11 +237,16 @@ export function mountMcpDiscovery(
       return c.json({ data });
     } catch (cause) {
       // Only this package's own messages are passed on: anything else, such
-      // as a fetch error, may quote the material the request carried.
+      // as a fetch error, may quote the material the request carried. An
+      // upstream status is not echoed, so the route is no port-probing oracle.
       const error =
-        cause instanceof McpError
+        cause instanceof McpError && cause.status === undefined
           ? cause
-          : new McpError("the handshake failed");
+          : new McpError(
+              cause instanceof McpError
+                ? "the server refused the request"
+                : "the handshake failed",
+            );
       opts.onError?.(error, { url: body.url });
       return c.json(
         {

src/naming.ts

@@ -3,12 +3,55 @@
 // onto identical tool names and identical approval marks.
 
 import type { McpTool } from "./client.js";
+import { parseMcpEndpoint } from "./url.js";
 
 /** `<server>.<tool>`, the grantable unit: `tool:<server>.*` covers a server. */
 export function qualifiedName(server: string, tool: string): string {
   return `${server}.${tool}`;
 }
 
+/**
+ * Refuse a catalog whose names could reach another server's grants: a server
+ * name with a `.` could make `<server>.<tool>` fall under another server's
+ * `tool:<other>.*`, and a repeated tool name would make two tools one name.
+ */
+export function assertCatalogNames(
+  server: string,
+  tools: readonly McpTool[],
+): void {
+  if (server.includes(".")) {
+    throw new Error(
+      `MCP server name "${server}" must not contain "."; it would overlap another server's grants`,
+    );
+  }
+  const seen = new Set<string>();
+  for (const tool of tools) {
+    if (seen.has(tool.name)) {
+      throw new Error(
+        `MCP server "${server}" lists the tool "${tool.name}" more than once`,
+      );
+    }
+    seen.add(tool.name);
+  }
+}
+
+/**
+ * Refuse a server list before any request: every name non-empty and unique,
+ * so no two servers share a grant prefix, and every URL a permitted endpoint.
+ */
+export function assertServers(
+  servers: readonly { name: string; url: string }[],
+  label: string,
+): void {
+  const seen = new Set<string>();
+  for (const { name, url } of servers) {
+    if (name.length === 0) throw new Error(`empty ${label}`);
+    if (seen.has(name)) throw new Error(`duplicate ${label} "${name}"`);
+    seen.add(name);
+    parseMcpEndpoint(url);
+  }
+}
+
 export function toolDescription(tool: McpTool, server: string): string {
   const hints = Object.entries(tool.annotations ?? {})
     .filter(([, v]) => v === true)

src/sidecar-bundle.test.ts

@@ -313,3 +313,40 @@ describe("mcpServers round trip through a mediated handle", () => {
     expect(result.content).toContain("was cancelled");
   });
 });
+
+describe("catalog names", () => {
+  const tool = (name: string): McpTool => ({ name, inputSchema: {} });
+
+  test("a catalog listing a tool twice is refused", () => {
+    expect(() =>
+      mcpServers({
+        servers: [
+          {
+            handle: "srv",
+            url: "https://x.example.test/mcp",
+            tools: [tool("dup"), tool("dup")],
+          },
+        ],
+      }),
+    ).toThrow(/more than once/);
+  });
+
+  test("a handle that would fall under another server's grant is refused", () => {
+    expect(() =>
+      mcpServers({
+        servers: [
+          {
+            handle: "srv",
+            url: "https://x.example.test/mcp",
+            tools: [tool("read")],
+          },
+          {
+            handle: "srv.read",
+            url: "https://y.example.test/mcp",
+            tools: [tool("x")],
+          },
+        ],
+      }),
+    ).toThrow(/must not contain "."/);
+  });
+});

src/sidecar-bundle.ts

@@ -26,8 +26,13 @@ import {
   McpToolSchema,
   type McpTool,
 } from "./client.js";
-import { isAskExempt, qualifiedName, toolDescription } from "./naming.js";
-import { parseMcpEndpoint } from "./url.js";
+import {
+  assertCatalogNames,
+  assertServers,
+  isAskExempt,
+  qualifiedName,
+  toolDescription,
+} from "./naming.js";
 
 /** The consumer key a host matches a credential binding against. */
 export const SIDECAR_BUNDLE_ID = "@corbits/mcp/sidecar-bundle";
@@ -94,21 +99,18 @@ function assertConfig(config: McpServersConfig): void {
   if (parsed instanceof type.errors) {
     throw new Error(`invalid @corbits/mcp server config: ${parsed.summary}`);
   }
-  const seen = new Set<string>();
-  for (const server of config.servers) {
-    if (seen.has(server.handle)) {
-      throw new Error(
-        `invalid @corbits/mcp server config: duplicate credential handle "${server.handle}"`,
-      );
-    }
-    seen.add(server.handle);
-    try {
-      parseMcpEndpoint(server.url);
-    } catch (cause) {
-      throw new Error(
-        `invalid @corbits/mcp server config: ${cause instanceof Error ? cause.message : String(cause)}`,
-      );
+  try {
+    assertServers(
+      config.servers.map(({ handle, url }) => ({ name: handle, url })),
+      "credential handle",
+    );
+    for (const server of config.servers) {
+      assertCatalogNames(server.handle, server.tools);
     }
+  } catch (cause) {
+    throw new Error(
+      `invalid @corbits/mcp server config: ${cause instanceof Error ? cause.message : String(cause)}`,
+    );
   }
 }
 

src/tool.test.ts

@@ -98,6 +98,30 @@ describe("mcpTools discovers a live server and floors every tool at ask", () =>
   });
 });
 
+describe("mcpTools refuses a bad server list before any request", () => {
+  test("empty, duplicate and non-https entries are refused", async () => {
+    handle = startTestMcpServer();
+    const url = handle.url;
+    await expect(mcpTools({ servers: [{ name: "", url }] })).rejects.toThrow(
+      "empty MCP server name",
+    );
+    await expect(
+      mcpTools({
+        servers: [
+          { name: "srv", url },
+          { name: "srv", url },
+        ],
+      }),
+    ).rejects.toThrow('duplicate MCP server name "srv"');
+    await expect(
+      mcpTools({
+        servers: [{ name: "srv", url: "http://mcp.example.test/mcp" }],
+      }),
+    ).rejects.toThrow("must be https");
+    expect(handle.requestsSeen).toHaveLength(0);
+  });
+});
+
 describe("mcpTools bounds a stalling server", () => {
   const env = {
     sources: [],

src/tool.ts

@@ -20,7 +20,13 @@ import {
   type McpClientOptions,
   type McpTool,
 } from "./client.js";
-import { isAskExempt, qualifiedName, toolDescription } from "./naming.js";
+import {
+  assertCatalogNames,
+  assertServers,
+  isAskExempt,
+  qualifiedName,
+  toolDescription,
+} from "./naming.js";
 
 export interface McpServerConfig {
   /** Namespace prefix for this server's tools: `<name>.<remote-tool-name>`. */
@@ -87,6 +93,7 @@ export async function mcpTools(
 ): Promise<AnnotatedToolFactory<McpToolsEnv>> {
   const allowWithoutAsk = options.allowWithoutAsk ?? [];
   const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS;
+  assertServers(options.servers, "MCP server name");
   const discovered: DiscoveredTool[] = [];
 
   for (const server of options.servers) {
@@ -97,6 +104,7 @@ export async function mcpTools(
     };
     const session = await mcpInitialize(server.url, clientOpts);
     const tools = await mcpListTools(server.url, { ...clientOpts, session });
+    assertCatalogNames(server.name, tools);
     for (const tool of tools) {
       discovered.push({
         server,