Skip to content

fix(sanitizer): redact a sensitive KEY=value chained after a harmless pair (#3311) - #3335

Merged
vybe merged 2 commits into
devfrom
feature/3311-sanitizer-chained-pairs
Oct 8, 2026
Merged

vybe merged 2 commits into
devfrom
feature/3311-sanitizer-chained-pairs

Conversation

@trinity-ability

Copy link
Copy Markdown
Contributor

Summary

Since #1670, sanitize_text's KEY=value pass took the key up to the first = and the value up to the next whitespace. In user=a&password=X the harmless user pair swallowed a&password=X, so the sensitive pair was never checked and the string came back unredacted. The same happened with ?client=me&access_token=X and --env=GITHUB_TOKEN=X.

Fix (identical in the backend and agent-server copies):

  • _KV_LINE_RE now matches only KEY=; a key also stops at &, ; and ,.
  • A new _redact_kv_pairs walks the keys in order. A harmless key consumes nothing, so the next key in the chain is still checked.
  • Only a sensitive key takes its value, in the old up-to-whitespace shape (_KV_PAIR_RE), then goes through the unchanged _redact_kv_match.
  • Still linear: the search only moves forward and the lookbehind is kept.

Behaviour to review: a sensitive value still runs to the next whitespace, so password=x&b=2 becomes password=***REDACTED*** and also hides &b=2. This is deliberate:

  • a secret containing &, ; or , is never split and partly leaked;
  • the Google consent-link exemption still sees the whole URL.

Harmless pairs before the secret are kept as they were. test_a_sensitive_value_is_never_split_at_a_separator pins this.

Tests

  • test_ec_input_hardening_edges.py::…::test_a_sensitive_pair_chained_after_a_harmless_one_is_redacted: strict-xfail marker removed.
  • New TestChainedPairs in test_1661_sanitizer_linear.py, run against both copies. It covers:
    • every row of the issue table;
    • each separator: &, ;, ,, space, tab;
    • two sensitive pairs in one chain, and a chain with no sensitive pairs left unchanged;
    • six adversarial 64 KB shapes for the linear-time check.
  • Linear-time and consent suites (test_1661, test_2398, test_google_consent_sanitizer): 205 passed.
  • Every unit file referencing the sanitizer: 1219 passed.
  • Mutation check: with the fix reverted in both copies, 18 cases fail; restored byte-identical.

Fixes #3311

🤖 Generated with Claude Code

… pair (#3311)

#1670's single KEY=value regex took the key up to the first `=` and the value
up to whitespace, so in `user=a&password=X` the harmless `user` pair consumed
`a&password=X` and the sensitive pair was never examined (same for
`?client=me&access_token=X` and `--env=GITHUB_TOKEN=X`). The pre-#1661 regex
redacted all of these.

_KV_LINE_RE now matches only `KEY=` (the key also stops at `&`, `;` and `,`),
and _redact_kv_pairs walks the keys in order. A harmless key consumes nothing,
so the next key in the chain is still checked. Only a sensitive key takes its
value, in the old up-to-whitespace shape, so a secret containing a separator
is never split and partly leaked, and the Google consent-link exemption still
sees the whole URL. The search only moves forward and the lookbehind is kept,
so the pass stays linear. Applied identically to the agent-server copy.

Red with the fix reverted: TestChainedPairs (both copies) and
test_ec_input_hardening_edges::TestSanitizerBoundaries::
test_a_sensitive_pair_chained_after_a_harmless_one_is_redacted (xfail removed).

Fixes #3311

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/backend/utils/credential_sanitizer.py Fixed
pos = key.end()
if not _is_sensitive_kv_key(key.group(1)):
continue
pair = _KV_PAIR_RE.match(text, key.start())
@vybe

vybe commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

merge-train: ejected from this batch — rides the next train once fixed.

Over-redaction vs #3311 AC1 ("harmless pairs are kept"). The .*AUTH.* / .*TOKEN.* substring keys now apply mid-chain, so harmless URL query pairs are redacted where dev left them alone, e.g. …/issues?q=is:open&author=bob&page=2 → &author=***REDACTED*** (and &page=2 goes with it), ?page=2&tokens_used=10&model=x, ?sort=asc&passwordless=true&lang=en. Either narrow the key match for chained pairs or amend AC1 deliberately.

CodeQL #383/#384 (py/polynomial-redos, credential_sanitizer.py:287,293) look like false positives — dismiss with evidence rather than rewriting: _KV_LINE_RE keeps the lookbehind CodeQL ignores (the reason #237 was dismissed); _KV_PAIR_RE is only used anchored via .match(text, pos); 12 adversarial shapes at 16K/64K/256K grew ~linearly (worst ~67ms at 256KB); removing the lookbehind turns test_1661_sanitizer_linear red. Don't bound the quantifiers — #2398 needs multi-KB keys.

Minor: a differential fuzz found TOKEN;=secret / TOKEN,=x no longer redacted (unrealistic, but a pinning test would record the intent); stale docstring at tests/unit/test_2398_sanitizer_key_redos.py:9.

@vybe vybe added the status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) label Oct 7, 2026
… sensitive word (#3311)

Merge-train finding on #3335: walking the KEY=value chain put keys under
the containment key rule (`.*AUTH.*`, `.*TOKEN.*`, ...) that dev never
examined, because they sat inside a harmless pair's value. Everyday query
parameters were redacted, and since a sensitive value runs to whitespace
each one took the rest of the URL with it:

  .../issues?q=is:open&author=bob&page=2
  ?page=2&tokens_used=10&model=x
  ?sort=asc&passwordless=true&lang=en

AC1 says the harmless pairs are kept.

For a chained key only, a closed list of harmless words is blanked out
before the same containment test: AUTHOR... (not authoriz/authoris),
PASSWORDLESS, SECRETARY/-IES/-IAT, TOKENIZ/TOKENIS, and the token-count
names (max/total/input/output/prompt/completion_tokens,
token(s)_used/_count/_limit/_usage). A word list rather than a
word-boundary rule, because a boundary rule leaks an open class
(secretkey, authkey, tokenvalue, passwords, credentials, authorization);
with the list, the remainder is still tested, so author_token and
max_tokens_secret still redact.

"Chained" means the key sits inside what #1670 consumed as a harmless
pair's value: after `=`, `&`, `;` or `,`, with only value characters back
to the previous `KEY=`. Every key dev already tested - one starting its
run, following a bare word, or following a redacted pair - keeps plain
containment, so this never redacts less than dev did. The gap check scans
only the text between two consecutive keys, so the walk stays linear.

Both copies (backend and agent-server) carry the identical change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@trinity-ability

Copy link
Copy Markdown
Contributor Author

Addressed the merge-train over-redaction finding in 619f8b24f.

Rule. For a chained key only (one sitting inside what dev consumed as a harmless pair's value), a closed list of harmless words is blanked out of the key before the same containment test runs on the remainder. The list: author… (not authoriz…/authoris…), passwordless, secretary/-ies/-iat, tokeniz…/tokenis…, and token-count names (max_/total_/input_/output_/prompt_/completion_tokens, token(s)_used/_count/_limit/_usage). Same hunks in the agent-server copy.

Now kept, as on dev: …/issues?q=is:open&author=bob&page=2, ?page=2&tokens_used=10&model=x, ?sort=asc&passwordless=true&lang=en.
Still redacted: FOO=bar API_KEY=…, a=1&access_token=abc, x=1&password=hunter2, and a harmless word beside a sensitive one (&author_token=, &max_tokens_secret=).

Known limits, stated plainly.

  • It is a word list, not a word-boundary rule, on purpose: a boundary rule would let through secretkey, authkey, passwords, authorization. So AC1 holds for the listed words, not universally. Harmless chained pairs outside the list are still redacted where dev kept them, e.g. &token_type=bearer, &token_id=, &oauth_provider=, &password_policy=.
  • The minor item is unchanged: TOKEN;=secret / TOKEN,=x are still not redacted (dev redacts them). No pinning test added. The stale docstring in test_2398 is untouched.
  • CodeQL Persistent state allowlist (S4) #383/Reset-preserve-state operation (S3) #384 regexes are untouched and the alerts are not dismissed.

Evidence. Red first: 26 failed on the previous head with the new cases. After: test_1661_sanitizer_linear.py 185 passed; 12 sanitizer-focused files 775 passed; 15 other importing files 550 passed. New patterns grow linearly at 16K/64K/256K (worst new shape 23.9 ms at 256K). Run on Python 3.14 locally, not 3.13.

pos = 0 # where the next key search starts; only ever moves forward
prev = -1 # end of the previous harmless `KEY=`; -1 = none in this chain
while True:
key = _KV_LINE_RE.search(text, pos)
@vybe vybe removed the status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) label Oct 8, 2026

@vybe vybe 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.

merge-train: validated at lane C (/validate-pr, /review, /cso --diff). CodeQL alerts 388/384 measured linear on 19 adversarial shapes up to 400k chars; left open for a human dismissal.

@vybe
vybe merged commit f6bedcd into dev Oct 8, 2026
22 of 23 checks passed
@vybe

vybe commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

merge-train: merged. One follow-up noted from validation, accepted at the merge gate: when &, ; or , sits inside the key run before its =, dev redacted the value because a sensitive word appeared earlier in the run, and this change keeps it (https://h/oauth/cb;jsessionid=…, Set-Cookie: auth,sid=…, PASSWORD,foo=…). The value belongs to a key the rule treats as harmless, so this reads as a correction, but the docstring on test_a_key_dev_already_examined_keeps_the_dev_rule ("must never redact LESS than dev did") no longer holds as written. CodeQL alerts 388 and 384 (py/polynomial-redos) measured linear up to 400k chars and are left open for a human dismissal.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants