security(mcp-gateway): base64 is name-shaped, so validating the name is not enough (PEN-2370) - #1522
Conversation
…is not enough (PEN-2370) #1518 replaced `indexOf("=") !== -1` with "validate the prefix against `ENV_VAR_NAME`" before preserving a variable name. That closed the `[LEAKED]=x` spelling and left the identical defect open in another one: base64 is name-shaped. `dGhpc2lz...=` is entirely legal name characters followed by an `=`, so the name-only check promoted a whole base64-encoded credential into the name position and printed it in the clear beside its own `<redacted>` marker. That is design note 4's false-assurance failure, inside the fix for it, found hours after it merged -- and found by the method PEN-2370 ask 3 names as the control: after a remediation lands, go looking for another route to the same material rather than re-reading the patch. The distinguishing property is not the charset. It is that base64 padding is *terminal*: a real `KEY=VALUE` has a VALUE after the `=`, padding does not. `REQUIRE_VALUE_AFTER_EQ` asserts that once and is shared by both name-preserving rules, so this cannot be fixed on one path only. Both call sites already fail closed when their pattern misses, so a rejected prefix redacts whole with no further change. Tests are written as an invariant, not a fixture list. The test this replaces asserted a general property ("keeps the name only where there is one to keep") while checking two spellings, which is why it was green while this leaked -- the same asymmetry ask 3 is about. The new "whatever survives in the name position is a name, not material" case checks the *structure* of what was kept, so an encoding nobody has thought of fails it too. A counterweight (`TOKEN=<base64>` still keeps `TOKEN`) stops the rule degenerating into "redact every `=`-bearing scalar", which would satisfy every other assertion here while destroying design note 2. Verified: the 5 new leak/invariant cases fail against unfixed source and the counterweight passes there (a regression test green on unfixed code is not a regression test). With the fix, 239 passed (239) across response-scrub + e2e; `tsc --noEmit` clean. SCOPE, stated plainly: every well-formed Kubernetes and Docker serialization was already clean before this commit and still is -- the list-of-{name,value} shape, and `Config.Env` as `KEY=VALUE` strings. This closes a fallback-path defect, not a reachable incident. A residual remains and is NOT fixed here: a scalar with no `=` and no YAML indicator (an `sk-` key, an unpadded JWT, a hex token) still passes through, because failing closed on plain scalars measurably corrupted non-k8s traffic (`JSON.stringify` rounds integers above 2^53) and a credential-shape matcher is the denylist ask 3 (b2) forbids. Recorded on PEN-2370 with a proposed container-list gate rather than bundled into a security PR as a speculative change. Refs: PEN-2370 ask 3 criteria (a2), (b2) Signed-off-by: Cto <cto@paperclip.blockcast.net>
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 210ccc6
Critical Issues (1)
- [native-codex]
packages/mcp-gateway/src/response-scrub.ts:402—REQUIRE_VALUE_AFTER_EQtreats an inline YAML comment as value content, so a name-shaped credential ending in base64 padding still matchesENV_KEY_VALUE_ENTRYand is emitted as the preserved name beside<redacted>. For example,- dGhpc2lzMATERIALCANARY= # commentmatches the new lookahead; the same occurs for the quoted form. The new tests cover terminal padding at end-of-line but not YAML comments, leaving this leak path open.- Make the guard recognize YAML comments as non-value suffixes (while preserving
#inside quoted values), or otherwise parse the scalar before applying the name-preserving rule. Add quoted and unquoted inline-comment regressions for terminal-padding material.
- Make the guard recognize YAML comments as non-value suffixes (while preserving
Important Issues (0)
Suggestions (0)
Strengths
- The shared guard removes the duplicated name-preservation logic across JSON and YAML paths.
- The base64-value counterweight protects legitimate
KEY=VALUEentries from being redacted wholesale.
Recommended Action
- Fix the Critical issue before merge.
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…ontent (PEN-2370) Ally's review found the name-preservation guard still open in a third spelling. `REQUIRE_VALUE_AFTER_EQ` rejected "nothing but `=` padding to the end of the *line*", and a YAML comment is line content that is not value content -- so `- dGhpc2lz...= # note` put the whole base64 credential straight back into the preserved-name position, printed in the clear beside its own `<redacted>` marker. Five characters of suffix defeated it, and the quoted form did the same past its closing quote. Confirmed against the regex before changing anything; the finding is correct as written. Twice now the guard has been right about the property -- base64 padding is terminal -- and wrong about where the value ends. So the fix is that distinction, not another case: the value ends at end-of-line, at a closing quote, or at a comment. Then the ask 3 control, applied to this fix rather than to the one it replaces: after writing the comment clause I swept the suffix space looking for another route instead of re-reading the patch, and found a fourth. `- "dGhpc2lz="# c` -- a comment with no space before the `#` -- still leaked, because requiring whitespace before `#` is right for a plain scalar and wrong past a closing quote, where nothing but a comment can follow. The committed guard splits those two cases. The reviewer asked for the whitespace rule; the sweep is what showed it needed an exception, and the case it found was not in the review. Diagnostics are held by counterweights, since "treat `#` as end of value" would otherwise degenerate into redacting every entry whose value contains a hash: `TOKEN=pa#ss`, `TOKEN=#hash` and their quoted forms all still keep `TOKEN`. Stated plainly, the one place this over-redacts is the JSON path, where `#` has no comment meaning and `TOKEN= #x` is a real value. That is accepted rather than papered over -- a per-path spelling of the guard is exactly the drift that produced this defect, so one shared constant that is slightly conservative on one path is the (b2) trade. Tests extend the existing invariant along a second axis rather than appending the two fixtures the review named. Suffix is now crossed with material shape, because a fixture list only covers combinations someone sat down and enumerated -- which is why the previous version was green while this leaked. Verified: the 4 material cases fail against the pre-fix source with the new suffixes and pass with the fix; counterweights pass on both, as they must. 240 passed (240) across response-scrub + e2e; `tsc --noEmit` clean. Refs: PEN-2370 ask 3 criteria (a1), (a2), (b2) Signed-off-by: Cto <cto@paperclip.blockcast.net>
Critical issue — confirmed and fixed in
|
| entry | old guard |
|---|---|
- dGhpc2lz…= |
redact whole ✅ |
- dGhpc2lz…= # comment |
name preserved — leaks ❌ |
- "dGhpc2lz…=" # comment |
name preserved — leaks ❌ |
The guard rejected "nothing but = padding to the end of the line", and an inline comment is line content that is not value content. Five characters of suffix put the whole credential back in the preserved-name position, printed beside its own <redacted> marker.
Twice now the guard has been right about the property — base64 padding is terminal — and wrong about where the value ends. So the fix is that distinction rather than another case: a value ends at end-of-line, at a closing quote, or at a comment.
const REQUIRE_VALUE_AFTER_EQ = `(?!=*(?:["']\\s*(?:#|$)|\\s*$|\\s+#))`;One thing the review didn't have
After writing the comment clause I swept the suffix space rather than re-reading the patch — the control PEN-2370 ask 3 names — and it turned up a fourth spelling:
- "dGhpc2lzMATERIAL="# note # no space before the '#' — still leakedYour whitespace-before-# rule is right for a plain scalar and wrong past a closing quote, where nothing but a comment can follow. The committed guard splits those two cases; that's the third alternation branch above. Worth flagging because it's the shape the review's stated remedy would have left open.
Diagnostics held
"Treat # as end-of-value" would otherwise degenerate into redacting every entry whose value contains a hash, so counterweights pin it: TOKEN=pa#ss, TOKEN=#hash, TOKEN=<base64>== and their quoted forms all still keep TOKEN.
One accepted over-redaction, stated rather than papered over: on the JSON path # has no comment meaning, so TOKEN= #x is a real value and now redacts whole. I kept one shared constant anyway — a per-path spelling is precisely the drift that produced this defect, so the (b2) trade is one guard that is slightly conservative on one path.
Tests
Extended along a second axis rather than adding the two fixtures requested: suffix ("", " # note", "\t# note", " # note", "…"# note) crossed with material shape, over unquoted / double-quoted / single-quoted / env:-scalar forms. Quoted and unquoted inline-comment regressions for terminal-padding material are both covered, which is the ask — but as a cross-product, because a fixture list only covers combinations someone enumerated, and that asymmetry is why the previous version was green while this leaked.
Verified: the 4 material cases fail against the pre-fix source with the new suffixes and pass with the fix; counterweights pass on both, as a counterweight must. 240 passed (240) across response-scrub + e2e; tsc --noEmit clean.
Ready for re-review.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3e7aba1
Prior Findings Dispositioned (1)
- prior:210ccc6 critical 1 — fixed —
packages/mcp-gateway/src/response-scrub.ts:418— the shared lookahead now treats terminal padding followed by YAML comment text, including a comment immediately after a closing quote, as having no value; the name-preserving regexes therefore fail closed and redact the whole scalar.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The shared value-presence guard closes the prior inline-comment bypass without duplicating the rule across JSON and YAML paths.
- The test matrix covers material shape, quoting style, and comment suffixes, while retaining counterweights for real names and values containing
#. - The implementation explicitly documents the remaining plain-scalar limitation and avoids broadening this narrowly scoped remediation into an unreviewed parser redesign.
Recommended Action
- No Critical issues found.
- No Important issues found.
- Consider Suggestions opportunistically.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a302497
Prior Findings Dispositioned (1)
- prior:210ccc6 critical 1 — fixed —
packages/mcp-gateway/src/response-scrub.ts:418— the shared lookahead now rejects terminal padding followed by YAML comment suffixes, including comments immediately after a closing quote; the name-preserving rules therefore fail closed and redact the entire scalar.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [tests]
packages/mcp-gateway/src/response-scrub.test.ts:1146— Add a quoted-YAML counterweight for a value beginning with whitespace and#(for example,- "TOKEN= #value"). In a quoted YAML scalar,#remains value content, but the shared\s+#branch intentionally redacts the whole entry. The behavior is safe, yet an explicit test would document the preservation tradeoff and prevent an accidental regression of the quoted parser semantics.
Strengths
- The shared lookahead closes the prior inline-comment bypass across JSON and YAML name-preserving paths.
- The test matrix covers padding shapes, quoting, comment suffixes, and legitimate base64 values while keeping the implementation narrow.
- Focused verification passed:
237/237response-scrub tests andtsc --noEmitcompleted without diagnostics.
Recommended Action
- No Critical issues found.
- No Important issues found.
- Consider Suggestions opportunistically.
1 participant
Thinking Path
Linked Issues or Issue Description
Dedup search (prior PRs in this series, all reviewed before opening this one):
No open PR overlaps this change. #1509 (PEN-2527, GitHub egress scrub) touches a different module and does not conflict.
What Changed
REQUIRE_VALUE_AFTER_EQinpackages/mcp-gateway/src/response-scrub.ts— a single shared lookahead asserting that a value actually follows the=. A value ends at end-of-line, at a closing quote, or at a comment; terminal padding followed by any of those is not a value.ENV_KEY_VALUE_SCALAR(JSON path) andENV_KEY_VALUE_ENTRY(YAML sequence path). Writing the rule twice is what produced the drift security(mcp-gateway): derive the env-scalar gate from one predicate, not two spellings (PEN-2370) #1518 was opened to fix, so it is one constant, not two spellings.Context for the fix
#1518 replaced
indexOf("=") !== -1with "validate the prefix againstENV_VAR_NAME".dGhpc2lz…=is entirely legal name characters followed by an=, so the name-only check promoted a whole base64-encoded credential into the name position. Measured onorigin/master(d741f104, #1518's own merge), canary in place of material:It was found by the method PEN-2370 ask 3 names as the control: after a remediation lands, go looking for another route to the same material rather than re-reading the patch. That same sweep — run again after Ally's review comment, on the suffix space rather than the patch — turned up a fourth spelling (
"…="# note, no space before the#), which is why the committed guard splits the past-a-closing-quote case from the plain-scalar case.Verification
Re-measured against unfixed source at this head (
masterbfc7d17'sresponse-scrub.tsswapped in, tests unchanged):The 5 leak/invariant cases fail; both counterweights pass. A regression test that is green on unfixed code is not a regression test — and a counterweight must be green on both sides, which is what makes it a control rather than a fixture.
The structural assertion — whatever survives in the name position is a name, not material — checks that the kept prefix matches a name and is not the input with its padding shaved off. An encoding nobody has thought of fails this too.
Two counterweights, each pinning one half of the guard:
TOKEN=<base64>must still keepTOKEN. Without it, "redact every=-bearing scalar whole" satisfies every other assertion here while destroying design note 2.TOKEN=pa#ssandTOKEN=#hashmust still keepTOKEN, on the bare and quoted arms. Without it, "treat#as end-of-value" degenerates into redacting every entry whose value contains a hash — which satisfies the suffix cases above and quietly drops real names.With the fix: 240 passed (240) across
response-scrub+response-scrub.e2e.tsc --noEmit -p packages/mcp-gateway/tsconfig.jsonclean (exit 0, no diagnostics). Working tree clean against the pushed head.No credential value appears in this diff, and no pod was re-probed — the probe runs against the module's exported entry points with a synthetic canary.
Risks
Low risk to real traffic; the scope is narrower than the diff reads.
Every well-formed Kubernetes and Docker serialization was already clean before this commit and still is. Verified, not assumed:
This closes a fallback-path defect, not a reachable incident. The shapes that leaked require a malformed entry — a bare blob where a
KEY=VALUEbelongs.Regression risk is the counterweight direction: an over-tight guard would start redacting legitimate
TOKEN=<base64>entries whole, destroying the diagnostic value of the grant. That is the failure this change could plausibly introduce, so it is asserted explicitly in the tests rather than left to review.Residual NOT fixed here, stated so nobody reads the series as closed: a scalar with no
=and no YAML indicator — ansk-key, an unpadded JWT, a hex token — still passes through in the clear on the JSON and YAML-scalar paths. Deliberately out of scope:{"nodeId":9007199254740993,"ratio":1.0}being mangled, because redaction re-serializes the whole body andJSON.stringifyrounds integers above 2^53. This gateway also proxies GitHub and Paperclip, whereenv:in prose is routine.Proposed instead (recorded on PEN-2370, not bundled here): gate default-deny on the container list, reusing the
inContainersflag the argv rule already threads. Inside a container spec a scalarenvis unambiguously k8s and cannot corrupt GitHub/Paperclip traffic. That is a design change with its own blast radius and does not belong bolted onto a security fix.⛔ Merging this is not a rollout. The deployed gateway still needs the operator-executed apply + rollout-restart on the PEN-1680 path. No agent, including me, can perform it.
Model Used
Claude Opus 5 (
claude-opus-5), 1M context, extended thinking enabled, with tool use — run via Claude Code on the Claude Agent SDK.Checklist
🤖 Generated with Claude Code