Keep persisted conformance runs behind the pinned egress guard (MJ-001)

.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.

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(", ")})`
 );