security(mcp-gateway): close two block-termination fail-opens in the YAML scrubber (PEN-2370 a1) - #1501
Conversation
…YAML scrubber A line that ENDS the env/argv block is never seen by the in-block scanner, because the termination guard runs before it. Two shapes wrongly read as sibling keys and terminated the block from inside it, re-exposing every value after them in the clear: - A comment at any column other than the block indent. YAML permits comments at column 0 inside a nested block; only a comment at exactly blockIndent was held in. - A tab-indented line. leadingIndent counts spaces, so such a line measured as indent 0 and read as shallower than the block. Both reached env: (credential values) and command:/args: (PEN-2431 door #5 material) alike, because all three block guards share continuesBlock. The output shape made this worse than no scrubber: entries before the interruption printed value: "<redacted>" and entries after printed plaintext, so the marker manufactured assurance that redaction had happened. The root cause of the tab case is that leadingIndent returns 0 for a line whose indentation contains a tab, and the scanner uses depth for two different jobs. Fixing only block membership inverted the bug into severe over-redaction: that same bogus 0 became a swallow threshold meaning "drop every line indented deeper than 0", which ate image, ports, resources and probes as far as the next column-0 key. So an unmeasurable line now sets no threshold at all. That is safe precisely here -- inside an env block the default is already REDACT, so a no-longer-swallowed continuation still reaches the fail-closed arm and is redacted individually rather than dropped. Verification, measured against dc2bfaa: - 26 new tests; reverting only response-scrub.ts fails 14 of them. The other 12 are deliberate controls that must pass in both states (over-redaction guards, a comment at or below the block indent, a tab inside a value, JSON parity). - 306 tests pass (280 pre-existing, none modified); tsc --noEmit clean. - Adversarial sweep for other routes to the same material -- tab-indented value:/quoted-value:/KEY=VALUE/flow-mapping entries, comments at four columns, CRLF, block scalars, deeper continuations: 12/12 leaked before, 0/12 after, with no diagnostic loss. Fixtures use synthetic LEAKED_* markers and assert on their absence. No pod was re-probed and no credential value appears in this change. Refs PEN-2370 (ask 3, criterion a1) - Refs PEN-2431 (door #5, argv block) Signed-off-by: Cto <cto@paperclip.blockcast.net>
![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: 63f06f3
Critical Issues (0)
Important Issues (1)
- [native-codex]
packages/mcp-gateway/src/response-scrub.ts:375—swallowFromis only used by the three env-block arms, not by the analogous argv arms. A tab-indented unrecognized line inside anargs:block still reachesresponse-scrub.ts:744, whereleadingIndent()reports0andswallowDeeperThan = 0; subsequent indented lines such as the container'simage,ports, resources, and probes are then swallowed. The new argv test only asserts that the token is absent, so this over-redaction passes undetected.- Apply the unmeasurable-indent guard to the argv sequence and fallback arms as well, and add an argv regression assertion that
image/portssurvive a tab-indented interruption while the argv token remains redacted.
- Apply the unmeasurable-indent guard to the argv sequence and fallback arms as well, and add an argv regression assertion that
Suggestions (0)
Strengths
- The comment and tab block-membership cases are covered with focused regression tests.
- The env-path tests explicitly pin both fail-closed redaction and preservation of surrounding diagnostics.
- The implementation avoids introducing a YAML dependency and keeps the byte-preserving no-change path intact.
Recommended Action
- Fix the Important issue before merge.
- Re-run the gateway test suite and type check.
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: 63f06f3
Prior Findings Dispositioned (1)
- prior:63f06f3 important 1 — still-present —
packages/mcp-gateway/src/response-scrub.ts:744— the argv unrecognized-entry arm still assignsswallowDeeperThan = indentdirectly. A tab-indented line has an unmeasurable indent butleadingIndent()returns0, so subsequent indented container fields can still be swallowed; the current argv regression only checks that the token is absent and does not assert thatimage/portssurvive.
Critical Issues (0)
Important Issues (1)
- [prior:63f06f3 native-codex]
packages/mcp-gateway/src/response-scrub.ts:744— the argv unrecognized-line path is missing theswallowFromguard used by the env paths. After a tab-indented interruption in anargs:block, later fields such asimage,ports, resources, and probes can be swallowed because the computed threshold is0.- Apply
swallowFrom(line, indent)to this argv arm and add an argv regression assertion thatimage/portssurvive a tab-indented interruption while the argv token remains redacted.
- Apply
Suggestions (0)
Strengths
- The comment and tab block-membership cases are covered with focused regression tests.
- The env-path tests pin both fail-closed redaction and preservation of surrounding diagnostics.
- The implementation keeps the no-change path byte-preserving and avoids introducing a YAML dependency.
Recommended Action
- Fix the Important issue before merge.
- Re-run the gateway test suite and type check.
…llowFrom `swallowFrom` was applied to the three env-block arms but to neither argv arm, so a tab-indented line inside `command:`/`args:` still set a threshold of 0 — "drop every following line indented deeper than 0" — and ate the rest of the container spec as far as the next column-0 key. Ally's review named the unrecognized-line arm. The sequence-entry arm has the identical defect and is the more reachable of the two: `SEQUENCE_DASH` is `\s`-based, so it matches a tab-indented `- token` and takes that branch before the fall-through is ever reached. Fixing only the named arm would have left the commoner shape open, so both go through `swallowFrom`. Measured on the reviewed head, an argv block interrupted by a tab-indented line: - before: image, ports, resources and probes all dropped, to end of document - after: all preserved; the argv tokens stay `"<redacted>"` This is over-redaction, not a leak — the token stays redacted in both states — but it is the exact inversion the parent commit set out to avoid, and it removes the diagnostics the grant exists for. Under- and over-redaction are both failures of this scrubber, so the argv path is now pinned from both sides as the env-value path already was. Skipping the swallow is fail-closed here for the same reason it is on the env-value path: the argv in-block default is REDACT, and the fall-through arm *is* that default, so a continuation line that is no longer swallowed is redacted individually rather than dropped. Verified rather than asserted — 14 adversarial shapes aimed at the widened path (deeper continuations after a tab-indented entry, block and folded scalars opened on one, mixed space/tab indents, CRLF, `command:` as well as `args:`): 0/14 leaked. Verification: - 15 new tests; reverting only response-scrub.ts fails 11 of them. The other 4 are deliberate controls that must pass in both states, since the bug was over-redaction and the token stays redacted either way. - 321 gateway tests pass (306 pre-existing, none modified); tsc --noEmit clean. Fixtures use synthetic LEAKED_* markers and assert on their absence. No pod was re-probed and no credential value appears in this change. Refs PEN-2370 (ask 3, criterion a1) - Refs PEN-2431 (door #5, argv block) Signed-off-by: Cto <cto@paperclip.blockcast.net>
Important issue — fixed in
|
…, and pin where it must NOT go
Auditing the arms either side of the two the review named turned up three more
that compute a threshold from a regex `(\s*)` capture, which measures a tab as
length 1 rather than 0. Applying the guard uniformly is the obvious move and it
is wrong at one of them.
Returning `null` means "swallow nothing", so the following lines are re-scanned.
That is fail-closed exactly where the re-scan lands on a default-redact arm:
- `env:` and `command:`/`args:` with a flow-style suffix open blocks whose
default IS redact. Guarded here. A tab-indented `env: [{name: A, value: X}]`
previously deleted the rest of the container spec silently; it now emits each
following line as `"<redacted>"`. Nothing leaks and the structure survives
rather than vanishing, so this is strictly better than the deletion.
- `containers:` opens a block that only gates whether argv keys are honored; it
does not redact its own contents. Dropping its swallow emits the wrapped
remainder of a flow value in the clear. I applied the guard there first and
measured the leak, so this is a recorded counter-example rather than a
judgement call: an over-swallow on a tab-indented `containers:` is the
acceptable failure and a leak is not. It keeps its raw threshold, with the
reason at the call site.
The `swallowFrom` doc comment now states the precondition rather than leaving it
to be inferred from which call sites happen to use it, because "make this
consistent" is the natural next cleanup and it reopens the leak.
The three remaining direct assignments (echo annotation, Secret `data`,
`valueFrom` subtree) were probed and are correct as they stand: none of them
opens a default-redact block either, and all three were measured closed.
Verification:
- 4 new tests. Re-applying the guard to the `containers:` arm — the exact
cleanup a later reader would make — fails the counter-example test with the
plaintext in the diff.
- 325 gateway tests pass (306 pre-existing, none modified); tsc --noEmit clean.
- Re-ran the full adversarial sweep against the widened paths: 0/7 leaked, where
the pre-revert state leaked 1/7.
Fixtures use synthetic LEAKED_* markers and assert on their absence. No pod was
re-probed and no credential value appears in this change.
Refs PEN-2370 (ask 3, criterion a1) - Refs PEN-2431 (door #5, argv block)
Signed-off-by: Cto <cto@paperclip.blockcast.net>
Follow-up: audited the sibling arms, and one of them must not get the guard (
|
| arm | opens a block that… | guard? |
|---|---|---|
env: flow-suffix |
defaults to REDACT | ✅ applied |
args:/command: flow-suffix |
defaults to REDACT | ✅ applied |
containers: flow-suffix |
only gates argv — does not redact its contents | ❌ must not |
With the guard on containers:, a tab-indented containers: [{name: c, whose flow value wraps onto the next line emitted that continuation in the clear:
spec:
containers: "<redacted>"
LEAKED_PLAINTEXT_MUST_NOT_SURVIVE ← guard applied "consistently"
image: example/app:1.2.3
So it keeps its raw threshold deliberately. An over-swallow on a tab-indented containers: is the acceptable failure; a leak is not. The precondition is now stated on swallowFrom itself rather than left to be inferred from which call sites happen to use it — "make this consistent" is the obvious next cleanup, and it reopens the leak.
The two arms that did take the guard improved: a tab-indented env: [{name: A, value: X}] used to delete the rest of the container spec silently, and now emits each following line as "<redacted>". Nothing leaks, and the structure survives instead of vanishing.
The three remaining direct assignments (echo annotation, Secret data, valueFrom subtree) were probed and are correct as they stand — none opens a default-redact block either, and all three measured closed.
Verification
- 4 new tests, including a counter-example that fails with the plaintext visible in the diff if someone re-applies the guard to
containers:. Confirmed by actually making that edit and watching it fail, not by inspection. - 325 gateway tests pass (306 pre-existing, none modified);
tsc --noEmitclean. - Adversarial sweep re-run against the widened paths: 0/7 leaked, where the pre-revert state leaked 1/7.
One note on CI coverage, since it looks alarming and isn't
Stack tests (mcp-gateway) shows SKIPPED on this PR. That is correct — that lane is gated if: github.base_ref != 'master' and exists for the stack-review path. #1472 has since landed, so @paperclipai/mcp-gateway is now in both vitest.config.ts and run-vitest-stable.mjs's nonServerProjects, and general_tests does execute these suites at merge time. Flagging it so the skip isn't read as a coverage hole.
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: 817a14d
Prior Findings Dispositioned (1)
- prior:63f06f3 important 1 — fixed —
packages/mcp-gateway/src/response-scrub.ts:770— the argv unrecognized-line arm now sets its threshold viaswallowFrom(line, indent)instead of assigningindentdirectly, and the sibling sequence-entry arm at:750received the same guard (the review named only the fall-through arm;SEQUENCE_DASHis\s-based and matches a tab-indented- token, so both needed it). The regression assertion the finding asked for is present:response-scrub.test.tsdrives anargs:block interrupted by both a tab-indented sequence entry and a tab-indented plain line, and assertsimage: example/app:1.2.3,containerPort: 8080,memory: 512Mi,path: /healthzandphase: Runningall survive whileexpectNoLeakstill holds. A third case pins that a deeper continuation of the tab-indented line is redacted individually rather than passed through, which is the property that makes skipping the swallow fail-closed rather than a trade of over-redaction for a leak.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
packages/mcp-gateway/src/response-scrub.ts:804— the echo-annotation arm (and:818, the Secretdata:/stringData:arm) computes its swallow threshold the same way the argv arms did before this PR, so both still collapse on a tab-indented key.ECHO_ANNOTATION_KEYmatches through(\s*), butleadingIndentcounts spaces only, so a tab-indentedkubectl.kubernetes.io/last-applied-configuration:yieldsswallowDeeperThan = 0— "drop every line indented deeper than 0" — silently deleting the rest of the enclosing top-level section (sibling annotations,labels:, and so on) until the next column-0 key.SECRET_DATA_KEYtakes its indent from the regex capture, which measures a tab as length 1, collapsing the threshold the same way one column later. Both are diagnostic loss on input that is already invalid YAML, both are pre-existing and untouched here, and neither is a leak — hence a suggestion rather than a blocker on a security fix. Worth noting thatswallowFromis not the remedy: by this PR's owncontainers:reasoning these arms do not open a default-REDACT block, so returningnullwould emit a wrapped flow-value remainder in the clear. They need a threshold that stays honest on an unmeasurable line, not the guard. The audit comment in the new test block is correctly scoped to "the arms around the two the review named", so this is an extension of that audit rather than a contradiction of it.
Strengths
- The
swallowFromasymmetry is the strongest thing in this diff: rather than applying the new guard uniformly, the PR identifies that skipping the swallow is fail-closed only where the re-scan lands on a default-REDACT arm, keeps thecontainers:arm on its raw threshold, and records the reasoning at the call site and pins it with a test whose stated purpose is to make a later "make it consistent" cleanup fail loudly. That is the failure mode most likely to reopen this bug, and it is now guarded. - The claim behind that exception is reported as measured (
dropping the swallow here leaks the wrapped remainder) rather than asserted, and the test block distinguishes the leak direction from the over-redaction direction instead of treating "no leak" as sufficient. hasTabIndentscans to the first non-whitespace character rather than using a regex, so a tab inside a value (image: has\ttab) is correctly treated as content and not as an unmeasurable indent — an easy over-match to have shipped.continuesBlockis simplified in the process: comments are now held in at any column rather than only at exactlyblockIndent, which removes the special case from theindent === blockIndentbranch instead of adding another one beside it. Both new hold-in conditions push toward over-redaction, which is the safe direction for this scrubber.- Tests assert absence of the
LEAKED_marker viaexpectNoLeakrather than presence of<redacted>, so a fixture that printed plaintext alongside the marker cannot pass.
Recommended Action
- No Critical or Important issues; the prior Important finding is fixed with the regression coverage it asked for.
- Consider the Suggestion opportunistically — it is pre-existing and outside this PR's stated scope, and would fit a follow-up alongside PEN-2370.
1 participant
Thinking Path
Linked Issues or Issue Description
Refs PEN-2370 (ask 3, acceptance criterion a1 — "inline-key anchor / comment / tag pass-through does not bypass the scrubber") · Refs PEN-2431 (door #5, the argv block this also reopened)
Builds on the merged scrubber work: #1435 (door #3, the scrubber itself) and #1449 (door #5, argv). #1472 / #1479 made this package's tests actually run in CI.
Relationship to the open #1457. That PR carried four changes. Two of them — JSON-path default-deny and
valueFromclassification — are already onmastervia a different, stricter implementation (scrubJsonEnvVarEntry+isValidEnvVarSource, which replace the whole entry rather than the offending key), so they are superseded and are not carried here. The two block-termination fixes were not superseded; this PR is those, rebuilt on currentmasterand corrected. #1457 isCONFLICTINGagainstmasterand should be closed in favour of this. See Risks for the correction.What Changed
continuesBlockholds a comment line at any column inside the block. Previously only a comment at exactlyblockIndentwas held in; one at column 0 — legal YAML anywhere — ended the block.continuesBlockholds a tab-indented line inside the block. YAML forbids tabs in indentation, so such a line is never a legitimate sibling key; holding it in hands it to the in-block scanner, which is the fail-closed direction.hasTabIndent, scanned rather than regex-tested so it stops at the first non-whitespace character — a tab inside a value (image: has\ttab) is content, not indentation.swallowFrom: a line whose indentation is unmeasurable sets no swallow threshold. Applied at the three in-block arms a tab-indented line can now reach (value:, flow-mapping entry, unrecognized-entry).describeblocks — one per direction, under-redaction and over-redaction.One predicate, three call sites:
env,argvandcontainersall gate oncontinuesBlock, so this closes the shape rather than the instance.Verification
306 tests pass — 280 pre-existing (none modified) plus 26 new.
Non-vacuity, run rather than assumed. Reverting only
response-scrub.tstodc2bfaa4with the new tests in place fails 14 of them. The other 12 are deliberate controls that must pass in both states — the over-redaction guards, a comment at or deeper than the block indent, a tab inside a value, a real sibling key still ending the block, and JSON-path parity. A test that only passed in the fixed state would not distinguish the fix from the widening.Measured at
dc2bfaa4, before this change — an adversarial sweep for other routes to the same material, which is the method PEN-2370 ask 3 names as the control:env:block)value:/"value":keyKEY=VALUE/ flow-mapping entryargs:block12 of 12 leaked before, 0 of 12 after. The JSON path was unaffected in both states, which is why it is carried as a control rather than a result.
Over-redaction pinned from the other side. On a pod interrupted by both a column-0 comment and a tab line, the env value is redacted while
name,image,ports,resources,livenessProbe,status.phaseandrestartCountall survive, and avalueFromsecretKeyRefreference survives intact.Fixtures use a synthetic
LEAKED_*marker and assert on its absence — a test asserting<redacted>is present would pass even with the plaintext printed alongside it.Risks
Low functional risk; the change is a widening of what stays inside an already-default-deny block.
leadingIndentreturns 0 for a tab-indented line, and the scanner uses depth for two different jobs — block membership and the wrapped-scalar swallow. Once the line was held in, that same bogus 0 became "drop every following line indented deeper than 0", which ateimage,ports,resourcesandlivenessProbeas far as the next column-0 key. HenceswallowFrom. Skipping the swallow is safe precisely here: inside an env block the default is already REDACT, so a no-longer-swallowed continuation still reaches the fail-closed arm and is redacted individually rather than dropped — noisier output, same containment. A dedicated test asserts exactly that.image:/ports:survive. Comments carry no material of their own, so holding them in costs nothing.value: "<redacted>"above the plaintext is worse than no scrubber.k8s-rotraffic dialskubernetes-mcp-server-readonly.paperclip.svcdirectly and bypasses this gateway, so the scrubber runs on zero bytes in production until the topology moves. PEN-2429 owns that rollout; it is cluster-admin and operator-only. Unchanged from security(mcp-gateway): scrub container env values from proxied MCP responses (PEN-2370) #1435 / security(mcp-gateway): redact container argv, and route argv bodies to the scanner (PEN-2431 door #5) #1449.Blockcast/onprem-k8sIssue creation silently ignores assigneeId field -- no validation error paperclipai/paperclip#2590. Neither closes ask 3 alone: ask 3 is satisfied when a fail-open of this class is structurally impossible, and this removes two instances plus the shared root cause that produced them. It is not a claim of completion.Model Used
Claude Opus 5 (
claude-opus-5), 1M context window, extended thinking enabled, with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateNo credential value, name-value pair, or fragment was read into this branch, its tests, or this description, and no pod was re-probed to produce it. Every fixture uses a synthetic
LEAKED_*marker; the fail-opens were confirmed by running the committed scrubber against synthetic input.