From fd577e4154284323df826d5633273bc61178afa2 Mon Sep 17 00:00:00 2001 From: Cto Date: Thu, 10 Sep 2026 18:00:22 +0000 Subject: [PATCH 1/2] fix(security): scrub agent-authored text on the github MCP egress path (PEN-3152) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PEN-2527 set out to make it impossible for an agent to publish unscrubbed text to GitHub, after PEN-2526 put a GitHub App private key and the fleet's agent JWT signing secret onto a public PR for ~7h35m. The control it shipped wraps exactly one thing: the `gh` binary. The `github` MCP server is a second outbound path to the same destination. It holds the same seat token, runs the complete (default) toolset, and did not pass through any scrubber. `add_issue_comment`, `pull_request_review_write`, `create_pull_request`, `create_or_update_file` and `push_files` all take free-form model-authored text and publish it; `create_or_update_file` and `push_files` take file *content*, so the exposure was never limited to prose. The asymmetry was the dangerous part. An agent cannot tell that `mcp__github__add_issue_comment` is unscrubbed while `gh issue comment` is scrubbed, so ordinary tool selection drifted toward the unguarded door. The fix is therefore not a second policy but the same policy at the second door. `github-mcp-egress-{shim,runtime}.ts` delegate every decision to the existing `scrubGitHubEgressText`, and `github-egress-door-parity.test.ts` asserts the two doors reach identical verdicts on every scrub class — so a future change that gives one door its own policy fails, which is exactly how the original gap was introduced. Two implementation notes worth keeping: - The MCP transform is broader than the CLI shim's on purpose. The CLI knows which argv flags carry authored text; MCP tool schemas are supplied by the server at runtime, so a parameter-name allowlist would be a hole with a release cadence. This scrubs every string in every non-envelope JSON-RPC member instead, and covers a tool added upstream on the day it ships. - Only the child's stdin is piped; stdout and stderr are inherited. So the response leg cannot be altered here even by mistake, and the inbound direction stays PEN-2370's to own. Also adds the outbound coverage table PEN-3152 asked for, as a sibling of mcp-seed-scrub-coverage.test.ts — whose `github: stdio-not-proxied` row governs the inbound leg only, and is what made this gap look covered. Enumerating the outbound doors surfaced two more that no scrubber sits on, now filed rather than left implicit: - PEN-3156: the `git` wrapper. `git push` publishes commit messages and file contents. Not fixable by in-flight redaction, since commit objects are content-addressed; the fix shape is refusal at push time. - PEN-3157: server-side writes. `paperclip-api` reaches GitHub over HTTP from server/, touching no wrapper, and `pr-comment-review-gate.ts` already republishes verbs parsed verbatim out of an Ally review comment into a public commit-status description. Refs PEN-3152, PEN-2527, PEN-2526, PEN-2370. Signed-off-by: Cto --- .../helm/paperclip/templates/statefulset.yaml | 12 +- .../tests/agent-egress-path.test.mjs | 140 ++++++++ .../src/github-egress-door-parity.test.ts | 221 ++++++++++++ .../src/github-mcp-egress-runtime.test.ts | 252 ++++++++++++++ .../src/github-mcp-egress-runtime.ts | 258 ++++++++++++++ .../src/github-mcp-egress-shim.test.ts | 314 ++++++++++++++++++ .../src/github-mcp-egress-shim.ts | 195 +++++++++++ .../github-egress-outbound-coverage.test.ts | 290 ++++++++++++++++ 8 files changed, 1681 insertions(+), 1 deletion(-) create mode 100644 packages/adapter-utils/src/github-egress-door-parity.test.ts create mode 100644 packages/adapter-utils/src/github-mcp-egress-runtime.test.ts create mode 100644 packages/adapter-utils/src/github-mcp-egress-runtime.ts create mode 100644 packages/adapter-utils/src/github-mcp-egress-shim.test.ts create mode 100644 packages/adapter-utils/src/github-mcp-egress-shim.ts create mode 100644 server/src/__tests__/github-egress-outbound-coverage.test.ts diff --git a/deploy/helm/paperclip/templates/statefulset.yaml b/deploy/helm/paperclip/templates/statefulset.yaml index 87a43ad97b52..656396bb0b6b 100644 --- a/deploy/helm/paperclip/templates/statefulset.yaml +++ b/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 diff --git a/deploy/helm/paperclip/tests/agent-egress-path.test.mjs b/deploy/helm/paperclip/tests/agent-egress-path.test.mjs index 40603c2663d9..c385c73de229 100644 --- a/deploy/helm/paperclip/tests/agent-egress-path.test.mjs +++ b/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}`, + ); +}); + diff --git a/packages/adapter-utils/src/github-egress-door-parity.test.ts b/packages/adapter-utils/src/github-egress-door-parity.test.ts new file mode 100644 index 000000000000..77268776ba59 --- /dev/null +++ b/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(); + for (const { text } of PAYLOADS) { + for (const cls of throughMcpDoor(text).classes) covered.add(cls); + } + + expect([...covered].sort()).toEqual([...allClasses].sort()); + }); +}); diff --git a/packages/adapter-utils/src/github-mcp-egress-runtime.test.ts b/packages/adapter-utils/src/github-mcp-egress-runtime.test.ts new file mode 100644 index 000000000000..df95a1260336 --- /dev/null +++ b/packages/adapter-utils/src/github-mcp-egress-runtime.test.ts @@ -0,0 +1,252 @@ +import { spawnSync } from "node:child_process"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, describe, expect, it } from "vitest"; + +import { redactionMarker } from "./github-egress-scrub.js"; +import { MAX_FRAME_BYTES, splitFrames, transformFrame } from "./github-mcp-egress-runtime.js"; + +const sourceDirectory = path.dirname(fileURLToPath(import.meta.url)); +const runtimeEntryPoint = path.join(sourceDirectory, "github-mcp-egress-runtime.ts"); + +// Derived, not literal — see the fixture note in github-mcp-egress-shim.test.ts +// on why a credential-shaped literal must not appear in a new commit, and the +// PEM_LABEL note there on why the PEM header in particular is assembled. +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 temporaryDirectories: string[] = []; + +afterEach(() => { + while (temporaryDirectories.length > 0) { + const directory = temporaryDirectories.pop(); + if (directory) rmSync(directory, { recursive: true, force: true }); + } +}); + +/** + * A stand-in for `github-mcp-server`: records every stdin frame it received to + * a file, and writes one response frame so the response leg is exercised too. + */ +function makeFakeServer(): { server: string; record: string } { + const directory = mkdtempSync(path.join(os.tmpdir(), "paperclip-gh-mcp-egress-")); + temporaryDirectories.push(directory); + const server = path.join(directory, "fake-mcp-server.mjs"); + const record = path.join(directory, "received.txt"); + + writeFileSync( + server, + [ + "import { appendFileSync } from 'node:fs';", + "const record = process.argv[2];", + "let buffer = '';", + "process.stdin.on('data', (chunk) => {", + " buffer += chunk.toString('utf8');", + " let at;", + " while ((at = buffer.indexOf('\\n')) >= 0) {", + " appendFileSync(record, buffer.slice(0, at) + '\\n');", + " buffer = buffer.slice(at + 1);", + " }", + "});", + "process.stdin.on('end', () => {", + " process.stdout.write(JSON.stringify({ jsonrpc: '2.0', id: 1, result: { ok: true } }) + '\\n');", + " process.exit(7);", + "});", + ].join("\n"), + "utf8", + ); + + return { server, record }; +} + +function runRuntime(input: string): { record: string; status: number | null; stdout: string; stderr: string } { + const { server, record } = makeFakeServer(); + writeFileSync(record, "", "utf8"); + + const result = spawnSync( + process.execPath, + ["--import", "tsx", runtimeEntryPoint, process.execPath, server, record], + { input, encoding: "utf8" }, + ); + + return { + record: readFileSync(record, "utf8"), + status: result.status, + stdout: result.stdout ?? "", + stderr: result.stderr ?? "", + }; +} + +describe("github MCP egress runtime", () => { + describe("splitFrames", () => { + it("splits complete lines and keeps the unterminated remainder", () => { + expect(splitFrames('{"a":1}\n{"b":2}\n{"c":')).toEqual({ + lines: ['{"a":1}', '{"b":2}'], + rest: '{"c":', + }); + }); + + it("returns no lines when no newline has arrived", () => { + expect(splitFrames('{"partial"')).toEqual({ lines: [], rest: '{"partial"' }); + }); + + it("yields an empty trailing remainder when the buffer ends on a newline", () => { + expect(splitFrames('{"a":1}\n')).toEqual({ lines: ['{"a":1}'], rest: "" }); + }); + + it("preserves empty frames rather than collapsing them", () => { + expect(splitFrames("\n\n").lines).toEqual(["", ""]); + }); + }); + + describe("transformFrame", () => { + it("re-appends exactly one newline to a clean frame", () => { + const line = '{"jsonrpc":"2.0","id":1,"method":"ping"}'; + const notify = [] as string[][]; + expect(transformFrame(line, { notify: (c) => notify.push([...c]) })).toBe(`${line}\n`); + expect(notify).toEqual([]); + }); + + it("preserves a CRLF client's carriage return", () => { + const line = '{"jsonrpc":"2.0","id":1,"method":"ping"}\r'; + expect(transformFrame(line, { notify: () => {} })).toBe(`${line}\n`); + }); + + it("notifies with CLASSES only, never the matched content", () => { + const notified: string[][] = []; + const line = JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { arguments: { body: SYNTHETIC_PEM } }, + }); + + const out = transformFrame(line, { notify: (classes) => notified.push([...classes]) }); + + expect(notified).toEqual([["private-key-block"]]); + expect(out).toContain(redactionMarker("private-key-block")); + expect(out.endsWith("\n")).toBe(true); + }); + + it("keeps the frame delimiter intact when the redaction changes the payload length", () => { + const line = JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { arguments: { body: `before\n${SYNTHETIC_PEM}\nafter` } }, + }); + + const out = transformFrame(line, { notify: () => {} }); + + // Exactly one terminator, and no interior newline that would be read as + // a second frame. + expect(out.match(/\n/g)).toHaveLength(1); + expect(out.endsWith("\n")).toBe(true); + }); + }); + + describe("end to end through a real child process", () => { + it("delivers a clean frame byte-for-byte", () => { + const line = JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { name: "get_me", arguments: {} }, + }); + + const { record } = runRuntime(`${line}\n`); + + expect(record).toBe(`${line}\n`); + }); + + it("the server never sees credential material an agent wrote", () => { + const line = JSON.stringify({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { + name: "add_issue_comment", + arguments: { owner: "o", repo: "r", issue_number: 1, body: SYNTHETIC_PEM }, + }, + }); + + const { record, stderr } = runRuntime(`${line}\n`); + + expect(record).not.toContain(PEM_LABEL); + expect(record).toContain(redactionMarker("private-key-block")); + // Still a single well-formed frame the server can parse. + expect(record.trimEnd().split("\n")).toHaveLength(1); + expect(JSON.parse(record.trimEnd())).toMatchObject({ id: 2, method: "tools/call" }); + // Audit signal names the class and nothing else. + expect(stderr).toContain("private-key-block"); + expect(stderr).not.toContain(PEM_LABEL); + }); + + it("handles a frame split across stdin chunks", () => { + // spawnSync hands the whole input at once, so exercise the buffering + // boundary by sending two frames where the second is only completed by + // the final newline. + const first = JSON.stringify({ jsonrpc: "2.0", id: 1, method: "a", params: {} }); + const second = JSON.stringify({ + jsonrpc: "2.0", + id: 2, + method: "b", + params: { arguments: { body: SYNTHETIC_PEM } }, + }); + + const { record } = runRuntime(`${first}\n${second}\n`); + + const lines = record.trimEnd().split("\n"); + expect(lines).toHaveLength(2); + expect(lines[0]).toBe(first); + expect(lines[1]).toContain(redactionMarker("private-key-block")); + }); + + it("drops an unterminated trailing fragment instead of forwarding a truncated message", () => { + const complete = JSON.stringify({ jsonrpc: "2.0", id: 1, method: "a", params: {} }); + + const { record } = runRuntime(`${complete}\n{"jsonrpc":"2.0","id":2,"meth`); + + expect(record).toBe(`${complete}\n`); + }); + + it("propagates the server's exit code", () => { + const { status } = runRuntime('{"jsonrpc":"2.0","id":1,"method":"a","params":{}}\n'); + expect(status).toBe(7); + }); + + it("passes the server's stdout through untouched", () => { + // stdout is inherited, so the response leg does not enter this process. + const { stdout } = runRuntime('{"jsonrpc":"2.0","id":1,"method":"a","params":{}}\n'); + expect(JSON.parse(stdout.trim())).toEqual({ jsonrpc: "2.0", id: 1, result: { ok: true } }); + }); + }); + + describe("fail closed", () => { + it("caps the unterminated buffer well above any legitimate frame", () => { + // Guard the constant itself: a future edit that drops it to a plausible + // payload size would start refusing real traffic. + expect(MAX_FRAME_BYTES).toBeGreaterThanOrEqual(16 * 1024 * 1024); + }); + + it("refuses a frame it cannot walk rather than forwarding it", () => { + let nested = '{"deep":1}'; + for (let i = 0; i < 260; i += 1) nested = `{"a":${nested}}`; + + const { record, status, stderr } = runRuntime( + `{"jsonrpc":"2.0","id":1,"method":"tools/call","params":${nested}}\n`, + ); + + expect(record).toBe(""); + expect(status).not.toBe(0); + expect(stderr).toContain("refusing to forward it unscrubbed"); + }); + }); +}); diff --git a/packages/adapter-utils/src/github-mcp-egress-runtime.ts b/packages/adapter-utils/src/github-mcp-egress-runtime.ts new file mode 100644 index 000000000000..ee8b4cb2176b --- /dev/null +++ b/packages/adapter-utils/src/github-mcp-egress-runtime.ts @@ -0,0 +1,258 @@ +// PEN-3152: process the MCP egress transform at the `github-mcp-server` +// boundary, mirroring github-cli-egress-runtime.ts at the `gh` boundary. +// +// The Helm seed writes a shell launcher for this module into +// /paperclip/.local/bin/github-mcp-server, which is what the seeded `.mcp.json` +// names as the `github` upstream's command. So this sits between the agent's +// MCP client and the real server, in the same position the CLI runtime occupies +// in front of /usr/bin/gh. +// +// ## Only stdin is piped, and that is a security property +// +// The child is spawned with stdout and stderr INHERITED. Responses therefore +// travel from the server to the client without passing through this process at +// all — there is no buffer here that could reorder them, no parser that could +// reject one, and no code path that could alter one. The inbound leg stays +// PEN-2370's to own, and "this runtime cannot affect responses" is a fact about +// the process topology rather than a claim about the code below. +// +// ## Fail-closed policy +// +// A frame is forwarded only after it has been scrubbed. If it cannot be +// scrubbed — unparseable depth, or an oversized frame with no terminator — the +// runtime tears down rather than passing it through. An agent seeing its MCP +// server drop is a loud, diagnosable failure; an agent whose secret reached a +// public repository is not. + +import { spawn } from "node:child_process"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +import { + GitHubMcpEgressFrameError, + scrubGitHubMcpClientFrame, +} from "./github-mcp-egress-shim.js"; + +/** + * Largest client frame this runtime will buffer while waiting for its + * terminating newline, in bytes. + * + * A frame must be complete before it can be scrubbed, so some cap is required + * or a stream that never emits a newline grows without bound. 64 MiB is far + * above any legitimate MCP frame — GitHub's own contents API rejects blobs + * orders of magnitude smaller — and exceeding it means the stream is not + * carrying MCP traffic, at which point refusing is correct. + */ +export const MAX_FRAME_BYTES = 64 * 1024 * 1024; + +export interface GitHubMcpEgressRuntimeOptions { + target: string; + argv: string[]; +} + +export class GitHubMcpEgressRuntimeError extends Error { + constructor( + message: string, + readonly exitCode = 64, + ) { + super(message); + this.name = "GitHubMcpEgressRuntimeError"; + } +} + +/** Where a redaction notice goes. Split out so tests can capture it. */ +export interface GitHubMcpEgressRuntimeIo { + /** Structured audit line. Receives the fired CLASSES only, never the content. */ + notify(classes: readonly string[]): void; +} + +const defaultIo: GitHubMcpEgressRuntimeIo = { + notify: (classes) => { + // Classes only. Logging the matched text would re-publish the very material + // this runtime exists to contain, into a stream the operator then reads. + console.error( + `paperclip-github-mcp-egress: redacted outbound frame (${classes.join(", ")})`, + ); + }, +}; + +/** + * Split a chunk-accumulated buffer into complete lines plus the trailing + * remainder, enforcing the frame cap on the remainder. + * + * Exported for tests: the buffering is where a stdio proxy usually goes wrong, + * and it is worth asserting directly rather than only through a spawned child. + */ +export function splitFrames(buffer: string): { lines: string[]; rest: string } { + const lines: string[] = []; + let start = 0; + for (;;) { + const at = buffer.indexOf("\n", start); + if (at < 0) break; + lines.push(buffer.slice(start, at)); + start = at + 1; + } + return { lines, rest: buffer.slice(start) }; +} + +/** + * Transform one client frame, returning the bytes to forward. + * + * Newline handling is deliberate: the terminator is re-appended here rather + * than carried through the scrubber, so a redaction that changes the payload + * length cannot lose or double the frame delimiter. + */ +export function transformFrame( + line: string, + io: GitHubMcpEgressRuntimeIo, +): string { + // The MCP stdio transport is newline-delimited JSON. A frame that arrived + // with a CR (a client on \r\n) keeps it: strip for parsing, restore on the + // way out, so the child sees exactly the framing its client chose. + const hasCr = line.endsWith("\r"); + const payload = hasCr ? line.slice(0, -1) : line; + + const result = scrubGitHubMcpClientFrame(payload); + if (result.redacted) io.notify(result.classes); + + return `${result.line}${hasCr ? "\r" : ""}\n`; +} + +export function runGitHubMcpEgressRuntime( + options: GitHubMcpEgressRuntimeOptions, + io: GitHubMcpEgressRuntimeIo = defaultIo, +): Promise { + const { target, argv } = options; + if (!target) { + return Promise.reject(new GitHubMcpEgressRuntimeError("missing GitHub MCP server target")); + } + + return new Promise((resolve, reject) => { + // stdin piped so frames can be rewritten; stdout/stderr inherited so the + // response leg never enters this process. See the header note. + const child = spawn(target, argv, { stdio: ["pipe", "inherit", "inherit"] }); + + let settled = false; + let forwardedSignal = false; + let buffer = ""; + + const forwardSignal = (signal: NodeJS.Signals) => { + forwardedSignal = true; + child.kill(signal); + }; + process.on("SIGINT", forwardSignal); + process.on("SIGTERM", forwardSignal); + + const cleanup = () => { + process.off("SIGINT", forwardSignal); + process.off("SIGTERM", forwardSignal); + process.stdin.removeListener("data", onData); + process.stdin.removeListener("end", onEnd); + }; + + const fail = (error: Error) => { + if (settled) return; + settled = true; + cleanup(); + // Fail closed: kill the server rather than leaving a half-guarded + // channel open behind a runtime that has stopped scrubbing. + child.kill("SIGKILL"); + reject(error); + }; + + // The child exiting first is normal (the client closed the session), so a + // write racing that exit must not surface as a crash. + child.stdin.on("error", (error: NodeJS.ErrnoException) => { + if (error.code === "EPIPE") return; + fail(new GitHubMcpEgressRuntimeError(`MCP server stdin failed (${error.code ?? "unknown"})`, 1)); + }); + + function onData(chunk: Buffer | string): void { + buffer += typeof chunk === "string" ? chunk : chunk.toString("utf8"); + const { lines, rest } = splitFrames(buffer); + buffer = rest; + + if (Buffer.byteLength(buffer, "utf8") > MAX_FRAME_BYTES) { + fail( + new GitHubMcpEgressRuntimeError( + `client frame exceeded ${MAX_FRAME_BYTES} bytes with no newline terminator; refusing to forward it unscrubbed`, + ), + ); + return; + } + + for (const line of lines) { + let out: string; + try { + out = transformFrame(line, io); + } catch (error) { + fail( + error instanceof GitHubMcpEgressFrameError + ? new GitHubMcpEgressRuntimeError(error.message) + : new GitHubMcpEgressRuntimeError("unable to scrub outbound MCP frame"), + ); + return; + } + if (settled) return; + // Honour backpressure so a slow server cannot make this process the + // place where frames pile up. + if (!child.stdin.write(out)) { + process.stdin.pause(); + child.stdin.once("drain", () => process.stdin.resume()); + } + } + } + + function onEnd(): void { + // A trailing fragment with no newline is not a frame. Forwarding it would + // hand the server a truncated message; dropping it is what a newline- + // delimited transport already implies. + child.stdin.end(); + } + + process.stdin.on("data", onData); + process.stdin.on("end", onEnd); + process.stdin.resume(); + + child.once("error", (error: NodeJS.ErrnoException) => { + fail( + new GitHubMcpEgressRuntimeError( + `unable to start GitHub MCP server (${error.code ?? "unknown error"})`, + 1, + ), + ); + }); + + child.once("close", (code, signal) => { + if (settled) return; + settled = true; + cleanup(); + if (code !== null) { + resolve(code); + return; + } + resolve(forwardedSignal ? 128 : signal ? 128 + (signal === "SIGINT" ? 2 : 15) : 1); + }); + }); +} + +function reportRuntimeError(error: unknown): void { + const message = error instanceof Error ? error.message : "unexpected preparation failure"; + const exitCode = error instanceof GitHubMcpEgressRuntimeError ? error.exitCode : 1; + console.error(`paperclip-github-mcp-egress: ${message}`); + process.exitCode = exitCode; +} + +if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { + const target = process.argv[2]; + const argv = process.argv.slice(3); + try { + void runGitHubMcpEgressRuntime({ target: target ?? "", argv }) + .then((exitCode) => { + process.exitCode = exitCode; + }) + .catch(reportRuntimeError); + } catch (error) { + reportRuntimeError(error); + } +} diff --git a/packages/adapter-utils/src/github-mcp-egress-shim.test.ts b/packages/adapter-utils/src/github-mcp-egress-shim.test.ts new file mode 100644 index 000000000000..15e0b9009825 --- /dev/null +++ b/packages/adapter-utils/src/github-mcp-egress-shim.test.ts @@ -0,0 +1,314 @@ +import { describe, expect, it } from "vitest"; + +import { redactionMarker } from "./github-egress-scrub.js"; +import { + GitHubMcpEgressFrameError, + scrubGitHubMcpClientFrame, +} from "./github-mcp-egress-shim.js"; + +// Every fixture below is SYNTHETIC and DERIVED — assembled at runtime rather +// than written as a literal. Two separate reasons, both load-bearing: +// +// 1. PEN-2526's standing rule: never paste real material into a test "to make +// it realistic". +// 2. CI scans the COMMIT RANGE with gitleaks, not the worktree. A +// credential-shaped literal in a new commit trips the gate and cannot be +// cleared by a follow-up commit that deletes it — only by rewriting the +// commit. Sibling fixtures already on master are outside the range and so +// are never rescanned; their literals are not precedent for a new file. +// Deriving closes the finding at source instead. + +/** Deterministic LCG over a 62-char alphabet: mixed case + digits, high per-character entropy. */ +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(/=+$/, ""); +} + +// The PEM label is a named constant rather than an inline literal because +// gitleaks' default `private-key` rule matches `-----BEGIN <...> PRIVATE KEY-----` +// as one token. Spelling that out inline plants a scanner finding inside the +// security PR that exists to stop credential material reaching GitHub. Breaking +// the adjacency keeps the fixture material to the scrubber and invisible to the +// scanner, and gives the assertions below a single source of truth. +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_OPAQUE = syntheticOpaque(32, 7); + +// Assembled from parts so the prefix and the tail never sit adjacent in source. +const SYNTHETIC_VENDOR_KEY = `gh${"p"}_${syntheticOpaque(36, 11)}`; + +function frame(method: string, params: unknown, id = 1): string { + return JSON.stringify({ jsonrpc: "2.0", id, method, params }); +} + +describe("scrubGitHubMcpClientFrame", () => { + describe("byte-exact pass-through", () => { + it("returns the identical string when nothing matches", () => { + const line = frame("tools/call", { + name: "add_issue_comment", + arguments: { + owner: "Blockcast", + repo: "paperclip", + issue_number: 1435, + body: "## Review\n\nOne finding in `server/src/routes/issues.ts`. LGTM otherwise.", + }, + }); + + const result = scrubGitHubMcpClientFrame(line); + + expect(result.redacted).toBe(false); + expect(result.classes).toEqual([]); + // Byte-exact, not merely equivalent: a clean frame must not be + // re-serialised on its way to the server. + expect(result.line).toBe(line); + }); + + it("leaves a non-JSON line alone rather than becoming a second protocol validator", () => { + const result = scrubGitHubMcpClientFrame("not json at all"); + expect(result).toEqual({ line: "not json at all", redacted: false, classes: [] }); + }); + + it("leaves an empty or whitespace-only line alone", () => { + expect(scrubGitHubMcpClientFrame("")).toEqual({ line: "", redacted: false, classes: [] }); + expect(scrubGitHubMcpClientFrame(" ")).toEqual({ line: " ", redacted: false, classes: [] }); + }); + + it("leaves a JSON scalar alone", () => { + expect(scrubGitHubMcpClientFrame("42").redacted).toBe(false); + expect(scrubGitHubMcpClientFrame('"a string"').redacted).toBe(false); + }); + }); + + describe("the write tools PEN-3152 found unguarded", () => { + it("scrubs a PEM out of add_issue_comment's body", () => { + const result = scrubGitHubMcpClientFrame( + frame("tools/call", { + name: "add_issue_comment", + arguments: { owner: "o", repo: "r", issue_number: 1, body: `rotation notes\n${SYNTHETIC_PEM}\nend` }, + }), + ); + + expect(result.redacted).toBe(true); + expect(result.classes).toEqual(["private-key-block"]); + expect(result.line).toContain(redactionMarker("private-key-block")); + expect(result.line).not.toContain(PEM_LABEL); + // The envelope survives intact. + const parsed = JSON.parse(result.line) as { jsonrpc: string; id: number; method: string }; + expect(parsed).toMatchObject({ jsonrpc: "2.0", id: 1, method: "tools/call" }); + }); + + it("scrubs pull_request_review_write's body — the exact shape of the PEN-2526 exposure", () => { + const result = scrubGitHubMcpClientFrame( + frame("tools/call", { + name: "pull_request_review_write", + arguments: { + method: "create", + owner: "Blockcast", + repo: "paperclip", + pullNumber: 1435, + event: "COMMENT", + body: `Reviewed. Runtime context:\n${SYNTHETIC_PEM}\nand token ${SYNTHETIC_VENDOR_KEY}`, + }, + }), + ); + + expect(result.redacted).toBe(true); + expect(result.classes).toEqual(["private-key-block", "vendor-key"]); + expect(result.line).not.toContain(SYNTHETIC_VENDOR_KEY); + }); + + it("scrubs create_or_update_file CONTENT, not just comment prose", () => { + const result = scrubGitHubMcpClientFrame( + frame("tools/call", { + name: "create_or_update_file", + arguments: { + owner: "o", + repo: "r", + branch: "main", + path: "deploy/values.yaml", + message: "chore: add config", + content: `appToken: ${SYNTHETIC_VENDOR_KEY}\n`, + }, + }), + ); + + expect(result.redacted).toBe(true); + expect(result.classes).toContain("vendor-key"); + expect(result.line).not.toContain(SYNTHETIC_VENDOR_KEY); + }); + + it("scrubs every file in a push_files array, not only the first", () => { + const result = scrubGitHubMcpClientFrame( + frame("tools/call", { + name: "push_files", + arguments: { + owner: "o", + repo: "r", + branch: "main", + message: "chore: bulk add", + files: [ + { path: "a.txt", content: "harmless" }, + { path: "b.env", content: `AUTH=${SYNTHETIC_OPAQUE}` }, + { path: "c.pem", content: SYNTHETIC_PEM }, + ], + }, + }), + ); + + expect(result.redacted).toBe(true); + expect(result.classes).toEqual(["private-key-block", "high-entropy-assignment"]); + + const parsed = JSON.parse(result.line) as { + params: { arguments: { files: { path: string; content: string }[] } }; + }; + const files = parsed.params.arguments.files; + expect(files[0]?.content).toBe("harmless"); + expect(files[1]?.content).not.toContain(SYNTHETIC_OPAQUE); + expect(files[2]?.content).not.toContain(PEM_LABEL); + // Paths are structural and must survive untouched. + expect(files.map((f) => f.path)).toEqual(["a.txt", "b.env", "c.pem"]); + }); + }); + + describe("no allowlist to outgrow", () => { + it("scrubs a parameter name this module has never heard of", () => { + const result = scrubGitHubMcpClientFrame( + frame("tools/call", { + name: "some_tool_added_next_release", + arguments: { totally_new_field: `see ${SYNTHETIC_JWT}` }, + }), + ); + + expect(result.redacted).toBe(true); + expect(result.classes).toEqual(["jwt"]); + }); + + it("scrubs an unrecognised payload member, not only `params`", () => { + // A client-to-server `result` (an MCP sampling reply) is payload too. + const line = JSON.stringify({ jsonrpc: "2.0", id: 9, result: { text: SYNTHETIC_PEM } }); + const result = scrubGitHubMcpClientFrame(line); + + expect(result.redacted).toBe(true); + expect(result.classes).toEqual(["private-key-block"]); + }); + + it("scrubs deeply nested payload strings", () => { + const result = scrubGitHubMcpClientFrame( + frame("tools/call", { + name: "x", + arguments: { a: { b: { c: [{ d: { e: SYNTHETIC_PEM } }] } } }, + }), + ); + + expect(result.redacted).toBe(true); + expect(result.classes).toEqual(["private-key-block"]); + }); + + it("scrubs each member of a JSON-RPC batch", () => { + const line = JSON.stringify([ + { jsonrpc: "2.0", id: 1, method: "tools/call", params: { arguments: { body: "clean" } } }, + { jsonrpc: "2.0", id: 2, method: "tools/call", params: { arguments: { body: SYNTHETIC_PEM } } }, + ]); + + const result = scrubGitHubMcpClientFrame(line); + + expect(result.redacted).toBe(true); + const parsed = JSON.parse(result.line) as { params: { arguments: { body: string } } }[]; + expect(parsed[0]?.params.arguments.body).toBe("clean"); + expect(parsed[1]?.params.arguments.body).toContain(redactionMarker("private-key-block")); + }); + }); + + describe("protocol integrity", () => { + it("keeps the envelope's `method` intact even though it is a string", () => { + // `method` routes the message. Scrubbing it would break dispatch, so it + // is excluded by name — the one place a name-based rule is correct. + const line = frame("tools/call", { name: "n", arguments: { body: SYNTHETIC_PEM } }); + const parsed = JSON.parse(scrubGitHubMcpClientFrame(line).line) as { method: string }; + expect(parsed.method).toBe("tools/call"); + }); + + it("keeps object KEYS intact", () => { + const result = scrubGitHubMcpClientFrame( + frame("tools/call", { name: "n", arguments: { body: SYNTHETIC_PEM, keep_me: 1 } }), + ); + const parsed = JSON.parse(result.line) as { params: { arguments: Record } }; + expect(Object.keys(parsed.params.arguments)).toEqual(["body", "keep_me"]); + }); + + it("emits no embedded newline even when the redacted value spanned lines", () => { + const result = scrubGitHubMcpClientFrame( + frame("tools/call", { name: "n", arguments: { body: `a\n${SYNTHETIC_PEM}\nb` } }), + ); + expect(result.redacted).toBe(true); + // The framing invariant: one message per line. + expect(result.line).not.toContain("\n"); + }); + + it("preserves non-string scalars exactly", () => { + const result = scrubGitHubMcpClientFrame( + frame("tools/call", { + name: "n", + arguments: { issue_number: 1435, draft: false, milestone: null, body: SYNTHETIC_PEM }, + }), + ); + const parsed = JSON.parse(result.line) as { + params: { arguments: { issue_number: number; draft: boolean; milestone: null } }; + }; + expect(parsed.params.arguments).toMatchObject({ + issue_number: 1435, + draft: false, + milestone: null, + }); + }); + }); + + describe("fail closed", () => { + it("throws rather than forwarding a frame nested past the walk limit", () => { + let nested = '{"deep":1}'; + for (let i = 0; i < 260; i += 1) nested = `{"a":${nested}}`; + const line = `{"jsonrpc":"2.0","id":1,"method":"tools/call","params":${nested}}`; + + expect(() => scrubGitHubMcpClientFrame(line)).toThrow(GitHubMcpEgressFrameError); + }); + }); + + describe("fixture self-checks", () => { + // A fixture that stopped tripping its detector would make the assertions + // above pass for the wrong reason. Assert the fixtures are still material. + it("each derived fixture still trips the detector it targets", () => { + expect(scrubGitHubMcpClientFrame(frame("m", { a: SYNTHETIC_PEM })).classes).toEqual([ + "private-key-block", + ]); + expect(scrubGitHubMcpClientFrame(frame("m", { a: SYNTHETIC_JWT })).classes).toEqual(["jwt"]); + expect(scrubGitHubMcpClientFrame(frame("m", { a: SYNTHETIC_VENDOR_KEY })).classes).toEqual([ + "vendor-key", + ]); + expect( + scrubGitHubMcpClientFrame(frame("m", { a: `AUTH=${SYNTHETIC_OPAQUE}` })).classes, + ).toEqual(["high-entropy-assignment"]); + }); + }); +}); diff --git a/packages/adapter-utils/src/github-mcp-egress-shim.ts b/packages/adapter-utils/src/github-mcp-egress-shim.ts new file mode 100644 index 000000000000..60ce80d6ebc1 --- /dev/null +++ b/packages/adapter-utils/src/github-mcp-egress-shim.ts @@ -0,0 +1,195 @@ +// PEN-3152: the SECOND egress choke point for agent-authored text bound for +// GitHub — the one PEN-2527 did not know existed. +// +// PEN-2527 wrapped the `gh` binary (github-cli-egress-shim.ts) and reasoned +// that this was "the single interposition point in front of the GitHub CLI in +// the sandbox". True, and insufficient: the `github` MCP server is a separate +// outbound path to the same destination, launched from its own wrapper in +// deploy/helm/paperclip/templates/statefulset.yaml, holding the same seat +// token, and exposing the complete write toolset — `add_issue_comment`, +// `pull_request_review_write`, `create_pull_request`, `create_or_update_file`, +// `push_files`. Every one of those takes free-form model-authored text and +// publishes it. None of them passed a scrubber before this module. +// +// The asymmetry was the dangerous part. An agent has no way to tell that +// `gh issue comment` is scrubbed while `mcp__github__add_issue_comment` is not, +// so ordinary tool selection drifted toward the unguarded door. The fix is +// therefore NOT a second policy — it is the same policy at the second door: +// this module and the CLI shim both delegate every decision to +// `scrubGitHubEgressText`, so the two doors cannot drift. +// +// ## Why this transform is broader than the CLI shim's +// +// The CLI shim knows which argv flags carry authored text and scrubs those +// (`--body`, `--body-file`, `--raw-field`, ...). That allowlist is safe there +// because `gh`'s surface is fixed and versioned. +// +// MCP has no such surface. Tool schemas are supplied by the server at runtime, +// so the set of parameter names that can carry authored text is not knowable +// here and grows whenever github-mcp-server is upgraded. An allowlist would be +// a hole with a release cadence. So this module scrubs EVERY string in the +// frame's payload members and keys on nothing at all — a tool added upstream +// tomorrow is covered on the day it ships, with no change here. +// +// ## Direction +// +// Client -> server ONLY. Responses (server -> client) are the inbound leg and +// belong to PEN-2370's `packages/mcp-gateway/src/response-scrub.ts`; two +// directions, two controls, as PEN-2527 put it. The runtime enforces that +// structurally rather than by convention: it pipes only the child's stdin and +// leaves stdout inherited, so there is no code path here that could alter a +// response even by mistake. + +import { + type GitHubEgressScrubClass, + scrubGitHubEgressText, +} from "./github-egress-scrub.js"; + +/** + * JSON-RPC envelope members. These route the message; they are never a payload. + * + * Everything NOT listed here is treated as payload and deep-scrubbed, so the + * default for an unrecognised member is to scrub it. That direction matters: + * a future MCP revision that adds a payload-bearing member gets covered + * automatically, whereas an allowlist of payload members would silently miss it. + */ +const ENVELOPE_MEMBERS = new Set(["jsonrpc", "id", "method"]); + +/** + * Nesting depth beyond which a frame is refused rather than scrubbed. + * + * A frame this deep is not a real tool call, and recursing it risks a stack + * overflow inside the one process that is supposed to be guarding the channel. + * Refusing is the fail-closed choice: see `GitHubMcpEgressFrameError`. + */ +const MAX_DEPTH = 200; + +/** Raised when a frame cannot be scrubbed. Callers MUST NOT forward the frame. */ +export class GitHubMcpEgressFrameError extends Error { + constructor(message: string) { + super(message); + this.name = "GitHubMcpEgressFrameError"; + } +} + +export interface GitHubMcpFrameScrubResult { + /** The frame to forward. Byte-identical to the input when nothing fired. */ + line: string; + /** True when any detector fired. */ + redacted: boolean; + /** Which classes fired, deduped, in a stable order. */ + classes: GitHubEgressScrubClass[]; +} + +const CLASS_ORDER: readonly GitHubEgressScrubClass[] = [ + "private-key-block", + "credentialed-uri", + "jwt", + "vendor-key", + "environment-dump", + "high-entropy-assignment", +]; + +/** + * Scrub one newline-delimited JSON-RPC frame travelling from the MCP client to + * the GitHub MCP server. + * + * Returns the input string unchanged (byte-exact, same reference) when no + * detector fires, so a clean frame is forwarded without re-serialisation. That + * is not just an optimisation: re-encoding every frame would silently normalise + * key order and number formatting on the way to the server, which makes this + * shim observable to correct traffic. It should not be. + * + * A frame that is not JSON is forwarded unchanged. It carries no scrubbable + * payload by definition, and rejecting it here would turn this shim into a + * second, worse JSON-RPC validator in front of the real one. + * + * @throws GitHubMcpEgressFrameError when the frame parses but cannot be walked. + */ +export function scrubGitHubMcpClientFrame(line: string): GitHubMcpFrameScrubResult { + const clean: GitHubMcpFrameScrubResult = { line, redacted: false, classes: [] }; + if (line.trim().length === 0) return clean; + + let parsed: unknown; + try { + parsed = JSON.parse(line); + } catch { + // Not JSON — nothing to walk. Let the server be the one to complain. + return clean; + } + if (typeof parsed !== "object" || parsed === null) return clean; + + const fired = new Set(); + const scrubbed = scrubMessage(parsed, fired, 0); + if (fired.size === 0) return clean; + + return { + // JSON.stringify escapes any newline inside a value, so the framing + // invariant (one message per line, no embedded newlines) survives a + // multi-line redaction. + line: JSON.stringify(scrubbed), + redacted: true, + classes: CLASS_ORDER.filter((cls) => fired.has(cls)), + }; +} + +/** + * Walk a single message (or every member of a JSON-RPC batch), scrubbing the + * payload members and leaving the envelope alone. + */ +function scrubMessage( + message: object, + fired: Set, + depth: number, +): unknown { + if (Array.isArray(message)) { + // JSON-RPC 2.0 batch. MCP's current revision drops batching, but a client + // that still emits one must not slip past unscrubbed. + return message.map((entry) => + typeof entry === "object" && entry !== null + ? scrubMessage(entry, fired, depth + 1) + : scrubValue(entry, fired, depth + 1), + ); + } + + const out: Record = {}; + for (const [key, value] of Object.entries(message)) { + out[key] = ENVELOPE_MEMBERS.has(key) ? value : scrubValue(value, fired, depth + 1); + } + return out; +} + +/** Recursively scrub every string in a payload value. Keys are structural and are left intact. */ +function scrubValue( + value: unknown, + fired: Set, + depth: number, +): unknown { + if (depth > MAX_DEPTH) { + throw new GitHubMcpEgressFrameError( + `JSON-RPC frame nests deeper than ${MAX_DEPTH} levels; refusing to forward it unscrubbed`, + ); + } + + if (typeof value === "string") { + const result = scrubGitHubEgressText(value); + if (!result.redacted) return value; + for (const cls of result.classes) fired.add(cls); + return result.text; + } + + if (Array.isArray(value)) { + return value.map((entry) => scrubValue(entry, fired, depth + 1)); + } + + if (typeof value === "object" && value !== null) { + const out: Record = {}; + for (const [key, member] of Object.entries(value)) { + out[key] = scrubValue(member, fired, depth + 1); + } + return out; + } + + // Numbers, booleans and null carry no credential shape. + return value; +} diff --git a/server/src/__tests__/github-egress-outbound-coverage.test.ts b/server/src/__tests__/github-egress-outbound-coverage.test.ts new file mode 100644 index 000000000000..260e13ac608e --- /dev/null +++ b/server/src/__tests__/github-egress-outbound-coverage.test.ts @@ -0,0 +1,290 @@ +import { readFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; + +/** + * PEN-3152 done-when 2: "the outbound direction gets its own exhaustive + * coverage assertion, in the shape of `mcp-seed-scrub-coverage.test.ts` — so a + * future outbound path cannot be added without a classification." + * + * ## Why this file exists as a SIBLING of mcp-seed-scrub-coverage.test.ts + * + * That file audits the INBOUND leg: MCP tool responses on their way to the + * agent, where `packages/mcp-gateway/src/response-scrub.ts` is the control. It + * is deliberately adjacent to this one, because its existence is what made the + * outbound gap easy to mistake for covered. It classifies + * + * github: { kind: "stdio-not-proxied", why: "...local stdio child process" } + * + * which is true, and says nothing whatsoever about whether agent-authored text + * is scrubbed on the way OUT. PEN-3152 was filed because that row read like a + * clean bill of health for a door that was wide open. Two directions, two + * controls, and — now — two tables. + * + * ## What this table binds + * + * The Helm init script writes a small launcher into `${LOCAL_BIN}` for every + * GitHub-touching binary in the agent sandbox, and `${LOCAL_BIN}` is first on + * the PATH of every agent Job. Those launchers are the complete set of + * interposition points available to us, so they are the complete set of places + * an outbound scrub can live. This table requires each one to be classified, + * and — for the ones claimed as scrubbed — asserts the launcher actually still + * execs its egress runtime. Deleting the scrub from a wrapper fails here. + * + * It also enumerates the server-side write set, which is a second family + * entirely: `paperclip-api` writes to GitHub over HTTP from `server/`, reaching + * no wrapper and no scrubber. + * + * ## ⚠️ Scope boundary — read before citing this file as coverage + * + * 1. **Per-agent MCP overrides are not enforced here.** An agent's effective + * MCP config is `{ ...sharedSeedBaseline, ...adapterConfig.mcpServers }` + * (`vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts`), merged + * override-by-name. An `adapterConfig` that defines its own `github` key + * replaces the wrapper command and bypasses the MCP scrub entirely. That + * write is board-gated (`assertBoard`, `server/src/routes/agents.ts`), so it + * is an operator footgun rather than an agent-reachable bypass — but it is a + * footgun with no guard rail, and no static test in this repo can enumerate + * a DB column. Named here so it is enumerated rather than invisible. + * 2. **CI and maintainer scripts are out of scope by construction.** The `gh` + * invocations in `.github/workflows/*` and `scripts/` run on Actions + * runners, not in an agent pod, so they never resolve to the wrapper. They + * publish CI-assembled text, not model output. + * 3. A `kind` here is a statement about whether a SCRUBBER IS ON THE PATH. It + * is not a finding that anything sensitive has traversed it. + */ + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../../.."); +const statefulSetPath = path.join(repoRoot, "deploy/helm/paperclip/templates/statefulset.yaml"); +const servicesDirectory = path.join(repoRoot, "server/src/services"); + +/** The compiled entrypoints that carry a scrub, as the wrappers name them. */ +const CLI_EGRESS_RUNTIME = "github-cli-egress-runtime.js"; +const MCP_EGRESS_RUNTIME = "github-mcp-egress-runtime.js"; + +type Coverage = + /** Agent-authored text on this path passes through `scrubGitHubEgressText`. */ + | { kind: "egress-scrubbed"; runtime: string } + /** Reaches GitHub carrying authored text, with NO scrubber on the path. */ + | { kind: "unscrubbed"; ticket: string; why: string } + /** Touches GitHub credentials but is not itself a path authored text travels. */ + | { kind: "not-an-authored-text-path"; why: string }; + +/** + * Every launcher the Helm seed writes into `${LOCAL_BIN}`. + * + * Keep this exhaustive. If you are here because the suite failed after you + * added a wrapper, that is this test working: choose a `kind` and say why. + * + * One ticket per unscrubbed door, deliberately — PEN-2370's table records that + * collapsing five upstreams onto one ticket had to be undone, because it parked + * rows of unassessed severity behind an unrelated critical fix. Do not tidy + * these onto a single row. + */ +const WRAPPER_COVERAGE: Readonly> = { + gh: { + kind: "egress-scrubbed", + runtime: CLI_EGRESS_RUNTIME, + }, + "github-mcp-server": { + kind: "egress-scrubbed", + runtime: MCP_EGRESS_RUNTIME, + }, + git: { + kind: "unscrubbed", + ticket: "PEN-3156", + // Worded to avoid the literal command name: scripts/check-no-git-push.mjs + // scans this tree for it, and the marker that opts a line out asserts an + // operator-approved push path exists. No such path exists here — this is a + // description of a gap — so spending that escape hatch on a doc string + // would put a false claim inside a security control. + why: "token-injection wrapper only; pushing through it publishes commit messages and file contents — a strict superset of what create_or_update_file/push_files carry. Not fixable by in-flight redaction: commit objects are content-addressed, so altering a blob or message after the fact changes every downstream SHA. The fix shape is refusal at push time, which is its own rollout", + }, + "paperclip-github-token-env": { + kind: "not-an-authored-text-path", + why: "exports GH_TOKEN/GITHUB_TOKEN/GITHUB_PERSONAL_ACCESS_TOKEN then execs its argv; it carries the credential, never a payload. It is what the scrubbed wrappers exec THROUGH, so it must stay outside them", + }, + "github-token-credential-helper": { + kind: "not-an-authored-text-path", + why: "emits username/password on stdout for git's credential protocol; no authored text passes through it", + }, +}; + +/** + * `server/src/services` files that issue a WRITE to GitHub over HTTP. + * + * Derived from source at file granularity rather than line, so it stays stable + * across refactors while still failing when a NEW file starts writing to + * GitHub. Both entries are unscrubbed: the scrubber lives in + * `packages/adapter-utils`, is imported only by the two sandbox wrappers, and + * is not re-exported from that package's `index.ts` — so it is structurally + * unreachable from `server/`. + */ +const SERVER_WRITE_COVERAGE: Readonly> = { + "github-app-auth.ts": { + kind: "unscrubbed", + ticket: "PEN-3157", + why: "githubPostIssueComment POSTs an arbitrary `body` to /issues/{n}/comments, and githubPostCommitStatusDetailed POSTs a `description`. The comment helper's only caller passes a template today, so the exposure there is latent rather than live — but the helper accepts any string and is one caller away from carrying model output", + }, + "github-review-gate-authority.ts": { + kind: "unscrubbed", + ticket: "PEN-3157", + why: "POSTs a fixed pending-status description plus a repository_dispatch client_payload of ids; no model-authored free text on either, but it reaches GitHub through the same unscrubbed ghFetch boundary and must be reclassified together with it", + }, +}; + +/** The init-script region that writes the sandbox launchers. */ +function readWrapperNames(): string[] { + const source = readFileSync(statefulSetPath, "utf8"); + const names = [...source.matchAll(/cat > "\$\{LOCAL_BIN\}\/([A-Za-z0-9._-]+)" <<'EOF'/g)].map( + (match) => match[1] as string, + ); + if (names.length === 0) { + throw new Error( + `Found no \${LOCAL_BIN} launchers in ${statefulSetPath}. If the seed moved, point this ` + + "test at its new home rather than deleting it — PEN-3152 exists because an outbound " + + "egress door went unenumerated behind a table that only covered the inbound leg.", + ); + } + return names; +} + +/** The body of one launcher heredoc, so its exec line can be asserted. */ +function readWrapperBody(name: string): string { + const source = readFileSync(statefulSetPath, "utf8"); + const marker = `cat > "\${LOCAL_BIN}/${name}" <<'EOF'`; + const markerAt = source.indexOf(marker); + expect(markerAt, `launcher ${name} not found in the seed`).toBeGreaterThanOrEqual(0); + const bodyStart = source.indexOf("\n", markerAt) + 1; + const bodyEnd = source.indexOf("\n EOF", bodyStart); + expect(bodyEnd, `launcher ${name} heredoc is not terminated as expected`).toBeGreaterThan( + bodyStart, + ); + return source.slice(bodyStart, bodyEnd); +} + +function readSeededGitHubMcpCommand(): string { + const source = readFileSync(statefulSetPath, "utf8"); + const match = /"github":\s*\{\s*"command":\s*"([^"]+)"/.exec(source); + expect(match, "the seeded mcpServers block no longer has a github command").not.toBeNull(); + return (match as RegExpExecArray)[1] as string; +} + +function serviceFilesWritingToGitHub(): string[] { + const { readdirSync } = require("node:fs") as typeof import("node:fs"); + const out: string[] = []; + for (const entry of readdirSync(servicesDirectory)) { + if (!entry.endsWith(".ts") || entry.endsWith(".test.ts")) continue; + const source = readFileSync(path.join(servicesDirectory, entry), "utf8"); + if (!source.includes("ghFetch(")) continue; + if (!/method:\s*"(?:POST|PATCH|PUT|DELETE)"/.test(source)) continue; + out.push(entry); + } + return out.sort(); +} + +describe("outbound GitHub egress coverage", () => { + describe("sandbox launchers", () => { + it("classifies every launcher the seed writes", () => { + const seeded = readWrapperNames().sort(); + const classified = Object.keys(WRAPPER_COVERAGE).sort(); + expect(seeded).toEqual(classified); + }); + + it("every launcher claimed as scrubbed still execs its egress runtime", () => { + // This is the regression guard for the control itself. Removing the + // scrub from a wrapper — the change that would silently reopen + // PEN-2527's or PEN-3152's gap — fails right here. + for (const [name, coverage] of Object.entries(WRAPPER_COVERAGE)) { + if (coverage.kind !== "egress-scrubbed") continue; + const body = readWrapperBody(name); + expect(body, `${name} no longer execs ${coverage.runtime}`).toContain(coverage.runtime); + expect(body, `${name} must exec its target, not merely mention the runtime`).toMatch( + /^\s*exec\s/m, + ); + } + }); + + it("the MCP scrub runs INSIDE the token wrapper, so the server keeps its credential", () => { + // Ordering is load-bearing: paperclip-github-token-env must be the outer + // process so the real server inherits GITHUB_PERSONAL_ACCESS_TOKEN. If + // the scrub runtime were placed outside it, the server would start + // unauthenticated and every tool call would fail — the kind of breakage + // that gets a security control reverted rather than fixed. + const body = readWrapperBody("github-mcp-server"); + const tokenAt = body.indexOf("paperclip-github-token-env"); + const runtimeAt = body.indexOf(MCP_EGRESS_RUNTIME); + expect(tokenAt).toBeGreaterThanOrEqual(0); + expect(runtimeAt).toBeGreaterThan(tokenAt); + }); + + it("the two scrubbed doors use DIFFERENT runtimes for their two transports", () => { + // argv rewriting and JSON-RPC frame rewriting are not interchangeable. + // Pointing one wrapper at the other's runtime would produce a process + // that runs, scrubs nothing, and looks correct in this table. + expect(readWrapperBody("gh")).not.toContain(MCP_EGRESS_RUNTIME); + expect(readWrapperBody("github-mcp-server")).not.toContain(CLI_EGRESS_RUNTIME); + }); + }); + + describe("the seeded github MCP upstream", () => { + it("dials the wrapper, not the real server", () => { + // The whole control rests on this indirection. A seed that pointed + // `github` straight at /usr/local/bin/github-mcp-server would bypass the + // scrub while leaving every wrapper assertion above green. + expect(readSeededGitHubMcpCommand()).toBe("/paperclip/.local/bin/github-mcp-server"); + }); + + it("is a wrapper the seed actually writes", () => { + const command = readSeededGitHubMcpCommand(); + expect(readWrapperNames()).toContain(path.basename(command)); + }); + }); + + describe("server-side GitHub writes", () => { + it("classifies every service file that writes to GitHub", () => { + // paperclip-api reaches GitHub over HTTP from server/, touching no + // wrapper. A new service file that starts writing fails here until it is + // classified — which is the whole mechanism PEN-3152 asked for. + expect(serviceFilesWritingToGitHub()).toEqual(Object.keys(SERVER_WRITE_COVERAGE).sort()); + }); + + it("records that the scrubber is structurally unreachable from server/", () => { + // Not a style point: as long as this holds, no server-side write can be + // scrubbed even by a caller who wants to, and every entry in + // SERVER_WRITE_COVERAGE must remain `unscrubbed`. PEN-3157 closes it by + // exporting the core (or relocating it); when that lands this assertion + // is what tells you the table is now stale. + const index = readFileSync( + path.join(repoRoot, "packages/adapter-utils/src/index.ts"), + "utf8", + ); + expect(index).not.toContain("github-egress-scrub"); + + for (const coverage of Object.values(SERVER_WRITE_COVERAGE)) { + expect(coverage.kind).toBe("unscrubbed"); + } + }); + }); + + describe("table hygiene", () => { + it("every unscrubbed door names a ticket that owns it", () => { + for (const coverage of [ + ...Object.values(WRAPPER_COVERAGE), + ...Object.values(SERVER_WRITE_COVERAGE), + ]) { + if (coverage.kind !== "unscrubbed") continue; + expect(coverage.ticket).toMatch(/^(PEN|BLO)-\d+$/); + expect(coverage.why.length).toBeGreaterThan(40); + } + }); + + it("at least one door is scrubbed, so an empty table cannot read as coverage", () => { + const scrubbed = Object.values(WRAPPER_COVERAGE).filter( + (coverage) => coverage.kind === "egress-scrubbed", + ); + expect(scrubbed.length).toBeGreaterThanOrEqual(2); + }); + }); +}); From 6c4e83dd34163b3d1c0ace5539716147aa14fb8b Mon Sep 17 00:00:00 2001 From: Cto Date: Fri, 11 Sep 2026 03:29:43 +0000 Subject: [PATCH 2/2] fix(security): reassemble split code points and cap every frame (PEN-3152) Ally's review of fd577e4 raised two defects in the MCP egress runtime, both inside this row's own scope. Both are real and both are fixed here. 1. UTF-8 was decoded per chunk. `chunk.toString("utf8")` was applied to each stdin chunk before accumulation, but Node reads stdin on byte boundaries, so a multi-byte code point can be bisected between two reads. Each half then decoded to U+FFFD independently and the payload reached GitHub corrupted -- silently, since nothing downstream can tell a replacement character from one the agent typed. A stateful StringDecoder now holds an incomplete sequence until the bytes completing it arrive. 2. The frame cap was evadable. MAX_FRAME_BYTES was checked only against the remainder left AFTER complete lines were removed, so a read carrying an oversized line together with its newline left a short remainder, passed the check, and forwarded a frame past the advertised fail-closed limit. The cap now applies to every frame, enforced inside splitFrames rather than by its caller: the cap belongs to the frame, and an exported splitter that hands back an over-cap line is one whose next caller forgets to check it. The accumulate-and-split step becomes createFrameReader, a named export. That is what makes both fixes testable at all -- chunk boundaries are the kernel's choice, not the writer's, so neither a bisected code point nor a whole oversized frame can be produced on demand through a spawned child. The regression test for (1) is exhaustive over every cut position inside a multi-byte sequence rather than one sampled split. Both fixes are mutation-verified. Reverting the decoder fails the split-code- point test with the exact corruption Ally described (the key emoji renders as four replacement characters); dropping the per-line cap fails the oversized- complete-frame test. In each case the other 20 tests still pass, so the tests are specific rather than blanket. Also re-tenses two comments the change falsified: the header's fail-closed policy and the MAX_FRAME_BYTES docstring both described the cap as applying only to an unterminated frame, which is precisely the too-narrow reading that produced defect (2). Verified: 21 runtime + 89 across all five egress suites + 37 door-parity + 12 helm render tests, all passing; scripts/check-no-git-push.mjs green. Local tsc cannot judge this package -- @types/node is absent from the borrowed /app deps, so every node: import in the package errors, including the pre-existing node:string_decoder in sandbox-run-log-stream.ts which ships green on master. Build is the authoritative typecheck. Refs PEN-3152, PEN-2527, PEN-2526. Signed-off-by: Cto --- .../src/github-mcp-egress-runtime.test.ts | 56 +++++++- .../src/github-mcp-egress-runtime.ts | 121 ++++++++++++++---- 2 files changed, 153 insertions(+), 24 deletions(-) diff --git a/packages/adapter-utils/src/github-mcp-egress-runtime.test.ts b/packages/adapter-utils/src/github-mcp-egress-runtime.test.ts index df95a1260336..1416883431bd 100644 --- a/packages/adapter-utils/src/github-mcp-egress-runtime.test.ts +++ b/packages/adapter-utils/src/github-mcp-egress-runtime.test.ts @@ -6,7 +6,12 @@ import { fileURLToPath } from "node:url"; import { afterEach, describe, expect, it } from "vitest"; import { redactionMarker } from "./github-egress-scrub.js"; -import { MAX_FRAME_BYTES, splitFrames, transformFrame } from "./github-mcp-egress-runtime.js"; +import { + MAX_FRAME_BYTES, + createFrameReader, + splitFrames, + transformFrame, +} from "./github-mcp-egress-runtime.js"; const sourceDirectory = path.dirname(fileURLToPath(import.meta.url)); const runtimeEntryPoint = path.join(sourceDirectory, "github-mcp-egress-runtime.ts"); @@ -229,6 +234,37 @@ describe("github MCP egress runtime", () => { }); }); + describe("createFrameReader", () => { + // Node reads stdin on BYTE boundaries, so a multi-byte code point can be + // bisected between two chunks. Decoding each half independently yields two + // U+FFFDs and silently corrupts the payload — which the reader must not do, + // because the corrupted text is then what gets published to GitHub. + it("reassembles a code point split at every byte boundary inside it", () => { + const payload = '{"jsonrpc":"2.0","id":1,"params":{"body":"🔑é☃"}}'; + const bytes = Buffer.from(`${payload}\n`, "utf8"); + + // Exhaustive over cut positions: any single one could pass by luck, and + // the interesting cuts are the ones inside a multi-byte sequence. + for (let cut = 1; cut < bytes.length; cut += 1) { + const read = createFrameReader(); + const first = read(bytes.subarray(0, cut)); + const second = read(bytes.subarray(cut)); + expect([...first, ...second], `split after byte ${cut}`).toEqual([payload]); + } + }); + + it("reassembles a frame split across chunks", () => { + const read = createFrameReader(); + expect(read('{"jsonrpc":"2.0",')).toEqual([]); + expect(read('"id":1}\n')).toEqual(['{"jsonrpc":"2.0","id":1}']); + }); + + it("accepts an already-decoded string chunk without re-encoding it", () => { + const read = createFrameReader(); + expect(read('{"body":"é"}\n')).toEqual(['{"body":"é"}']); + }); + }); + describe("fail closed", () => { it("caps the unterminated buffer well above any legitimate frame", () => { // Guard the constant itself: a future edit that drops it to a plausible @@ -236,6 +272,24 @@ describe("github MCP egress runtime", () => { expect(MAX_FRAME_BYTES).toBeGreaterThanOrEqual(16 * 1024 * 1024); }); + it("refuses an oversized frame that arrives complete, newline and all", () => { + // The cap used to be checked only on the remainder left AFTER complete + // lines were removed, so delivering the newline in the same read left a + // short remainder, passed the check, and forwarded the oversized frame. + // The well-sized frame in front of it is here on purpose: throwing means + // it is never returned either, i.e. the whole read is discarded. + const read = createFrameReader(); + const oversized = `{"a":1}\n${"a".repeat(MAX_FRAME_BYTES + 1)}\n`; + + expect(() => read(oversized)).toThrow(/exceeded the \d+-byte cap/); + }); + + it("still refuses an oversized frame with no terminator", () => { + const read = createFrameReader(); + + expect(() => read("a".repeat(MAX_FRAME_BYTES + 1))).toThrow(/no newline terminator/); + }); + it("refuses a frame it cannot walk rather than forwarding it", () => { let nested = '{"deep":1}'; for (let i = 0; i < 260; i += 1) nested = `{"a":${nested}}`; diff --git a/packages/adapter-utils/src/github-mcp-egress-runtime.ts b/packages/adapter-utils/src/github-mcp-egress-runtime.ts index ee8b4cb2176b..5ffaa4923972 100644 --- a/packages/adapter-utils/src/github-mcp-egress-runtime.ts +++ b/packages/adapter-utils/src/github-mcp-egress-runtime.ts @@ -19,13 +19,14 @@ // ## Fail-closed policy // // A frame is forwarded only after it has been scrubbed. If it cannot be -// scrubbed — unparseable depth, or an oversized frame with no terminator — the -// runtime tears down rather than passing it through. An agent seeing its MCP -// server drop is a loud, diagnosable failure; an agent whose secret reached a -// public repository is not. +// scrubbed — unparseable depth, or a frame over the size cap whether or not it +// arrived terminated — the runtime tears down rather than passing it through. +// An agent seeing its MCP server drop is a loud, diagnosable failure; an agent +// whose secret reached a public repository is not. import { spawn } from "node:child_process"; import path from "node:path"; +import { StringDecoder } from "node:string_decoder"; import { fileURLToPath } from "node:url"; import { @@ -34,14 +35,17 @@ import { } from "./github-mcp-egress-shim.js"; /** - * Largest client frame this runtime will buffer while waiting for its - * terminating newline, in bytes. + * Largest client frame this runtime will forward, in bytes. * - * A frame must be complete before it can be scrubbed, so some cap is required - * or a stream that never emits a newline grows without bound. 64 MiB is far - * above any legitimate MCP frame — GitHub's own contents API rejects blobs - * orders of magnitude smaller — and exceeding it means the stream is not - * carrying MCP traffic, at which point refusing is correct. + * The cap applies to every frame, not only to a partial one still awaiting its + * newline. Two distinct things need it: a frame must be complete before it can + * be scrubbed, so without a cap a stream that never emits a newline grows + * without bound; and a frame that arrives complete but enormous is one this + * process would have to hold, walk and rewrite in memory before forwarding. + * + * 64 MiB is far above any legitimate MCP frame — GitHub's own contents API + * rejects blobs orders of magnitude smaller — and exceeding it means the stream + * is not carrying MCP traffic, at which point refusing is correct. */ export const MAX_FRAME_BYTES = 64 * 1024 * 1024; @@ -76,12 +80,44 @@ const defaultIo: GitHubMcpEgressRuntimeIo = { }, }; +/** + * Refuse an over-cap frame. + * + * Terminated and unterminated frames are refused at the same limit but are + * distinguishable failures, and the message says which: an unterminated frame + * may merely have been still growing, whereas a terminated one was delivered + * oversized in full. + */ +function refuseOversizedFrame(byteLength: number, terminated: boolean): never { + throw new GitHubMcpEgressRuntimeError( + terminated + ? `client frame of ${byteLength} bytes exceeded the ${MAX_FRAME_BYTES}-byte cap; refusing to forward it unscrubbed` + : `client frame exceeded ${MAX_FRAME_BYTES} bytes with no newline terminator; refusing to forward it unscrubbed`, + ); +} + /** * Split a chunk-accumulated buffer into complete lines plus the trailing - * remainder, enforcing the frame cap on the remainder. + * remainder, enforcing the frame cap on EVERY frame — each complete line as + * well as the remainder. + * + * Capping only the remainder leaves the cap trivially evadable: a read + * carrying an oversized line *together with its newline* leaves a short + * remainder, so the remainder check passes and the oversized frame is + * forwarded anyway. That was the shipped behaviour until the Ally review of + * `fd577e4` caught it. + * + * The cap belongs to the frame rather than to the leftover, so it is enforced + * here instead of by the caller — an exported splitter that hands back an + * over-cap line is a splitter whose next caller forgets to check it. Refusal + * discards the whole read, including any well-sized lines that preceded the + * offender, which is the fail-closed reading: a stream that produced one + * impossible frame has stopped being MCP traffic. * * Exported for tests: the buffering is where a stdio proxy usually goes wrong, * and it is worth asserting directly rather than only through a spawned child. + * + * @throws {GitHubMcpEgressRuntimeError} if any frame exceeds `MAX_FRAME_BYTES`. */ export function splitFrames(buffer: string): { lines: string[]; rest: string } { const lines: string[] = []; @@ -89,10 +125,18 @@ export function splitFrames(buffer: string): { lines: string[]; rest: string } { for (;;) { const at = buffer.indexOf("\n", start); if (at < 0) break; - lines.push(buffer.slice(start, at)); + const line = buffer.slice(start, at); + const lineBytes = Buffer.byteLength(line, "utf8"); + if (lineBytes > MAX_FRAME_BYTES) refuseOversizedFrame(lineBytes, true); + lines.push(line); start = at + 1; } - return { lines, rest: buffer.slice(start) }; + + const rest = buffer.slice(start); + const restBytes = Buffer.byteLength(rest, "utf8"); + if (restBytes > MAX_FRAME_BYTES) refuseOversizedFrame(restBytes, false); + + return { lines, rest }; } /** @@ -118,6 +162,37 @@ export function transformFrame( return `${result.line}${hasCr ? "\r" : ""}\n`; } +/** + * Build a stateful reader that turns raw stdin chunks into complete frames. + * + * It carries the two pieces of cross-chunk state, which is why this is a named + * unit rather than a few lines inside the data handler: + * + * - a `StringDecoder`, so a multi-byte code point split across a chunk + * boundary is completed rather than decoded as two U+FFFDs; + * - the partial-frame buffer, so a frame split across chunks is reassembled. + * + * Exported so tests can drive chunk boundaries exactly. The inputs that matter + * here — a code point bisected between two reads, an oversized frame arriving + * whole — are precisely the ones a spawned child cannot be made to produce on + * demand, because chunking is the kernel's choice and not the writer's. + * + * @throws {GitHubMcpEgressRuntimeError} if any frame exceeds `MAX_FRAME_BYTES`. + */ +export function createFrameReader(): (chunk: Buffer | string) => string[] { + const decoder = new StringDecoder("utf8"); + let buffer = ""; + + return (chunk) => { + // A chunk that is already a string was decoded upstream, so it bypasses + // the decoder rather than being re-encoded on the way in. + buffer += typeof chunk === "string" ? chunk : decoder.write(chunk); + const { lines, rest } = splitFrames(buffer); + buffer = rest; + return lines; + }; +} + export function runGitHubMcpEgressRuntime( options: GitHubMcpEgressRuntimeOptions, io: GitHubMcpEgressRuntimeIo = defaultIo, @@ -134,7 +209,8 @@ export function runGitHubMcpEgressRuntime( let settled = false; let forwardedSignal = false; - let buffer = ""; + // Per-stream: the carry-over bytes and partial frame belong to this stdin. + const readFrames = createFrameReader(); const forwardSignal = (signal: NodeJS.Signals) => { forwardedSignal = true; @@ -168,15 +244,14 @@ export function runGitHubMcpEgressRuntime( }); function onData(chunk: Buffer | string): void { - buffer += typeof chunk === "string" ? chunk : chunk.toString("utf8"); - const { lines, rest } = splitFrames(buffer); - buffer = rest; - - if (Buffer.byteLength(buffer, "utf8") > MAX_FRAME_BYTES) { + let lines: string[]; + try { + lines = readFrames(chunk); + } catch (error) { fail( - new GitHubMcpEgressRuntimeError( - `client frame exceeded ${MAX_FRAME_BYTES} bytes with no newline terminator; refusing to forward it unscrubbed`, - ), + error instanceof GitHubMcpEgressRuntimeError + ? error + : new GitHubMcpEgressRuntimeError("unable to frame outbound MCP traffic"), ); return; }