Skip to content

fix(security): redact secrets in background process notifications - #59858

Closed
necoweb3 wants to merge 2 commits into
NousResearch:mainfrom
necoweb3:fix/background-notification-redact
Closed

fix(security): redact secrets in background process notifications#59858
necoweb3 wants to merge 2 commits into
NousResearch:mainfrom
necoweb3:fix/background-notification-redact

Conversation

@necoweb3

@necoweb3 necoweb3 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Apply _redact_process_result() to background process completion and watch_match notifications before enqueuing them in the completion_queue. Previously these notifications only applied strip_ansi(), leaving API keys, tokens, and other secrets in process output unmasked when injected into the LLM conversation.

The bug

Two code paths deliver background process output to the LLM:

  1. Explicit process tool (poll/log/wait) — calls _redact_process_result() which applies redact_terminal_output()
  2. Automatic notifications (notify_on_complete, watch_patterns) — only applies strip_ansi()

When a background process finishes or matches a watch pattern, its raw output is formatted as [IMPORTANT: ...] and injected as a user message to the LLM. If that output contains API keys, tokens, passwords, or other secrets, they reach the model context unmasked.

Example

A background env or printenv command completes with notify_on_complete=true. The process output contains OPENAI_API_KEY=sk-.... The explicit process(action='poll') path would redact this to OPENAI_API_KEY=REDACTED. The notification path injects OPENAI_API_KEY=sk-... verbatim into the LLM context.

Fix

tools/process_registry.py — 2 changes:

  • _move_to_finished() (line ~1085): Extract notification dict to variable, apply _redact_process_result(), then enqueue
  • _check_watch_patterns() (line ~340): Same pattern — extract, redact, enqueue

Both paths now match the explicit process tool behavior. 8 lines added, 4 removed.

Tests

tests/tools/test_process_registry.py — 2 new tests:

  • test_completion_notification_redacts_secret — verifies _move_to_finished redacts API keys in completion notifications
  • test_watch_match_notification_redacts_secret — verifies _check_watch_patterns redacts secrets in watch_match notifications

necoweb3 added 2 commits July 7, 2026 00:14
Apply _redact_process_result() to completion and watch_match
notifications before enqueuing them in the completion_queue.

Previously, the explicit process tool path (poll/log/wait) applied
redact_terminal_output() via _redact_process_result(), but the
automatic notification delivery path (notify_on_complete, watch_patterns)
only applied strip_ansi(). This meant API keys, tokens, and other
secrets from background process output were injected into the LLM
conversation unmasked.

The fix ensures both code paths apply the same redaction, matching
the foreground terminal tool behavior.
Add TestNotificationRedaction class with two tests:

1. test_completion_notification_redacts_secret — verifies _move_to_finished
   redacts API keys in completion notifications before enqueueing
2. test_watch_match_notification_redacts_secret — verifies _check_watch_patterns
   redacts secrets in watch_match notifications before enqueueing

These tests cover the gap identified in NousResearch#43025 where the explicit process
tool path (poll/log/wait) was redacted but the automatic notification
delivery path was not.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data needs-repro Bug needs reproduction steps P3 Low — cosmetic, nice to have labels Jul 6, 2026

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

Thanks for finding a real notification-path redaction gap. Current main still enqueues raw output in both automatic paths: tools/process_registry.py:347-361 for watch_match and tools/process_registry.py:1092-1105 for completion events; formatted notifications then include that output at tools/process_registry.py:2162-2175.

Problems

  • The new watch test fails in PR CI. tests/tools/test_process_registry.py:1902 calls _check_watch_patterns with three arguments, but current main defines it as _check_watch_patterns(self, session, new_text) at tools/process_registry.py:235; CI reports TypeError.
  • The completion hunk needs manual salvage because current main added the stable started_at event field at tools/process_registry.py:1101-1104; git apply --check rejects the hunk.

Suggested changes

  • Call _check_watch_patterns(sess, "API_TOKEN=ghp_abc123def456") in the watch test, then keep asserting the drained formatted notification is redacted.
  • Preserve started_at while wrapping the current completion event with _redact_process_result() before enqueueing it.

Automated hermes-sweeper review.

reg._running[sess.id] = sess
monkeypatch.setattr(pr, "process_registry", reg)

reg._check_watch_patterns(sess, ["API_TOKEN=ghp_abc123def456"], "API_TOKEN")

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.

This uses an obsolete three-argument call. _check_watch_patterns now accepts only (session, new_text), so CI fails with TypeError; pass the matching output as a string, e.g. "API_TOKEN=ghp_abc123def456".

@teknium1

Copy link
Copy Markdown
Contributor

Thanks @necoweb3 — your enqueue-site redaction landed in PR #85923 with your commits and authorship preserved via cherry-pick (plus a small test-signature repair on top). Closing this original as merged-via-salvage.

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

Labels

comp/tools Tool registry, model_tools, toolsets needs-repro Bug needs reproduction steps P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

3 participants