diff --git a/README.md b/README.md index 23107b4..3e6157c 100644 --- a/README.md +++ b/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. diff --git a/src/hub/discover.test.ts b/src/hub/discover.test.ts index f256b92..c0f8b63 100644 --- a/src/hub/discover.test.ts +++ b/src/hub/discover.test.ts @@ -26,6 +26,9 @@ function appWith( opts: { readonly apiBaseUrl?: string | null; readonly extraOrigins?: Record; + readonly status?: string; + readonly expiresAt?: Date | null; + readonly onError?: (error: unknown) => void; } = {}, ): Hono { const app = new Hono(); @@ -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); + }); }); diff --git a/src/hub/discover.ts b/src/hub/discover.ts index 1fb9548..f4b926f 100644 --- a/src/hub/discover.ts +++ b/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, );