security(mcp-gateway): derive every body sniff from one prefix predicate (PEN-2370) - #1556
Conversation
|
🔗 Paperclip issue: PEN-2370 |
1 similar comment
|
🔗 Paperclip issue: PEN-2370 |
![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: b179f38
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The shared
significantByteOffsetpredicate is used by both body-shape sniffers, preventing detection and framing from drifting apart again (packages/mcp-gateway/src/response-scrub.ts:1337-1388). - The JSON parse path removes the same leading BOM that the byte-level classifier accepts, closing the fail-open parse-side gap while preserving byte-exact pass-through for unchanged bodies (
packages/mcp-gateway/src/response-scrub.ts:1406-1413). - The test matrix covers both entry points across all supported prefixes and retains no-resource byte-exact controls (
packages/mcp-gateway/src/response-scrub.test.ts:1206-1262). - The PR's completed checks include the security review, policy, Helm, review, and vendored-adapter checks; the remaining general test, build, typecheck, and e2e jobs were still in progress at review time.
Recommended Action
- No code changes are required from this review.
- Confirm the remaining CI jobs complete successfully before merging.
Follow-up: a fourth classifier still fails open — pushed
|
| BOM | |
|---|---|
trimStart() |
accepts it (ECMAScript WhiteSpace includes U+FEFF) |
JSON.parse |
rejects it |
So a BOM-prefixed nested document classifies as JSON, throws on parse, and falls through the catch into the YAML scanner — which does not match a compact single-line JSON document. It passes through unscrubbed. Identical fail-open to the one this PR unified the byte-level sniffs to close, surviving one layer down because this classifier is reached by recursion rather than by dispatch, and the parent commit only audited dispatch.
Driving the real scrubResponseBody on the queued head — controls alongside, no credential quoted:
scrubbed scrubResponseBody nested text, no BOM (control)
LEAK scrubResponseBody nested text, BOM-prefixed
The fix
stripLeadingBom is now the single string-side definition, used by both scrubJsonRpcBody and scrubTextTracked; significantByteOffset stays the byte-side one. Two representations, one definition each — the recurring failure in this file is a second private copy, not a second representation.
The YAML fallback deliberately keeps the original text: that scanner is line-based and already tolerates a BOM, so handing it the stripped copy would drop that byte from every YAML body we pass through unchanged and break byte-exact pass-through for a case that was never broken. Verified both directions.
The matrix gains the nested entry point with the prefix applied to the inner document — the offset the recursive classifier actually sees. Wrapping it outside would have tested the parent's fix a second time and this one not at all.
Evidence
Reverting the source with the new tests fails exactly the 2 BOM rows of the nested entry point. The other 4 nested rows and all 12 pre-existing rows pass in both states, so this is a tightening, not a widening into redact-everything.
Test Files 1 failed | 5 passed (6)
Tests 2 failed | 359 passed (361)
× scrubs a resource nested in content[].text that opens on a BOM
× scrubs a resource nested in content[].text that opens on a BOM then a blank line
With the fix: 361/361, tsc --noEmit clean.
What I'd flag for your re-review
The parent commit asserted an invariant ("every body sniff") and shipped a green matrix that did not test it — the same shape the ticket has now hit seven times. Worth weighing whether the enumerated matrix is the right control here at all, or whether this wants an assertion that every JSON-parsing site derives from the shared helper. I did not attempt that in this PR; it is a larger change than a review follow-up should smuggle in, and I'd rather it be a deliberate decision than my unilateral one.
Not self-approving or merging — over to you.
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: 4b02628
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
significantByteOffsetgives the byte-level JSON and SSE classifiers one consistent BOM-and-whitespace rule, preventing dispatch drift.scrubTextTrackedandscrubJsonRpcBodystrip the BOM only for parsing while preserving the original text or buffer on unchanged pass-through paths.- The expanded matrix covers SSE, JSON-RPC, and nested
content[].textpayloads across the supported leading prefixes, including byte-exact no-resource controls.
Recommended Action
- No code changes are required from this review.
- Confirm the remaining CI jobs complete successfully before merging.
…ate (PEN-2370) `scrubResponseBody` classified a body with three independent notions of where that body begins, and they disagreed: startsWithSseField skips a BOM, then whitespace startsWithJsonPunctuation skips whitespace only JSON.parse skips neither So a JSON-RPC body carrying a leading UTF-8 BOM matched no classifier and was returned unchanged, `spec.containers[].env[].value` in the clear. Driving the real `scrubResponseBody`: a BOM-prefixed pod body leaked its value while the same body without the BOM, and the same payload inside an SSE `data:` frame with a BOM, both scrubbed. This is the fail-open the SSE sniff was already widened to close, surviving in the one direction that widening did not reach. The commit that closed it even records the reason -- "`startsWithJsonPunctuation` already skips whitespace, and that asymmetry was the entire gap" -- and then fixed the asymmetry from only one side. The fix is the shape `b43437f4`/`cd14342`/`0e5d9151` used for the env-name guard: one shared, named predicate rather than N spellings of it. `significantByteOffset` is now the single definition of a body prefix, and both sniffs derive from it. Detection alone would have moved the fail-open one step down rather than closing it: the body would classify as JSON and then fail to parse, and `scrubJsonRpcBody`'s `null` return sends it through unscrubbed exactly as before. `scrubSseFrames` documents that same half-fix for its own framing regex. So the BOM is stripped parse-side too. Dropping it from the output is safe -- that path only ever returns a re-serialized document; an unchanged body is still returned as the original Buffer, BOM intact. The test matrix ran against the SSE entry point only, because that is the one an observer had seen fail -- and it was green while this leaked. It now runs every prefix against every entry point, so it fails on the next classifier that does not derive its offset from the shared predicate rather than on the one spelling someone happened to probe. Enumerating known cases is what let this reach production twice. Reverting the source with the new tests fails exactly the 2 BOM/JSON rows; the other 10 matrix rows pass in both states, so this is a tightening and not a widening into redact-everything. 354/354 in the package, tsc clean. Not fixed here, and recorded rather than left implied: an NDJSON body and a bare-YAML body still pass through unscrubbed. Neither is an MCP transport shape -- JSON-RPC and SSE are -- so both are speculative rather than reachable, and a scrubber that corrupts unrelated proxied traffic is the worse failure. Signed-off-by: Cto <cto@paperclip.blockcast.net>
…fier (PEN-2370)
The parent commit claimed one prefix predicate for "every body sniff". It
missed one, and the miss was reachable.
`scrubTextTracked` -- the classifier that recurses into a resource nested in
`content[].text`, which is how the k8s MCP servers actually deliver a resource
-- kept its own notion of where a document begins:
trimStart() counts U+FEFF as whitespace (ECMAScript WhiteSpace), so it
ACCEPTS a BOM
JSON.parse rejects one
So a BOM-prefixed nested document classified as JSON, threw on parse, and fell
through to the `catch` into the YAML scanner -- which does not match a compact
single-line JSON document. The entry passed through with
`spec.containers[].env[].value` in the clear. Exactly the fail-open the parent
commit unified the byte-level sniffs to close, surviving one layer down because
this classifier was reached by recursion rather than by dispatch, and the
parent only audited dispatch.
Driving the real `scrubResponseBody` before the fix: a pod body whose nested
`content[].text` opens on a BOM leaked its value, while the same body without
the BOM scrubbed. That asymmetry is the whole bug.
The fix is the parent's own shape, applied where it was missed.
`stripLeadingBom` is now the single string-side definition, used by both
`scrubJsonRpcBody` and `scrubTextTracked`; `significantByteOffset` remains the
byte-side one. Two representations, one definition each -- the failure this
file keeps re-learning is a second private copy, not a second representation.
The YAML fallback deliberately keeps the ORIGINAL text: that scanner is
line-based and already tolerates a BOM, and handing it the stripped copy would
drop that byte from every YAML body we pass through unchanged, breaking
byte-exact pass-through for a case that was never broken. Verified: a
BOM-prefixed YAML pod scrubs, and a BOM-prefixed benign body round-trips
identical.
The matrix gains the nested entry point, with the prefix applied to the INNER
document -- that is the offset the recursive classifier actually sees. Wrapping
the prefix outside would have tested the parent's fix a second time and this
one not at all.
Reverting the source with the new tests fails exactly the 2 BOM rows of the
nested entry point; the other 4 nested rows and all 12 pre-existing rows pass
in both states, so this is a tightening, not a widening into redact-everything.
361/361 in the package, tsc clean.
Method note, since PEN-2370 ask 3 names it as the control: this was not found
by re-reading the patch. Ally's review returned zero findings on the parent and
CI was fully green -- including the mcp-gateway lane, which I confirmed really
executed its 245 suites rather than trusting the aggregate. It was found by
going looking for the same material through a different route, which is the
criterion the ticket asks every remediation to be followed by. Door #5 and #6
came from auditing a fix's own blind spots; so did this one.
Signed-off-by: Cto <cto@paperclip.blockcast.net>
4b02628 to
c2abc1e
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: c2abc1e
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- provides one consistent BOM-and-whitespace rule for the byte-level JSON and SSE classifiers, preventing dispatch drift (, ).
- The JSON parse paths strip the BOM only for parsing, while unchanged nested text and response bodies return their original values, preserving byte-exact pass-through (, ).
- The expanded tests cover SSE, top-level JSON-RPC, and nested payloads across the supported prefixes, with no-resource identity controls ().
- The diff remains narrowly scoped to the scrubber and its regression coverage, and the comments clearly document why detection and parsing must agree.
Recommended Action
- No code changes are required from this review.
- Confirm the remaining CI jobs, especially policy, complete successfully before merging.
1 participant
Thinking Path
Linked Issues or Issue Description
Refs PEN-2370 (ask 3 — "fix the invariant, not the instance", criteria (a3)/(b2))
Related merged work on the same module: #1435, #1449, #1501, #1518, #1522, #1552.
What Changed
significantByteOffset(body)— one definition of a body prefix (an optional UTF-8 BOM, then whitespace).startsWithJsonPunctuationandstartsWithSseFieldnow both derive from it. The SSE sniff previously carried a correct-but-private copy of this logic; that privacy is exactly how the JSON sniff was left behind.scrubJsonRpcBodystrips a leading BOM beforeJSON.parse, which rejects one.The bug, concretely
Three classifiers disagreed about where a body starts:
startsWithSseFieldstartsWithJsonPunctuationJSON.parseSo a JSON-RPC body carrying a leading BOM matched no classifier and was returned unscrubbed. Driving the real
scrubResponseBodywith a synthetic marker: the BOM-prefixed pod body leaked, while the same body without the BOM — and the same payload inside a BOM-prefixed SSEdata:frame — both scrubbed.This is the fail-open the SSE sniff was already widened to close, surviving in the one direction that widening did not reach. The commit that closed it records the reason in a comment — "
startsWithJsonPunctuationalready skips whitespace, and that asymmetry was the entire gap" — and then fixed the asymmetry from only one side.Fixing detection alone would have moved the fail-open one step down rather than closing it: the body would classify as JSON and then fail to parse, and
scrubJsonRpcBody'snullreturn sends it through unscrubbed exactly as before.scrubSseFramesdocuments that same half-fix for its own framing regex. Hence the parse-side strip.Verification
Negative control (the part that matters). Reverting only
response-scrub.tsto master while keeping the new tests fails exactly 2 rows —a BOManda BOM then a blank lineon the JSON-RPC entry point — and the other 10 matrix rows pass in both states. So the matrix is a tightening, not a widening into redact-everything, and the two new rows are not vacuous.Byte-exact pass-through is pinned by two counterweights that pass in both states: a leading-whitespace non-stream body and a BOM-prefixed body carrying no resource are both returned as the identical Buffer (
toBe, nottoEqual).No pod was probed and no credential value appears anywhere in this change — fixtures use the existing synthetic
LEAKED_*markers.Risks
Low. Widening detection is safe in this direction by the module's existing argument: a body that now classifies but carries no
env/Secret/argv material leavesctx.changedfalse, andscrubResponseBodyreturns the original Buffer byte-for-byte. The only behavioural change on a body we do rewrite is that its leading BOM is dropped — and that path already re-serializes, so it never preserved the original byte layout anyway.Knowingly not fixed, recorded so a green run does not imply otherwise: an NDJSON body and a bare-YAML body still pass through unscrubbed. Neither is an MCP transport shape (JSON-RPC and SSE are), so both are speculative rather than reachable, and the module's stated position is that corrupting unrelated proxied traffic is the worse failure. Named here rather than silently patched.
Model Used
Claude Opus 4.5 (
claude-opus-5[1m]), 1M context, extended thinking, with tool use and code execution — used to drive the real transpiledscrubResponseBodyagainst controls rather than to reason about the code from reading alone.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template