fix(core): redact a secret assignment nested in a harmless assignment's value - #5434
Totoro-qaq wants to merge 2 commits into
Conversation
…'s value redactSecrets replaced key=value assignments with one global replace. A harmless key such as `excerpt` matched together with its whole value, so `Config excerpt: password=...` came back unchanged: the replace had already consumed the value, and the nested password was never tested. Scan assignment prefixes in a loop instead, so a harmless key's value is still searched and a sensitive key's value is redacted once. Match each key-character run once and split uppercase runs linearly, so long hyphenated or base64 values stay linear now that values are searched. Generated-by: Claude Code
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 5f28235 (packages/core/src/redaction.ts, packages/core/src/__tests__/redaction.test.ts). I ran the new suite (33/33 pass) and diffed the new redactSecrets against main's on generated corpora. Verdict: nits only — I could not find a leak, a missed nested secret, or a behavior regression. Evidence below in case it's useful, then two nits.
What I verified
- Nested descent is complete for the
key=valueshape. I generated 174,960 nested combinations (16 harmless keys × 6 separators × 15 joiners × 9 sensitive keys × 4 separators × 3 quote styles) wrapped around a fake secret: 0 leaks. Covered: three levels (a: b: c: password=…), JSON string values ({"excerpt":"note: password=…"}→ nested value redacted, still valid JSON), quoted values (excerpt: "client_secret=…"), spaced separators (api_key = …),;/&/newline joins (user=alice;password=…), and template-literal text. - No regression vs
main. 300k-input differential fuzz over the same grammar: no input where the new output exposes a secret the old output hid. 400k random keys × 12 leading shapes (-,--,_,.,",(,$,2,x-,x_,{, space):isSensitiveKeyand single-assignment redaction are byte-identical tomain, so both rewrites —\b→ key-run atredaction.ts:51-52and thesensitiveKeySegmentssplit at:252— are behavior-preserving. In the ~50 cases where the[redacted]count drops, a single greedy value span simply covers more text (e.g.…x-password=\tsk-live-…$api_key=…), not a dropped redaction. - Idempotence improves. Same corpus: 1,098 inputs are non-idempotent on
mainvs 28 with this change, and all 28 are also non-idempotent onmain(e.g.redactUrlQuerySecretson&password:), so none are introduced here. - Perf claim holds and is understated.
main's old pattern is quadratic on a bare hyphenated key-character run:'a-'.repeat(120_000)= 33.9 s onmainvs 3 ms here (Node 26). The three long inputs in the new bounded-time test run in ~25 ms total, and thevalueStart < copiedguard (redaction.ts:132) prevents double redaction — no duplicated output in 300k inputs. - False positives are contained. 12 of 13 realistic benign samples are unchanged (
export PATH=…,time=12:30:00,cache-key=…,issue_key=ISSUE-1359,error: token limit reached,level=info msg="started" port=8080,fmt: %s=%v). The only new difference is theshortcut=key:Entercase you already call out (barekeyis sensitive onmaintoo, just not in nested position).biome checkis clean, and onmainexactly the two new tests that encode the bug fail.
Nits
-
redaction.ts:134-136— a stickytest()failure would silently setcopied = 0and emit the secret. A sticky regex whoselastIndexpoints at a non-matching position resetslastIndexto0on failure (I confirmed this against the spec and empirically). IfASSIGNED_SECRET_VALUE_PATTERN.test()ever returned false,copiedwould become0and the function would return…prefix[redacted]plus the entire original value — the secret printed right after the marker. It is unreachable today only because the prefix lookahead and the value pattern shareASSIGNED_SECRET_VALUE_CHARACTER_SOURCE, which your comment at:47-48does state. Given the "never echo any part of the match back" rule a few lines up inredactTextSecrets, I'd make that invariant local rather than cross-pattern:const valueMatch = ASSIGNED_SECRET_VALUE_PATTERN.exec(value); if (!valueMatch) continue; // defensive: keeps copied monotonic next += `${value.slice(copied, valueStart)}[redacted]`; copied = valueStart + valueMatch[0].length;
-
nit: the bounded-time test passes on
main, so it doesn't lock in this PR's linearization. In all three inputs the outer harmless assignment swallows the long value before any key run is scanned (data=+a-a-…matches outright with keydata), somaindoes them in ~5 ms and the guard cannot fail there. The shape that is quadratic onmainis a bare run with no leadingkey=; adding`note: ${'a-'.repeat(100_000)}`(~34 s onmain, 3 ms here) would turn it into a real regression guard — ideally with a{ timeout: … }like the existing bounded-time test atredaction.test.ts:342so a future regression fails instead of hanging. Your#4930note does still hold, for what it's worth: with that PR'sQUOTED_SECRET_ASSIGNMENT_PATTERN(which requires a quote, so the prefix attempt fails and restarts per hyphen) I measure 27 s ondata=${'a-'.repeat(100_000)}. -
nit, no action needed:
env: API_TOKEN= DB_PASSWORD="…"→env: API_TOKEN= [redacted]"[redacted]"(redaction.test.ts:292). The empty-value key emits a marker for what is really the next assignment's key name. It's safe and documented; just noting the output reads like two redactions of one secret.
Optional follow-up, pre-existing and deliberately preserved: _password=/_token= are still not redacted because the key must start at a word-initial letter (identical to main's \b behavior, and your PR body notes the same trade-off). Fine to leave out of this PR; worth a separate issue if you want it closed.
Take the sticky value match with exec and skip the assignment if it ever fails. A failed sticky match resets lastIndex, and copying from there would echo the value after its marker; today the prefix lookahead rules that out, and this keeps the guarantee local. Also add a bare hyphenated run to the bounded-time test. It retried a key at every hyphen on the old pattern and is the input that takes seconds there, unlike values an outer assignment swallows. Generated-by: Claude Code
|
Thanks for the thorough pass. Addressed in 321c2a8:
On |
Summary
redactSecretsapplied itskey=valueassignment pattern with one global replace. A harmless key such asexcerptmatched together with its whole value. So inConfig excerpt: password=..., the nestedpassword=had already been consumed and was never tested.[A-Za-z0-9_-]run is matched once, and the key is taken from its first word-initial letter. That is the same key the old\bstart produced.sensitiveKeySegmentssplits uppercase runs with a linear regex.Behavior change: an assignment nested in another value is now redacted the way it already was on its own.
url=https://h/p;password=xredacts the password, andshortcut=key:Enterbecomesshortcut=key:[redacted].Note for #4930: its new
QUOTED_SECRET_ASSIGNMENT_PATTERNstarts keys with the old\b[A-Za-z]...form, which is quadratic on hyphenated runs. The bounded-time test here will flag that if both land. The key-run prefix used here avoids it.Fixes #5433
Verification
redaction.test.ts:env: API_TOKEN= DB_PASSWORD="..."), which must still redact the password;valueStart <= copiedboundary;_or a digit.@maka/core851/851,@maka/mcp250/250,@maka/runtime0 failures (3,521 passed, 14 skipped),@maka/ui491/491.@maka/storagehas one failure inmanaged-dependency-environment-crash(a Node 22 SQLite warning on child stderr).@maka/runtime-hosthas 6 cancelled tests inresumable-peer-stream. The runtime-host result is the same without this change.git diff --checkand the Windows test inventory pass.AI use
Tool(s) and scope: Claude Code helped investigate, implement and test this change. The commit carries a
Generated-by: Claude Codetrailer.Checklist
Does this PR entail a change in behavior?