fix(redact): non-reusable sentinel for prefix secrets in file reads (#35519) - #54166
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM (approve-ready, COMMENT due to token permissions)
Security fix for #35519: when file content is read by the agent (read_file/search_files), prefix-matched credentials are now redacted to a non-reusable sentinel (e.g. "redacted:ghp_...") instead of a head/tail-preserving mask that looks like a real-but-truncated key.
Changes
- New _mask_token_nonreusable() function produces syntactically invalid sentinels
- redact_sensitive_text() gains file_read parameter that implies code_file=True
- file_tools.py updated to pass file_read=True for all read operations
- Comprehensive test class with 6 test cases covering: sentinel shape, no secret body leak, non-plausible-key check, default mode unchanged, code_file implication, and sk- prefix
Looks Good
- Sentinel markers (brackets, ellipsis, colon) make the redaction unambiguous
- Default mode (logs/display) preserves existing behavior — only file content gets the sentinel
- The agent can still identify which credential is present via the vendor prefix
- Well-documented docstring explains the rationale and the issue it fixes
Reviewed by Hermes Agent
🔎 Lint report:
|
…35519) When security.redact_secrets is on (default), read_file/search_files/cat applied redact_sensitive_text(code_file=True) to file content, which still ran prefix masking. An API key in config.yaml (ghp_..., sk-..., xai-..., etc.) came back as a head/tail mask like `ghp_S1...Pn2T` — a plausible-looking truncated key. When an agent read that and wrote it back to config, the masked value replaced the real credential, silently breaking auth (401). Production evidence: a config.yaml found containing the exact 13-char masked GitHub PAT. The two community PRs (#35529, #35534) fixed the corruption by NOT redacting prefixes for config reads — but that exposes the user's real keys to the agent context, model, and logs (a security regression). This takes the safer route: keep redacting, but for file content emit a NON-REUSABLE sentinel. - New `_mask_token_nonreusable`: prefix secrets -> `«redacted:ghp_…»` (vendor label preserved for debuggability; zero secret bytes; angle-bracket/ellipsis wrapper is syntactically invalid as a token so it can't be mistaken for or written back as a usable key). - New `redact_sensitive_text(file_read=True)` routes prefix matches through it (implies code_file=True). Default/log/display mode is UNCHANGED — `_mask_token` still keeps head/tail (fine for logs, never written back). - Wired the 3 file_tools.py call sites (read_file / search_files / cat) to file_read=True. Fixes both the corruption AND avoids the secret-exposure of the un-redact approach. 6 new tests (sentinel shape, no-leak, not-a-plausible-key, default mode unchanged, file_read implies code_file, sk- prefix); 88 redact tests pass; mutation-verified (reverting to the old mask fails the sentinel/leak tests). Co-authored-by: liuhao1024 <sunsky.lau@gmail.com> Co-authored-by: adammatski1972 <289282750+adammatski1972@users.noreply.github.com> Closes #35519. Supersedes #35529, #35534.
…llapse The salvaged #35519 regression guard asserted that default (non-file_read) mode keeps a head/tail `ghp_S1...Pn2T` mask for a `token: <key>` line. On current main the YAML config pass (`_YAML_ASSIGN_RE`, key `token`) re-masks the already-prefix-masked value to `***`, so the assertion was stale. Switch to a bare-token context so the guard isolates what it claims (prefix-mask head/tail shape in default mode) without depending on the YAML collapse.
fc68198 to
3237b5e
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Summary
A user's API keys are no longer corrupted (or exposed) when the agent reads a config/data file. Prefix-matched credentials in file content are redacted to a non-reusable sentinel (
«redacted:ghp_…») instead of a plausible-looking truncated key — so the agent can't write the masked value back and silently break auth.Root cause (#35519)
With
security.redact_secretson (the default),read_file/search_files/catappliedredact_sensitive_text(code_file=True)to file content.code_file=Trueskips the ENV/JSON patterns but still runs prefix masking, so an API key inconfig.yaml(ghp_…,sk-…,xai-…) came back as a head/tail mask likeghp_S1...Pn2T— a plausible-looking truncated key. When the agent read that value and wrote it back (re-configuring, building a curl), the mask replaced the real credential → 401. The issue cites a productionconfig.yamlfound containing exactly that 13-char masked PAT.Why not just stop redacting config files?
The two community PRs (#35529
config_file=True, #35534redact_prefixes=False) fix corruption by not masking prefixes for config reads — but that returns the user's real, unmasked keys into the agent context, the model, and any logs. This PR keeps redacting while making the output non-destructive: zero secret bytes emitted, and the sentinel is syntactically invalid as a token so it can't round-trip into a dead key.Scope audit (the B in "salvage + widen")
Audited every
redact_sensitive_text(code_file=True)call site. The read-config-then-write-back vector is only the threefile_tools.pysites.code_execution_tool.py(sandbox stdout/stderr) andprocess_registry.py(command echo) are program output / display, not file content the agent re-saves — head/tail masking is correct there and switching them would gain nothing. Terminalcatdisplay goes throughredact_terminal_output, which the issue explicitly calls out as a separate, fine display path. No widening needed — the 3-site scope is exactly right.Changes
agent/redact.py: new_mask_token_nonreusable()(prefix →«redacted:<label>…», vendor prefix kept for debuggability, no secret bytes) +redact_sensitive_text(file_read=True)routes prefix matches through it (impliescode_file=True). Log/display mode unchanged.tools/file_tools.py: 3 sites (read_file,search_files,cat) switchedcode_file=True→file_read=True.tests/agent/test_redact.py: 6 new tests (sentinel shape, no-leak, not-a-plausible-key, default mode unchanged,file_readimpliescode_file,sk-prefix).Validation
GITHUB_PERSONAL_ACCESS_TOKEN: ghp_…ghp_S1...Pn2T(looks real → 401)«redacted:ghp_…»api_key: sk-proj-…sk-pr...4321«redacted:sk-…»xai_key: xai-…xai-A...XYZ«redacted:xai-…»max_tokens: 8000tests/agent/test_redact.py— 113 passed.tests/tools/test_file_tools.py— 43 passed.read_file_toolagainst a tempHERMES_HOMEconfig.yaml with real-shapedghp_/sk-/xai-keys — confirmed no secret body/head/tail leaks, no...shape, sentinels present, non-secrets intact.Credit
Diagnosis and the config-read fix direction from @liuhao1024 (#35529) and @adammatski1972 (#35534). Salvage commit authored by @kshitijk4poor (#53146), which implements the safer redact-but-non-destructive variant. One follow-up commit fixes the regression-guard test (the salvaged test asserted stale default-mode behavior — on current
mainatoken: <key>line additionally hits the YAML config pass and collapses to***; the guard now uses a bare-token context to isolate the prefix-mask shape).Closes #35519. Supersedes #35529, #35534, #53146.
Infographic