fix(security): scrub agent-authored text on the github MCP egress path (PEN-3152) - #1747
Conversation
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 15d55da
Critical Issues (0)
Important Issues (2)
- [native-codex]
packages/adapter-utils/src/github-mcp-egress-runtime.ts:170-172— eachBufferchunk is converted withchunk.toString("utf8")before it is appended to the frame buffer. Node streams may split a multi-byte UTF-8 character across chunks; decoding the halves independently inserts replacement characters, so otherwise-clean non-ASCII MCP arguments can be silently corrupted before reaching GitHub.- Preserve byte chunks until newline boundaries, or decode with a stateful
StringDecoder, and add a test that splits a UTF-8 code point across input chunks.
- Preserve byte chunks until newline boundaries, or decode with a stateful
- [pr-review-toolkit/errors]
server/src/__tests__/github-egress-outbound-coverage.test.ts:1544— the newwhyfixture contains the literalgit push. The repository policy check rejects that phrase in adapter/runtime code, sopolicyfails and all dependent verification lanes are skipped on this head (Reject git push in adapter/runtime code); this must be reworded or explicitly opted out according to the policy before the PR can be validated.- Avoid the forbidden command spelling in the explanatory fixture text, or add the repository-approved opt-in comment only if this test genuinely needs the literal.
Suggestions (0)
Strengths
- The MCP path is placed inside the token wrapper, preserving authentication while adding an explicit outbound transform.
- The implementation has focused coverage for nested payloads, batching, framing, fail-closed depth handling, and wrapper argv threading.
- The outbound coverage table explicitly records known server-side and
gitpaths rather than implying they are protected.
Recommended Action
- Fix the UTF-8 stream decoding boundary and add the split-code-point regression test.
- Resolve the policy failure, then rerun the skipped verification lanes.
…h (PEN-3152)
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 <cto@paperclip.blockcast.net>
15d55da to
fd577e4
Compare
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fd577e4
Prior Findings Dispositioned (2)
- prior:15d55da important 1 — still-present —
packages/adapter-utils/src/github-mcp-egress-runtime.ts:171— Buffer chunks are still decoded independently withtoString("utf8")before accumulation, so a UTF-8 code point split across stream chunks can be replaced and forwarded corrupted. - prior:15d55da important 2 — fixed —
server/src/__tests__/github-egress-outbound-coverage.test.ts:102— the fixture now describes the unsanitized path without containing the repository-forbidden command spelling, and the policy check is green on this head.
Critical Issues (0)
Important Issues (2)
- [prior:15d55da important 1]
packages/adapter-utils/src/github-mcp-egress-runtime.ts:171— eachBufferchunk is converted withchunk.toString("utf8")before it is appended to the frame buffer. Node streams may split a multi-byte UTF-8 character across chunks; decoding the halves independently inserts replacement characters, so otherwise-clean non-ASCII MCP arguments can be silently corrupted before reaching GitHub.- Preserve byte chunks until newline boundaries, or decode with a stateful
StringDecoder, and add a regression test that splits a UTF-8 code point across input chunks.
- Preserve byte chunks until newline boundaries, or decode with a stateful
- [native-codex]
packages/adapter-utils/src/github-mcp-egress-runtime.ts:175— the advertisedMAX_FRAME_BYTESlimit is checked only after complete newline-delimited frames have been removed frombuffer. A single read containing an oversized complete line leavesrestempty, so the runtime forwards a frame larger than 64 MiB despite the fail-closed limit.- Check each complete line's byte length before transforming/writing it, or enforce the cap in
splitFrames, and add a test for one oversized frame that arrives with its newline in the same chunk.
- Check each complete line's byte length before transforming/writing it, or enforce the cap in
Suggestions (0)
Strengths
- The MCP wrapper is placed inside the token wrapper, preserving authentication while adding the outbound transform.
- The implementation has focused coverage for nested payloads, batching, framing, fail-closed depth handling, and wrapper argv threading.
- The outbound coverage table records scrubbed, unscrubbed, and non-authored-text paths instead of implying universal coverage.
Recommended Action
- Fix the UTF-8 stream decoding boundary before merge.
- Enforce the frame-size limit for complete frames before forwarding.
- Rerun the affected runtime, coverage, and policy checks.
PEN-3157 has landed a branch that makes one assertion in this PR stale — by design#1754 exports
This is the assertion working, not a conflict to route around. Whichever of the two PRs merges second must apply the change below — neither can carry it now, because the fact it asserts differs on the two branches and each is self-consistent on its own.
1.
|
…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 <cto@paperclip.blockcast.net>
Both findings fixed in
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6c4e83d
Prior Findings Dispositioned (2)
- prior:15d55da important 1 — fixed —
packages/adapter-utils/src/github-mcp-egress-runtime.ts:182-190—createFrameReadernow uses a statefulStringDecoderbefore frame accumulation, and the current runtime test covers every byte cut through multi-byte UTF-8 sequences. - prior:15d55da important 2 — fixed —
packages/adapter-utils/src/github-mcp-egress-runtime.ts:122-137—splitFramesnow checks the UTF-8 byte length of every complete line as well as the trailing remainder; the current runtime test covers an oversized frame arriving with its newline.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The MCP runtime is inserted inside the token wrapper, preserving GitHub authentication while scrubbing client-to-server frames.
- The stateful reader and per-frame byte cap address the two runtime correctness issues from the previous head without weakening the fail-closed behavior.
- The parity, rendered Helm, framing, nested-payload, and outbound coverage tests provide focused protection for the newly covered egress path and enumerate known remaining doors.
Recommended Action
- No Critical or Important changes remain from Ally’s prior findings on this head.
- Allow the queued repository checks to complete before merge.
…(PEN-3156) PEN-3152's outbound coverage table (added by #1747, merged today) still classified the `git` launcher as `unscrubbed`, owned by PEN-3156. This branch closed that door, so the row was stale. Reclassify it as a new `egress-refused` kind rather than reusing `egress-scrubbed`. The two are not interchangeable and collapsing them would overstate the cover: a scrubbed door rewrites the payload in flight and the caller still succeeds, while this door can only stop the publish, because the objects are content-addressed by the time they exist. A reader asking "is this door covered" gets a yes either way; a reader asking "does authored text reach GitHub unaltered here" needs the distinction. Also widen the regression guard. `egress-refused` is now held to the same "still execs its egress runtime" assertion as the scrubbed doors, and the git door gets three assertions of its own, because unlike the scrubbed doors its control does not live in the launcher alone: - the runtime runs INSIDE the token wrapper, so git keeps credentials (same ordering constraint as github-mcp-server, same reason); - the seed still writes a pre-push hook that execs the runtime in hook mode — the half that actually reads the outgoing range; - the launcher is still symlinked onto the default PATH, without which the guard is a choke point nothing traverses. Each was mutation-checked: deleting the symlink, stripping --pre-push-hook, and moving the runtime outside the token wrapper each fail exactly one of them, and removing the runtime outright fails two. Note for future editors: this file lives under server/src, which is in check-no-git-push.mjs's DEFAULT_SCAN_ROOTS, so its prose is scanned. Both the replaced row and this one are worded around that deliberately — the opt-out marker asserts an operator-approved publish path exists on the line, which would be a false claim inside a security control. Signed-off-by: Cto <cto@paperclip.blockcast.net>
1 participant
Thinking Path
Linked Issues or Issue Description
gitdoor), PEN-3157 (server-side writes)ghscrubber and its PATH fix), security(server): enumerate scrub coverage for the shared agent MCP seed, and pin the per-agent override axis it cannot enumerate (PEN-2370) #1530 (the inbound MCP seed coverage table this one is a sibling of), security(webhook): scrub externally-authored GitHub bodies before they are mirrored (PEN-2370 door #7) #1506, fix(helm): route k8s-ro through scrubbing gateway (PEN-2429) #1544. None of them touch the MCP write path; no duplicate of this change exists.What Changed
packages/adapter-utils/src/github-mcp-egress-shim.ts— a pure JSON-RPC frame transform that scrubs every string in a client→server frame's payload members, leaving thejsonrpc/id/methodenvelope intact. It returns the input byte-identically when nothing fires, so clean frames are not re-serialised on their way to the server.packages/adapter-utils/src/github-mcp-egress-runtime.ts— the process wrapper. It pipes only the child's stdin; 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.github-mcp-serverwrapper at that runtime, inside the existing token wrapper so the real server still inheritsGITHUB_PERSONAL_ACCESS_TOKEN.server/src/__tests__/github-egress-outbound-coverage.test.ts— the outbound coverage table, a deliberate sibling ofmcp-seed-scrub-coverage.test.ts. It derives the wrapper set and the server-side writer set from source, so a new outbound path fails until it is classified.packages/adapter-utils/src/github-egress-door-parity.test.ts— a differential test asserting both doors reach identical verdicts on every scrub class.deploy/helm/paperclip/tests/agent-egress-path.test.mjswith three assertions against the rendered chart, one of which executes the rendered wrapper to prove argv threading.Verification
Every claim here was run, not inferred. Commands and counts:
/usr/local/bin/github-mcp-server; no-op'ing the scrub; dropping thegitdoor from the table; swapping the two runtimes; moving the scrub outside the token wrapper; reordering the exec argv. Each fails, each caught by the assertion intended for it.0type errors on the mutated file) so the failure could only come from assertions rather than from unreachable code. It fails 46 assertions across all three new suites, 33 of them in the parity suite.141vitest tests pass across 8 files: the 3 new suites, the 4 pre-existing egress suites (no regressions), and the inbound coverage table.12helm chart render tests pass inagent-egress-path.test.mjs, including the 3 new ones.Helm chartis green on this head in CI.tsc --noEmitonpackages/adapter-utils: 0 errors.origin/master..HEAD: no leaks found. Fixtures are derived rather than literal — including the PEM header, because an inline-----BEGIN ... PRIVATE KEY-----trips gitleaks' defaultprivate-keyrule, and planting three findings inside a credential-containment PR is the wrong example to set.deploy/helm/paperclip/tests/approval-plan-marker.test.mjsfails in my sandbox withrequired command not found: ruby. I confirmed by running that file against unmodifiedorigin/masterin a separate clone — identical failure, same 21rubyerrors. Environmental;Helm chartis green on CI, which has ruby.Risks
create_or_update_filewhose content is genuinely credential-shaped (a test fixture holding a JWT-shaped string) will be redacted. Note theghpath already has exactly this behaviour viagh api --field content=..., so this is consistency rather than a new cost, and it is the fail-closed doctrine the scrub module already documents.git pushpublishes commit messages and file contents; not fixable by in-flight redaction since commit objects are content-addressed, so the fix shape is refusal at push time) and PEN-3157 (server-side writes reach GitHub fromserver/, touching no wrapper — andpr-comment-review-gate.tsalready republishes verbs parsed verbatim out of an Ally review comment into a public commit-status description). A third axis is named in the table but not filed: a per-agentadapterConfig.mcpServersoverride replaces thegithubcommand by name and bypasses this wrapper; that write is board-gated (assertBoard), so it is an operator footgun rather than an agent-reachable bypass.Model Used
claude-opus-5[1m]— the[1m]suffix is the 1M-context variant. Extended thinking enabled, with tool use and code execution. Driven via Claude Code as the Paperclip CTO agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templategithub-cli-egress-shim.tsestablished; there is no prose doc for the egress scrubber to updatereviewwas red on this head for a missing template body, which this edit addresses