fix(hub): never echo credential material and only use active credentials

README.md

@@ -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, 422 when the server fails discovery or the request would leave the credential's origin. `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 (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.
 
 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.
 

src/hub/discover.test.ts

@@ -26,6 +26,9 @@ function appWith(
   opts: {
     readonly apiBaseUrl?: string | null;
     readonly extraOrigins?: Record<string, string[]>;
+    readonly status?: string;
+    readonly expiresAt?: Date | null;
+    readonly onError?: (error: unknown) => void;
   } = {},
 ): Hono<TenantEnv> {
   const app = new Hono<TenantEnv>();
@@ -38,7 +41,15 @@ function appWith(
   const row = (id: string) =>
     secrets[id] === undefined
       ? []
-      : [{ id, secret: secrets[id], apiBaseUrl: opts.apiBaseUrl ?? null }];
+      : [
+          {
+            id,
+            secret: secrets[id],
+            status: opts.status ?? "active",
+            expiresAt: opts.expiresAt ?? null,
+            apiBaseUrl: opts.apiBaseUrl ?? null,
+          },
+        ];
   const db = {
     select: () => ({
       from: () => ({
@@ -62,6 +73,7 @@ function appWith(
       await next();
     },
     extraOrigins: opts.extraOrigins,
+    onError: opts.onError,
   } as unknown as MountMcpDiscoveryOpts;
   mountMcpDiscovery(app, mountOpts);
   return app;
@@ -273,4 +285,56 @@ describe("POST /mcp/discover", () => {
       await stalled.stop(true);
     }
   });
+
+  test.each([
+    ["CRLF", "sk-live-SECRET\r\nX-Injected: 1"],
+    ["LF", "sk-live-SECRET\n"],
+    ["NUL", "sk-live-SECRET\0"],
+  ])(
+    "a secret with %s is refused before any request and never echoed",
+    async (_, secret) => {
+      handle = startTestMcpServer();
+      const errors: unknown[] = [];
+      const { status, json } = await post(
+        appWith(
+          { cred_1: secret },
+          {
+            apiBaseUrl: handle.url,
+            onError: (error) => errors.push(error),
+          },
+        ),
+        { url: handle.url, credentialId: "cred_1" },
+      );
+      expect(status).toBe(422);
+      expect(String(json["error"])).toContain("not a valid header value");
+      expect(JSON.stringify(json)).not.toContain("sk-live");
+      expect(String(errors[0])).not.toContain("sk-live");
+      expect(handle.requestsSeen).toHaveLength(0);
+    },
+  );
+
+  test("a fetch failure is reported as a generic handshake error", async () => {
+    const errors: unknown[] = [];
+    const { status, json } = await post(
+      appWith({}, { onError: (error) => errors.push(error) }),
+      { url: "http://127.0.0.1:1/mcp" },
+    );
+    expect(status).toBe(422);
+    expect(String(json["error"])).toEndWith("the handshake failed");
+    expect(String(errors[0])).toContain("the handshake failed");
+  });
+
+  test.each([
+    ["revoked", { status: "revoked" }],
+    ["errored", { status: "error" }],
+    ["expired", { expiresAt: new Date(Date.now() - 1_000) }],
+  ])("a credential that is %s reads as absent", async (_, row) => {
+    handle = startTestMcpServer({ requireAuth: "Bearer tok-1" });
+    const { status } = await post(
+      appWith({ cred_1: "tok-1" }, { apiBaseUrl: handle.url, ...row }),
+      { url: handle.url, credentialId: "cred_1" },
+    );
+    expect(status).toBe(404);
+    expect(handle.requestsSeen).toHaveLength(0);
+  });
 });

src/hub/discover.ts

@@ -17,6 +17,7 @@ import { and, eq } from "drizzle-orm";
 import type { Hono, MiddlewareHandler } from "hono";
 
 import {
+  McpError,
   mcpInitialize,
   mcpListTools,
   type McpTool,
@@ -54,7 +55,8 @@ export type McpCredential = {
 /**
  * Read a tenant credential's decrypted secret and the origin it is pinned to.
  * Scoped by tenant so a credential id from another tenant reads as absent,
- * not as a secret.
+ * not as a secret; a revoked, errored or expired credential reads as absent
+ * too.
  */
 export async function readCredential(opts: {
   readonly db: DB["db"];
@@ -66,6 +68,8 @@ export async function readCredential(opts: {
     .select({
       id: credential.id,
       secret: credential.secret,
+      status: credential.status,
+      expiresAt: credential.expiresAt,
       apiBaseUrl: provider.apiBaseUrl,
     })
     .from(credential)
@@ -77,7 +81,13 @@ export async function readCredential(opts: {
       ),
     )
     .limit(1);
-  if (row === undefined) return undefined;
+  if (
+    row === undefined ||
+    row.status !== "active" ||
+    (row.expiresAt !== null && row.expiresAt.getTime() <= Date.now())
+  ) {
+    return undefined;
+  }
   const secret = await opts.cipher.decrypt(
     row.secret,
     credentialAad(row.id, "secret"),
@@ -87,13 +97,15 @@ export async function readCredential(opts: {
     : { secret, origin: new URL(row.apiBaseUrl).origin };
 }
 
+const HEADER_VALUE = /^[\t\x20-\x7e]*$/;
+
 /** A 3xx would send the bearer onward, so the pinned fetch's manual redirect
  * is turned into a refusal rather than a response the client tries to read. */
 function refusingRedirects(inner: FetchLike): FetchLike {
   return async (input, init) => {
     const response = await inner(input, init);
     if (response.status >= 300 && response.status < 400) {
-      throw new Error(
+      throw new McpError(
         `the MCP server answered a ${String(response.status)} redirect; refusing to follow it`,
       );
     }
@@ -128,11 +140,22 @@ export async function discoverMcpServer(args: {
       : secret;
   const origin = token === undefined ? target.origin : args.credential?.origin;
   if (origin === undefined) {
-    throw new Error("the credential's provider has no API origin to pin to");
+    throw new McpError("the credential's provider has no API origin to pin to");
+  }
+  // A secret that is not a valid header value makes fetch throw a message
+  // quoting it, so it is refused here with a fixed one.
+  if (token !== undefined && !HEADER_VALUE.test(token)) {
+    throw new McpError("the credential's secret is not a valid header value");
   }
   const extraOrigins = Object.entries(args.extraOrigins ?? {}).flatMap(
     ([pinned, extra]) => (new URL(pinned).origin === origin ? extra : []),
   );
+  const allowed = [origin, ...extraOrigins.map((o) => new URL(o).origin)];
+  if (!allowed.includes(target.origin)) {
+    throw new McpError(
+      `credential is pinned to ${origin}; refusing cross-origin request to ${target.origin}`,
+    );
+  }
   const pinned = refusingRedirects(
     createOriginPinnedFetch({
       origin,
@@ -197,11 +220,16 @@ export function mountMcpDiscovery(
       });
       return c.json({ data });
     } catch (cause) {
-      opts.onError?.(cause, { url: body.url });
-      // The message describes the handshake, never the material it used.
+      // Only this package's own messages are passed on: anything else, such
+      // as a fetch error, may quote the material the request carried.
+      const error =
+        cause instanceof McpError
+          ? cause
+          : new McpError("the handshake failed");
+      opts.onError?.(error, { url: body.url });
       return c.json(
         {
-          error: `the MCP server at ${target.origin} could not be discovered: ${cause instanceof Error ? cause.message : String(cause)}`,
+          error: `the MCP server at ${target.origin} could not be discovered: ${error.message}`,
         },
         422,
       );