Skip to content

fix(redact): close emission gaps - env suffix keys, control-char splits, process(list) (#77484) - #80643

Closed
thatssoheil wants to merge 2 commits into
NousResearch:mainfrom
thatssoheil:fix/redaction-emission-gaps
Closed

fix(redact): close emission gaps - env suffix keys, control-char splits, process(list) (#77484)#80643
thatssoheil wants to merge 2 commits into
NousResearch:mainfrom
thatssoheil:fix/redaction-emission-gaps

Conversation

@thatssoheil

Copy link
Copy Markdown
Contributor

Fixes #77484

Closes the three-part redaction emission class verified live on main:

1. _ENV_ASSIGN_RE misses *_KEY/*_PASS/*_PW (+ lowercase env names)

  • Added bare KEY/PASS/PW suffixes to _SECRET_ENV_NAMES (FAL_KEY=…, MYSQL_PASS=…, DB_PW=… now mask)
  • New _ENV_ASSIGN_LOWER_RE catches lowercase env names (openai_key=…) — underscore-boundary only, so password=/token= in prose/URLs/form bodies are untouched (URL guard: skipped when :// present)
  • _key_has_secret_keyword all-caps branch now uses word-boundary matching: KEYBOARD=/PASSAGE= (prose) stay, API_KEY/FAL_KEY/DB_PW mask

2. Control-char / zero-width token splits escape every regex

sk-abc\x1bdef…, ghp_abc\n123…, zero-width splits — the contiguous _PREFIX_RE couldn't match across the split, leaking the secret verbatim. New _mask_control_split_tokens strips control chars for matching (token becomes contiguous, matching even when each fragment alone is too short), then masks the original span — with guard rails so unrelated env lines (SHELL=/bin/bash\nEXA_API_KEY=…) never get joined into a false match.

3. process(action=list) returns unredacted output

tools/process_registry.py:2495 returned raw command[:200] + output_preview[-200:] with no redaction wrap, unlike poll/log/wait/kill. The list branch now runs each entry through _redact_process_result.

Tests

  • 8 new redact regression tests (TestBareSecretEnvSuffixes, TestControlCharSplitTokens) — RED on old code
  • 1 new process_registry test (test_list_redacts_command_and_output) — RED on old code
  • Full tests/agent/test_redact.py + tests/tools/test_process_registry.py + browser/kanban redaction suites: 144/144 green

Copilot AI lite review requested due to automatic review settings August 6, 2026 23:26

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 tool/terminal Terminal execution and process management area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 6, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #77768, which already addresses the #77484 redaction and process-list gaps and also covers checkpoint persistence and masked-display control characters. The control-character handling differs, so these are competing implementations rather than duplicates; consider consolidating.

@thatssoheil

Copy link
Copy Markdown
Contributor Author

Review resolved (dbbf447): the reviewer found 4 of 9 new tests false-passed on old code — test_lowercase_env_name_masks used a sk- value (old _PREFIX_RE masked it anyway), and the 3 control-split tests asserted the whole token absent (trivially true since the splitter fragments it in ANY output, even a full leak).

Fixed:

  • lowercase-env test now uses an opaque value (openai_key=xyzzyplugh1234567890abcd) only the env-name regex can catch
  • control-split tests now assert the longest fragment is absent, and use bare tokens (no KEY= context)

Re-verified RED-GREEN: on old code 5/8 new tests fail (suffix keys, lowercase env, all 3 control-splits); on new code 8/8 pass. Full 4-suite run: 144/144 green.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #80965 — all four of your commits were cherry-picked with your authorship preserved (you show as author on main for each).

The salvage combined the best halves of your two PRs: this PR's body (span-local control-split masking, word-bounded all-caps matching, the hardened #77484 tests, process(list) via inline map) plus the two pieces #77768 had that this one lacked — the mask_secret control stripping (#55319/#55321) and the processes.json checkpoint redaction.

One follow-up landed on top: CI caught that _mask_control_split_tokens joined a complete token at end-of-line with the next text line (browser accessibility annotations lost "button [ref=e3]" into the mask). Fixed by skipping the join when a fragment already matches _PREFIX_RE on its own — all the smuggling shapes still mask.

Thanks for the thorough work across both iterations, including hardening your own tests after the first review round.

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

Labels

area/auth Authentication, OAuth, credential pools comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/terminal Terminal execution and process management type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(security): emission gaps — process(list) raw, traceback in tool result, *_KEY regex miss, control-char split, ACP plain formatter

4 participants