Skip to content

fix(core): redact a secret assignment nested in a harmless assignment's value - #5434

Open
Totoro-qaq wants to merge 2 commits into
apache:mainfrom
Totoro-qaq:fix/redaction-nested-assignment
Open

Totoro-qaq wants to merge 2 commits into
apache:mainfrom
Totoro-qaq:fix/redaction-nested-assignment

Conversation

@Totoro-qaq

Copy link
Copy Markdown
Contributor

Summary

redactSecrets applied its key=value assignment pattern with one global replace. A harmless key such as excerpt matched together with its whole value. So in Config excerpt: password=..., the nested password= had already been consumed and was never tested.

  • Assignment prefixes (key, separator, optional quote) are now scanned in a loop. A harmless key's value is still searched for nested sensitive assignments, and a sensitive key's value is redacted once. A value that starts inside an already redacted value is not redacted again.
  • The scan stays linear on long key-character runs even though values are now searched:
    • Each [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 \b start produced.
    • sensitiveKeySegments splits uppercase runs with a linear regex.
    • A 256 KB base64url blob that took about 3 s on main now takes about 5 ms.
  • The other patterns, their order, the sensitive-key rules and the value terminators are unchanged.

Behavior change: an assignment nested in another value is now redacted the way it already was on its own. url=https://h/p;password=x redacts the password, and shortcut=key:Enter becomes shortcut=key:[redacted].

Note for #4930: its new QUOTED_SECRET_ASSIGNMENT_PATTERN starts 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

  • New tests in redaction.test.ts:
    • 8 nested forms, each unchanged on main;
    • harmless values that must stay unchanged;
    • sensitive keys without a value;
    • an empty value followed by another assignment (env: API_TOKEN= DB_PASSWORD="..."), which must still redact the password;
    • redacting twice gives the same result;
    • a bounded-time test on 200k-character hyphenated, uppercase and base64 inputs.
  • Mutation checks on the new guards. Two mutants survive:
    • the unreachable valueStart <= copied boundary;
    • a key-start variant that differs only for keys beginning with _ or a digit.
  • Differential fuzz against main:
    • 600k inputs: every difference is added redaction of a nested assignment.
    • 800k leak-fuzz inputs: 0 secrets shown. An earlier version of this change leaked 1,074 of 400k; that is fixed.
    • 1.5M short inputs: redacting the output again changes nothing (120 such cases on main).
  • Package tests:
    • @maka/core 851/851, @maka/mcp 250/250, @maka/runtime 0 failures (3,521 passed, 14 skipped), @maka/ui 491/491.
    • @maka/storage has one failure in managed-dependency-environment-crash (a Node 22 SQLite warning on child stderr).
    • @maka/runtime-host has 6 cancelled tests in resumable-peer-stream. The runtime-host result is the same without this change.
  • Checked together with the Deep Research fix for bug(runtime): Deep Research artifact pages are redacted separately, so a secret split across pages is shown #5432: the two commits cherry-pick cleanly, and core passes 852/852.
  • Core typecheck, biome lint and format, git diff --check and the Windows test inventory pass.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code helped investigate, implement and test this change. The commit carries a Generated-by: Claude Code trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

…'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
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 17, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=value shape. 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): isSensitiveKey and single-assignment redaction are byte-identical to main, so both rewrites — \b → key-run at redaction.ts:51-52 and the sensitiveKeySegments split 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 main vs 28 with this change, and all 28 are also non-idempotent on main (e.g. redactUrlQuerySecrets on &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 on main vs 3 ms here (Node 26). The three long inputs in the new bounded-time test run in ~25 ms total, and the valueStart < copied guard (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 the shortcut=key:Enter case you already call out (bare key is sensitive on main too, just not in nested position). biome check is clean, and on main exactly the two new tests that encode the bug fail.

Nits

  1. redaction.ts:134-136 — a sticky test() failure would silently set copied = 0 and emit the secret. A sticky regex whose lastIndex points at a non-matching position resets lastIndex to 0 on failure (I confirmed this against the spec and empirically). If ASSIGNED_SECRET_VALUE_PATTERN.test() ever returned false, copied would become 0 and 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 share ASSIGNED_SECRET_VALUE_CHARACTER_SOURCE, which your comment at :47-48 does state. Given the "never echo any part of the match back" rule a few lines up in redactTextSecrets, 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;
  2. 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 key data), so main does them in ~5 ms and the guard cannot fail there. The shape that is quadratic on main is a bare run with no leading key=; adding `note: ${'a-'.repeat(100_000)}` (~34 s on main, 3 ms here) would turn it into a real regression guard — ideally with a { timeout: … } like the existing bounded-time test at redaction.test.ts:342 so a future regression fails instead of hanging. Your #4930 note does still hold, for what it's worth: with that PR's QUOTED_SECRET_ASSIGNMENT_PATTERN (which requires a quote, so the prefix attempt fails and restarts per hyphen) I measure 27 s on data=${'a-'.repeat(100_000)}.

  3. 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
@Totoro-qaq

Totoro-qaq commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough pass. Addressed in 321c2a8:

  1. redactAssignedSecrets now takes the sticky value match with exec and skips the assignment if it ever fails. copied only advances past a value it actually matched, so a failed sticky match can no longer reset the copy point and echo the value.

  2. The bounded-time test now includes a bare hyphenated run. I measured before choosing the input:

    • On main with n = 30,000, 'a-'.repeat(n) takes about 1.9 s, both bare and after x .
    • note: … and data=… take about 1 ms on main, because the outer assignment still swallows the run.
    • With this change, all four take about 1 ms.

    So I added 'a-'.repeat(100_000) rather than the note: form. Pointed at main's redaction, the test now fails its bound (scanned in 21082ms). I kept the elapsed assertion rather than { timeout }, because node:test cannot interrupt synchronous work, so a timeout would not stop a slow regex either.

  3. Agreed on the API_TOKEN= DB_PASSWORD="…" output. Leaving it as is.

On _password= / _token=: agreed that it predates this PR. I'll keep it out of scope here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(core): redactSecrets misses a sensitive assignment inside a harmless assignment's value

2 participants