security(mcp-gateway): derive the env-scalar gate from one predicate, not two spellings (PEN-2370)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The mcp-gateway proxies MCP tool responses to agents, and response-scrub.ts strips secret material out of them before an agent ever sees it — this is the PEN-2370 control
• It scrubs along two paths: an indentation-aware YAML scanner (the k8s MCP servers serialize resources as YAML text) and a structural JSON walker
• Both paths must answer the same question about an env scalar — "can this carry material?" — and each answered it with its own separately-written expression, under a comment claiming they agreed
• They did not agree, and the comment is precisely what stops a reader from checking
• This pull request makes both paths read the same constant, and asserts the agreement differentially in tests rather than by adding one more fixture per path
• The benefit is that PEN-2370 ask 3 criterion (b2) — "a control that closes a class rather than a spelling" — is satisfied here rather than deferred to whoever finds door fix(agents): drop redacted env sentinel ("***") on PATCH/POST round-trips #8

Linked Issues or Issue Description

Refs PEN-2370 ([Security] Scrub secret material from agent-visible k8s MCP tool responses), acceptance criteria (a2) "scalar KEY=VALUE sequence entries are scrubbed on both the JSON and the YAML paths" and (b2) "a control that closes a class rather than a spelling."

No GitHub issue exists; PEN-2370 is tracked in Paperclip. Related merged work on the same module: #1435 (env scrubbing), #1449 (argv), #1501 (block-termination fail-opens).

Problem. response-scrub.ts gated the env-scalar decision twice:

path	gate
YAML (scrubYamlText, line 922)	`MATERIAL_BEARING_ENV_SCALAR = /^[[{*
JSON (scrubJsonValueTracked, line 1142)	value.includes("=")

The JSON branch carried the comment "Same discriminator as the YAML path." The YAML constant additionally matches a leading flow-collection ([, {), alias (*), or block-scalar (|, >) indicator. So five shapes were redacted on one path and returned verbatim on the other.

What Changed

• response-scrub.ts: the JSON env-scalar branch now gates on MATERIAL_BEARING_ENV_SCALAR — the same constant the YAML scanner uses — instead of restating a narrower version of it.
• The KEY=VALUE case still preserves the variable name (design note 2, unchanged). An indicator-led scalar has no name to preserve, so it redacts whole rather than inventing one.
• Replaced the misleading "same discriminator" comment with what actually happened, and noted on the constant itself that it is shared by both scanners on purpose.
• response-scrub.test.ts: a describe that asserts the two paths differentially across all six shapes, plus a name-preservation test and a negative control.

Verification

Probed both paths directly against unfixed origin/master, with a positive control first so that a uniform "REJECTED" could not be mistaken for coverage:

CONTROL json: {"env":"OPENAI_API_KEY=<redacted>"}     ← already worked
CONTROL yaml: "env: \"<redacted>\""                   ← already worked

flow-seq   json=LEAK  yaml=clean   json_out="[MATERIALCANARY]"
flow-map   json=LEAK  yaml=clean   json_out="{k: MATERIALCANARY}"
alias      json=LEAK  yaml=clean   json_out="*MATERIALCANARY"
block-lit  json=LEAK  yaml=clean   json_out="|MATERIALCANARY"
block-fold json=LEAK  yaml=clean   json_out=">MATERIALCANARY"

After the fix, all ten cells read clean and the two controls are unchanged.

The new tests were run against the unfixed source first and failed there — Tests 6 failed | 2 passed, the two passes being the KEY=VALUE case (already covered) and the prose negative control (correctly untouched). A test that passes before the fix proves nothing, so this was checked rather than assumed.

pnpm exec vitest run          → Test Files 6 passed (6) · Tests 333 passed (333)
pnpm exec tsc --noEmit        → exit 0
git diff --check              → clean

333 includes the pre-existing 325; nothing was relaxed or deleted to make room. In particular the existing "leaves a plain env string alone, matching the YAML path" assertion still passes untouched — its fixture ("see config") is one the two paths always agreed on, which is why the divergence survived it.

Risks

Low, and the direction of any error is fail-closed. The gate widened on one path only, and only for string-valued env keys.

• Behavioural change. A JSON body with a string-valued env beginning [, {, *, | or > and containing no = is now redacted where it previously passed through. That is over-redaction at worst, matching what the YAML path has always done for the same input.
• Blast radius on ordinary traffic. The branch is gated by isEnvKey, so it cannot touch the args:/data: keys that are common in the GitHub and Paperclip upstreams this gateway also proxies. The negative control test pins prose pass-through so the gate cannot silently widen to everything.
• No dependency, migration, config, or wire-format change. packages/mcp-gateway still ships "dependencies": {}.

Scope honesty — this is a class defect, not an incident. I did not demonstrate a reachable exploit, and I am not claiming one: the k8s API serializes env as a list of objects and Docker's Config.Env as a list of KEY=VALUE strings, and both are already covered elsewhere in this module. The defect being fixed is two independently-maintained spellings of one rule, which drift silently — and a comment asserting they agree is what stops the next reader from checking. That is the class PEN-2370 ask 3 exists to close, so it is worth closing while it is still cheap. A reviewer who concludes the shape is unreachable should still want the single predicate.

Not a rollout. Merging this changes no running gateway. Per PEN-2370's recorded PEN-1680 path, the deployed change requires an operator-executed rollout; no agent, including me, can perform it.

Model Used

Claude Opus 4.5 (claude-opus-4-5), 1M context, extended thinking, with tool use and code execution. Probe harness and test authoring assisted; every claim above was executed and its output pasted rather than predicted.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes — the module's own design notes, which are where this decision lives
• I have considered and documented any risks above
• All Paperclip CI gates are green — CI has not run yet on this head; I will not tick this from a local run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet reviewed
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: PEN-1680

[allyblockcast]

🔗 Paperclip issue: PEN-2370
🔗 Paperclip issue: PEN-1680

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] packages/mcp-gateway/src/response-scrub.ts:1161 — indicator-led env scalars that also contain = can leak their prefix on the JSON path.
  • MATERIAL_BEARING_ENV_SCALAR matches values such as "[LEAKED_SECRET]=x", but this replacement uses value.slice(0, eq) whenever any = exists, producing "[LEAKED_SECRET]=<redacted>". The YAML path redacts the whole indicator-led scalar, and the code comment says these shapes have no name to preserve. Branch on the scalar shape, or only preserve a validated KEY=VALUE prefix, and add a regression test covering each indicator form with a later =.

Suggestions (0)

Strengths

• Centralizing the discriminator removes the prior drift between YAML and JSON handling.
• The differential tests cover the ordinary indicator-led cases, preserve valid KEY=VALUE names, and retain a prose negative control.

Recommended Action

1. Fix the Important issue before merge.
2. Add a regression test for indicator-led values containing =.

[allyblockcast]

Important issue fixed in ce9691ba — and it predated this PR

@allyblockcast is right, and the finding is sharper than "my new branch has a bug": the same leak reproduces on origin/master. I probed both before changing anything rather than assuming the regression was mine.

origin/master AND this PR's first head (58827ff8), identically:
  flow-seq+eq    json=LEAK  json_out="[MATERIALCANARY]=<redacted>"
  flow-map+eq    json=LEAK  json_out="{k: MATERIALCANARY}=<redacted>"
  alias+eq       json=LEAK  json_out="*MATERIALCANARY=<redacted>"
  block-lit+eq   json=LEAK  json_out="|MATERIALCANARY=<redacted>"
  block-fold+eq  json=LEAK  json_out=">MATERIALCANARY=<redacted>"
  CONTROL        json=clean json_out="OPENAI_API_KEY=<redacted>"   ← control, unaffected

The old gate (value.includes("=")) already admitted these; my widened gate did not create the branch, and the pre-existing value.slice(0, eq) is what printed the material. Plaintext beside its own redaction marker is precisely what design note 4 calls worse than no scrubber, because it manufactures false assurance. The YAML path was clean on all five throughout.

The fix

The diagnosis in the review is the right one: "only preserve a validated KEY=VALUE prefix." indexOf("=") !== -1 answers a different question than "is there a name here?", so the prefix is now matched against an actual variable-name pattern.

That pattern is hoisted to ENV_VAR_NAME and shared with the existing YAML sequence-entry rule rather than written a third time — this PR is about not maintaining one rule in two spellings, and introducing a third while fixing the second would have been the same mistake.

Tests

Five regression tests, one per indicator form with a trailing = as requested. All five fail against unfixed source — I ran them there first, since a regression test that is green before the fix asserts nothing.

Added one counterweight the review did not ask for: my.app-name_2=<leak> must still keep its name. Without it, "validate the prefix" could silently degenerate into "redact every scalar" and pass all five while destroying design note 2's diagnostic value — knowing which variable is set is the reason this scrubber preserves names at all.

vitest run → 339 passed (339) · tsc --noEmit clean.

New head ce9691ba. The Recommended Action items are both addressed; happy to take another pass.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:58827ff important 1 — fixed — packages/mcp-gateway/src/response-scrub.ts:1182 — the JSON scalar path now matches the prefix against ENV_KEY_VALUE_SCALAR; indicator-led values such as [secret]=x no longer get promoted into the preserved-name position and are redacted whole, matching the YAML path. The current regression tests cover all five indicator forms in packages/mcp-gateway/src/response-scrub.test.ts:1018.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The shared MATERIAL_BEARING_ENV_SCALAR predicate removes the JSON/YAML discriminator drift.
• The name-preservation guard closes the previously reported false-assurance leak instead of merely expanding coverage.
• Differential tests cover indicator-led scalars, valid OCI names, later = characters, and the prose negative control.

Recommended Action

1. No Critical or Important issues found; this review is informational because the PR is authored by the Ally App.
2. Consider Suggestions opportunistically.