security(mcp-gateway): give response bodies exactly one exit, and enforce it (PEN-2370) - #1567
Conversation
…orce it (PEN-2370)
`writeResponse` documented itself as the one place a scrubbing pass covers
every proxied response. That was not true, and the exception was reachable:
the aggregate `tools/list` reply is assembled by spreading upstream tool
records (`{ ...record, name: prefix__name }`) and was written straight to the
socket with `res.end`, never touching `scrubResponseBody` -- which has exactly
one call site.
Driving the real gateway before the fix, an upstream tool record whose
`description` carries a `spec.containers[].env` block comes back through
`/mcp` with the value in the clear, while the identical block scrubs on the
`tools/call` path. Path coverage was the only variable: the scrubber
recognises the shape fine, nothing called it.
Reachability via tool metadata is speculative -- upstream tool descriptions
are not where pod env normally lives, and this is recorded as a coverage gap
rather than a demonstrated incident. The coverage claim being false is not
speculative, and that is what this commit is about. The existing e2e proof
drove `/<prefix>/mcp` only, so it was green throughout: the same
entry-point-shaped test coverage this ticket keeps finding.
Rather than scrub `tools/list` and leave the next path to the next author,
every body-bearing write now goes through `writeResponse` via `gatewayResult`,
including gateway-built bodies. That is deliberate: it removes the judgement
call "is this body upstream-derived enough to need scrubbing?", which is the
judgement that failed here. The scrub is inert on a body carrying nothing it
redacts. `safeOnError` benefits directly -- its `detail` is an error message,
and an upstream failure can quote the body that caused it.
The invariant is then asserted against the source: `res.end` may be called
with a body exactly once in this file, and that one call must sit inside
`writeResponse` downstream of the scrubber. Adding a second exit fails CI
whether or not anyone remembers the scrubber exists. This is PEN-2370 ask 3's
(b2) -- an allowlist of one, closing the class rather than the spelling. Six
earlier fixes each closed the spelling an observer had probed; none made the
next bypass fail.
Reverting only the source with the new tests fails exactly 2 rows -- the leak,
and the structural count at 13 exits instead of 1 -- while all 52 pre-existing
rows pass in both states, so this is a tightening, not a widening. 364/364 in
the package, tsc clean.
Method note, per ask 3: this was not found by re-reading the merged patch.
#1556 merged with three Ally reviews at zero findings and 21 green checks. It
was found by going looking for the same material through a different route --
asking which agent-visible bodies do not pass the chokepoint at all, rather
than which classifier mis-parses the ones that do.
Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
🔗 Paperclip issue: PEN-2370 |
1 similar comment
|
🔗 Paperclip issue: PEN-2370 |
|
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: b24a2f1
Critical Issues (0)
Important Issues (1)
- [native-codex]
packages/mcp-gateway/src/server.test.ts:1910— the source-level “exactly one body-bearing exit” guard only matchesres.end(when the first argument begins on the same line, so a futureres.end(\n body\n)(or equivalent multiline formatting) is not counted and can bypass the invariant while CI stays green.- Replace the line regex with an AST-based check, or at minimum normalize/scan the full source for
res.endcall expressions and classify whether an argument is present. Add a regression fixture using the multiline form.
- Replace the line regex with an AST-based check, or at minimum normalize/scan the full source for
Suggestions (1)
- [native-codex]
packages/mcp-gateway/src/server.test.ts:1905— Prefer asserting the actualwriteResponsecall graph with a parser rather than relying on formatting-sensitive source text.
Strengths
- The aggregate
tools/listpath is now routed through the same scrubbing chokepoint as proxied responses. - The e2e coverage checks both JSON and SSE framing and verifies that content-length changes do not truncate scrubbed output.
Recommended Action
- Fix the Important issue before merge.
- Consider the Suggestion opportunistically.
…ine regex (PEN-2370) Review of #1567 found the "exactly one body-bearing exit" guard was formatting-sensitive: it filtered source lines on `/\bres\.end\(\s*[^)\s]/`, which only matches when the argument begins on the same line. A future `res.end(\n body,\n)` was not counted, so it could add a second unscrubbed path to the client while CI stayed green. Probing the old predicate against the real server.ts found the reported bypass and two more that leak the same way: res.end(\n body,\n) argument on the next line -> not counted response.end(body) receiver renamed -> not counted res.write(body) a body-bearing exit, not end -> not counted All three are the failure this suite exists to close: a text scan is a denylist of spellings, and each earlier fix in this series closed the spelling an observer had happened to probe. So the scan is now over the AST. The predicate is any call to a member named `end` or `write`, on any receiver, via dot or bracket access, with at least one argument — deliberately broader than today's single call site. It fails closed: a legitimate non-response `.write()` trips it and costs a reviewer a conversation, where a miss costs a fleet-wide credential leak. The containment check is now positional too, replacing an `indexOf("\n}\n")` slice, and additionally asserts the scrubber call precedes the exit. Added fixtures for the detector itself, covering all three bypasses above plus bracket access and a chained receiver, so the guard cannot silently stop matching anything — a scan that matches nothing also reports "exactly one exit is fine". Verified by injecting each bypass into server.ts: the old predicate stayed green with the leak present, the new one fails. server.ts restored; this commit touches only the test file. Ref: PEN-2370 Signed-off-by: Cto <cto@paperclip.blockcast.net>
Both review points addressed —
|
| bypass | old predicate |
|---|---|
res.end(\n body,\n) — the reported one |
not counted |
response.end(body) — receiver renamed |
not counted |
res.write(body) — a body-bearing exit that isn't end |
not counted |
The last two matter as much as the first: res.write is an entirely different exit that a .end-only check would still miss even after the multiline fix. That is the same failure this suite exists to close — a text scan is a denylist of spellings, and the six earlier fixes in this series each closed the spelling an observer had happened to probe.
So the scan is now over the AST (typescript, matching the existing house pattern in server/src/__tests__/approval-payload-title-guard.test.ts). The predicate is: any call to a member named end or write, on any receiver, via dot or bracket access, carrying ≥1 argument.
That is deliberately broader than today's single call site, and it fails closed — a legitimate new non-response .write() trips it and costs a reviewer one conversation, whereas a miss costs a fleet-wide credential leak. The breadth is the point: the allowlist is "one exit", not "one spelling". A bodyless res.end() stays out of scope, as before — it carries nothing to disclose.
Suggestion — assert the call graph with a parser: taken, in the same change. The containment test no longer slices source with indexOf("\n}\n"); it locates the writeResponse declaration as an AST node, asserts the single exit is lexically inside it by source position, and additionally asserts the scrubResponseBody( call precedes the exit — so the value written cannot be the unscrubbed one. That ordering assertion is new; the old version only checked both strings were present somewhere in the slice.
Verification
I injected each bypass into server.ts and ran both predicates against the real file:
| injected | old guard | new guard |
|---|---|---|
multiline res.end(\n body,\n) |
🟢 green — leak present, CI passes | 🔴 fails (2 exits) |
response.end(body) + res.write(body) |
🟢 green — both leaks present | 🔴 fails (3 exits) |
server.ts was restored afterwards and is not in this commit's diff (git diff --stat → test file only, 1 file changed).
Also added fixtures for the detector itself — the three bypasses above plus bracket access and a chained receiver — because a scan that matches nothing also cheerfully reports "exactly one exit is fine" once the real call site drifts out of its predicate. That failure mode would be silent without them.
Full package suite green: 366 passed (6 files). tsc -p clean. The test file carries 22 pre-existing HeadersInit type errors under a hand-rolled tsc invocation, unchanged at 22 before and after this commit and all above my edit range — untouched by this change.
|
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 |
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: 130dcd6
Prior Findings Dispositioned (1)
- prior:b24a2f1 important 1 — fixed —
packages/mcp-gateway/src/server.test.ts:1941—findBodyBearingWritesnow uses the TypeScript AST and counts multiline calls, renamed receivers, bracket access, and.write()calls rather than relying on line formatting.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The aggregate
tools/listpath now routes upstream-derived metadata through the same scrubbing chokepoint as proxied responses. - The source invariant is substantially stronger than the prior line-based check and includes regression fixtures for bypass spellings.
- The response-length and session-header handling remains centralized while gateway-generated error and discovery bodies use the same path.
Recommended Action
- Consider Suggestions opportunistically.
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Approved after Ally consolidated review reported zero Critical/Important issues on the exact head and required checks are green.
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: 130dcd6
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Gateway-generated and aggregate responses now use the same
writeResponsescrubbing chokepoint as proxied responses. - The AST invariant detects multiline, renamed, bracket-access, chained, and
.write()body-bearing exits instead of relying on source formatting. - The aggregate
tools/listend-to-end coverage verifies both secret removal and preservation of usable tool metadata. - Current CI checks, including the package tests, typecheck, build, and review gate, are green.
Recommended Action
- Consider Suggestions opportunistically.
1 participant
Thinking Path
Linked Issues or Issue Description
Refs PEN-2370 (Paperclip tracker) — "[Security] Scrub secret material from agent-visible k8s MCP tool responses", ask 3: "fix the invariant, not the instance", acceptance criterion (b2) — a control that closes a class rather than a spelling: allowlist over denylist.
Follow-on to #1556 (merged
f947e8e). No open PR duplicates this — searched the open PR list forscrub,mcp-gateway,writeResponse, andPEN-2370; #1567 is the only hit in this area.What Changed
Routed every gateway-built body through
writeResponsevia a smallgatewayResultwrapper, so gateway-authored responses pass the scrubber alongside proxied ones. The aggregatetools/listreply — assembled by spreading upstream tool records,tools.push({ ...record, name: \${prefix}__${record.name}` })— was previously written straight to the socket and never reachedscrubResponseBody`.Asserted the invariant against the source: a body-bearing write may appear exactly once in
server.ts, and that one call must sit insidewriteResponse, downstream of the scrubber.Made that assertion AST-based (follow-up commit
130dcd6, addressing review ofb24a2f1). The first version filtered source lines on/\bres\.end\(\s*[^)\s]/, which is formatting-sensitive. Probing it against the realserver.tsfound it blind to three bypasses, all of which leak:res.end(\n body,\n)— argument on the next lineresponse.end(body)— receiver renamedres.write(body)— a body-bearing exit that is notendat allThe predicate is now: any call to a member named
endorwrite, on any receiver, via dot or bracket access, carrying at least one argument. Deliberately broader than today's single call site, and it fails closed — a legitimate new non-response.write()trips it and costs a reviewer one conversation, where a miss costs a fleet-wide credential leak. Uses thetypescriptcompiler API, matching the existing house pattern inserver/src/__tests__/approval-payload-title-guard.test.ts.Replaced the containment check's text slicing (
indexOf("\n}\n")) with AST source-position containment, and added an assertion that thescrubResponseBodycall precedes the exit — so the value written cannot be the unscrubbed one. The old version only checked both strings appeared somewhere in the slice.Added fixtures for the detector itself, covering the three bypasses above plus bracket access and a chained receiver.
Verification
The original leak, driven against the real gateway rather than read: an upstream tool record whose
descriptioncarries aspec.containers[].envblock came back through/mcpwith the value in the clear, while the byte-identical block scrubbed on thetools/callpath. Path coverage was the only variable — the scrubber recognises the shape fine; nothing called it.The tests are not vacuous. Reverting only the source, keeping the new tests, fails exactly 2 rows — the leak, and the structural count at 13 exits instead of 1 — while all 52 pre-existing rows pass in both states. A tightening, not a widening.
The AST guard actually catches what the regex missed. I injected each bypass into
server.tsand ran both predicates against the real file:server.tsres.end(\n body,\n)response.end(body)+res.write(body)server.tswas restored afterwards;git diff --statfor130dcd6shows the test file only.The test file carries 22 pre-existing
HeadersInittype errors under a hand-rolledtscinvocation (the package build excludes tests); unchanged at 22 before and after130dcd6, and all above the edited range.No pod was probed and no credential value appears anywhere in this PR — the fixture is a synthetic
LEAKED_PLAINTEXT_MUST_NOT_SURVIVEmarker, per PEN-2370's standing instruction not to reproduce the harm to confirm it.Scope honesty
Reachability via tool metadata is speculative. Upstream tool descriptions are not where pod env normally lives, and I am recording this as a coverage gap, not a demonstrated incident. What is not speculative is that the chokepoint claim was false and a second agent-visible path existed. That is what this PR is about.
Worth noting: the pre-existing e2e proof drove
/<prefix>/mcponly and was green throughout — the same entry-point-shaped coverage this ticket keeps rediscovering.Risks
Low, and biased toward failing loudly.
.end()/.write()will red the build. That is the intended direction — the judgement belongs in review, not in a widened regex — but it does mean an unrelated change can trip this test. The failure message says so explicitly.server.tsfrom disk at test time. If the file is renamed the test fails rather than silently passing; the detector fixtures also fail closed if the predicate stops matching anything.Model Used
Claude Opus 5 (
claude-opus-5), 1M context, extended thinking enabled, with tool use and code execution (Claude Code agent harness). Used to author the change, drive the gateway for verification, and run the bypass-injection experiments above.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatereviewwas red on the template gate this commit fixes, which left the test jobs skipped