Skip to content

fix(secrets): close redaction gaps - env-name variants, control-char splits, process(list) - #77768

Closed
thatssoheil wants to merge 4 commits into
NousResearch:mainfrom
thatssoheil:fix/secrets-redaction-gaps
Closed

fix(secrets): close redaction gaps - env-name variants, control-char splits, process(list)#77768
thatssoheil wants to merge 4 commits into
NousResearch:mainfrom
thatssoheil:fix/secrets-redaction-gaps

Conversation

@thatssoheil

@thatssoheil thatssoheil commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #77484 — closes three verified secret-redaction gaps (all reproduced live on current main before fixing):

1. _ENV_ASSIGN_RE misses *_KEY/*_PASS/*_PW + lowercase env keys

FAL_KEY=sk-abc123, OPENAI_KEY=sk-def456, MYSQL_PASS=ghi789, DB_PW=jkl012, and lowercase openai_key=sk-def456 all leaked verbatim through redact_sensitive_text, while MY_API_KEY / GITHUB_TOKEN masked correctly.

  • _SECRET_ENV_NAMES gains bare KEY / PASS / _?PW alternatives.
  • _SECRET_CFG_NAMES (line-anchored + dotted config passes) gains key / pass / pw, so lowercase .env keys like openai_key= mask without touching mid-string prose or URLs.
  • _KEY_KEYWORD_RE (word-boundary gate) gains bare key / pass / pw so openai_key / mysql_pass pass validation while monkey / keyboard / passage / secretary stay untouched.

2. Control-char token splitting

sk-abc123\x1bsk-def456, sk-abc123\u200bsk-def456, and CR-split tokens passed every regex unmasked. redact_sensitive_text now strips C0 controls (except \n/\t) + zero-width/format chars before matching, and normalizes \r → \n. \n/\t are preserved (they carry line structure; the line-anchored _CFG_*_RE passes depend on it).

3. process(action=list) unredacted

tools/process_registry.py returned raw list_sessions(...) JSON (raw command[:200] + output_preview[-200:] with inline secrets), unlike poll/log/wait/kill which are wrapped in _redact_process_result. Added _redact_process_list mapping the existing redactor over each entry.

Tests

  • tests/agent/test_redact.py: env-name gap (bare suffixes, lowercase, prose-not-mangled), control-char splits (ESC/zero-width/CR, newline-structure preserved).
  • tests/tools/test_process_registry.py: process(action=list) redacts command + output_preview.
  • Verified: test_redact.py 80/80, test_process_registry.py 55/55, redaction-adjacent suites 62/62, ruff clean.

Also fixes #55319 and #55321 (mask_secret() no longer emits control bytes in the visible mask head/tail — the prior PR #58079 was closed without merging).

@thatssoheil
fix(secrets): close redaction gaps - env-name variants, control-char …
2a1c462 
…splits, process(list)

- _ENV_ASSIGN_RE / _SECRET_CFG_NAMES now recognize bare KEY/PASS/PW
  suffixes (FAL_KEY, MYSQL_PASS, DB_PW) and lowercase .env keys via the
  guarded config passes; prose (author=, monkey=, keyboard=) untouched.
- redact_sensitive_text strips C0 controls (except \n/\t) and zero-width
  chars before matching, and normalizes \r -> \n, so secrets split by
  ESC/CR/zero-width no longer escape every regex.
- process(action=list) now wraps entries in _redact_process_result via
  _redact_process_list, matching poll/log/wait.

Closes NousResearch#77484.
Copilot AI review requested due to automatic review settings August 3, 2026 14:22
Copilot AI reviewed Aug 3, 2026

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have labels Aug 3, 2026
@alt-glitch

alt-glitch commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related: #55321 and #55319 cover a narrower control-character redaction facet. This PR also addresses env-name and process-list emission gaps; please consolidate overlapping redaction changes before merge.

@thatssoheil
fix(redact): strip control chars from mask_secret display (NousResear…
64ee645 
…ch#55319, NousResearch#55321)

A masked secret's visible head/tail could carry control bytes (newline,
NUL, DEL, C1 0x80-0x9F, zero-width) into config/status/dump output.
Strip every control incl. \n/\t (display differs from redact_sensitive_text,
which preserves \n/\t as line structure) before slicing; all-control values
return the configured empty fallback.

Consolidates the previously-closed NousResearch#58079 approach (strip controls before
masking) - supersedes it.
@thatssoheil
fix(secrets): redact command in process checkpoint file (NousResearch…
fb8a210 
…#77484)

_write_checkpoint persisted s.command verbatim to ~/.hermes/processes.json.
Recovery only uses command for display/logging (the process is already
running; adoption re-validates PID + start time, never re-runs the
command), so masking is lossless.
@thatssoheil
fix(redact): drop dead _HAS_CR_RE constant, fix comment
ec85ad3 
Review cleanup: _HAS_CR_RE was defined but never used (the C0 class
already covers \r, and the call site guards \r inline). The 'gate +
full pattern' comment referenced a gate that no longer exists.
@thatssoheil

thatssoheil commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the consolidation nudge — I agree, and I've folded #55319/#55321 into this PR.

What I added:

  • mask_secret() now strips ALL control bytes (C0 incl. \n/\t, C1, DEL, zero-width) from the visible head/tail before slicing. The Masked secret display can preserve control characters #55319 probe (mask_secret("ab\ncd0123456789zzzz") previously returned 'ab\nc...zzzz' with a raw newline in the mask) now returns 'abcd...zzzz'; a pure-control value returns the configured empty fallback.
  • Regression tests in TestMaskSecretControlStripping (newline, C1, printable-unchanged, all-control → empty).

Verified on current main that #55319 and #55321 are both still open (prior PR #58079 was closed, not merged), so this closes the gap they describe. Updated the PR body to note both are fixed by this change.

Also pushed a review-cleanup commit (dead _HAS_CR_RE constant removed, misleading comment fixed) and a checkpoint-persistence fix — _write_checkpoint() now redacts command before writing ~/.hermes/processes.json, which was the one persisted surface the original fix missed.

@alt-glitch alt-glitch mentioned this pull request Aug 6, 2026
Closed
This was referenced Aug 7, 2026
Merged
Merged
Open
@kshitijk4poor

kshitijk4poor commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Merged via #80965, which combined this PR with your #80643 — your authorship is preserved on main for all four commits.

What survived from each: #80643's implementation was taken for the shared #77484 scope (its span-local control handling preserves ANSI/ZWJ output and its word-bounded all-caps matching avoids the KEYBOARD=/MONKEY= prose false positives; this PR's global strip and the \b in _SECRET_ENV_NAMES regressed those — the \b made suffixed keys like MY_TOKEN_2= leak where main masked them). The two pieces unique to THIS PR were cherry-picked on top exactly as you wrote them:

Closing as superseded-and-merged. Thanks for covering the full emission surface — the checkpoint file was a gap nobody else had touched.

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

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have type/security Security vulnerability or hardening

Projects

None yet

Milestone

No milestone

4 participants

@thatssoheil @alt-glitch @kshitijk4poor