diff --git a/.changeset/persisted-conformance-egress-guard.md b/.changeset/persisted-conformance-egress-guard.md new file mode 100644 index 0000000000..f6aa0cff64 --- /dev/null +++ b/.changeset/persisted-conformance-egress-guard.md @@ -0,0 +1,19 @@ +--- +"@mcpjam/sdk": patch +"@mcpjam/inspector": patch +--- + +Keep persisted conformance runs behind the hosted egress guard. + +`runConformance`'s protocol suite now dials through the fetch attached to the server config — `fetchFn`, then `baseFetch` — which is what the apps and tasks suites of the same run already did. It rebuilt its config from the URL, token and headers alone, so a caller's fetch never reached it and the suite, raw probes and MCP client both, fell back to the global `fetch`. An explicit `protocol.fetchFn` still wins. + +In the hosted inspector, every persisted conformance run — the public `/v1` start route, the GitHub checks worker and the benchmark worker — now dials through the DNS-pinned, hop-by-hop egress guard whoever starts it: + +- the executor defaults the MCP and OAuth transports to the hosted conformance guard when a caller passes none, and both workers now pass it explicitly instead of a bare `{ url }`; +- a target the guard refuses outright is never handed to a suite. The run records the refusal as each suite's could-not-run reason. This also covers the protocol suite's localhost host-header checks, which open raw sockets that no fetch can guard; +- a refused or failed dial reaches the stored report as the guard's verdict or one uniform message, never as the address a hostname resolved to or the socket, TLS or DNS error text; +- the GitHub-check health probe dials the pull request's server through the hosted MCP transport rather than the global `fetch`. + +The CI guard (`check-hosted-manager-base-fetch.mjs`) now also scans `server/routes/shared` and fails when a hosted file imports an `@mcpjam/sdk` entry point that opens its own connection (`runConformance`, the conformance suites, `withEphemeralClient`, `probeMcpServer`, `runServerDoctor` and the like) without being listed with the guard it dials through. + +Local and desktop behaviour is unchanged: every guard, the up-front refusal and the redaction are no-ops outside hosted mode. diff --git a/mcpjam-inspector/scripts/check-hosted-manager-base-fetch.mjs b/mcpjam-inspector/scripts/check-hosted-manager-base-fetch.mjs index 8025cb10cc..7e2cd1fda7 100644 --- a/mcpjam-inspector/scripts/check-hosted-manager-base-fetch.mjs +++ b/mcpjam-inspector/scripts/check-hosted-manager-base-fetch.mjs @@ -17,19 +17,37 @@ * this check by existing, which is the point — the failure arrives at the * moment the decision is made, not the moment somebody audits it. * + * THE SECOND RULE: SDK ENTRY POINTS THAT DIAL FOR THEMSELVES. `new + * MCPClientManager` is not the only way to open a connection. `runConformance`, + * the conformance suites, `withEphemeralClient`, the probe and the doctor each + * build their own client or fetch from a config, and each has a fetch seam that + * falls back to the global one when nobody fills it. The persisted conformance + * path went through exactly that gap after the first rule was in place: its + * callers handed `runConformance` a bare `{ url }`, and the protocol suite + * dropped even the fetch it was given. So a hosted file that IMPORTS one of + * those entry points from `@mcpjam/sdk` must be on a second allowlist that + * names the guard it dials through, and must still mention that guard. A new + * importer fails by existing, for the same reason as above. + * * SCOPE. Hosted server code only: `server/routes/web/**`, - * `server/routes/v1/**`, `server/services/**`. Not `server/index.ts` or - * `server/app.ts` — those are the LOCAL/desktop entrypoints, where reaching - * `http://localhost:3000/mcp` is the entire product and the guard is - * deliberately absent. Not `sdk/**` or `cli/**`, which are not this deployment. - * Test files are exempt: a test asserting the unguarded behavior is legitimate. + * `server/routes/v1/**`, `server/routes/shared/**` (the halves those two share) + * and `server/services/**`. Not `server/index.ts` or `server/app.ts` — those are + * the LOCAL/desktop entrypoints, where reaching `http://localhost:3000/mcp` is + * the entire product and the guard is deliberately absent. Not `sdk/**` or + * `cli/**`, which are not this deployment. Test files are exempt: a test + * asserting the unguarded behavior is legitimate. * * WHAT IT DOES NOT CATCH, stated so nobody reads more into a green run: it is a * source scan, so it sees `new MCPClientManager` and not a manager obtained by - * other means, and it says nothing about whether the injected fetch actually - * guards anything. That second property is the runtime test's job - * (`server/utils/__tests__/hosted-mcp-base-fetch.test.ts`), and it asserts a - * refusal rather than a non-null field for exactly this reason. + * other means, and it sees a STATIC import of an entry point (a namespace + * import of the SDK is refused outright, since it would hide one) but not a + * dynamic `import()`. For the second rule it checks a file-level mention of the + * guard, not each call's arguments — configs there are usually built a few + * lines above the call. And it says nothing about whether the fetch actually + * guards anything. Those properties are the runtime tests' job + * (`server/utils/__tests__/hosted-mcp-base-fetch.test.ts`, + * `server/services/__tests__/conformance-run-executor-egress.test.ts`), and + * they assert refusals rather than non-null fields for exactly this reason. */ import { readFileSync, readdirSync, statSync } from "node:fs"; @@ -43,6 +61,7 @@ const serverDir = resolve(__dirname, "..", "server"); const GUARDED_DIRS = [ join(serverDir, "routes", "web"), join(serverDir, "routes", "v1"), + join(serverDir, "routes", "shared"), join(serverDir, "services"), ]; @@ -82,6 +101,139 @@ const ALLOWED = new Set( const CONSTRUCTION = /new\s+MCPClientManager\s*\(/g; const GUARD_INJECTION = /baseFetch:\s*hostedMcpBaseFetch\(\)/; +/** + * `@mcpjam/sdk` exports that open their own connection to a caller-named + * target, each through a fetch seam that is the global `fetch` unless filled. + * + * Deliberately NOT listed: `executeOAuthProxy`, `executeDebugOAuthProxy` and + * `fetchOAuthMetadata`, which pin and re-check every hop themselves — there is + * no seam to leave open — and anything that takes a manager the caller built, + * which the first rule already covers. + */ +const SELF_DIALING_SDK_ENTRY_POINTS = new Set([ + "runConformance", + "MCPConformanceTest", + "MCPAppsConformanceTest", + "MCPTasksConformanceTest", + "OAuthConformanceTest", + "withEphemeralClient", + "probeMcpServer", + "runServerDoctor", + "discoverOAuthServerInfo", + "gatherClaudeReadinessEvidence", + "gatherOpenAIReadinessEvidence", +]); + +/** + * The hosted files allowed to import a self-dialing entry point, each with the + * guard it dials through. The guard must still appear in the file (outside + * comments and strings); an entry whose file no longer imports any entry point + * is stale and fails, for the reason the manager allowlist gives. + */ +const SELF_DIALING_ALLOWED = new Map( + [ + [ + ["services", "conformance-run-executor.ts"], + { + // Anchored on the assignment because the guard is DEFINED in this same + // file: a bare name match is already satisfied by `export function + // guardPersistedConformanceTransport(`, so deleting the call site would + // still pass and the rule would miss the regression it exists to catch. + guard: /=\s*guardPersistedConformanceTransport\(/, + why: "every persisted run's transports go through guardPersistedConformanceTransport", + }, + ], + [ + ["routes", "shared", "conformance.ts"], + { + guard: /(fetchFn|baseFetch):[^,;]*createConformanceFetch\(/, + why: "each suite is handed createConformanceFetch as fetchFn/baseFetch", + }, + ], + [ + ["routes", "web", "servers.ts"], + { + guard: /hostedMcpBaseFetch\(\)/, + why: "the doctor's probe and connection both dial hostedMcpBaseFetch()", + }, + ], + [ + ["routes", "web", "oauth-connections.ts"], + { + guard: /HOSTED_MODE\s*\?/, + why: "withEphemeralClient is the LOCAL branch; hosted uses createAuthorizedManager", + }, + ], + [ + ["services", "server-connection-worker.ts"], + { + guard: /createPinnedFetch\(/, + why: "the validation probe dials a DNS-pinned fetch", + }, + ], + [ + ["services", "server-connection-discovery.ts"], + { + guard: /createPinnedFetch\(/, + why: "the discovery probe dials a DNS-pinned fetch", + }, + ], + [ + ["services", "server-connection-authorize.ts"], + { + guard: /createPinnedFetch\(/, + why: "OAuth discovery dials a DNS-pinned fetch", + }, + ], + [ + ["services", "readiness", "runner.ts"], + { + guard: /fetchFn:\s*options\.fetchFn/, + why: "the gatherers REQUIRE fetchFn and the runner passes the caller's guard through", + }, + ], + ].map(([segments, rule]) => [resolve(join(serverDir, ...segments)), rule]) +); + +/** + * Blank out comments only, keeping string literals — the module specifier of + * an import is a string, and it is what says the binding came from the SDK. + */ +function stripComments(source) { + return source + .replace(/\/\*[\s\S]*?\*\//g, (match) => match.replace(/[^\n]/g, " ")) + .replace(/(^|[^:"'`\\])\/\/[^\n]*/g, (match, lead) => + lead + " ".repeat(match.length - lead.length) + ); +} + +const SDK_NAMED_IMPORT = + /\b(?:import|export)\s+(?:type\s+)?\{([^}]*)\}\s*from\s*["'](@mcpjam\/sdk(?:\/[^"']*)?)["']/g; +const SDK_NAMESPACE_IMPORT = + /\bimport\s+\*\s+as\s+\w+\s+from\s*["'](@mcpjam\/sdk(?:\/[^"']*)?)["']/g; + +/** + * The self-dialing entry points a file brings in from `@mcpjam/sdk`, by their + * EXPORTED name (so `runConformance as run` still counts), ignoring `type`-only + * specifiers, which cannot dial anything. A namespace import is reported as + * `*`: it would make every entry point reachable without naming one. + */ +function selfDialingImports(source) { + const code = stripComments(source); + const found = new Set(); + for (const match of code.matchAll(SDK_NAMED_IMPORT)) { + if (/^\s*(?:import|export)\s+type\b/.test(match[0])) continue; + for (const raw of match[1].split(",")) { + const specifier = raw.trim(); + if (!specifier || specifier.startsWith("type ")) continue; + const exported = specifier.split(/\s+as\s+/)[0].trim(); + if (SELF_DIALING_SDK_ENTRY_POINTS.has(exported)) found.add(exported); + } + } + for (const _ of code.matchAll(SDK_NAMESPACE_IMPORT)) found.add("*"); + return [...found].sort(); +} + /** * Blank out comments and string literals so neither can satisfy — or trip — * the checks below. Replaced with equal-length runs of spaces so every @@ -205,10 +357,32 @@ function* walk(dir) { const violations = []; const unguardedAllowed = []; const seenAllowed = new Set(); +const selfDialingViolations = []; +const selfDialingUnguarded = []; +const seenSelfDialingAllowed = new Set(); for (const dir of GUARDED_DIRS) { for (const file of walk(dir)) { - const code = stripCommentsAndStrings(readFileSync(file, "utf8")); + const source = readFileSync(file, "utf8"); + const code = stripCommentsAndStrings(source); + + // Rule two runs first: a file that dials through the SDK need not + // construct a manager at all, and the rule-one `continue` below would skip + // it. + const entryPoints = selfDialingImports(source); + if (entryPoints.length > 0) { + const rel = relative(resolve(serverDir, ".."), file); + const rule = SELF_DIALING_ALLOWED.get(resolve(file)); + if (!rule || entryPoints.includes("*")) { + selfDialingViolations.push({ file: rel, entryPoints }); + } else { + seenSelfDialingAllowed.add(resolve(file)); + if (!rule.guard.test(code)) { + selfDialingUnguarded.push({ file: rel, entryPoints, rule }); + } + } + } + const opens = []; CONSTRUCTION.lastIndex = 0; for (let m = CONSTRUCTION.exec(code); m; m = CONSTRUCTION.exec(code)) { @@ -239,6 +413,57 @@ for (const dir of GUARDED_DIRS) { const stale = [...ALLOWED] .filter((p) => !seenAllowed.has(p)) .map((p) => relative(resolve(serverDir, ".."), p)); +const staleSelfDialing = [...SELF_DIALING_ALLOWED.keys()] + .filter((p) => !seenSelfDialingAllowed.has(p)) + .map((p) => relative(resolve(serverDir, ".."), p)); + +const thisScript = relative( + resolve(serverDir, ".."), + fileURLToPath(import.meta.url) +); + +if ( + selfDialingViolations.length || + selfDialingUnguarded.length || + staleSelfDialing.length +) { + console.error("Hosted self-dialing SDK entry point guard failed (MJ-001).\n"); + if (selfDialingViolations.length) { + console.error( + "These hosted files import an `@mcpjam/sdk` entry point that opens its\n" + + "own connection. Each one's fetch seam is `globalThis.fetch` unless it is\n" + + "filled — loopback and private ranges reachable, redirects unvalidated.\n" + + "Dial through a guard (`createConformanceFetch`, `hostedMcpBaseFetch()`,\n" + + "`createPinnedFetch`) and add the file, with that guard, to\n" + + `SELF_DIALING_ALLOWED in ${thisScript}. A namespace import (\`*\`) of the\n` + + "SDK is refused outright: it would hide which entry points are in use.\n" + ); + for (const { file, entryPoints } of selfDialingViolations) { + console.error(` - ${file} (${entryPoints.join(", ")})`); + } + console.error(""); + } + if (selfDialingUnguarded.length) { + console.error( + "These allowlisted files no longer mention the guard they are listed with:\n" + ); + for (const { file, entryPoints, rule } of selfDialingUnguarded) { + console.error( + ` - ${file} (${entryPoints.join(", ")}): expected ${rule.guard} — ${rule.why}` + ); + } + console.error(""); + } + if (staleSelfDialing.length) { + console.error( + "These SELF_DIALING_ALLOWED entries no longer import an entry point.\n" + + "Remove them — a stale entry silently permits a future import:\n" + ); + for (const file of staleSelfDialing) console.error(` - ${file}`); + console.error(""); + } + process.exitCode = 1; +} if (violations.length || unguardedAllowed.length || stale.length) { console.error("Hosted MCPClientManager guard failed (MJ-001).\n"); @@ -274,7 +499,10 @@ if (violations.length || unguardedAllowed.length || stale.length) { process.exit(1); } +if (process.exitCode) process.exit(process.exitCode); + console.log( `hosted-manager-base-fetch: ok (${seenAllowed.size} guarded factories, ` + + `${seenSelfDialingAllowed.size} guarded SDK entry-point importers, ` + `${GUARDED_DIRS.map((d) => relative(resolve(serverDir, ".."), d)).join(", ")})` ); diff --git a/mcpjam-inspector/server/services/__tests__/conformance-run-executor-egress.test.ts b/mcpjam-inspector/server/services/__tests__/conformance-run-executor-egress.test.ts new file mode 100644 index 0000000000..c701940963 --- /dev/null +++ b/mcpjam-inspector/server/services/__tests__/conformance-run-executor-egress.test.ts @@ -0,0 +1,343 @@ +/** + * MJ-001, persisted conformance runs: every suite dials through the hosted + * egress guard whoever started the run, a target the guard refuses is never + * dialled at all, and the stored report says no more about a refused target + * than the verdict. + * + * These go through the REAL executor and the REAL SDK suites — only Convex is + * stubbed — because the hole was in the seam between them: the executor + * handed the SDK a server config and the protocol suite dropped its fetch. A + * suite that mocks `runConformance` cannot see that. + */ + +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const { createConvexClientMock } = vi.hoisted(() => ({ + createConvexClientMock: vi.fn(), +})); + +vi.mock("../evals/route-helpers.js", () => ({ + createConvexClient: (...args: unknown[]) => createConvexClientMock(...args), +})); + +// The SDK's DNS-pinned transport resolves with `node:dns`'s `lookup`. These +// tests never want a real resolver, so every lookup fails the way an unknown +// name does; the cases that need a hostname to classify inject a resolver +// into the guard instead. +vi.mock("node:dns", async (importOriginal) => { + const actual = await importOriginal(); + const lookup = ( + hostname: string, + options: unknown, + callback?: (error: Error | null) => void, + ) => { + const done = (typeof options === "function" ? options : callback) as ( + error: Error | null, + ) => void; + process.nextTick(() => + done( + Object.assign(new Error(`getaddrinfo ENOTFOUND ${hostname}`), { + code: "ENOTFOUND", + }), + ), + ); + }; + return { ...actual, default: { ...actual, lookup }, lookup }; +}); + +const ORIGINAL_HOSTED_MODE = process.env.VITE_MCPJAM_HOSTED_MODE; +const PUBLIC_ADDRESS = "93.184.216.34"; + +/** + * `HOSTED_MODE` is read once, when `config.ts` is first imported, and every + * guard on this path no-ops outside it — so each case loads a fresh module + * graph with hosted mode on. + */ +async function loadHosted() { + process.env.VITE_MCPJAM_HOSTED_MODE = "true"; + vi.resetModules(); + const [executor, guard] = await Promise.all([ + import("../conformance-run-executor.js"), + import("../../utils/hosted-egress-guard.js"), + ]); + return { ...executor, ...guard }; +} + +/** Every report the run persisted, as the JSON it was stored as. */ +function convexClient() { + const stored: Array<{ suiteKind: string; json: string }> = []; + const mutation = vi.fn(async (fn: string) => { + if (fn === "conformanceRuns:startRun") return { runId: "run_1" }; + if (fn === "conformanceRuns:finalizeRun") { + return { outcome: "failed", score: 0 }; + } + return {}; + }); + const action = vi.fn(async (fn: string, args: Record) => { + if (fn === "conformanceRuns:upsertReportAction") { + stored.push({ + suiteKind: String(args.suiteKind), + json: JSON.stringify(args.report), + }); + } + return undefined; + }); + createConvexClientMock.mockReturnValue({ mutation, action }); + return { stored, mutation }; +} + +const servers: http.Server[] = []; + +/** + * A target on loopback that counts TCP CONNECTIONS, not requests: the run + * addresses it over https and it speaks plain http, so an unguarded dial would + * fail its handshake without ever reaching a request handler. The connection + * is what the guard must prevent, so it is what gets counted. + */ +async function loopbackTarget(): Promise<{ + url: string; + connections: () => number; +}> { + let connections = 0; + const server = http.createServer((_req, res) => { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end("{}"); + }); + server.on("connection", () => { + connections += 1; + }); + servers.push(server); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as AddressInfo; + return { + url: `https://127.0.0.1:${port}/mcp`, + connections: () => connections, + }; +} + +const originalFetch = globalThis.fetch; + +beforeEach(() => { + vi.clearAllMocks(); +}); + +afterEach(async () => { + globalThis.fetch = originalFetch; + if (ORIGINAL_HOSTED_MODE === undefined) { + delete process.env.VITE_MCPJAM_HOSTED_MODE; + } else { + process.env.VITE_MCPJAM_HOSTED_MODE = ORIGINAL_HOSTED_MODE; + } + vi.resetModules(); + await Promise.all( + servers.splice(0).map( + (server) => + new Promise((resolve) => { + server.closeAllConnections?.(); + server.close(() => resolve()); + }), + ), + ); +}); + +describe("a persisted run in hosted mode", () => { + it("refuses a private target before any suite dials it, raw-socket probes included", async () => { + // A bare `{ url }` is what the benchmark and GitHub-check workers used to + // pass. For a loopback target the protocol suite's host-header checks go + // straight to `node:http`, past any fetch, so only refusing the target up + // front keeps them from connecting. + const { executePersistedConformanceRun } = await loadHosted(); + const { stored, mutation } = convexClient(); + const target = await loopbackTarget(); + + const result = await executePersistedConformanceRun({ + convexToken: "tok", + projectId: "p1", + server: { url: target.url }, + source: "benchmark", + target: { kind: "server", serverId: "s1", serverUrl: target.url }, + }); + + expect(target.connections()).toBe(0); + expect(result.runId).toBe("run_1"); + expect(mutation.mock.calls.map(([fn]) => fn)).toContain( + "conformanceRuns:finalizeRun", + ); + expect(stored.map((entry) => entry.suiteKind).sort()).toEqual([ + "apps", + "protocol", + "tasks", + ]); + for (const { json } of stored) { + expect(json).toMatch( + /Server URL points at a private or internal address/, + ); + expect(json).toContain('"skipReason":"could-not-run"'); + } + }, 60_000); + + it("refuses a public target's redirect inward at the hop, and stores only the verdict", async () => { + const { + executePersistedConformanceRun, + createGuardedFetch, + setEgressHostResolverForTests, + } = await loadHosted(); + const { stored } = convexClient(); + + const resolver = async (hostname: string) => + hostname === "internal.example.test" ? ["10.1.2.3"] : [PUBLIC_ADDRESS]; + // The executor's own up-front judgement of the starting URL. + setEgressHostResolverForTests(resolver); + + const INTERNAL_BODY_MARKER = "internal-service-response-body"; + const internalDialled: string[] = []; + const network = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input instanceof Request ? input.url : input); + if (url.startsWith("https://public.example.test/")) { + // Every request the suites make is answered with a method-preserving + // redirect to a name that resolves privately. + return new Response(null, { + status: 307, + headers: { Location: "https://internal.example.test/admin" }, + }); + } + internalDialled.push(url); + return Response.json({ marker: INTERNAL_BODY_MARKER }); + }) as unknown as typeof fetch; + + await executePersistedConformanceRun({ + convexToken: "tok", + projectId: "p1", + server: { + url: "https://public.example.test/mcp", + // The caller's own guard, re-checking every hop with the same + // resolver, so the chain runs without a network. + baseFetch: createGuardedFetch({ + hosted: true, + baseFetch: network, + resolver, + }), + }, + source: "api", + target: { kind: "server", serverId: "s1" }, + }); + + expect(network).toHaveBeenCalled(); + expect(internalDialled).toEqual([]); + expect(stored).toHaveLength(3); + const all = stored.map((entry) => entry.json).join("\n"); + expect(all).toMatch( + /hostname \\"internal\.example\.test\\" resolves to a private or internal address/, + ); + // The guard keeps the resolved address on `cause`, for the logs. The + // report serializer copies `cause`, so it must not survive the transport. + expect(all).not.toMatch(/10\.1\.2\.3|resolved address/); + expect(all).not.toContain(INTERNAL_BODY_MARKER); + }, 60_000); + + it("never reaches for the global fetch when the caller passed a bare { url }", async () => { + const { executePersistedConformanceRun, setEgressHostResolverForTests } = + await loadHosted(); + const { stored } = convexClient(); + setEgressHostResolverForTests(async () => [PUBLIC_ADDRESS]); + const globalFetch = vi.fn(async () => { + throw new Error("the global fetch must not be dialled"); + }); + globalThis.fetch = globalFetch as unknown as typeof fetch; + + await executePersistedConformanceRun({ + convexToken: "tok", + projectId: "p1", + server: { url: "https://public.example.test/mcp" }, + source: "github_app", + target: { kind: "external", serverRef: "acme/widgets" }, + }); + + // Every suite went through the executor's default transport — the pinned + // one, whose lookup fails here — and none fell back to the global fetch. + expect(globalFetch).not.toHaveBeenCalled(); + expect(stored).toHaveLength(3); + const all = stored.map((entry) => entry.json).join("\n"); + expect(all).toContain( + "The inspector could not establish a connection to this server.", + ); + expect(all).not.toMatch(/ENOTFOUND|getaddrinfo|could not resolve/i); + }, 60_000); +}); + +describe("guardPersistedConformanceTransport", () => { + it("defaults the MCP and OAuth transports to the hosted guard", async () => { + const { guardPersistedConformanceTransport, BlockedEgressTargetError } = + await loadHosted(); + const guarded = guardPersistedConformanceTransport({ + server: { url: "https://connector.example.test/mcp" }, + oauth: { + serverUrl: "https://connector.example.test/mcp", + protocolVersion: "2025-11-25", + registrationStrategy: "dcr", + auth: { mode: "client_credentials", clientId: "c", clientSecret: "s" }, + } as never, + }); + + const transports = [ + (guarded.server as { baseFetch?: typeof fetch }).baseFetch, + guarded.server.fetchFn, + guarded.oauth?.fetchFn, + ]; + for (const transport of transports) { + expect(typeof transport).toBe("function"); + const refused = await transport!("https://127.0.0.1:6274/mcp").catch( + (error: unknown) => error, + ); + expect(refused).toBeInstanceOf(BlockedEgressTargetError); + expect((refused as Error).cause).toBeUndefined(); + } + }); + + it("keeps a transport the caller chose, for the probes as well as the client", async () => { + const { guardPersistedConformanceTransport } = await loadHosted(); + const chosen = vi.fn( + async () => new Response("{}", { status: 200 }), + ) as unknown as typeof fetch; + const guarded = guardPersistedConformanceTransport({ + server: { url: "https://connector.example.test/mcp", baseFetch: chosen }, + }); + + await (guarded.server as { baseFetch: typeof fetch }).baseFetch( + "https://connector.example.test/mcp", + ); + await guarded.server.fetchFn!("https://connector.example.test/mcp"); + expect(chosen).toHaveBeenCalledTimes(2); + expect(guarded.oauth).toBeUndefined(); + }); +}); + +describe("persistedConformanceTargetRefusal", () => { + it("is a no-op outside hosted mode, where reaching localhost is the point", async () => { + process.env.VITE_MCPJAM_HOSTED_MODE = "false"; + vi.resetModules(); + const { persistedConformanceTargetRefusal } = await import( + "../conformance-run-executor.js" + ); + await expect( + persistedConformanceTargetRefusal({ url: "http://127.0.0.1:6274/mcp" }), + ).resolves.toBeNull(); + }); + + it("reports a resolver failure as the uniform message, not the resolver's text", async () => { + const { persistedConformanceTargetRefusal, setEgressHostResolverForTests } = + await loadHosted(); + setEgressHostResolverForTests(async () => { + throw new Error("queryA ESERVFAIL flaky.example.test"); + }); + await expect( + persistedConformanceTargetRefusal({ + url: "https://flaky.example.test/mcp", + }), + ).resolves.toBe( + "The inspector could not establish a connection to this server.", + ); + }); +}); diff --git a/mcpjam-inspector/server/services/__tests__/conformance-worker-egress.test.ts b/mcpjam-inspector/server/services/__tests__/conformance-worker-egress.test.ts new file mode 100644 index 0000000000..576ed2b2f8 --- /dev/null +++ b/mcpjam-inspector/server/services/__tests__/conformance-worker-egress.test.ts @@ -0,0 +1,120 @@ +/** + * MJ-001: the two workers that start persisted conformance runs hand the + * executor a GUARDED server config, not a bare `{ url }`. + * + * The executor now puts an unguarded config behind the hosted guard itself + * (`conformance-run-executor-egress.test.ts`), so these are the explicit half: + * each worker states which transport its target is dialled through, and that + * transport refuses what the hosted guard refuses. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const { executePersistedConformanceRunMock } = vi.hoisted(() => ({ + executePersistedConformanceRunMock: vi.fn(), +})); + +vi.mock("../conformance-run-executor.js", () => ({ + executePersistedConformanceRun: (...args: unknown[]) => + executePersistedConformanceRunMock(...args), +})); + +const ORIGINAL_HOSTED_MODE = process.env.VITE_MCPJAM_HOSTED_MODE; + +/** + * `HOSTED_MODE` is read at import, and the guard no-ops outside it. Each case + * therefore loads its worker into a fresh module graph, which is slow — hence + * the explicit timeouts below. + */ +async function withHostedModules() { + process.env.VITE_MCPJAM_HOSTED_MODE = "true"; + vi.resetModules(); + return await import("../../utils/hosted-egress-guard.js"); +} + +/** The server config the worker handed the executor. */ +function serverPassed(): { + url?: string; + accessToken?: string; + baseFetch?: typeof fetch; +} { + expect(executePersistedConformanceRunMock).toHaveBeenCalledTimes(1); + return executePersistedConformanceRunMock.mock.calls[0]![0].server; +} + +async function expectRefusesLoopback( + transport: typeof fetch | undefined, + BlockedEgressTargetError: new (...args: never[]) => Error, +) { + expect(typeof transport).toBe("function"); + await expect(transport!("https://127.0.0.1:6274/mcp")).rejects.toBeInstanceOf( + BlockedEgressTargetError, + ); +} + +beforeEach(() => { + vi.clearAllMocks(); + executePersistedConformanceRunMock.mockResolvedValue({ runId: "run_1" }); +}); + +afterEach(() => { + if (ORIGINAL_HOSTED_MODE === undefined) { + delete process.env.VITE_MCPJAM_HOSTED_MODE; + } else { + process.env.VITE_MCPJAM_HOSTED_MODE = ORIGINAL_HOSTED_MODE; + } + vi.resetModules(); +}); + +describe("benchmark conformance child", () => { + it("dials the benchmarked connector through the hosted conformance guard", async () => { + const { BlockedEgressTargetError } = await withHostedModules(); + const { defaultRunConformanceChildForTests } = await import( + "../bench-worker.js" + ); + + await defaultRunConformanceChildForTests()({ + job: { + runnerBearer: "runner-bearer", + projectId: "p1", + serverId: "s1", + benchmarkRunId: "bench_1", + } as never, + entry: { evidenceKey: "conformance" } as never, + spec: { + serverUrl: "https://connector.example.test/mcp", + suites: ["protocol", "apps", "tasks"], + }, + }); + + const server = serverPassed(); + expect(server.url).toBe("https://connector.example.test/mcp"); + await expectRefusesLoopback(server.baseFetch, BlockedEgressTargetError); + }, 90_000); +}); + +describe("GitHub checks conformance step", () => { + it("dials the pull request's server through the hosted conformance guard", async () => { + const { BlockedEgressTargetError } = await withHostedModules(); + const { defaultRunConformanceForTests } = await import( + "../github-checks-worker.js" + ); + + await defaultRunConformanceForTests()({ + claimed: { + projectId: "p1", + triggerId: "trig_1", + repoFullName: "acme/widgets", + } as never, + bearer: "execution-bearer", + serverUrl: "https://3001-sb_1.e2b.app/mcp", + candidateId: "cand_1", + oauthAccessToken: "pr-server-token", + }); + + const server = serverPassed(); + expect(server.url).toBe("https://3001-sb_1.e2b.app/mcp"); + expect(server.accessToken).toBe("pr-server-token"); + await expectRefusesLoopback(server.baseFetch, BlockedEgressTargetError); + }, 90_000); +}); diff --git a/mcpjam-inspector/server/services/bench-worker.ts b/mcpjam-inspector/server/services/bench-worker.ts index 4cd936b81c..917cd54937 100644 --- a/mcpjam-inspector/server/services/bench-worker.ts +++ b/mcpjam-inspector/server/services/bench-worker.ts @@ -1638,7 +1638,15 @@ async function defaultRunConformanceChild( const result = await executePersistedConformanceRun({ convexToken: job.runnerBearer, projectId: job.projectId, - server: { url: spec.serverUrl } as never, + // The benchmarked connector's URL comes off a saved server row, so it is + // somebody else's choice of target. The protocol, apps and tasks suites + // all dial through this transport — the same guard the hosted conformance + // routes use — rather than a bare `{ url }` the suites would have taken to + // the global fetch, redirects and all. + server: { + url: spec.serverUrl, + baseFetch: createConformanceFetch("MCP server"), + }, suites: spec.suites, source: "benchmark", target: { @@ -1663,6 +1671,9 @@ async function defaultRunConformanceChild( return { runId: result.runId }; } +export const defaultRunConformanceChildForTests = () => + defaultRunConformanceChild; + export type RunAuthProbeArgs = { job: ClaimedBenchmarkJob; entry: BenchmarkRosterEntry; diff --git a/mcpjam-inspector/server/services/conformance-run-executor.ts b/mcpjam-inspector/server/services/conformance-run-executor.ts index 30657766c1..55fa5fdfa0 100644 --- a/mcpjam-inspector/server/services/conformance-run-executor.ts +++ b/mcpjam-inspector/server/services/conformance-run-executor.ts @@ -5,6 +5,9 @@ * finalizes once every requested suite has settled. Directory readiness is * deliberately absent — it grades publisher policy and must never enter the * conformance score or CI verdict. + * + * Every suite dials through the hosted egress guard no matter which caller + * started the run: see {@link guardPersistedConformanceTransport}. */ import { @@ -18,6 +21,14 @@ import { import { createConvexClient } from "./evals/route-helpers.js"; import { reconcileHeadlessOAuthScope } from "./conformance-oauth-headless-scope.js"; import { logger } from "../utils/logger.js"; +import { redactHostedTransportFailures } from "../utils/hosted-transport-failure-redaction.js"; +import { HOSTED_TRANSPORT_FAILURE_DETAIL } from "../utils/hosted-doctor-redaction.js"; +import { + BlockedEgressTargetError, + EgressResolutionError, + assertAllowedHostedTargetUrl, +} from "../utils/hosted-egress-guard.js"; +import { createConformanceFetch } from "../routes/shared/conformance.js"; export type ConformanceRunSource = | "ui" @@ -31,7 +42,13 @@ export type ConformanceRunSource = export type ExecutePersistedConformanceArgs = { convexToken: string; projectId: string; - server: MCPServerConfig; + /** + * The target. A `baseFetch` (or `fetchFn`) on it is the transport every + * suite dials through, and it is honored as given; when neither is present + * the run is put behind the hosted conformance guard anyway — see + * {@link guardPersistedConformanceTransport}. + */ + server: MCPServerConfig & { fetchFn?: typeof fetch }; suites?: ConformanceSuiteKind[]; source: ConformanceRunSource; /** @@ -267,6 +284,105 @@ function prepareReportForPersistence( return redactConformanceReportForSharing(scoped); } +/** + * Put every suite of a persisted run behind the hosted egress guard, whoever + * the caller was. + * + * WHY HERE. This executor is the one door every persisted run walks through — + * the public `/v1` start route, the GitHub checks worker and the benchmark + * worker — and the target of each is a URL somebody else chose: a saved + * server, a sandbox running a pull request's code, a benchmarked connector. + * Leaving the guard to each caller is how two of the three came to pass a bare + * `{ url }`, which the suites dialled through the global fetch: no address + * classification, and redirects followed wherever they led (pentest finding + * MJ-001's shape, on a route the doctor fix never touched). A caller that + * forgets now gets the guard anyway. + * + * A DELIBERATE TRANSPORT STILL WINS. A `baseFetch`/`fetchFn` on the server + * config, or a `fetchFn` on the OAuth config, is used as given — the same rule + * as `createAuthorizedManager`'s per-server `baseFetch`. The defaults are the + * transports the hosted conformance routes already dial through, and both are + * plain `fetch` outside hosted mode, where reaching localhost is the point. + * + * EVERY transport is then wrapped by {@link redactHostedTransportFailures}, so + * a refused or failed dial reaches the persisted report as a verdict or one + * uniform sentence, never as the resolved address, socket error or TLS text + * the report is read back with. Also a no-op outside hosted mode. + */ +export function guardPersistedConformanceTransport(args: { + server: ExecutePersistedConformanceArgs["server"]; + oauth?: OAuthConformanceConfig; +}): { + server: ExecutePersistedConformanceArgs["server"]; + oauth?: OAuthConformanceConfig; +} { + // `baseFetch` is the transport the MCP client dials through in every suite; + // `fetchFn` is what the protocol suite's raw probes use. Either one alone + // stands in for both, so a caller that set one did not leave the other open. + // Read structurally: `baseFetch` exists only on the HTTP member of the union. + const declared = args.server as { + baseFetch?: typeof fetch; + fetchFn?: typeof fetch; + }; + const transport = + declared.baseFetch ?? + declared.fetchFn ?? + createConformanceFetch("MCP server"); + const probes = declared.fetchFn ?? transport; + const server = { + ...args.server, + baseFetch: redactHostedTransportFailures(transport), + fetchFn: redactHostedTransportFailures(probes), + }; + if (!args.oauth) return { server }; + return { + server, + oauth: { + ...args.oauth, + // The OAuth suite dials URLs it DISCOVERS — metadata, authorization, + // token and registration endpoints all come out of the target's own + // documents — so it needs the guard as much as the target itself does. + fetchFn: redactHostedTransportFailures( + args.oauth.fetchFn ?? createConformanceFetch("OAuth endpoint") + ), + }, + }; +} + +/** + * The reason a hosted run must not dial `server` at all, or `null` to proceed. + * + * The transport guard above decides each request as it is made, and that is + * enough for everything that goes through a fetch. It is not enough for the + * protocol suite's localhost host-header checks, which open raw `node:http` + * sockets — `fetch` cannot set `Host` — whenever the TARGET is a loopback + * name. The `/v1` start route refuses such a target before it gets here; the + * workers did not ask. So the executor judges the starting URL itself, with the + * same check the routes use, and a target it refuses is never handed to a + * suite. No-op outside hosted mode, where reaching localhost is the point. + * + * The verdict is what gets persisted, so it follows the transport's rules: a + * refusal keeps its wording (which names the host, never what it resolved + * to), and a resolver failure becomes the uniform connection message rather + * than the resolver's own text. + */ +export async function persistedConformanceTargetRefusal( + server: ExecutePersistedConformanceArgs["server"] +): Promise { + const url = (server as { url?: string | URL }).url; + if (url === undefined) return null; + try { + await assertAllowedHostedTargetUrl(String(url), "Server URL"); + return null; + } catch (error) { + if (error instanceof BlockedEgressTargetError) return error.message; + if (error instanceof EgressResolutionError) { + return HOSTED_TRANSPORT_FAILURE_DETAIL; + } + throw error; + } +} + export async function executePersistedConformanceRun( args: ExecutePersistedConformanceArgs ): Promise { @@ -350,14 +466,19 @@ export async function executePersistedConformanceRun( (heartbeat as unknown as { unref?: () => void }).unref?.(); try { + const refusal = await persistedConformanceTargetRefusal(args.server); + const guarded = guardPersistedConformanceTransport({ + server: args.server, + oauth: args.oauth, + }); const report = - executionSuites.length > 0 + executionSuites.length > 0 && refusal === null ? await runConformance({ - server: args.server, + server: guarded.server, suites: executionSuites, protocolVersion: args.protocolVersion as never, engineVersion: args.engineVersion, - ...(args.oauth ? { oauth: args.oauth } : {}), + ...(guarded.oauth ? { oauth: guarded.oauth } : {}), onProgress: async (event) => { if (event.status === "running") return; const body = prepareReportForPersistence( @@ -426,6 +547,29 @@ export async function executePersistedConformanceRun( }) : null; + if (refusal !== null) { + // Nothing was dialled, so every suite that would have run records the + // refusal as its could-not-run reason: the run finalizes with an honest + // verdict instead of three suites' worth of refused requests. + for (const suiteKind of executionSuites) { + const body = prepareReportForPersistence( + suiteKind, + syntheticIncompleteReport(suiteKind, refusal), + args.oauthHeadlessCheckIds + ); + await client.action( + "conformanceRuns:upsertReportAction" as never, + { + runId: started.runId, + suiteKind, + report: body, + status: "failed", + durationMs: body.durationMs, + } as never + ); + } + } + if (unsupportedOAuth) { const body = prepareReportForPersistence( "oauth", diff --git a/mcpjam-inspector/server/services/github-checks-worker.ts b/mcpjam-inspector/server/services/github-checks-worker.ts index 1fe3a5fceb..3bc8b33b85 100644 --- a/mcpjam-inspector/server/services/github-checks-worker.ts +++ b/mcpjam-inspector/server/services/github-checks-worker.ts @@ -69,6 +69,7 @@ import { postServiceRoute, } from "./github-checks/service-route.js"; import { executePersistedConformanceRun } from "./conformance-run-executor.js"; +import { createConformanceFetch } from "../routes/shared/conformance.js"; const POLL_INTERVAL_MS = 15_000; const POLL_JITTER_MS = 5_000; @@ -1442,6 +1443,13 @@ async function defaultRunConformance(args: { server: { url: args.serverUrl, ...(args.oauthAccessToken ? { accessToken: args.oauthAccessToken } : {}), + // The URL is ours — the sandbox's public HTTPS edge — but what answers + // it is the pull request's code, which can redirect anywhere. Every + // suite dials through the hosted conformance guard: the same pinned, + // hop-by-hop transport the eval half of this check already reaches the + // URL through (`createAuthorizedManager`), so the sandbox needs no + // allowance. + baseFetch: createConformanceFetch("MCP server"), }, suites, source: "github_app", @@ -1453,6 +1461,8 @@ async function defaultRunConformance(args: { return { runId: result.runId }; } +export const defaultRunConformanceForTests = () => defaultRunConformance; + function defaultDeps(): CheckExecutionDeps { return { beginPlan: (claimed) => diff --git a/mcpjam-inspector/server/services/github-checks/__tests__/sandbox-health-egress.test.ts b/mcpjam-inspector/server/services/github-checks/__tests__/sandbox-health-egress.test.ts new file mode 100644 index 0000000000..5ac2b31852 --- /dev/null +++ b/mcpjam-inspector/server/services/github-checks/__tests__/sandbox-health-egress.test.ts @@ -0,0 +1,98 @@ +/** + * MJ-001: the GitHub-check health probe dials the pull request's server + * through the hosted MCP transport, not the global fetch. + * + * The URL is ours — the sandbox's public edge — but the code answering it is + * the pull request's, and it can redirect. So the property worth pinning is the + * transport, and the observable is a target the hosted guard refuses: under the + * global fetch the probe would open a connection to it, under the guard it + * never does. Connections are counted rather than requests because the probe + * speaks https and this target speaks plain http — an unguarded dial fails its + * handshake before any request handler could see it. + */ + +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { CheckSandbox } from "../sandbox"; + +const ORIGINAL_HOSTED_MODE = process.env.VITE_MCPJAM_HOSTED_MODE; +const RECIPE = { + build: "npm ci && npm run build", + start: "npm start", + port: 3001, + mcpPath: "/mcp", +}; + +const servers: http.Server[] = []; + +async function countingTarget(): Promise<{ + host: string; + connections: () => number; +}> { + let connections = 0; + const server = http.createServer((_req, res) => { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end("{}"); + }); + server.on("connection", () => { + connections += 1; + }); + servers.push(server); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as AddressInfo; + return { host: `127.0.0.1:${port}`, connections: () => connections }; +} + +/** Just enough box for build → start → probe; every command succeeds. */ +function sandboxServing(host: string): CheckSandbox { + return { + sandboxId: "sb_test", + getHost: () => host, + commands: { + run: async (_command: string, opts?: { background?: boolean }) => + opts?.background + ? { pid: 1234 } + : { exitCode: 0, stdout: "", stderr: "" }, + }, + kill: async () => {}, + } as unknown as CheckSandbox; +} + +afterEach(async () => { + if (ORIGINAL_HOSTED_MODE === undefined) { + delete process.env.VITE_MCPJAM_HOSTED_MODE; + } else { + process.env.VITE_MCPJAM_HOSTED_MODE = ORIGINAL_HOSTED_MODE; + } + vi.resetModules(); + await Promise.all( + servers.splice(0).map( + (server) => + new Promise((resolve) => { + server.closeAllConnections?.(); + server.close(() => resolve()); + }), + ), + ); +}); + +describe("GitHub-check health probe in hosted mode", () => { + it("never dials a target the hosted guard refuses", async () => { + const target = await countingTarget(); + process.env.VITE_MCPJAM_HOSTED_MODE = "true"; + vi.resetModules(); + const { buildAndStart } = await import("../sandbox"); + + const error = await buildAndStart(sandboxServing(target.host), RECIPE, { + healthTimeoutMs: 300, + healthIntervalMs: 20, + }).then( + () => undefined, + (thrown: unknown) => thrown, + ); + + expect(error).toMatchObject({ outcome: "server_unhealthy" }); + expect(target.connections()).toBe(0); + }, 60_000); +}); diff --git a/mcpjam-inspector/server/services/github-checks/sandbox.ts b/mcpjam-inspector/server/services/github-checks/sandbox.ts index ed4f335531..ce6f504161 100644 --- a/mcpjam-inspector/server/services/github-checks/sandbox.ts +++ b/mcpjam-inspector/server/services/github-checks/sandbox.ts @@ -39,6 +39,7 @@ import { } from "./egress-policy.js"; import { startNetworkMonitor, stopNetworkMonitor } from "./network-monitor.js"; import { logger } from "../../utils/logger.js"; +import { hostedMcpBaseFetch } from "../../utils/hosted-mcp-base-fetch.js"; import type { CheckRecipe } from "./recipes.js"; /** Outcomes this module can produce. A subset of the worker's full taxonomy. */ @@ -865,7 +866,11 @@ export async function buildAndStart( const probe = await probeMcpInitialize(url, { timeoutMs: options?.healthTimeoutMs ?? HEALTH_TIMEOUT_MS, intervalMs: options?.healthIntervalMs ?? HEALTH_INTERVAL_MS, - fetchImpl: options?.fetchImpl, + // The host is the sandbox's public edge, but the code answering it is the + // pull request's, and a POST it redirects would otherwise be followed by + // the global fetch to wherever the Location points. The hosted MCP + // transport re-checks every hop; it is plain `fetch` outside hosted mode. + fetchImpl: options?.fetchImpl ?? hostedMcpBaseFetch(), }); if (probe === "unhealthy") { throw new CheckStepError( diff --git a/mcpjam-inspector/server/utils/__tests__/hosted-transport-failure-redaction.test.ts b/mcpjam-inspector/server/utils/__tests__/hosted-transport-failure-redaction.test.ts new file mode 100644 index 0000000000..685364425c --- /dev/null +++ b/mcpjam-inspector/server/utils/__tests__/hosted-transport-failure-redaction.test.ts @@ -0,0 +1,171 @@ +/** + * MJ-001, persisted conformance: what a failed hosted dial may still say once + * a suite has recorded it. + * + * The property under test is the one the SDK's report serializer breaks by + * design — `deepJsonSafe` keeps an `Error`'s `cause` chain and its own fields — + * so every case asserts on everything such a serializer can reach, not only on + * `error.message`. The end-to-end half (a real run, a real stored report) is + * `services/__tests__/conformance-run-executor-egress.test.ts`. + */ + +import { describe, expect, it, vi } from "vitest"; +import { redactHostedTransportFailures } from "../hosted-transport-failure-redaction.js"; +import { HOSTED_TRANSPORT_FAILURE_DETAIL } from "../hosted-doctor-redaction.js"; +import { + BlockedEgressTargetError, + EgressResolutionError, +} from "../hosted-egress-guard.js"; + +/** + * Everything a report serializer can reach on a thrown value: the fields + * `deepJsonSafe` names (`name`, `message`, `code`, `statusCode`, `cause`) plus + * every own enumerable property, recursively through `cause`. + */ +function reachable(value: unknown, depth = 0): unknown { + if (!(value instanceof Error) || depth > 8) return value; + const record = value as Error & Record; + return { + name: record.name, + message: record.message, + code: record.code, + statusCode: record.statusCode, + ...Object.fromEntries(Object.entries(record)), + cause: reachable(record.cause, depth + 1), + }; +} + +/** What a conformance report could persist for the rejection of `fetchFn`. */ +async function storedFor( + fetchFn: typeof fetch, +): Promise<{ thrown: unknown; stored: string }> { + const thrown = await fetchFn("https://target.example.test/mcp").then( + () => undefined, + (error: unknown) => error, + ); + return { thrown, stored: JSON.stringify(reachable(thrown)) }; +} + +function rejectingWith(error: unknown): typeof fetch { + return vi.fn(async () => { + throw error; + }) as unknown as typeof fetch; +} + +/** A Node socket error as `node:http` raises it: message, code and address. */ +function socketError(message: string, fields: Record): Error { + return Object.assign(new Error(message), fields); +} + +describe("redactHostedTransportFailures (hosted)", () => { + it("passes a response through untouched", async () => { + const response = new Response("body", { status: 418 }); + const wrapped = redactHostedTransportFailures( + vi.fn(async () => response) as unknown as typeof fetch, + { hosted: true }, + ); + await expect(wrapped("https://target.example.test/mcp")).resolves.toBe( + response, + ); + }); + + it("keeps a refusal's verdict and drops the cause that names the resolved address", async () => { + const refusal = new BlockedEgressTargetError( + 'Request URL hostname "internal.example.test" resolves to a private or internal address that the hosted inspector will not dial.', + { cause: new Error("resolved address: 10.1.2.3") }, + ); + const { thrown, stored } = await storedFor( + redactHostedTransportFailures(rejectingWith(refusal), { hosted: true }), + ); + + expect(thrown).toBeInstanceOf(BlockedEgressTargetError); + expect((thrown as Error).message).toBe(refusal.message); + expect((thrown as Error).cause).toBeUndefined(); + expect(stored).not.toMatch(/10\.1\.2\.3|resolved address/); + // The guard's own object keeps its cause: that copy belongs to the logs. + expect(refusal.cause).toBeDefined(); + }); + + it("collapses open-versus-closed socket outcomes to one message", async () => { + const outcomes = [ + socketError("connect ECONNREFUSED 203.0.113.7:6379", { + code: "ECONNREFUSED", + address: "203.0.113.7", + port: 6379, + syscall: "connect", + }), + socketError( + "C0B6F9E3:error:0A00010B:SSL routines:ssl3_get_record:wrong version number", + { code: "ERR_SSL_WRONG_VERSION_NUMBER", library: "SSL routines" }, + ), + new TypeError("fetch failed", { + cause: socketError("connect ETIMEDOUT 203.0.113.7:22", { + code: "ETIMEDOUT", + }), + }), + new Error("MCP server did not answer within 300000ms."), + ]; + + const stored = await Promise.all( + outcomes.map((outcome) => + storedFor( + redactHostedTransportFailures(rejectingWith(outcome), { + hosted: true, + }), + ), + ), + ); + + for (const { thrown, stored: json } of stored) { + expect((thrown as Error).message).toBe(HOSTED_TRANSPORT_FAILURE_DETAIL); + expect(json).not.toMatch( + /ECONN|ETIMEDOUT|203\.0\.113\.7|6379|ssl|wrong version|did not answer|cause/i, + ); + } + // The point of the exercise: the outcomes are indistinguishable. + expect(new Set(stored.map((entry) => entry.stored)).size).toBe(1); + }); + + it("collapses a resolution failure too, so a name's existence is not reported", async () => { + const { thrown, stored } = await storedFor( + redactHostedTransportFailures( + rejectingWith( + new EgressResolutionError( + 'Could not check "missing.example.test" for a safe address: getaddrinfo ENOTFOUND missing.example.test', + ), + ), + { hosted: true }, + ), + ); + expect((thrown as Error).message).toBe(HOSTED_TRANSPORT_FAILURE_DETAIL); + expect(stored).not.toMatch(/ENOTFOUND|getaddrinfo/); + }); + + it("passes the caller's own cancellation through by name", async () => { + const aborted = new DOMException( + "This operation was aborted", + "AbortError", + ); + const timedOut = new DOMException( + "The operation timed out", + "TimeoutError", + ); + for (const cancellation of [aborted, timedOut]) { + const { thrown } = await storedFor( + redactHostedTransportFailures(rejectingWith(cancellation), { + hosted: true, + }), + ); + expect(thrown).toBe(cancellation); + } + }); +}); + +describe("redactHostedTransportFailures (local)", () => { + it("is the identity outside hosted mode, where the socket error is the answer", () => { + const fetchFn = vi.fn() as unknown as typeof fetch; + expect(redactHostedTransportFailures(fetchFn, { hosted: false })).toBe( + fetchFn, + ); + }); +}); diff --git a/mcpjam-inspector/server/utils/hosted-doctor-redaction.ts b/mcpjam-inspector/server/utils/hosted-doctor-redaction.ts index 8d625bc84c..ddfa0efc66 100644 --- a/mcpjam-inspector/server/utils/hosted-doctor-redaction.ts +++ b/mcpjam-inspector/server/utils/hosted-doctor-redaction.ts @@ -18,7 +18,7 @@ import { HOSTED_MODE } from "../config.js"; * two different facts to someone walking a port range, which is what made them * the finding's Scenario B port scanner. */ -const HOSTED_TRANSPORT_FAILURE_DETAIL = +export const HOSTED_TRANSPORT_FAILURE_DETAIL = "The inspector could not establish a connection to this server."; /** diff --git a/mcpjam-inspector/server/utils/hosted-transport-failure-redaction.ts b/mcpjam-inspector/server/utils/hosted-transport-failure-redaction.ts new file mode 100644 index 0000000000..e797a878e2 --- /dev/null +++ b/mcpjam-inspector/server/utils/hosted-transport-failure-redaction.ts @@ -0,0 +1,84 @@ +/** + * Decide what a failed HOSTED dial may say about its target, at the one point + * where "did the target answer?" is still a fact rather than a guess: the fetch. + * + * WHY AT THE TRANSPORT. Conformance suites record the raw thrown value on every + * failed check (`failedResult(..., errorDetails)`), and the SDK's `deepJsonSafe` + * serializes an `Error` WITH its `cause` chain and its own enumerable fields. A + * hosted run is persisted and read back by the caller, so without this: + * + * - a REFUSAL carries its `cause` into the report. The egress guard keeps the + * address a hostname resolved to on `cause` precisely so it reaches logs + * and nothing else (`BlockedEgressTargetError`), and the pinned transport's + * original error names it too. Persisting the cause turns a refusal back + * into the resolution oracle the verdict's wording was written to avoid; + * - a socket, TLS or DNS failure keeps its own text — `ECONNREFUSED`, a TLS + * record error, a timeout — which is the open-versus-closed differential + * the hosted doctor already strips (`hosted-doctor-redaction.ts`). + * + * Deciding afterwards, over a finished report, would mean pattern-matching + * error spellings; deciding here is structural. A fetch that RESOLVED handed + * back a response from a host the guard allowed, and that response is the + * diagnostic the product exists to show, so it passes untouched. A fetch that + * REJECTED never got a response, so whatever its error says can only describe + * a refusal, a socket, TLS or DNS — and is reduced before any suite sees it: + * + * - the guard's refusal keeps its verdict and loses its `cause`. The verdict + * names only the host the caller (or the target's redirect) chose, never + * what it resolved to, which is what makes it safe to show; + * - the caller's OWN cancellation passes through unchanged — it describes our + * deadline, not the target, and the OAuth suite tells it apart by name; + * - everything else becomes the doctor's uniform message, with no code, no + * address and no cause. + * + * A no-op outside hosted mode, like the doctor's redaction: locally the socket + * error is the answer, and a developer whose server is not running needs to + * be told `ECONNREFUSED`. + */ + +import { HOSTED_MODE } from "../config.js"; +import { HOSTED_TRANSPORT_FAILURE_DETAIL } from "./hosted-doctor-redaction.js"; +import { BlockedEgressTargetError } from "./hosted-egress-guard.js"; + +/** + * The caller's own cancellation: an aborted signal, or a signal's timeout. Its + * message describes our deadline, and the only `cause` it can carry is the + * reason the CALLER aborted with. + */ +function isCallerCancellation(error: unknown): boolean { + return ( + error instanceof Error && + (error.name === "AbortError" || error.name === "TimeoutError") + ); +} + +function reduceTransportFailure(error: unknown): unknown { + if (isCallerCancellation(error)) return error; + if (error instanceof BlockedEgressTargetError) { + // A fresh instance rather than a mutated one: the original is the same + // object the guard may still be logging, and its `cause` belongs there. + return new BlockedEgressTargetError(error.message); + } + return new Error(HOSTED_TRANSPORT_FAILURE_DETAIL); +} + +/** + * Wrap `fetchFn` so a rejection can only say what is safe to persist. + * + * Responses are returned untouched, body and all. `hosted` defaults to + * `HOSTED_MODE`; pass it explicitly in tests. + */ +export function redactHostedTransportFailures( + fetchFn: typeof fetch, + options: { hosted?: boolean } = {}, +): typeof fetch { + const hosted = options.hosted ?? HOSTED_MODE; + if (!hosted) return fetchFn; + return (async (input: RequestInfo | URL, init?: RequestInit) => { + try { + return await fetchFn(input, init); + } catch (error) { + throw reduceTransportFailure(error); + } + }) as typeof fetch; +} diff --git a/sdk/src/conformance-run.ts b/sdk/src/conformance-run.ts index 92522d357f..d8e1e64a81 100644 --- a/sdk/src/conformance-run.ts +++ b/sdk/src/conformance-run.ts @@ -50,7 +50,18 @@ export type ConformanceRunProgress = { }; export type RunConformanceConfig = { - server: MCPServerConfig; + /** + * The target, and the transport every suite dials it through. + * + * `baseFetch` is the HTTP config's own seam (see `HttpServerConfig`), and + * `fetchFn` may ride alongside it. Whichever is set reaches EVERY suite: the + * apps and tasks suites connect with this config as-is, and the protocol + * suite adopts it as its own `fetchFn` — the one fetch its raw probes and + * its MCP client both dial through — unless `protocol.fetchFn` names one + * explicitly. A caller that guards the server config has guarded the run; + * there is no second place it has to remember. + */ + server: MCPServerConfig & { fetchFn?: typeof fetch }; suites?: ConformanceSuiteKind[]; protocolVersion?: MCPConformanceConfig["protocolVersion"]; protocol?: Partial>; @@ -70,6 +81,26 @@ function isHeaderRecord(value: unknown): value is Record { ); } +/** + * The fetch the caller attached to the server config, if any: `fetchFn` first, + * then the HTTP transport's `baseFetch`. + * + * Read structurally because `server` is a union, and a stdio config simply has + * neither — its suites never reach a fetch. + */ +function serverConfigFetch( + server: RunConformanceConfig["server"] +): typeof fetch | undefined { + const candidate = server as { fetchFn?: unknown; baseFetch?: unknown }; + if (typeof candidate.fetchFn === "function") { + return candidate.fetchFn as typeof fetch; + } + if (typeof candidate.baseFetch === "function") { + return candidate.baseFetch as typeof fetch; + } + return undefined; +} + async function runSuite( kind: ConformanceSuiteKind, config: RunConformanceConfig, @@ -89,6 +120,15 @@ async function runSuite( requestInit?: { headers?: unknown }; }; const headers = http.requestInit?.headers; + // THE SERVER CONFIG'S TRANSPORT IS THE SUITE'S TRANSPORT. Mapping the + // HTTP fields one by one (above) used to drop the caller's fetch on the + // floor, and `MCPConformanceTest` falls back to the global one: a hosted + // caller that handed over a DNS-pinned, redirect-revalidating fetch had + // the apps and tasks suites dial through it while this suite — raw + // probes and MCP client alike — followed redirects wherever they led. + // Applied AFTER the `protocol` spread so an explicit `fetchFn: undefined` + // there cannot quietly unset it; a real `protocol.fetchFn` still wins. + const fetchFn = config.protocol?.fetchFn ?? serverConfigFetch(server); const result = await new MCPConformanceTest({ ...(http.url !== undefined ? { serverUrl: String(http.url) } : {}), ...(http.accessToken !== undefined @@ -96,6 +136,7 @@ async function runSuite( : {}), ...(isHeaderRecord(headers) ? { customHeaders: headers } : {}), ...config.protocol, + ...(fetchFn ? { fetchFn } : {}), ...(config.protocolVersion ? { protocolVersion: config.protocolVersion } : {}), diff --git a/sdk/tests/conformance-run.test.ts b/sdk/tests/conformance-run.test.ts index ca57ab4aca..831e8c910a 100644 --- a/sdk/tests/conformance-run.test.ts +++ b/sdk/tests/conformance-run.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { buildConformanceRunReport, normalizeConformanceSuites, @@ -142,6 +142,124 @@ describe("conformance run bundle", () => { }); }); +describe("the protocol suite dials through the server config's transport", () => { + // Regression: the protocol suite rebuilt its config from url, token and + // headers only, so a fetch attached to the server config never reached it + // and `MCPConformanceTest` fell back to the global one. The apps and tasks + // suites of the same run used the caller's (guarded) fetch; this one + // followed redirects wherever they led. + const originalFetch = global.fetch; + const unauthorized = () => + vi.fn(async () => new Response("Unauthorized", { status: 401 })); + let globalFetch: ReturnType; + + beforeEach(() => { + globalFetch = vi.fn(async () => { + throw new Error("the global fetch must not be dialled"); + }); + global.fetch = globalFetch as never; + }); + + afterEach(() => { + global.fetch = originalFetch; + }); + + // One selection per path. `server-initialize` is the MCP client connection; + // `protocol-invalid-method-error` with a pinned version is raw probes only + // (a pin lets the raw runners skip the client connection entirely). + const PATHS = [ + { path: "MCP client", protocol: { checkIds: ["server-initialize"] } }, + { + path: "raw probes", + protocol: { checkIds: ["protocol-invalid-method-error"] }, + protocolVersion: "2025-11-25", + }, + ] as const; + + for (const { path, protocol, protocolVersion } of PATHS) { + it(`uses server.baseFetch for the ${path}`, async () => { + const baseFetch = unauthorized(); + await runConformance({ + server: { + url: "https://mcp.example.test/mcp", + baseFetch: baseFetch as never, + }, + suites: ["protocol"], + protocol: { + ...protocol, + checkIds: [...protocol.checkIds], + checkTimeout: 2_000, + }, + ...(protocolVersion ? { protocolVersion } : {}), + }); + expect(baseFetch).toHaveBeenCalled(); + for (const [input] of baseFetch.mock.calls as unknown as Array< + [unknown] + >) { + expect(String(input instanceof Request ? input.url : input)).toMatch( + /^https:\/\/mcp\.example\.test\// + ); + } + expect(globalFetch).not.toHaveBeenCalled(); + }, 30_000); + } + + it("prefers server.fetchFn over server.baseFetch", async () => { + const fetchFn = unauthorized(); + const baseFetch = unauthorized(); + await runConformance({ + server: { + url: "https://mcp.example.test/mcp", + fetchFn: fetchFn as never, + baseFetch: baseFetch as never, + }, + suites: ["protocol"], + protocol: { checkIds: ["server-initialize"], checkTimeout: 2_000 }, + }); + expect(fetchFn).toHaveBeenCalled(); + expect(baseFetch).not.toHaveBeenCalled(); + expect(globalFetch).not.toHaveBeenCalled(); + }, 30_000); + + it("keeps an explicit protocol.fetchFn override", async () => { + const override = unauthorized(); + const baseFetch = unauthorized(); + await runConformance({ + server: { + url: "https://mcp.example.test/mcp", + baseFetch: baseFetch as never, + }, + suites: ["protocol"], + protocol: { + checkIds: ["server-initialize"], + checkTimeout: 2_000, + fetchFn: override as never, + }, + }); + expect(override).toHaveBeenCalled(); + expect(baseFetch).not.toHaveBeenCalled(); + expect(globalFetch).not.toHaveBeenCalled(); + }, 30_000); + + it("does not let an explicit protocol.fetchFn: undefined unset the server's fetch", async () => { + const baseFetch = unauthorized(); + await runConformance({ + server: { + url: "https://mcp.example.test/mcp", + baseFetch: baseFetch as never, + }, + suites: ["protocol"], + protocol: { + checkIds: ["server-initialize"], + checkTimeout: 2_000, + fetchFn: undefined, + }, + }); + expect(baseFetch).toHaveBeenCalled(); + expect(globalFetch).not.toHaveBeenCalled(); + }, 30_000); +}); + describe("conformance run reporter", () => { const originalFetch = global.fetch; const originalEnv = { ...process.env };