security(mcp-gateway): the name guard reached two of three call sites (PEN-2370) - #1552
Merged
Merged
Conversation
… (PEN-2370)
`b43437f4` and `cd14342` moved name preservation onto a shared, validated
predicate so that material could not be promoted into the name position.
Both fixed the JSON `env` *scalar* branch and the YAML sequence entry. The
JSON `env` **array** entry kept `indexOf("=")` and was not touched:
const eq = entry.indexOf("=");
return eq > 0 ? `${entry.slice(0, eq)}=${REDACTED}` : REDACTED;
Fifty lines below it, the branch that was fixed carries a comment saying
this exact expression "promotes `[LEAKED]` into the name position and emits
it in the clear next to a redaction marker". That is design note 4's
false-assurance failure -- the output is strictly worse than not scrubbing,
because the marker asserts a redaction happened beside the plaintext.
Measured through `scrubResponseBody`, same input to all three paths:
JSON array [MATERIAL]=x -> "[MATERIAL]=<redacted>" LEAK
JSON scalar [MATERIAL]=x -> "<redacted>"
YAML entry [MATERIAL]=x -> "<redacted>"
JSON array ZHVtbXk<MATERIAL>== -> "ZHVtbXk<MATERIAL>=<redacted>" LEAK
JSON scalar ZHVtbXk<MATERIAL>== -> "<redacted>"
YAML entry ZHVtbXk<MATERIAL>== -> "<redacted>"
The fix is to use the constant, not to write a fourth spelling of it.
`ENV_KEY_VALUE_SCALAR` already encodes "a validated name followed by a real
value"; the array entry is the same question about the same shape. No new
trade is introduced -- the `#`-clause's JSON-path conservatism was already
accepted for the scalar branch this now joins.
SCOPE, stated plainly: this is a fallback-path defect, not a reachable
incident. The k8s API serializes container env as a list of {name, value}
objects, which takes the object branch and was always clean; the string
branch exists for the OCI/Docker `Config.Env` shape. It matters because
the JSON path's stated reason to exist is that an upstream shape change
cannot silently turn the scrubber into a no-op, and a shape it prints in
the clear defeats that.
Tests extend the existing invariant along the *path* axis rather than
adding fixtures. The list previously read "the two paths that print a
name" and asserted two of three -- the same enumeration-not-invariant
asymmetry this ticket is about, reproduced inside the test written to
prevent it. The structural assertion ("whatever survives in the name
position is a name, not material") now runs over every name-preserving
path instead of one, so it fails for an encoding nobody has thought of on
any of them.
Verified, run not assumed:
- Reverting ONLY response-scrub.ts with the new tests in place fails 5:
the 4 material shapes plus the structural class assertion. 232 pass.
- With the fix: 346 passed (346) across the package; tsc --noEmit clean.
- The two counterweights (`TOKEN=<base64>` keeps its name on the array
path; a bare scalar redacts whole) pass in BOTH states, as controls
must -- they are what stops this degenerating into "redact every
`=`-bearing entry" and destroying design note 2.
Found by the method PEN-2370 ask 3 names as the control: after the
remediation landed, go looking for another route to the same material
rather than re-reading the patch.
Refs: PEN-2370 ask 3 criteria (a2), (b2)
Signed-off-by: Cto <cto@paperclip.blockcast.net>
Author
1 similar comment
Author
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) [degraded: skill unavailable] + gstack/review [degraded: executable unavailable] + native-codex.
Reviewed head: fc67d7b
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Routes the JSON
envarray scalar path through the same validated name predicate as the existing name-preserving paths. - Adds regression coverage for malformed name-shaped material, valid padded values, and the array path itself.
- Keeps the change narrowly scoped and documents the security invariant and conservative redaction tradeoff.
Recommended Action
- No Critical or Important issues found; this review is ready for the next merge-gate evaluation.
kkroo
approved these changes
Aug 30, 2026
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Approved: exact-head Ally review reports 0 Critical / 0 Important / 0 Suggestions; required verify passes; merge state is clean.
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Linked Issues or Issue Description
KEY=VALUEscrubbed on both the JSON and YAML paths) and (b2) (a control that closes a class rather than a spelling)command/argshalf of the same scrubberRelated PRs, all in this chain and none touching this code path: #1435 (the scrubber itself), #1518 →
d741f104and #1530 →b43437f4/cd14342(the two prior name-guard fixes this one completes), #1544 (routedk8s-rothrough the gateway, which is what made the scrubber load-bearing), #1551 (open — editsSEED_COVERAGErationales in a different file). Searched open/closed/merged forresponse-scrub,ENV_KEY_VALUE,REQUIRE_VALUE_AFTER_EQandname position; no other PR touches this branch of the code.What Changed
scrubJsonValue, JSONenvarray entry (response-scrub.ts): replacedentry.indexOf("=")slicing with the sharedENV_KEY_VALUE_SCALARguard, matching the JSON scalar branch and the YAML sequence-entry rule. A prefix that is not a validated variable name now has no name to keep and redacts whole.response-scrub.test.ts: theredacts <label> whole on every path that preserves namesinvariant now asserts the JSON array path alongside the JSON scalar and YAML paths. Its comment previously said "the two paths that print a name" and named two of three.response-scrub.test.ts: the structural class assertion (whatever survives in the name position is a name, not material) now runs over every name-preserving path rather than only the scalar one.response-scrub.test.ts: added the array-path counterweight (TOKEN=<base64>keeps its name), so the change cannot degenerate into "redact every=-bearing entry".Verification
Measured through the real entry point
scrubResponseBody, not by calling an inner function:response-scrub.tswith the new tests in place fails 5: the four material shapes plus the structural class assertion. 232 pass. (Reverted withgit show HEAD:<path> >rather thangit checkout <sha> -- <path>— the latter stages the old blob and is exactly how14e54f1silently reverted this file's fix once before, pere231c024.)vitest run→ 346 passed (346) across the package.tsc --noEmitclean.TOKEN=<base64>keeps its name on the array path, and a bare scalar with no=redacts whole. These are what distinguish tightening the guard from redacting everything, which would satisfy every leak assertion above while destroying design note 2.Fixtures use a synthetic
MATERIALCANARYmarker and assert on its absence. No pod was probed, and no credential value appears in this diff, its tests, or this description — per PEN-2370's standing constraint that a verification which reproduces the harm is not diligence.Risks
Low risk, and the direction of any error is toward over-redaction rather than disclosure.
envarray entries whose prefix is not a valid variable name. Those previously printed the prefix; they now redact whole. Well-formed OCIConfig.Enventries (KEY=VALUE) are unaffected — pinned by the counterweight test.REQUIRE_VALUE_AFTER_EQcarries a documented conservatism on the JSON path (#has no comment meaning there, soTOKEN= #xover-redacts). That trade was already accepted for the JSON scalar branch incd14342; this change puts a third call site behind the same constant rather than inventing a fourth spelling tuned to it — which is the drift that produced this defect.Scope honesty, stated plainly: this is a fallback-path defect, not a reachable incident. The Kubernetes API serializes container env as a list of
{name, value}objects, which takes the object branch and was always clean. The string branch exists for the OCI/DockerConfig.Envshape. It still matters, because the JSON path's stated reason to exist is that an upstream shape change cannot silently turn the scrubber into a no-op — and a shape it prints in the clear defeats that reason.Found by the method PEN-2370 ask 3 names as its control: after a remediation lands, go looking for another route to the same material instead of re-reading the patch.
Model Used
Claude Opus 4.5 (
claude-opus-4-5), 1M context, extended thinking, running as the Paperclip CTO agent via Claude Code with tool use (Bash, file edits, GitHub + Paperclip + k8s-ro MCP). One subagent performed the initial adversarial sweep ofresponse-scrub.ts; its central finding was then re-derived independently by reading the source and re-measured throughscrubResponseBodybefore any code was changed.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatetsc --noEmit, and the 5-failure non-vacuity measurement above