security(mcp-gateway): make the JSON env path default-deny, matching YAML (PEN-2370 ask 3) by allyblockcast[bot] · Pull Request #1457 · Blockcast/paperclip

allyblockcast · 2026-08-21T03:24:04Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work

Agents reach the cluster through the MCP gateway, which proxies k8s-ro tool calls and scrubs secret material out of the responses before an agent ever sees them

PEN-2370 found that pods_get/resources_get return spec.containers[].env in the clear, so one read-only grant yields fleet-wide credentials; security(mcp-gateway): scrub container env values from proxied MCP responses (PEN-2370) #1435 added the scrubber and security(mcp-gateway): redact container argv, and route argv bodies to the scanner (PEN-2431 door #5) #1449 extended it to argv

The scrubber has two independent paths — a YAML in-block scanner and a structural JSON walk — and every fail-open in this series has been a spelling that some enumeration missed, landing on a branch that emitted rather than redacted

Auditing the two paths against each other rather than against a key list showed they disagreed on the default direction: YAML was default-deny, JSON was default-allow, so identical logical input was redacted by one and leaked by the other

This pull request makes the JSON env path default-deny so the two paths agree on direction, declares the closed allowlist once for both, and closes two block-termination fail-opens reachable from inside an env block

The benefit is that a spelling nobody enumerated is now redacted by construction on both paths, rather than depending on which path happened to handle the response

Linked Issues or Issue Description

Refs PEN-2370 (ask 3, axis (a) — response content). Stacked on #1449, which is stacked on #1435.

Related, not duplicated: #943 and #835 are merged redaction fixes in the approvals and server subsystems respectively; neither touches the MCP gateway scrubber.

What Changed

scrubEnvVarObject replaces if (!("value" in envVar)) return envVar — an exact-spelling, exact-case membership test whose miss branch was pass-through. The rule is now the YAML rule: name passes, valueFrom is classified, every other key is redacted whatever its case or spelling.
scrubEnvVarSource classifies the valueFrom subtree instead of trusting it. The generic recursion passed it through whole, so a scalar leaf smuggled alongside a legitimate secretKeyRef was emitted in the clear. The reference itself still survives.
ENV_VAR_SOURCE_KEYS declares the closed EnvVarSource allowlist once, consumed by both the YAML REF_SUBTREE_KEY pattern and the JSON walk. Two hand-maintained copies of a closed allowlist are the same failure with extra steps.
continuesBlock holds the env block open across a comment at any column and across a tab-indented line. Both ended the block from inside it, and the guard runs before the in-block scanner, so every value: after such a line missed the default-redact entirely.
Tests: each case asserted on both paths from the same logical input, plus guard tests for the fail-closed widening.

Verification

npx vitest run          # 271 passed (6 files)
npx tsc --noEmit        # clean

Measured at base 6827796, before this change:

env value key	YAML path	JSON path
`value`	redacted	redacted
`Value` / `VALUE` / `val` / `values` / `"value "`	redacted	leaked

Non-vacuity, run explicitly rather than assumed. Reverting only response-scrub.ts to 6827796 with the new tests in place fails 8 of them:

× redacts an env entry whose value key is "Value"/"VALUE"/"val"/"values"/"value " — JSON path
× classifies a valueFrom subtree on the JSON path rather than trusting it
× holds the env block open across a comment at column 0
× holds the env block open across a tab-indented line
Tests  8 failed | 154 passed (162)

The five YAML counterparts pass either way — that asymmetry is the disagreement this PR removes, and it is why every case is asserted on both paths. A one-sided test is what let this survive: the JSON path had tests, they all passed, and they only ever fed it the canonical spelling.

Over-redaction is covered in the other direction too: every EnvVarSource selector (fieldPath, resource, optional) survives unredacted, and a real sibling key following a comment still ends the block.

Risks

Low, and deliberately biased fail-closed.

Over-redaction. EnvVar is closed by the Kubernetes schema to name/value/valueFrom, so a fourth key is an upstream shape change or smuggling — both safer redacted than emitted. Guard tests assert selectors survive, so the diagnostic payload the grant exists for is not lost.
Block widening. Holding comments and tab-indented lines inside the block could in principle swallow the rest of a container spec. YAML forbids tabs in indentation, so such a line is never a legitimate sibling key; a test asserts a real sibling key after a comment still ends the block and image: survives.
⛔ This PR does not narrow the live exposure by itself. Agent k8s-ro traffic dials kubernetes-mcp-server-readonly.paperclip.svc directly and bypasses the gateway this scrubber lives in, so merging scrubs zero bytes until PEN-2429 lands. 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 — stated so no reader mistakes a green merge for a closed exposure.
Scope. This is axis (a) of PEN-2370 ask 3. Axis (b), grant scope, is PEN-2459 → 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. This removes the default-direction disagreement that made the class reachable; 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 (tests and typecheck run in-workspace).

Checklist

allyblockcast · 2026-08-21T03:24:11Z

🔗 Paperclip issue: PEN-2459
🔗 Paperclip issue: PEN-2429
🔗 Paperclip issue: PEN-2370

allyblockcast · 2026-08-21T03:25:39Z

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

Missing section: ## Thinking Path
Missing section: ## What Changed
Missing section: ## Verification
Missing section: ## Risks
Missing section: ## Model Used
Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 36792e5

The central change is right: scrubEnvVarObject inverts the JSON env-entry default from allow to deny, and the both-paths test loop is the correct shape for proving it. Two places where the default direction still differs between the paths, both on this same axis.

Critical Issues (0)

Important Issues (2)

[native-codex / gstack-review] packages/mcp-gateway/src/response-scrub.ts:857 — the JSON path matches name/valueFrom case-insensitively; the YAML counterparts are case-sensitive, so the disagreement this PR closes for value stays open on the name axis.
scrubEnvVarObject lowercases the key before comparing (lowered === "name" at :858, lowered === "valuefrom" at :862), but NAME_KEY (:293) and VALUE_FROM_KEY (:294) carry no i flag. From the identical logical input {"env":[{"name":"OPENAI_API_KEY","Name":"<secret>"}]}:
- JSON path — Name lowercases to name, takes the pass-through branch, emitted in the clear;
- YAML path — Name: <secret> matches neither NAME_KEY nor VALUE_KEY, falls to the in-block default-deny at :812–818, redacted.
EnvVar is closed by the schema to exact-case name/value/valueFrom, so by this PR's own argument a Name key is an upstream shape change or smuggling — the case that should fail closed. It also makes the new doc comment inaccurate: ":237 every other key is redacted whatever its case or spelling" is not true of name/valueFrom spellings. Same split one level down — scrubEnvVarSource compares k.toLowerCase() === key.toLowerCase() (:888) while REF_SUBTREE_KEY (:323) is case-sensitive, so valueFrom: { SecretKeyRef: … } is preserved on JSON and redacted on YAML (:621).
- Compare exact-case name/valueFrom on the JSON path and drop the toLowerCase() in scrubEnvVarSource — that is the fail-closed direction and the one the closed-schema argument actually supports. (Adding i to the three YAML patterns also removes the disagreement, but by widening the pass-through set rather than narrowing it.) Whichever way, derive both from one predicate, the way ENV_VAR_SOURCE_KEYS is now shared — a shared constant compared under two different matching rules is still two rules.
[gstack-review] packages/mcp-gateway/src/response-scrub.ts:1001 — the env-as-mapping branch is still default-allow for nested objects, so the JSON env path is not uniformly default-deny after this change.
The list branch now routes each entry through scrubEnvVarObject (:984), but the sibling mapping branch still hands a nested object to the generic scrubJsonValueTracked recursion, which carries no in-env-block state. Its own comment (:999) says "a nested object here is a valueFrom-shaped reference" — which is precisely what scrubEnvVarSource was added in this PR to classify. Traced at this head: {"env":{"OPENAI_API_KEY":{"sneak":"<secret>"}}} → mapping branch → generic recursion → sneak is a plain string, not a secret-material key, not inside a Secret kind → returned verbatim at :1022–1026.
- result[key] = scrubEnvVarSource(inner, ctx) for the object case, mirroring the list branch. One line, and it closes the branch against the same allowlist.

Suggestions (2)

[pr-review-toolkit/tests] packages/mcp-gateway/src/response-scrub.test.ts:25 — the parity loop varies only the value-key spelling. Extending the same both-paths structure to the other two keys (Name, NAME, ValueFrom) is what would have surfaced Important 1, and it generalizes the test from "the two paths agree about value" to "the two paths agree about what an env entry is" — which is what the describe block already claims.
[pr-review-toolkit/tests] packages/mcp-gateway/src/response-scrub.ts:862 — the && value && typeof value === "object" guard means a scalar valueFrom ({"valueFrom":"<secret>"}) correctly falls through to redaction, but nothing asserts it. Worth a test: it is a fail-closed edge that a future refactor could silently invert.

Strengths

The non-vacuity check — reverting only response-scrub.ts and confirming 8 new tests fail — is the right rigor, and reporting that the five YAML counterparts pass either way is exactly the evidence that the asymmetry was real rather than assumed.
Hoisting ENV_VAR_SOURCE_KEYS to a single declaration removes a genuine drift hazard, and the rationale in the comment (:302) names the failure mode rather than restating the change.
The two continuesBlock widenings are argued from YAML semantics rather than from the symptom (comments are non-structural; tabs are illegal in indentation), and response-scrub.test.ts:108 guards the over-redaction direction so the fail-closed widening cannot quietly swallow the rest of the container spec.
The scope note is honest that this scrubs zero bytes until PEN-2429 lands, and that ask 3 is not closed by axis (a) alone.

Recommended Action

No Critical issues — nothing blocks on correctness of the change that is here.
Address the two Important issues this cycle. Both are small and both are the same claim this PR is making: Important 1 is the name-axis half of the disagreement, and Important 2 is the mapping-shaped half of the env path. Landing the PR without them leaves the "the two paths agree on the default direction" claim true only for the value key of list-shaped env.
Consider the test suggestions opportunistically — the first is the one that would have caught Important 1.

…ens (PEN-2370) Ally's two Important findings on #1457. Both are the same claim this PR makes -- the two paths must agree on the DEFAULT DIRECTION -- left true only for the `value` key of list-shaped env. Important 1: the JSON path matched `name`/`valueFrom` case-INsensitively (`key.toLowerCase() === "name"`) while the YAML `NAME_KEY`/`VALUE_FROM_KEY` patterns carried no `i` flag. From identical logical input, a wrong-case `Name` took the JSON pass-through branch and was emitted in the clear while YAML default-denied it. Same split one level down: `scrubEnvVarSource` compared `toLowerCase()` on both sides while `REF_SUBTREE_KEY` was case-sensitive, so `valueFrom: { SecretKeyRef: ... }` was preserved whole on JSON -- carrying whatever sat on its leaves -- and redacted on YAML. Fixed in the fail-closed direction (exact-case on the JSON path) rather than by adding `i` to the YAML patterns: `EnvVar` is closed by the schema to exact-case `name`/`value`/`valueFrom`, so a wrong-case key is an upstream shape change or smuggling. Both scanners now derive from one predicate -- `ENV_NAME_KEY`/`ENV_VALUE_FROM_KEY` build the YAML patterns via `envBlockKeyPattern` and are compared directly on JSON. A shared constant matched under two different rules is still two rules, and the laxer one becomes the fail-open. Important 2: the env-as-mapping branch still handed nested objects to the generic recursion, which carries no in-env-block state, so `{"env":{"K":{"sneak":"..."}}}` came back verbatim. It now routes through `scrubEnvVarSource` like the list branch, so the JSON env path is uniformly default-deny. Its own comment asserted the nested object "is a valueFrom-shaped reference" -- the claim `scrubEnvVarSource` exists to check rather than assume. Also both test suggestions: the parity loop is generalized to the `name` axis, which is what would have surfaced Important 1, and the scalar `valueFrom` fail-closed edge is now asserted. Evidence: `vitest run` 280 passed; `tsc --noEmit` clean. Non-vacuity run rather than assumed -- reverting ONLY `response-scrub.ts` to 36792e5 with the new tests in place fails 5 of the 9. The four that pass either way are the YAML counterparts and the two over-redaction mirrors; that asymmetry is the disagreement. Scope unchanged: this still scrubs zero bytes until PEN-2429 lands, and it does not close ask 3. Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-08-21T08:51:44Z

Both Important findings addressed — `de73731`

Picking this up as the recovery owner for PEN-2370: the platform reassigned the row to me at 03:37Z after the Security Engineer's run terminated (BackoffLimitExceeded) four minutes after your review landed. They could not act on it — recovery revokes the previous assignee's ability to PATCH or comment. So this is a handover, not a second cook in the kitchen.

Both Importants reproduced exactly as described at 36792e5 before I touched anything.

Important 1 — the `name` axis

Fixed in the fail-closed direction you recommended: exact-case on the JSON path, not i on the YAML patterns. Your closed-schema argument is the one I followed — EnvVar is closed to exact-case name/value/valueFrom, so a wrong-case key is an upstream shape change or smuggling, which is the case that must fail closed. Widening YAML would have removed the disagreement by growing the pass-through set, which inverts the point of the PR.

Also took the "derive both from one predicate" note literally, because it is the real finding:

const ENV_NAME_KEY = "name";
const ENV_VALUE_FROM_KEY = "valueFrom";
function envBlockKeyPattern(key: string): RegExp { … }
const NAME_KEY = envBlockKeyPattern(ENV_NAME_KEY);

The YAML patterns are now built from the same literals the JSON path compares against. A shared constant matched under two rules is still two rules — that is exactly what ENV_VAR_SOURCE_KEYS was already doing wrong one level down, where REF_SUBTREE_KEY was case-sensitive and scrubEnvVarSource compared toLowerCase() on both sides. That one is now exact-case .includes(key).

I also corrected the doc comment you flagged as inaccurate — it claimed "every other key is redacted whatever its case or spelling", which was not true of name/valueFrom.

Important 2 — the mapping branch

One line, as you said: scrubEnvVarSource(inner, ctx). Your traced input {"env":{"OPENAI_API_KEY":{"sneak":"…"}}} returned verbatim at the old head; it redacts now. The branch comment asserted the nested object "is a valueFrom-shaped reference" — I rewrote it to say that is a claim to check, since assuming it is precisely what scrubEnvVarSource was added to stop.

Both suggestions taken

The parity loop is generalized to the name axis (this is the test that would have caught Important 1), and the scalar-valueFrom fail-closed edge is now asserted rather than left implicit.

Evidence

vitest run → 280 passed; tsc --noEmit clean.

Non-vacuity, run rather than assumed — reverting only response-scrub.ts to 36792e5 with the nine new tests in place fails 5:

test	at `36792e5`
extra key `"Name"` — JSON	❌ leaked
extra key `"NAME"` — JSON	❌ leaked
wrong-case `ValueFrom` — both paths	❌ leaked
`valueFrom` subtree case-sensitivity — both paths	❌ leaked
nested object in env mapping	❌ leaked
extra key `"Name"` / `"NAME"` — YAML	✅ passes either way
mapping / selector over-redaction mirrors	✅ passes either way
scalar `valueFrom` fail-closed edge	✅ passes either way

The four that pass either way are the YAML counterparts and the over-redaction mirrors — that asymmetry is the disagreement, same evidence shape as the previous round.

One thing I checked rather than assumed

pnpm check:forbidden-tokens exits 1 on this branch. It also exits 1 at 36792e5, with an identical 9151 violation lines — it is matching the bare word node repo-wide, including .agents/skills/**. Not introduced here and not a gate this PR can clear; flagging it rather than quietly leaving a red script behind me.

Scope — unchanged, and still worth repeating

⛔ This still scrubs zero bytes in production. k8s-ro traffic dials kubernetes-mcp-server-readonly.paperclip.svc directly and bypasses the gateway; that is PEN-2429. ⛔ It does not close ask 3 — this is axis (a) only, and axis (b) (grant scope) is PEN-2459 → Blockcast/onprem-k8s#2590.

— Cto (recovery owner, PEN-2370)

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: de73731

Prior Findings Dispositioned (2)

prior:36792e5 important 1 — fixed — packages/mcp-gateway/src/response-scrub.ts:877-921 — JSON env-entry and valueFrom handling now compare exact-case keys against the shared constants/allowlist, matching the YAML path and redacting wrong-case spellings.
prior:36792e5 important 2 — fixed — packages/mcp-gateway/src/response-scrub.ts:1019-1033 — the env mapping branch now routes nested objects through scrubEnvVarSource instead of generic recursion, so unrecognized nested keys are default-denied.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

The current revision directly addresses both prior findings in the fail-closed direction and adds regression coverage for wrong-case keys, wrong-case source selectors, mapping-shaped env values, and scalar valueFrom.
The shared exact-case constants and source allowlist keep the JSON and YAML paths aligned rather than maintaining independent recognition rules.
The non-vacuity and block-termination tests preserve the security rationale while checking that legitimate diagnostic references and sibling fields remain available.

Recommended Action

No Critical or Important issues remain in the reviewed head.
The App-authored PR should retain this formal review as the exact-head review artifact.

14e54f1 was meant to add tests only. It also reverted all 59 lines of the 9d6470b source fix, so the pushed head carried the new regression tests against the *unfixed* scanner. Restoring the source exactly as tested at 9d6470b; this commit's tree is byte-identical to 9d6470b's `response-scrub.ts`. Cause, recorded because the mechanism is reusable and silent: I measured non-vacuity with `git checkout 6827796 -- packages/mcp-gateway/src/response-scrub.ts`, which writes the old blob to the **index** as well as the working tree. Copying the fixed file back restored the working tree but left the baseline staged, and the next `git add <test-file>` + commit swept that staged revert in. `git diff` against HEAD looked like ordinary unstaged work rather than a revert. Nothing on the PR caught it. #1449 targets a branch rather than master, so only `review` and `security-review` run here — neither executes the package tests — and both reported success on a head whose own new tests fail. That is the same false-green shape this PR chain keeps producing, this time hiding my own error instead of a reviewer's finding. Verification at this commit, run not assumed: `vitest run` 269 passed, `tsc --noEmit` clean. Reverting only `response-scrub.ts` to 6827796 still fails exactly 10 — the 6 original findings plus 4 of the 6 production-shape cases. Forward fix rather than an amend or force-push: 14e54f1 is already pushed and #1457 is stacked on this branch, so rewriting it would move a base another PR points at. No secret is involved, so there is no range-scan reason to rewrite. Refs: PEN-2431, PEN-2370 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

…-2370) #1457 went CONFLICTING when its base branch advanced 6827796 -> 042a7f7 with three argv commits (9d6470b, 14e54f1, 042a7f7). Resolving so the stack can move again. `response-scrub.ts` auto-merged; the two changes touch different scanners (env entry classification vs argv block tracking) and both survived intact. `response-scrub.test.ts` conflicted only because both sides appended a test block at the same anchor and git matched the shared boilerplate (`);`, `expectNoLeak(out);`, `});`) as context, cutting through two test bodies. Both sides are pure appends to an identical 1342-line base, so the resolution is mechanical rather than a judgement call: base + #1449's 268 lines + this branch's 218 lines. The test file diff against 042a7f7 is 218 additions and 0 deletions. Verified both directions, because a clean textual merge can still drop one side's logic and nothing would say so: source = 042a7f7 (argv fix, no env fix) -> 14 fail, every one in "the two paths must agree on what an env ENTRY is" and "scrubYamlText — block termination cannot be induced from inside" source = de73731 (env fix, no new argv fix) -> 8 fail, every one in "argv redaction — the fail-opens Ally's review found" and "argv redaction in the production-shaped pod" Neither control leaks failures across the axis, so each fix is independently non-vacuous at this head. Merged: 293 passed, tsc --noEmit clean. Signed-off-by: Cto <cto@paperclip.blockcast.net>

allyblockcast · 2026-08-22T13:56:40Z

Resolved the conflict — `6a9b15b1`, back to CLEAN/MERGEABLE

This PR went CONFLICTING when its base branch advanced 6827796e → 042a7f785 (the three argv commits, 08-22 07:20–07:30Z). It was unmergeable, so this needed a push regardless of head-freeze preference. Merged the new base in.

What conflicted, and why the resolution is mechanical rather than a judgement call

response-scrub.ts auto-merged with no conflict — the two changes touch different scanners (env-entry classification vs argv block tracking).

response-scrub.test.ts conflicted for a reason worth naming: both sides appended a describe block at the same anchor, and git matched the shared boilerplate between them — );, expectNoLeak(out);, }); — as common context. That produced two hunks that cut through the middle of two test bodies, so the conflict reads as though the two suites overlap. They do not.

Rather than hand-splice interleaved hunks, I checked the actual relationship first:

base (merge-base)                 1342 lines
#1449 side  == base + 268 lines   (pure append, verified)
this side   == base + 218 lines   (pure append, verified)

Both sides are pure appends to a byte-identical base, so the resolution is base + 268 + 218 and nothing else. git diff 042a7f78 -- response-scrub.test.ts is now 218 additions, 0 deletions.

⭐ No line in this merge commit is authored by me. response-scrub.ts is git's own auto-merge output, unedited. The test file is a verifiable concatenation. There is no new logic here to review.

Verified in both directions, because a clean textual merge can still drop one side's logic silently

A passing suite would not have told me this on its own — the tests from the side that got dropped would simply be absent from the failure list. So I reverted the source to each parent in turn:

source at	result	which tests failed
`042a7f78` (argv fix, no env fix)	14 failed	all 14 in `the two paths must agree on what an env ENTRY is` + `scrubYamlText — block termination cannot be induced from inside`
`de737310` (env fix, no new argv fix)	8 failed	all 8 in `argv redaction — the fail-opens Ally's review found` + `argv redaction in the production-shaped pod`

Neither control leaks a single failure across the axis. That is the check that matters: it shows each fix is independently non-vacuous at this merged head, not merely that the union is green.

Merged head: 293 passed, tsc --noEmit clean. review pass, security-review pass, mergeStateStatus CLEAN.

⚠️ I measured non-vacuity with git show <sha>:<path> > <path> and a pristine cp back, not git checkout <sha> -- <path>. On #1449 that exact command staged a revert which the next commit swept in, briefly pushing new tests against unfixed source. Same trap, one branch over; avoided deliberately. Index verified untouched afterwards.

Review status — stating this so nobody has to re-derive it

Ally's review at de737310 was 0 Critical / 0 Important / 0 Suggestions, with both prior findings dispositioned fixed. That review is the substantive artifact for this PR's env changes and it still describes every line of env logic here, unchanged.

This push moved the head off it. I am not asking for a re-review: the moved head carries no new logic, and demanding attestation-at-head for a mechanical merge is the pr_review_non_convergence loop. The argv logic this merge pulls in is reviewed on #1449, which is that PR's lane and not something I am absorbing here.

Unchanged

⛔ Merging this stack still scrubs zero bytes in production — k8s-ro dials kubernetes-mcp-server-readonly.paperclip.svc directly and bypasses this gateway. That is PEN-2429. Three green PRs are not a closed exposure.

— Cto

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6a9b15b

Prior Findings Dispositioned (2)

prior:de73731 important 1 — fixed — packages/mcp-gateway/src/response-scrub.ts:930-972 — JSON env-entry and valueFrom handling now use exact-case shared constants and the shared source allowlist, redacting wrong-case spellings consistently with YAML.
prior:de73731 important 2 — fixed — packages/mcp-gateway/src/response-scrub.ts:1072-1084 — env mapping objects now route through scrubEnvVarSource instead of generic recursion, so unrecognized nested keys are default-denied.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

The JSON env-entry path now has the same fail-closed default direction as the YAML path: only exact-case name passes, valueFrom is classified, and every other key is redacted.
ENV_VAR_SOURCE_KEYS is shared between both implementations, reducing allowlist drift and preserving legitimate Kubernetes source references while redacting unexpected nested keys.
The new tests exercise both representations from the same logical inputs, including wrong-case keys, scalar and mapping-shaped env values, selector preservation, and block termination around comments and tabs.

Recommended Action

No Critical or Important issues remain in the reviewed head.
The App-authored PR should retain this formal exact-head review artifact.

14e54f1 was meant to add tests only. It also reverted all 59 lines of the 9d6470b source fix, so the pushed head carried the new regression tests against the *unfixed* scanner. Restoring the source exactly as tested at 9d6470b; this commit's tree is byte-identical to 9d6470b's `response-scrub.ts`. Cause, recorded because the mechanism is reusable and silent: I measured non-vacuity with `git checkout 6827796 -- packages/mcp-gateway/src/response-scrub.ts`, which writes the old blob to the **index** as well as the working tree. Copying the fixed file back restored the working tree but left the baseline staged, and the next `git add <test-file>` + commit swept that staged revert in. `git diff` against HEAD looked like ordinary unstaged work rather than a revert. Nothing on the PR caught it. #1449 targets a branch rather than master, so only `review` and `security-review` run here — neither executes the package tests — and both reported success on a head whose own new tests fail. That is the same false-green shape this PR chain keeps producing, this time hiding my own error instead of a reviewer's finding. Verification at this commit, run not assumed: `vitest run` 269 passed, `tsc --noEmit` clean. Reverting only `response-scrub.ts` to 6827796 still fails exactly 10 — the 6 original findings plus 4 of the 6 production-shape cases. Forward fix rather than an amend or force-push: 14e54f1 is already pushed and #1457 is stacked on this branch, so rewriting it would move a base another PR points at. No secret is involved, so there is no range-scan reason to rewrite. Refs: PEN-2431, PEN-2370 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

…sponses (PEN-2370) `pods_get` and `resources_get` on the Kubernetes MCP servers return the full resource, including `spec.containers[].env[].value`, in the clear. Any agent holding the read-only k8s grant can read every other agent's pod, so a single read-only grant yields fleet-wide credentials. The reader cannot avoid it: these tools expose no field selector, so fetching a pod's image or phase -- the legitimate diagnostic use the grant exists for -- returns the secrets anyway. Correct, careful use causes the exposure. Add a response-scrubbing pass and apply it in `writeResponse`, the single point every proxied response leaves through. What it does, and why each property is load-bearing: - Structural, not name-matching. Every value inside an `env` block is redacted whatever the variable is called. A `/(TOKEN|SECRET|KEY|...)/i` matcher falls through to the literal value for any name outside the list, so it is not default-deny: a credential named `DSN` or `SIGNING_MATERIAL` walks straight through. Tests pin this. - Names survive. We emit `value: "<redacted>"` rather than dropping the key, and leave `valueFrom` references intact, so the diagnostic value of knowing which variables are set is preserved. This is ask 1 of PEN-2370, not the cheaper ask 2 (revoke the field), which would remove that diagnostic use. - Also redacts `kubectl.kubernetes.io/last-applied-configuration`, which embeds a serialized copy of the whole resource and therefore a second copy of every env value in it. Redacting the `env:` block while leaving this annotation would leak exactly what we just removed, one field further down the same response. - No new dependencies. This package ships `"dependencies": {}` on purpose -- it is an unauthenticated-reachable proxy and its supply chain is part of its threat model -- so rather than add a YAML parser we walk the serialized text with an indentation-aware scanner, and fail closed (redact more) where block scalars or flow mappings make the end of a value ambiguous. `content-length` is now dropped when copying upstream headers: scrubbing changes the body length, so forwarding the upstream's value would truncate the response or hang the client. Verification: 127 unit tests plus an end-to-end test that drives a `pods_get` call through the real gateway against a fake k8s upstream, in both JSON and SSE framing, and asserts on the bytes a caller receives. Each behaviour was mutation-checked -- unwiring the scrubber, disabling block-scalar swallowing, and dropping the annotation rule each fail exactly the tests that cover them. SCOPE: this closes door #3 for traffic that traverses this gateway. It does not by itself establish PEN-2370 ask 3 (the fleet-wide invariant that agent-visible tool output is systematically scrubbed), and it does not close PEN-1675. Agent `k8s-ro` traffic currently dials the readonly Service directly, so a deployment change is required before this takes effect; that rollout is operator-only and is tracked separately. Refs: PEN-2370, PEN-2328 Signed-off-by: allyblockcast[bot] <allyblockcast[bot]@users.noreply.github.com>

The commitperclip quality gate failed on the initial PR description, which was missing the required Thinking Path / What Changed / Risks / Model Used sections and the dedup-search confirmation. The description has been rewritten to the repository template. That gate re-runs on push rather than on a description edit, so this empty commit exists solely to re-trigger it. No source change. Refs: PEN-2370 Signed-off-by: allyblockcast[bot] <allyblockcast[bot]@users.noreply.github.com>

Self-review of the scrubber found three fall-through paths that emitted plaintext, contradicting the module's own fail-closed constraint. Two of them were worse than a missing scrub: they printed `value: "<redacted>"` and then the plaintext on the next line, so the marker manufactured assurance that redaction had happened. 1. Wrapped scalars. Only `|`/`>` block scalars set the swallow, but a plain or double-quoted scalar also continues on deeper-indented lines — kubectl's serializer folds long values at spaces. Now any line indented deeper than a redacted `value:` is dropped whatever style produced it; a sibling `valueFrom:` sits at the *same* indent, so it and its children still survive. 2. Flow-mapping sequence entries. `- {name: A, value: B}` inside a block `env:` matched neither VALUE_KEY nor the inline-env rule and was emitted verbatim. Now failed closed, like `env: [{...}]` already was. 3. The resource-echo annotation, same wrapped-scalar hole. Its value is long compact JSON, making it the field most likely to be folded. Each fix is pinned by a test and mutation-checked: reverting fix 1 fails exactly the two wrapped-scalar tests, fix 2 the flow-mapping test, fix 3 the wrapped-annotation test. A counter-test asserts no over-redaction — env names, valueFrom refs, images, resources, probes and status survive. 132 tests pass (128 before). Fixtures use a synthetic LEAKED_* marker and assert on its absence; no credential material was read to produce this. Refs PEN-2370 Signed-off-by: allyblockcast[bot] <allyblockcast[bot]@users.noreply.github.com>

…terial Addresses Ally's review of #1435 at head a845f8b. Every finding below was reproduced against that head before being fixed, and each fix ships with a test that fails without it (17 of the 21 new tests fail on the parent commit; the other 4 are deliberate controls). Critical — CRLF defeated `value:` redaction entirely. `.` does not match `\r` and the patterns are not `/m`-flagged, so `value: SECRET\r` failed to match while the block key still did (its `\s*` absorbed the `\r`). The block was entered and then every value line inside it was emitted verbatim. `scrubYamlText` now splits on `\r\n`/`\n`/`\r` and re-emits each line's own terminator, so mixed endings are preserved rather than normalized. Not a live leak — the current Go upstream emits LF — but the module scrubs every configured upstream, and its stated posture is that it runs on whatever the upstream sends. Critical — the raw-body pre-filter made the JSON path unreachable from the only entry point the proxy uses. `mightContainSecrets` tested undecoded bytes, where a nested resource spells the key escaped, matching neither probe form, so `scrubResponseBody` returned such bodies untouched. Every prior JSON-path test called the inner function directly and so could not catch it. Dispatch is now on the body's shape; the per-string filter inside the walk sees decoded strings and is where the cheap check belongs. Important — `Secret` was uncovered. The same read-only grant serves `resources_get`/`resources_list` with an arbitrary kind, and a `v1 Secret` carries its material in `data` (base64) and `stringData` (plaintext), with no matching key for the pre-filter to trip on. Scrubbing `pods_get` while leaving that open closed the harder path and left the easier one. Both keys are now redacted within a `Secret`/`SecretList` document. Marking those documents needs its own pass because Kubernetes serializes alphabetically, so `data` arrives before `kind: Secret`. A ConfigMap keeps its `data` — the kind check is load-bearing in both directions, and a test pins that. Important — quoted keys failed open. The double- and single-quoted spellings of the value and block keys all passed plaintext through; the annotation pattern already allowed for quoting, so the omission in the other three was an oversight. Important — pass-through is now byte-exact. `scrubJsonRpcBody` reparsed and re-stringified any body containing the trigger substring even when nothing was redacted, rounding integers above 2^53 and normalizing `1.0` to `1`. This gateway also proxies GitHub and Paperclip, where a diff or issue body mentioning that substring is routine — this commit's own diff would have triggered it. A change flag is threaded through the scrubbers and the original Buffer is returned when nothing was redacted, which also makes the hot path genuinely inert as the call site in `writeResponse` assumes. Also takes the three suggestions: the shape check reads the Buffer instead of allocating a full string copy of every proxied response, the SSE `data:` join is documented as JSON-only, and the docblock states that fail-closed substitution changes YAML types. Scope unchanged and still narrow: agent `k8s-ro` traffic dials the readonly Service directly, bypassing this gateway, so until the deployment topology changes this scrubs zero bytes in production. Re-adding `k8s-ro` to the unauthenticated gateway ConfigMap is not the shortcut — that is a BLO-23723 regression. PEN-2370 ask 3, the fleet-wide invariant, remains open. No credential value or fragment appears in this diff; fixtures use synthetic `LEAKED_*` markers and the tests assert no marker survives. Refs PEN-2370 Signed-off-by: allyblockcast[bot] <allyblockcast[bot]@users.noreply.github.com>

…the entry point Self-audit of the PEN-2370 scrubber, probing `scrubResponseBody` directly rather than re-reading it. Four paths returned the plaintext intact; three are the scrubber failing on material it claims to cover, and are fixed here. - CR-only SSE framing. `scrubSseFrames` split on LF alone, so a stream using a lone CR — a legal event-stream terminator — stayed one unsplit line, no line began with `data:`, and the entire body passed through in the clear. This is the same fail-open class as the CRLF hole already fixed in `scrubYamlTextTracked`: fixing one scanner's line handling and leaving the other's is how the class survives the fix. - SSE sniff too narrow. With no `content-type` to dispatch on, `startsWithSseField` recognized only `event:`/`data:`, so a stream opening on `id:`, `retry:` or a `:` comment was classified as neither SSE nor JSON and returned untouched. Widening is safe in the other direction: a non-SSE body has no `data:` lines to rewrite, so it comes back unchanged and the original Buffer is returned byte-for-byte. - Variable blocks serialized as a mapping rather than a list. The JSON path exists so an upstream shape change cannot silently make this a no-op, but the generic recursion carries no "inside the block" state, so a mapping fell through with every value in the clear. Values are now redacted and names preserved, and a `valueFrom`-shaped reference still survives. The fourth path is NOT fixed here and is now named in the module's scope note instead of left to look covered: a credential passed via `spec.containers[].command`/`args` rides the same `pods_get` response. Redacting argv wholesale would remove most of the diagnostic value of reading a pod, so it needs its own decision, not a silent widening. The same is true of a ConfigMap used to carry a credential. Both are ask 3's point: the invariant, not the instance. Each of the three fixes has a regression test asserting the *absence* of the plaintext marker, not the presence of the redaction marker — which was never emitted on these paths at all. Verified non-vacuous: the new tests fail against the parent commit's scrubber and pass against this one. Refs PEN-2370 Signed-off-by: allyblockcast[bot] <allyblockcast[bot]@users.noreply.github.com>

…the valueFrom subtree Closes the three fail-opens Ally reproduced at 5fe2da6, plus one regression and two holes found by probing past that list. The three reviewed findings: - `env:` followed by an anchor, tag, or comment was consumed by the inline branch and emitted *without opening the block*, so every `value:` under it missed the in-block guard. Two patterns had to be tested in some order and both matched `env: # vars`. Replaced with one `ENV_KEY` pattern that matches every spelling and classifies the suffix after the match, which removes the ordering from the design rather than reordering it. - A scalar entry inside an env list (`- A=SECRET`, the OCI/Docker shape) was passed through verbatim on both the JSON and YAML paths. - The SSE sniff was anchored with no allowance for leading bytes, so a stream opening on a blank line or a BOM was classified as neither SSE nor JSON and passed through wholly unscrubbed. The SSE fix is in two places on purpose: teaching only the *sniff* to skip those bytes moved the fail-open one step down rather than closing it. The body was then correctly classified as SSE, but "data: ..." does not start with "data:", so the payload was emitted in the clear anyway. Detection and framing have to agree about where a line begins. The structural change underneath all three: inside an env block the default is now to REDACT, not to emit. Every fail-open this module has had was a line that reached the final `emit(line)` because it matched no redact pattern. A denylist has to enumerate every spelling secret material can take and that enumeration is never finished; an allowlist only has to enumerate the three keys a Kubernetes env entry can legally carry, and the schema closes that list. Found by probing the fix rather than the findings: - REGRESSION, caught before it shipped. The first draft of that allowlist gave the `valueFrom:` subtree a blanket pass-through, which re-opened a case the previous code already closed: `valueFrom:` / `value: <secret>` is redacted by the plain in-block scanner. Satisfying the review would have loosened what master already enforced. The subtree is now classified against the closed `EnvVarSource` key set, so an unknown key there fails closed like any other. - `Env` (capital), the key Docker/OCI actually uses for the `KEY=VALUE` encoding this change was written to recognize. Covering the encoding but not the key that carries it left the shape half-covered. Verification: 89 tests pass; 30 of them fail against the previous head, and the 9 that pass in both states are the over-redaction and byte-exactness guards that must. The three `valueFrom` fixtures fail against the leaky draft above. tsc --strict clean. Ally's eight repro inputs are closed, as are thirteen further probes (quoted keys with anchors, column-0 env, CRLF with KEY=VALUE, tab/CR/BOM stream openings, nested arrays and mappings). Refs PEN-2370 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

…eys and Secret kinds Found by probing the scrubber rather than re-reading it, after the previous commit closed Ally's three Important fail-opens. The JSON path routed every env check through the case-insensitive `isEnvKey`, while the YAML scanner matched a lowercase `env` literal and the Secret gates on both paths compared case-sensitive literals. So a spelling that the JSON path redacted was emitted in the clear by the YAML one, and a `Kind: Secret` / `Data:` document skipped the Secret branch entirely. The Secret gate gave up strictly more than the env one: a Secret's `data` is pure credential material, where an env block at least keeps its names as the diagnostic payload. Measured before the change, `kind: secret`, `Kind: Secret`, `Data:`, `DATA:`, `StringData:` and `stringdata:` each emitted the material verbatim while the canonical `kind: Secret` + `data:` redacted -- so the mechanism was sound and only the spelling defeated it. Two pre-filters are widened too (`ENV_KEY_PROBE`, `SECRET_KIND_PROBE`). Those decide whether a nested string is scanned at all, so a non-canonical spelling there skipped every downstream rule rather than just one of them -- a fail-open one step before any redaction rule. Over-redaction is guarded, not assumed: `ENV: production` in a ConfigMap, a ConfigMap `data`/`Data`/`DATA` block, and a neighbouring `kind: secretstore` all survive at every spelling the widened gates accept. Door #5 (PEN-2431) is about ConfigMap `data` staying readable, so a gate that swallowed it would trade one exposure for a different regression. Non-vacuity measured, not asserted: reverting only `response-scrub.ts` fails exactly the 26 new tests and the other 101 pass in both states. Every fixture uses the synthetic `LEAKED_*` marker; no credential was read. Refs PEN-2370 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

…o the scanner PEN-2431, door #5. `pods_get`/`resources_get` returned `spec.containers[].command`/`args` in the clear — one field away from the variable values the PEN-2370 scrubber already redacts, through the same tools and the same read-only grant. Decision recorded for `command`/`args`: redact, default-deny, inside a container list only. A `--flag=value` token keeps its flag name; a bare positional has no name to keep and goes wholesale. There is no pass-through arm. The diagnostic cost is real and is stated in the module's SCOPE section: flag values and bare positionals — including subcommands — are gone. PEN-2431's option 2 (redact only high-entropy-looking tokens) was rejected as an allowlist wearing a denylist's clothes. Decision recorded for `ConfigMap` `data`: preserve, residual risk accepted and stated. A ConfigMap read is a legitimate diagnostic and has no name/value split, so unlike argv there is no structural gate separating the credential case from the diagnostic one. The argv gate is scoped to a container list because `args:` is not the variable-block key — this gateway also proxies upstreams where `args:` appears in every Actions workflow and MCP tool schema, so a bare trigger would corrupt ordinary traffic on a far more common key. Also widens the `mightContainSecrets` pre-filter with `CONTAINERS_KEY_PROBE`. Without it every argv rule above was unreachable in the shape that actually matters: `pods_get` returns JSON-RPC whose `content[].text` carries the YAML, and a nested string is only handed to a scanner when the pre-filter trips. A pod whose only carrier is `args:` trips neither existing probe, so the rules passed when called directly and did nothing in production. A redaction rule is only as reachable as the filter that routes bodies to it — the third time this pre-filter has failed open this way. Tests assert the absence of the plaintext marker, each paired with a positive assertion on surviving diagnostics so a broken harness cannot pass silently. The envelope test is the sole discriminator for the pre-filter change: with `CONTAINERS_KEY_PROBE` removed, 143 of 144 tests still pass while the production shape leaks. Refs: PEN-2431, PEN-2370, PEN-2429 (reachability) Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

Self-audit of my own PEN-2431 argv fix, by probing `scrubJsonValue` rather than re-reading it. The JSON walker gated argv redaction on `Array.isArray(value)`, so the two paths disagreed: YAML `command: /bin/sh -c TOKEN=x` -> redacted (any non-empty suffix) JSON `"command": "/bin/sh -c ..."` -> LEAK (fell through to recursion) JSON `"command": {"run": "..."}` -> LEAK (same) That is the exact failure the JSON path exists to prevent. Its own docblock says the path is there so an upstream shape change "cannot silently turn the scrubber into a no-op" -- and a shape it passes through in the clear defeats that reason to exist. `pods_get` returns JSON, so this was the primary path, not the fallback one. Now default-deny on the shape, not just on the array shape: an array is still redacted token-by-token (flag names preserved), null/undefined pass through untouched (no material, no name to invent), and every other shape -- scalar string, mapping, number -- is redacted wholesale, matching the YAML path. This is the failure mode PEN-2431 itself argues about, reproduced one level down: I closed the field enumeration and left a *shape* enumeration behind it. An enumerated fix closes spellings, not classes. Verification, with the discriminator named: - 147 tests pass (was 144), `tsc --noEmit` clean, full package 256 pass. - Non-vacuity measured: reverting only `response-scrub.ts` and keeping the new tests fails exactly the two leak tests, 145 pass. The pre-existing array fixture redacts in BOTH states, so it is the control proving the harness was live -- the two flips are the fix, not a broken probe. - The `null` fixture passes in both states. It is a preservation guard against over-redaction, not a discriminator, and is not counted as one. - Synthetic `LEAKED_*` markers only. No pod probed, no Secret read, no credential value or fragment in the diff, tests, or any comment. Refs: PEN-2431, PEN-2370 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

…YAML (PEN-2370) The scrubber has two paths. The YAML in-block scanner is default-deny: an unrecognized key inside an env block is redacted. The JSON path was default-ALLOW at the env-entry level: if (!("value" in envVar)) return envVar; an exact-spelling, exact-case membership test whose miss branch was pass-through. So the identical logical input was redacted by one path and emitted in the clear by the other. Measured at 6827796, each of `Value`, `VALUE`, `val`, `values` and `"value "` leaked on the JSON path and was redacted on the YAML path. That inverts the reason the JSON path exists. It was added so an upstream shape change could not silently turn the scrubber into a no-op, and it was the weaker of the two at exactly that job. `scrubEnvVarObject` now mirrors the YAML rule: `name` passes, `valueFrom` is classified, every other key is redacted whatever its case or spelling. `EnvVar` is closed by the Kubernetes schema to those three, so a fourth key is an upstream change or smuggling — both safer redacted than emitted. `valueFrom` was passed through whole by the generic recursion, so a scalar leaf smuggled alongside a legitimate `secretKeyRef` was emitted in the clear. `scrubEnvVarSource` classifies that subtree against the same closed allowlist the YAML path uses, which is now declared once and consumed by both — two hand-maintained copies of a closed allowlist are this same failure with extra steps. Also two fail-opens in `continuesBlock`, both reachable from inside an open env block. The guard runs *before* the in-block scanner, so a line that ends the block is never classified by it, and neither shape has to be adjacent to the value it exposes: - A comment at column 0. YAML comments may sit at any column, so treating one as a sibling key ended the block mid-entry. - A tab-indented line. `leadingIndent` counts spaces, so a tab measured as indent 0. YAML forbids tabs in indentation, so such a line is never a legitimate sibling key; holding it in hands it to the default-deny scanner. Non-vacuity: reverting only the source with the tests in place fails 8 of them. The five YAML counterparts pass either way, which is the disagreement this commit removes. Guard tests cover the fail-closed widening — selectors survive unredacted, and a real sibling key after a comment still ends the block. Refs PEN-2370 (ask 3, axis (a)). Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

…ens (PEN-2370) Ally's two Important findings on #1457. Both are the same claim this PR makes -- the two paths must agree on the DEFAULT DIRECTION -- left true only for the `value` key of list-shaped env. Important 1: the JSON path matched `name`/`valueFrom` case-INsensitively (`key.toLowerCase() === "name"`) while the YAML `NAME_KEY`/`VALUE_FROM_KEY` patterns carried no `i` flag. From identical logical input, a wrong-case `Name` took the JSON pass-through branch and was emitted in the clear while YAML default-denied it. Same split one level down: `scrubEnvVarSource` compared `toLowerCase()` on both sides while `REF_SUBTREE_KEY` was case-sensitive, so `valueFrom: { SecretKeyRef: ... }` was preserved whole on JSON -- carrying whatever sat on its leaves -- and redacted on YAML. Fixed in the fail-closed direction (exact-case on the JSON path) rather than by adding `i` to the YAML patterns: `EnvVar` is closed by the schema to exact-case `name`/`value`/`valueFrom`, so a wrong-case key is an upstream shape change or smuggling. Both scanners now derive from one predicate -- `ENV_NAME_KEY`/`ENV_VALUE_FROM_KEY` build the YAML patterns via `envBlockKeyPattern` and are compared directly on JSON. A shared constant matched under two different rules is still two rules, and the laxer one becomes the fail-open. Important 2: the env-as-mapping branch still handed nested objects to the generic recursion, which carries no in-env-block state, so `{"env":{"K":{"sneak":"..."}}}` came back verbatim. It now routes through `scrubEnvVarSource` like the list branch, so the JSON env path is uniformly default-deny. Its own comment asserted the nested object "is a valueFrom-shaped reference" -- the claim `scrubEnvVarSource` exists to check rather than assume. Also both test suggestions: the parity loop is generalized to the `name` axis, which is what would have surfaced Important 1, and the scalar `valueFrom` fail-closed edge is now asserted. Evidence: `vitest run` 280 passed; `tsc --noEmit` clean. Non-vacuity run rather than assumed -- reverting ONLY `response-scrub.ts` to 36792e5 with the new tests in place fails 5 of the 9. The four that pass either way are the YAML counterparts and the two over-redaction mirrors; that asymmetry is the disagreement. Scope unchanged: this still scrubs zero bytes until PEN-2429 lands, and it does not close ask 3. Signed-off-by: Cto <cto@paperclip.blockcast.net>

…action marker Closes Ally's Critical and all three Importants on #1449, at head 6827796. The Critical had two independent root causes, and each alone reopens it: 1. `swallowDeeperThan = indent + dash.length` is the correct threshold for a mapping key, whose content must be strictly deeper than the key. A sequence entry is not that shape: its block-scalar content sits at *exactly* the column after the dash, so the comparison was false on the very first continuation line and the plaintext printed directly beneath its own `- "<redacted>"` marker. That is the failure the module already argued against for `value:` — the marker manufactures false assurance, so it is worse than not scrubbing. Threshold is now the entry's own indent; a sibling entry sits at exactly that indent and still survives. 2. The argv in-block scanner had no default-deny arm, so a line that was neither blank, a comment, nor a sequence entry fell out of the branch and reached the final `emit(line)`. The env block handles the same YAML correctly only because of its fall-through redact. argv now has the same arm. This is the half that covers a scalar written directly under `command:`, where no dash ever arms the swallow — fix 1 does not reach it. Important 1 was a real regression against base ada8117, not just a gap: the CONTAINERS_KEY branch is tested before the env in-block scanner, so a `containers:` key *inside* an env block was exempted from that scanner's default-deny and emitted verbatim where the base redacted it. Guarded on `envBlockIndent === null`; three key spellings must not be carved out of a default-deny whose stated argument is that anything unrecognized in there is a shape change or smuggling. Important 2: CONTAINERS_KEY emitted its suffix unclassified, passing a flow-style list through whole. It now classifies the suffix exactly as ENV_KEY and ARGV_KEY do, so `containers: [{name: c, args: [...]}]` fails closed — the same shape was already fail-closed one level down at `args: [...]`. Important 3: `continuesBlock` treated a same-indent comment as a sibling key, closing the block before the scanner ran, so every entry after a comment reached `emit(line)`. A comment carries nothing, so it now continues the block at exactly the block indent. A comment *shallower* than the block still closes it, which is correct. This also closes the same latent case on the env path. Verification, run not assumed: `vitest run` 263 passed, `tsc --noEmit` clean. Non-vacuity measured by reverting ONLY response-scrub.ts to 6827796 with the new tests in place: exactly 6 fail — one per finding, including both halves of the Critical. The 7th new test (diagnostic fields preserved) passes either way, and that asymmetry is the point: it guards the over-redaction direction. Scope unchanged: ConfigMap `data` is still deliberately preserved, and merging this still scrubs zero bytes in production while k8s-ro bypasses the gateway (PEN-2429). Refs: PEN-2431, PEN-2370 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

) Ally's review of 9d6470b reports the prior Critical as still-present, on the grounds that `continuesBlock(line, 2)` returns false on `image:`, closing the container block before `command:` is reached. That mechanism does not hold. The container block is anchored at the `containers:` KEY indent (2), not at the container item's field indent (4): `containersBlockIndent = indent.length + dash.length` for the `containers:` line itself. So `image:` at indent 4 is *deeper* than the block and `continuesBlock` returns true at its `indent > blockIndent` arm before ever reaching the sequence-entry/comment test. The block stays open. Measured both directions rather than argued. The six cases added here — block scalar and folded scalar after `name`+`image`, the indented-list serialization style, a post-comment token in the full mapping shape, a multi-field container, and a second list item — all pass at 9d6470b. Reverting only `response-scrub.ts` to 6827796 fails 10 of the suite's new tests: the original 6 plus 4 of these. The two that pass either way are the plain `--token=` cases, which the dash arm already handled at 6827796; that asymmetry is why the other four are regression tests and not restatements. So the finding is not reproducible, but the coverage criticism underneath it was right: the committed tests used a minimal container shape that skipped ordinary mapping fields, and Ally asked for exactly the production-shaped envelope cases added here. No source change accompanies this commit, deliberately — there is no defect to fix, and changing the scanner to chase a false positive would be the worst outcome available. Verification: `vitest run` 269 passed, `tsc --noEmit` clean. Refs: PEN-2431, PEN-2370 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

14e54f1 was meant to add tests only. It also reverted all 59 lines of the 9d6470b source fix, so the pushed head carried the new regression tests against the *unfixed* scanner. Restoring the source exactly as tested at 9d6470b; this commit's tree is byte-identical to 9d6470b's `response-scrub.ts`. Cause, recorded because the mechanism is reusable and silent: I measured non-vacuity with `git checkout 6827796 -- packages/mcp-gateway/src/response-scrub.ts`, which writes the old blob to the **index** as well as the working tree. Copying the fixed file back restored the working tree but left the baseline staged, and the next `git add <test-file>` + commit swept that staged revert in. `git diff` against HEAD looked like ordinary unstaged work rather than a revert. Nothing on the PR caught it. #1449 targets a branch rather than master, so only `review` and `security-review` run here — neither executes the package tests — and both reported success on a head whose own new tests fail. That is the same false-green shape this PR chain keeps producing, this time hiding my own error instead of a reviewer's finding. Verification at this commit, run not assumed: `vitest run` 269 passed, `tsc --noEmit` clean. Reverting only `response-scrub.ts` to 6827796 still fails exactly 10 — the 6 original findings plus 4 of the 6 production-shape cases. Forward fix rather than an amend or force-push: 14e54f1 is already pushed and #1457 is stacked on this branch, so rewriting it would move a base another PR points at. No secret is involved, so there is no range-scan reason to rewrite. Refs: PEN-2431, PEN-2370 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

kkroo · 2026-08-23T14:27:29Z

@ally please re-review at head 3bd503f8918dd3bf98927a8963856a9c17a142d3 — this is the required conflict-resolution rebase of #1457 onto current master. The source preserves the reviewed env default-deny changes and the #1449 argv fixes; only commit IDs and the mechanically concatenated test append changed.

Focused verification: packages/mcp-gateway scrubber/e2e tests 187/187 pass and the package TypeScript build passes. Please attest this exact head, including the merged test coverage, before queue admission.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (degraded: nested runtime invocation unavailable; direct exact-diff adversarial pass applied).
Reviewed head: 3bd503f

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

scrubEnvVarObject and scrubEnvVarSource now fail closed on unrecognized and wrong-case keys, while sharing the closed source allowlist with the YAML path.
The JSON mapping and list env shapes both classify nested references instead of trusting generic recursion, and the tests cover scalar, mapping, wrong-case, and scalar-leaf cases.
Response dispatch reaches nested JSON-RPC text and SSE payloads, including leading-byte and CR-only framing cases, while preserving untouched buffers byte-for-byte.
server.ts applies scrubbing at the single response egress and removes upstream content-length before sending a changed body; the end-to-end tests exercise JSON and SSE gateway responses.

Recommended Action

No Critical or Important issues remain in the reviewed head.
Merge when the repository's remaining CI policy gate is satisfied.

14e54f1 was meant to add tests only. It also reverted all 59 lines of the 9d6470b source fix, so the pushed head carried the new regression tests against the *unfixed* scanner. Restoring the source exactly as tested at 9d6470b; this commit's tree is byte-identical to 9d6470b's `response-scrub.ts`. Cause, recorded because the mechanism is reusable and silent: I measured non-vacuity with `git checkout 6827796 -- packages/mcp-gateway/src/response-scrub.ts`, which writes the old blob to the **index** as well as the working tree. Copying the fixed file back restored the working tree but left the baseline staged, and the next `git add <test-file>` + commit swept that staged revert in. `git diff` against HEAD looked like ordinary unstaged work rather than a revert. Nothing on the PR caught it. #1449 targets a branch rather than master, so only `review` and `security-review` run here — neither executes the package tests — and both reported success on a head whose own new tests fail. That is the same false-green shape this PR chain keeps producing, this time hiding my own error instead of a reviewer's finding. Verification at this commit, run not assumed: `vitest run` 269 passed, `tsc --noEmit` clean. Reverting only `response-scrub.ts` to 6827796 still fails exactly 10 — the 6 original findings plus 4 of the 6 production-shape cases. Forward fix rather than an amend or force-push: 14e54f1 is already pushed and #1457 is stacked on this branch, so rewriting it would move a base another PR points at. No secret is involved, so there is no range-scan reason to rewrite. Refs: PEN-2431, PEN-2370 Signed-off-by: Security Engineer <security-engineer@paperclip.blockcast.net>

allyblockcast · 2026-08-25T18:45:21Z

Disposition: superseded in half, rebuilt in half — closing in favour of #1501

Read live against master at dc2bfaa4 while working PEN-2370. This PR is CONFLICTING because its stack collapsed underneath it: #1435 and #1449 both merged (2026-08-24 and 08-23), so the base it was written against no longer exists. Rather than rebase it as a unit, I checked each of its four changes against what is actually on master now.

Two are superseded — master already implements them, via a different and stricter route:

this PR	on `master` today	verdict
`scrubEnvVarObject` — per-key default-deny on the JSON env path	`scrubJsonEnvVarEntry` — replaces the whole entry unless it is exactly `{name,value}` or `{name,valueFrom}`	superseded, and `master`'s is stricter
`scrubEnvVarSource` — classify the `valueFrom` subtree	`isValidEnvVarSource` — exactly one selector from a closed set, only allowed ref keys, type-checked	superseded, and `master`'s is stricter

I verified this by construction rather than by reading the diff: {"name":"X","Name":"<secret>"}, {"name":"X","Value":"…"}, a three-key entry, and a valueFrom carrying a smuggled extra leaf all redact on master today. The "JSON is default-allow while YAML is default-deny" disagreement this PR was written to remove is already gone.

Two are not superseded, and are the reason this PR still had value:

continuesBlock holding a comment at any column
continuesBlock holding a tab-indented line

Both are still live fail-opens on master. I confirmed empirically against the committed scrubber with a positive control, on env: and on args: — 12 of 12 adversarial shapes leaked.

⚠️ One correction, which is why I rebuilt rather than rebased

The tab rule as written here causes severe over-redaction. leadingIndent counts spaces, so a tab-indented line measures as indent 0 — and this scanner uses depth for two jobs, not one. Holding the line in the block fixes the membership job and leaves the second inverted: that same bogus 0 becomes a swallow threshold meaning "drop every following line indented deeper than 0". Running this PR's continuesBlock change on a pod interrupted by a tab line, the entire rest of the container spec — image, ports, resources, livenessProbe — collapses into a single "<redacted>", as far as the next column-0 key.

That is the diagnostic payload the grant exists for. This PR's Risks section says a guard test asserts image: survives after a comment, and it does — but the assertion was made on the comment case, and the destructive case is the tab case.

#1501 carries both fixes with the root cause addressed: an unmeasurable line sets no swallow threshold. That is safe precisely inside an env block, where the default is already REDACT, so a no-longer-swallowed continuation still reaches the fail-closed arm and is redacted individually rather than dropped. Both directions are pinned by tests — 14 of the 26 new tests fail without the fix, and 12 are controls that must pass in both states, including the over-redaction guards this PR would have tripped.

Closing

Closing rather than rebasing: half of the diff would conflict against code that already does its job better, and the surviving half needed a correction that changes its shape. The branch is untouched and this is reopenable if anyone disagrees with the supersession read above.

Credit where it is due — the two block-termination fail-opens were found here, and they are real. #1501 is that finding, landed.

— Cto (PEN-2370)

allyblockcast Bot commented Aug 21, 2026

View reviewed changes

allyblockcast Bot commented Aug 22, 2026

View reviewed changes

kkroo changed the base branch from security/pen-2431-argv-configmap-scope to master August 23, 2026 13:48

kkroo enabled auto-merge August 23, 2026 13:49

allyblockcast Bot and others added 14 commits August 23, 2026 07:10

kkroo force-pushed the security/pen-2370-json-path-default-deny branch from 6a9b15b to 3bd503f Compare August 23, 2026 14:23

allyblockcast Bot commented Aug 23, 2026

View reviewed changes

kkroo added this pull request to the merge queue Aug 23, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 23, 2026

allyblockcast Bot mentioned this pull request Aug 25, 2026

security(mcp-gateway): close two block-termination fail-opens in the YAML scrubber (PEN-2370 a1) #1501

Merged

13 tasks

allyblockcast Bot closed this Aug 25, 2026

allyblockcast Bot mentioned this pull request Aug 27, 2026

security(mcp-gateway): base64 is name-shaped, so validating the name is not enough (PEN-2370) #1522

Merged

3 tasks

Navigation Menu

Uh oh!

security(mcp-gateway): make the JSON env path default-deny, matching YAML (PEN-2370 ask 3) - #1457

security(mcp-gateway): make the JSON env path default-deny, matching YAML (PEN-2370 ask 3)#1457
allyblockcast[bot] wants to merge 14 commits into
masterfrom
security/pen-2370-json-path-default-deny

allyblockcast Bot commented Aug 21, 2026 •

edited by kkroo

Loading

Uh oh!

allyblockcast Bot commented Aug 21, 2026

Uh oh!

allyblockcast Bot commented Aug 21, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Aug 21, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Aug 22, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

kkroo commented Aug 23, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

Uh oh!

allyblockcast Bot commented Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

allyblockcast Bot commented Aug 21, 2026 • edited by kkroo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

Linked Issues or Issue Description

What Changed

Verification

Risks

Model Used

Checklist

Uh oh!

allyblockcast Bot commented Aug 21, 2026

Uh oh!

allyblockcast Bot commented Aug 21, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (2)

Suggestions (2)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Aug 21, 2026

Both Important findings addressed — de73731

Important 1 — the name axis

Important 2 — the mapping branch

Both suggestions taken

Evidence

One thing I checked rather than assumed

Scope — unchanged, and still worth repeating

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (2)

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Aug 22, 2026

Resolved the conflict — 6a9b15b1, back to CLEAN/MERGEABLE

What conflicted, and why the resolution is mechanical rather than a judgement call

Verified in both directions, because a clean textual merge can still drop one side's logic silently

Review status — stating this so nobody has to re-derive it

Unchanged

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (2)

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

Recommended Action

Uh oh!

kkroo commented Aug 23, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

Recommended Action

Uh oh!

Uh oh!

allyblockcast Bot commented Aug 25, 2026

Disposition: superseded in half, rebuilt in half — closing in favour of #1501

⚠️ One correction, which is why I rebuilt rather than rebased

Closing

Uh oh!

Reviewers

Assignees

allyblockcast Bot commented Aug 21, 2026 •

edited by kkroo

Loading

Both Important findings addressed — `de73731`

Important 1 — the `name` axis

Resolved the conflict — `6a9b15b1`, back to CLEAN/MERGEABLE