Skip to content

fix(audit): mask string values under credential-named keys in _clean - #40

Open
schenksj wants to merge 4 commits into
mainfrom
fix/audit-redact-sensitive-keys
Open

schenksj wants to merge 4 commits into
mainfrom
fix/audit-redact-sensitive-keys

Conversation

@schenksj

@schenksj schenksj commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Closes #37.

Problem

_clean in agent/audit.py redacted string values by their shape (via redact()) and never looked at keys. A bare secret under a credential-named key has no shape to match, so it was written to the audit JSONL unmasked:

>>> _clean({"client_secret": "SUPERSECRET", "token": "ghp_abc", "password": "hunter2"})
{'client_secret': 'SUPERSECRET', 'token': 'ghp_***', 'password': 'hunter2'}   # main
{'client_secret': '***', 'token': '***', 'password': '***'}                   # this PR

Fix

A key is treated as credential-named after it's normalized: lowercased, with _ and - removed. It then matches one of three rules:

  • Exact name: authorization, proxyauthorization, cookie, setcookie
  • Suffix: secret(s), password(s), passwd, passphrase, token, apikey(s), secretkey, accesskey, credential(s)
  • Substring: privatekey only, so that private_key_pem is caught

What happens to a value under such a key depends on its type:

  • A non-empty string or bytes value becomes ***.
  • A list or tuple is masked element by element, e.g. multi-valued Set-Cookie or a list of tokens.
  • A dict is still walked, and its own keys decide what gets masked: {"token": {"access_token": "x", "expires_in": 3600}} masks only access_token.
  • Empty values and other scalars (ints, bools) pass through, so an unset secret still shows as unset.

Why suffix/exact matching, not substring

Audit records and config already contain fields such as input_tokens, cache_read_input_tokens, token_endpoint, token_budget_fraction, broker_token_dir, auth_mode, vulnfix_key and access_key_id, and masking any of those would destroy audit value. For the same reason a tokens (plural) suffix is deliberately left out.

I checked every key emitted by the build_* builders in audit.py, plus the agent's config and usage field names: none of them match. The accepted false positive is any string under a key ending in e.g. token or secret, such as a future next_page_token or an operator-named repo_properties field. That trade-off is documented next to the rules.

Tests

In tests/test_audit.py:

  • test_sensitive_key_values_masked: every exact name and suffix, snake/camel/kebab case, nesting, and lists of dicts.
  • test_extended_sensitive_key_shapes_masked: parametrized over credentials, aws_secret_access_key, private_key_pem, proxy-authorization, plurals, and more.
  • test_sensitive_key_non_string_values: list, tuple, bytes, nested dicts, empty values and ints.
  • test_non_sensitive_token_and_key_fields_preserved: real usage, endpoint, mode and identifier fields come through unchanged.
  • test_non_string_key_not_treated_as_sensitive.

Mutation-tested: removing any exact name, suffix or substring, the _/- normalization, list or bytes masking, the empty-value guard, or the non-string key guard each makes at least one test fail. Full agent suite: 1154 passed, 1 skipped.

Scope note

This covers the structured audit path (_serialize → _clean). The stream-event path (agent/_stream_events.py) still uses shape-based redact() only, and isn't changed here.

🤖 Generated with Claude Code

redact() matches secrets by shape, so a bare secret value under a
credential-named key ({"client_secret": "x"}) reached the audit JSONL
unmasked. _clean now masks string values whose normalized key name is a
known credential name or ends in secret/password/passwd/token/apikey/
privatekey/secretkey. Exact/suffix matching only, so usage and identifier
fields (input_tokens, token_endpoint, auth_mode, vulnfix_key) are kept.

Closes #37

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@schenksj
schenksj requested a review from a team as a code owner October 5, 2026 15:30
@CLAassistant

CLAassistant commented Oct 5, 2026 •

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ schenksj
❌ xxxxxxxxxxxxx
You have signed the CLA already but the status is still pending? Let us recheck it.

schenksj and others added 2 commits October 5, 2026 11:45
The masked-keys test only used keys ending in a bare suffix, so removing
the "_" normalization (which api_key/private_key/secret_key depend on) or
dropping cookie/setcookie/passwd/privatekey/secretkey left it green. Add a
key for each; every such mutant now fails.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@schenksj
schenksj requested a review from mihirv0ra October 5, 2026 15:51
Address the non-blocking review items on #40:
- Mask list/tuple elements and bytes under a sensitive key (multi-valued
  Set-Cookie, lists of tokens); dicts are still walked so their own keys
  decide. Empty values and ints still pass through.
- Add the credential/credentials suffixes from #37, plus secrets,
  passwords, passphrase, apikeys, accesskey (aws_secret_access_key,
  access_key), the exact name proxy-authorization, and a "privatekey"
  substring so private_key_pem is caught.
- Document the accepted suffix false positives (e.g. next_page_token,
  operator-named repo_properties).
- Tests pin every new rule, list/bytes masking and the non-str key guard;
  each corresponding mutant fails. No real audit/config field is matched.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

audit: _clean() redacts values but never keys — secrets under credential-named keys reach the audit stream unmasked

3 participants