fix(security): scrub agent-authored text on the github MCP egress path (PEN-3152)

deploy/helm/paperclip/templates/statefulset.yaml

@@ -459,7 +459,17 @@ spec:
               EOF
               cat > "${LOCAL_BIN}/github-mcp-server" <<'EOF'
               #!/bin/sh
-              exec /paperclip/.local/bin/paperclip-github-token-env /usr/local/bin/github-mcp-server "$@"
+              # PEN-3152: the SECOND egress door. The `gh` wrapper above was the
+              # whole of PEN-2527's coverage, but github-mcp-server is an
+              # independent path to the same destination holding the same seat
+              # token, and it runs the complete (default) toolset — so
+              # add_issue_comment / pull_request_review_write / create_pull_request
+              # / create_or_update_file / push_files all published model-authored
+              # text unscrubbed. The scrub runtime goes INSIDE the token wrapper so
+              # the server still inherits GITHUB_PERSONAL_ACCESS_TOKEN, exactly as
+              # before; the only change to its environment is that its stdin now
+              # arrives scrubbed. Do not "simplify" this back to a direct exec.
+              exec /paperclip/.local/bin/paperclip-github-token-env /usr/local/bin/node /opt/paperclip-bundled-adapters/node_modules/@paperclipai/adapter-utils/dist/github-mcp-egress-runtime.js /usr/local/bin/github-mcp-server "$@"
               EOF
               cat > "${LOCAL_BIN}/git" <<'EOF'
               #!/bin/sh

deploy/helm/paperclip/tests/agent-egress-path.test.mjs

@@ -268,3 +268,143 @@ test("the Blockcast overlay renders the PATH the chart now derives", () => {
     "/paperclip/.local/bin:/paperclip/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
   );
 });
+
+// --- The second door: the github MCP server (PEN-3152) --------------------
+//
+// The `gh` tests above turn on PATH ordering, because `gh` is resolved by name.
+// The MCP door is reached differently and so fails differently: the seeded
+// `.mcp.json` names an ABSOLUTE command, so PATH is irrelevant and the
+// equivalent question is whether that absolute path is the scrubbing wrapper or
+// the image server. PEN-3152 was filed because it was the latter — the wrapper
+// existed and injected a token, and no scrubber sat on the path.
+//
+// Same discipline as above: read both halves out of the render, and never
+// restate the value under test.
+//
+// One asymmetry worth naming rather than fixing here: the `gh` door's
+// reachability follows `persistence.mountPath` (see the test above), whereas
+// both the MCP wrapper's inner exec and the seeded command hardcode
+// `/paperclip/.local/bin`. That coupling predates PEN-3152 — the wrapper it
+// replaced hardcoded the same path — and the two halves hardcode it
+// consistently, so it holds at the default mountPath. The assertions below pin
+// the current reality; they are not an endorsement of the hardcode.
+
+// One wrapper's heredoc body, lifted from the rendered seed script. Asserts
+// rather than returning empty, so deleting a wrapper fails loudly.
+function extractWrapperBody(rendered, name) {
+  const lines = rendered.split("\n");
+  const startIdx = lines.findIndex(
+    (line) => line.trim() === `cat > "\${LOCAL_BIN}/${name}" <<'EOF'`,
+  );
+  assert.notEqual(startIdx, -1, `seed script no longer writes a ${name} wrapper`);
+  const body = [];
+  for (let i = startIdx + 1; i < lines.length; i += 1) {
+    if (lines[i].trim() === "EOF") return body.join("\n");
+    body.push(lines[i].trim());
+  }
+  throw new Error(`${name} wrapper heredoc is not terminated`);
+}
+
+// The `github` upstream's command as the seeded .mcp.json carries it.
+function seededMcpGitHubCommand(rendered) {
+  const match = /"github":\s*\{\s*"command":\s*"([^"]+)"/.exec(rendered);
+  assert.notEqual(match, null, "the seeded mcpServers block no longer has a github command");
+  return match[1];
+}
+
+test("the seeded github MCP upstream dials the scrubbing wrapper, not the image server", () => {
+  const rendered = render("templates/statefulset.yaml");
+  const command = seededMcpGitHubCommand(rendered);
+
+  // The whole control rests on this indirection. Pointing the seed at
+  // /usr/local/bin/github-mcp-server restores the PEN-3152 gap exactly, while
+  // leaving every wrapper assertion in this file green.
+  assert.equal(command, "/paperclip/.local/bin/github-mcp-server");
+
+  // ...and the thing it names must be a wrapper the seed actually writes.
+  assert.ok(
+    rendered.includes(`cat > "\${LOCAL_BIN}/${path.basename(command)}" <<'EOF'`),
+    `the seed does not write a ${path.basename(command)} wrapper for the mcp.json command to reach`,
+  );
+});
+
+test("the rendered github-mcp-server wrapper execs the scrub runtime inside the token wrapper", () => {
+  const body = extractWrapperBody(render("templates/statefulset.yaml"), "github-mcp-server");
+
+  const tokenAt = body.indexOf("paperclip-github-token-env");
+  const runtimeAt = body.indexOf("github-mcp-egress-runtime.js");
+  assert.notEqual(tokenAt, -1, "the MCP wrapper no longer injects the seat token");
+  assert.notEqual(runtimeAt, -1, "the MCP wrapper no longer execs the egress scrub runtime");
+
+  // Ordering is load-bearing in one direction only. The token wrapper must be
+  // OUTERMOST so the real server still inherits GITHUB_PERSONAL_ACCESS_TOKEN;
+  // putting the scrub outside it would start the server unauthenticated and
+  // fail every tool call, which is the shape that gets a security control
+  // reverted rather than fixed.
+  assert.ok(runtimeAt > tokenAt, `scrub runtime must run inside the token wrapper: ${body}`);
+
+  // The CLI runtime rewrites argv and the MCP runtime rewrites JSON-RPC frames;
+  // they are not interchangeable. Pointing this wrapper at the CLI runtime
+  // yields a process that starts, scrubs nothing, and looks plausible.
+  assert.ok(
+    !body.includes("github-cli-egress-runtime.js"),
+    "the MCP wrapper must not exec the CLI runtime",
+  );
+});
+
+test("the rendered MCP wrapper hands the real server to the scrub runtime as its target, with args after it", () => {
+  // Execute the wrapper the chart actually renders, with each absolute path
+  // replaced by a stub, so this fails if the exec chain is reordered — a
+  // runtime that received `stdio` as its target and the server path as an
+  // argument would still "run", and would scrub nothing.
+  const base = fs.mkdtempSync(path.join(os.tmpdir(), "gh-mcp-egress-"));
+  const stubs = path.join(base, "stubs");
+  fs.mkdirSync(stubs, { recursive: true });
+
+  const body = extractWrapperBody(render("templates/statefulset.yaml"), "github-mcp-server");
+  const execLine = body.split("\n").find((line) => line.startsWith("exec "));
+  assert.ok(execLine, `no exec line in the MCP wrapper: ${body}`);
+
+  // Stand-ins, each preserving the real component's argv contract: token-env
+  // and node both exec their remaining argv; the runtime reports what it got.
+  const tokenEnv = writeExecutable(stubs, "token-env", '#!/bin/sh\nexec "$@"\n');
+  const node = writeExecutable(stubs, "node", '#!/bin/sh\nexec "$@"\n');
+  const runtime = writeExecutable(
+    stubs,
+    "runtime",
+    '#!/bin/sh\nprintf "args=%s\\n" "$*"\n',
+  );
+
+  const rewritten = execLine
+    .replace("/paperclip/.local/bin/paperclip-github-token-env", tokenEnv)
+    .replace("/usr/local/bin/node", node)
+    .replace(/\S*github-mcp-egress-runtime\.js/, runtime);
+
+  // The rewrite must have consumed every path this host lacks, or the
+  // assertions below would be testing a line that cannot run for the wrong
+  // reason.
+  assert.ok(
+    !rewritten.includes("/paperclip/.local/bin/paperclip-github-token-env"),
+    `token-env path not substituted: ${rewritten}`,
+  );
+  assert.ok(
+    !rewritten.includes("github-mcp-egress-runtime.js"),
+    `runtime path not substituted: ${rewritten}`,
+  );
+
+  // "$@" in the wrapper takes the mcp.json args; $0 is supplied separately.
+  const result = spawnSync("/bin/sh", ["-c", rewritten, "sh", "stdio"], {
+    encoding: "utf8",
+  });
+  assert.equal(result.status, 0, result.stderr);
+
+  // The real server must be the runtime's target, with the mcp.json args after
+  // it — which is the argv contract github-mcp-egress-runtime.js reads as
+  // process.argv[2] (target) and slice(3) (args).
+  assert.equal(
+    result.stdout.trim(),
+    "args=/usr/local/bin/github-mcp-server stdio",
+    `unexpected argv threading: ${result.stdout}`,
+  );
+});
+

packages/adapter-utils/src/github-egress-door-parity.test.ts

@@ -0,0 +1,221 @@
+import { describe, expect, it } from "vitest";
+
+import type { GitHubEgressScrubClass } from "./github-egress-scrub.js";
+import { scrubGitHubCliInvocation } from "./github-cli-egress-shim.js";
+import { scrubGitHubMcpClientFrame } from "./github-mcp-egress-shim.js";
+
+/**
+ * PEN-3152 done-when 3: "a regression test that fails if a `github` MCP write
+ * tool can publish text the `gh` path would scrub."
+ *
+ * This file asserts PARITY rather than either door's behaviour in isolation,
+ * because the defect PEN-3152 recorded was not that the MCP door scrubbed
+ * badly — it was that the two doors DISAGREED, while looking to an agent like
+ * interchangeable ways to do the same thing:
+ *
+ *   > an agent has no way to tell that `mcp__github__add_issue_comment` is
+ *   > unscrubbed while `gh issue comment` is scrubbed. The safe path is the one
+ *   > with the *worse* ergonomics, so ordinary tool selection drifts toward the
+ *   > unguarded door.
+ *
+ * A per-door test cannot catch a re-divergence: both would keep passing while
+ * one door quietly stopped covering a class. Only a differential test does.
+ *
+ * The two shims delegate every decision to `scrubGitHubEgressText`, so parity
+ * is currently structural and these assertions are cheap. That is the point —
+ * they fail the moment someone gives one door its own policy, which is exactly
+ * how the original gap was introduced.
+ */
+
+// Derived fixtures. See github-mcp-egress-shim.test.ts on why no
+// credential-shaped literal appears in a new commit.
+function syntheticOpaque(length: number, seed: number): string {
+  const alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
+  let out = "";
+  let x = seed;
+  for (let i = 0; i < length; i += 1) {
+    x = (x * 1103515245 + 12345) % 2147483648;
+    out += alphabet[x % alphabet.length] as string;
+  }
+  return out;
+}
+
+function base64Url(value: string): string {
+  return Buffer.from(value, "utf8")
+    .toString("base64")
+    .replace(/\+/g, "-")
+    .replace(/\//g, "_")
+    .replace(/=+$/, "");
+}
+
+// Assembled, not written out — see the PEM_LABEL note in
+// github-mcp-egress-shim.test.ts on why an inline PEM header trips gitleaks.
+const PEM_LABEL = "RSA PRIVATE KEY";
+
+const SYNTHETIC_PEM = [
+  `-----BEGIN ${PEM_LABEL}-----`,
+  Buffer.from("SYNTHETIC-NOT-A-REAL-KEY-".repeat(3), "utf8").toString("base64"),
+  `-----END ${PEM_LABEL}-----`,
+].join("\n");
+
+const SYNTHETIC_JWT = [
+  base64Url(JSON.stringify({ alg: "HS256", typ: "JWT" })),
+  base64Url("synthetic-payload-not-real"),
+  base64Url("synthetic-signature"),
+].join(".");
+
+const SYNTHETIC_VENDOR_KEY = `gh${"p"}_${syntheticOpaque(36, 11)}`;
+const SYNTHETIC_OPAQUE = syntheticOpaque(32, 7);
+
+/**
+ * One payload per scrub class, plus the prose control.
+ *
+ * Every class the shared core can emit must appear here. The final test in this
+ * file asserts that, so adding a seventh class to `GitHubEgressScrubClass`
+ * fails until it is exercised through both doors.
+ */
+const PAYLOADS: readonly { name: string; text: string }[] = [
+  { name: "private-key-block", text: `rotation notes\n${SYNTHETIC_PEM}\ndone` },
+  {
+    name: "credentialed-uri",
+    text: `clone from https://oauth2:${SYNTHETIC_OPAQUE}@github.com/o/r.git and retry`,
+  },
+  { name: "jwt", text: `Authorization: Bearer ${SYNTHETIC_JWT}` },
+  { name: "vendor-key", text: `the seat token is ${SYNTHETIC_VENDOR_KEY}` },
+  {
+    name: "environment-dump",
+    text: [
+      "PAPERCLIP_ONE=alpha",
+      "PAPERCLIP_TWO=beta",
+      "PAPERCLIP_THREE=gamma",
+      "PAPERCLIP_FOUR=delta",
+      "PAPERCLIP_FIVE=epsilon",
+    ].join("\n"),
+  },
+  { name: "high-entropy-assignment", text: `SOME_UNENUMERATED_NAME=${SYNTHETIC_OPAQUE}` },
+  {
+    name: "clean prose (control)",
+    text: "## Review\n\nOne finding in `server/src/routes/issues.ts`. LGTM otherwise.",
+  },
+];
+
+/** What the `gh` door does with a body. */
+function throughCliDoor(text: string): { redacted: boolean; classes: GitHubEgressScrubClass[] } {
+  const result = scrubGitHubCliInvocation(["issue", "comment", "1435", "--body", text], {
+    readText: () => {
+      throw new Error("no file-backed text in this fixture");
+    },
+    writeTempText: () => {
+      throw new Error("no file-backed text in this fixture");
+    },
+  });
+  return { redacted: result.redacted, classes: result.classes };
+}
+
+/** What the MCP door does with the same body. */
+function throughMcpDoor(
+  text: string,
+  tool = "add_issue_comment",
+  field = "body",
+): { redacted: boolean; classes: GitHubEgressScrubClass[] } {
+  const result = scrubGitHubMcpClientFrame(
+    JSON.stringify({
+      jsonrpc: "2.0",
+      id: 1,
+      method: "tools/call",
+      params: { name: tool, arguments: { owner: "o", repo: "r", issue_number: 1435, [field]: text } },
+    }),
+  );
+  return { redacted: result.redacted, classes: result.classes };
+}
+
+describe("GitHub egress doors agree", () => {
+  describe.each(PAYLOADS)("$name", ({ text }) => {
+    it("both doors reach the same verdict and fire the same classes", () => {
+      const cli = throughCliDoor(text);
+      const mcp = throughMcpDoor(text);
+
+      expect(mcp.redacted).toBe(cli.redacted);
+      expect(mcp.classes).toEqual(cli.classes);
+    });
+
+    it("the MCP door never publishes what the CLI door removed", () => {
+      const cli = throughCliDoor(text);
+      if (!cli.redacted) return;
+
+      const raw = scrubGitHubMcpClientFrame(
+        JSON.stringify({
+          jsonrpc: "2.0",
+          id: 1,
+          method: "tools/call",
+          params: { name: "add_issue_comment", arguments: { body: text } },
+        }),
+      );
+
+      // Whatever the CLI door judged to be material must not survive in the
+      // frame the MCP server is handed.
+      for (const marker of [SYNTHETIC_PEM, SYNTHETIC_JWT, SYNTHETIC_VENDOR_KEY, SYNTHETIC_OPAQUE]) {
+        if (text.includes(marker)) expect(raw.line).not.toContain(marker);
+      }
+    });
+  });
+
+  /**
+   * The write tools PEN-3152 attested were live in an agent session, with the
+   * free-text parameter each one publishes.
+   *
+   * This list is the row's own evidence turned into an assertion. Its value is
+   * that it names TOOLS, so it keeps holding if the transform is ever narrowed
+   * from "every payload string" to something field-aware — which is the most
+   * likely future regression, since a field allowlist is the obvious
+   * optimisation and is precisely what would reopen the gap.
+   */
+  const WRITE_TOOLS: readonly { tool: string; field: string }[] = [
+    { tool: "add_issue_comment", field: "body" },
+    { tool: "pull_request_review_write", field: "body" },
+    { tool: "add_comment_to_pending_review", field: "body" },
+    { tool: "add_reply_to_pull_request_comment", field: "body" },
+    { tool: "create_pull_request", field: "body" },
+    { tool: "create_pull_request", field: "title" },
+    { tool: "update_pull_request", field: "body" },
+    { tool: "issue_write", field: "body" },
+    { tool: "create_or_update_file", field: "content" },
+    { tool: "create_or_update_file", field: "message" },
+    { tool: "push_files", field: "message" },
+  ];
+
+  describe.each(WRITE_TOOLS)("$tool.$field", ({ tool, field }) => {
+    it("cannot publish a private key", () => {
+      const result = throughMcpDoor(`context:\n${SYNTHETIC_PEM}`, tool, field);
+      expect(result.redacted).toBe(true);
+      expect(result.classes).toContain("private-key-block");
+    });
+
+    it("cannot publish a seat token", () => {
+      const result = throughMcpDoor(`token ${SYNTHETIC_VENDOR_KEY}`, tool, field);
+      expect(result.redacted).toBe(true);
+      expect(result.classes).toContain("vendor-key");
+    });
+  });
+
+  it("exercises every class the shared core can emit", () => {
+    // Keeps PAYLOADS exhaustive. If a new detector class is added to
+    // github-egress-scrub.ts, this fails until both doors are shown to agree
+    // on it — rather than the new class silently going untested at one door.
+    const allClasses: readonly GitHubEgressScrubClass[] = [
+      "private-key-block",
+      "credentialed-uri",
+      "jwt",
+      "vendor-key",
+      "environment-dump",
+      "high-entropy-assignment",
+    ];
+
+    const covered = new Set<GitHubEgressScrubClass>();
+    for (const { text } of PAYLOADS) {
+      for (const cls of throughMcpDoor(text).classes) covered.add(cls);
+    }
+
+    expect([...covered].sort()).toEqual([...allClasses].sort());
+  });
+});