From dc87fd288feda7df1297cf8b7d53b73e60cec15d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 06:38:12 +0000 Subject: [PATCH 1/4] Dial the conformance protocol suite through the server's fetch runConformance's protocol suite rebuilt its config from the server's URL, token and headers only, so a fetch attached to the server config never reached it and MCPConformanceTest fell back to the global fetch. The apps and tasks suites of the same run connected through the caller's fetch; this one, raw probes and MCP client alike, followed redirects wherever they led. A hosted caller that guarded the server config had therefore guarded two of the three suites. The protocol suite now adopts server.fetchFn, then server.baseFetch, as its own fetchFn. It is applied after the protocol spread so an explicit fetchFn: undefined cannot unset it, and a real protocol.fetchFn still wins. Callers that attach no fetch (the CLI) are unaffected. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Xjc8uLMHU952pqKaowF2LA --- sdk/src/conformance-run.ts | 43 ++++++++++- sdk/tests/conformance-run.test.ts | 120 +++++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 2 deletions(-) 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 }; From d60eee6e32c0082cacd36f7a6c3a7c136f78e073 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 06:38:23 +0000 Subject: [PATCH 2/4] Keep persisted conformance runs behind the egress guard (MJ-001) Persisted conformance runs dial a URL somebody else chose: a saved server, a sandbox running a pull request's code, a benchmarked connector. The benchmark and GitHub-check workers handed the executor a bare { url }, so every suite dialled through the global fetch, with no address classification and redirects followed unchecked. The /v1 start route did attach the pinned guard, but the protocol suite dropped it (fixed in the previous commit). The executor is now the chokepoint for all three callers: - It defaults the MCP and OAuth transports to the hosted conformance guard when the caller passes none. A deliberate transport still wins. Both workers now pass the guard explicitly as well. - It judges the starting URL with the same check the routes use, and a refused target is never handed to a suite. Each suite records the refusal as its could-not-run reason. The up-front check matters for the protocol suite's localhost host-header checks, which open raw node:http sockets that no fetch can guard. - Every transport is wrapped so that a rejected dial reaches the stored report as the guard's verdict without its cause, or as the doctor's uniform connection message. Before this, the report serializer copied the refusal's cause, which carries the address a hostname resolved to, and socket, TLS and DNS error text. The GitHub-check health probe also dials the pull request's server through the hosted MCP transport instead of the global fetch. The sandbox URL is E2B's public HTTPS edge, and the eval half of the check already reaches it through the same pinned transport, so it needs no allowance. All of this is a no-op outside hosted mode. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Xjc8uLMHU952pqKaowF2LA --- .../persisted-conformance-egress-guard.md | 19 + .../conformance-run-executor-egress.test.ts | 343 ++++++++++++++++++ .../conformance-worker-egress.test.ts | 120 ++++++ .../server/services/bench-worker.ts | 13 +- .../services/conformance-run-executor.ts | 152 +++++++- .../server/services/github-checks-worker.ts | 10 + .../__tests__/sandbox-health-egress.test.ts | 98 +++++ .../server/services/github-checks/sandbox.ts | 7 +- ...hosted-transport-failure-redaction.test.ts | 171 +++++++++ .../server/utils/hosted-doctor-redaction.ts | 2 +- .../hosted-transport-failure-redaction.ts | 84 +++++ 11 files changed, 1012 insertions(+), 7 deletions(-) create mode 100644 .changeset/persisted-conformance-egress-guard.md create mode 100644 mcpjam-inspector/server/services/__tests__/conformance-run-executor-egress.test.ts create mode 100644 mcpjam-inspector/server/services/__tests__/conformance-worker-egress.test.ts create mode 100644 mcpjam-inspector/server/services/github-checks/__tests__/sandbox-health-egress.test.ts create mode 100644 mcpjam-inspector/server/utils/__tests__/hosted-transport-failure-redaction.test.ts create mode 100644 mcpjam-inspector/server/utils/hosted-transport-failure-redaction.ts 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/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 d5ae0fb1c6..97424cf865 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; +} From 59b07c12d866d209507d8db428226d643e976cbb Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 06:38:30 +0000 Subject: [PATCH 3/4] Check hosted imports of SDK entry points that dial for themselves The hosted-manager guard only saw `new MCPClientManager`. runConformance, the conformance suites, withEphemeralClient, the probe and the doctor each open their own connection through a fetch seam that is the global fetch unless filled. The persisted conformance path went through exactly that gap while the first rule was green. The script now also scans server/routes/shared, and a hosted file that statically imports one of those entry points from @mcpjam/sdk must be on a second allowlist. Each entry names the guard the file dials through, and that guard must still appear in the file. A new importer fails by existing, a namespace import of the SDK is refused outright, and stale entries fail as they do for the first rule. The header states what a source scan cannot see (dynamic imports, per-call arguments) and points at the runtime tests that cover it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Xjc8uLMHU952pqKaowF2LA --- .../check-hosted-manager-base-fetch.mjs | 244 +++++++++++++++++- 1 file changed, 234 insertions(+), 10 deletions(-) diff --git a/mcpjam-inspector/scripts/check-hosted-manager-base-fetch.mjs b/mcpjam-inspector/scripts/check-hosted-manager-base-fetch.mjs index 8025cb10cc..16475b2eb6 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,135 @@ 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"], + { + guard: /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 +353,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 +409,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 +495,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(", ")})` ); From c0b3dc29afb63ba9b5e2e36e9a27d87629fb577c Mon Sep 17 00:00:00 2001 From: olartgabo Date: Wed, 23 Sep 2026 12:28:42 -0400 Subject: [PATCH 4/4] Anchor the conformance egress guard rule on the call, not the name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rule for `conformance-run-executor.ts` matched a bare `guardPersistedConformanceTransport(`, and that file also DEFINES the guard. The definition satisfied the pattern on its own, so deleting the call site at `:470` and passing `args.server` straight to `runConformance` left CI green — the exact MJ-001 regression this rule exists to catch. Anchoring on the assignment makes the pattern satisfiable only by a call. Verified both ways: the script passes as-is, and removing the call site now fails it with the rule's own message. --- .../scripts/check-hosted-manager-base-fetch.mjs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mcpjam-inspector/scripts/check-hosted-manager-base-fetch.mjs b/mcpjam-inspector/scripts/check-hosted-manager-base-fetch.mjs index 16475b2eb6..7e2cd1fda7 100644 --- a/mcpjam-inspector/scripts/check-hosted-manager-base-fetch.mjs +++ b/mcpjam-inspector/scripts/check-hosted-manager-base-fetch.mjs @@ -135,7 +135,11 @@ const SELF_DIALING_ALLOWED = new Map( [ ["services", "conformance-run-executor.ts"], { - guard: /guardPersistedConformanceTransport\(/, + // 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", }, ],