Skip to content

fix(security): sanitize env and redact output in quick commands + remove write-only _pending_messages#1806

Closed
0xbyt4 wants to merge 1 commit into
NousResearch:mainfrom
0xbyt4:fix/gateway-quickcmd-leak-pending-messages
Closed

fix(security): sanitize env and redact output in quick commands + remove write-only _pending_messages#1806
0xbyt4 wants to merge 1 commit into
NousResearch:mainfrom
0xbyt4:fix/gateway-quickcmd-leak-pending-messages

Conversation

@0xbyt4

@0xbyt4 0xbyt4 commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  1. Quick command exec leaks credentials — type: exec quick commands ran via create_subprocess_shell() in the gateway process's full environment. Unlike terminal_tool (which uses _sanitize_subprocess_env), quick commands had no env filtering or output redaction. A command like env would return all API keys and bot tokens to the chat.

    Fix: apply _sanitize_subprocess_env() before exec and redact_sensitive_text() on output.

  2. _pending_messages write-only memory leak — GatewayRunner._pending_messages was appended on every interrupt but never read. The actual interrupt delivery uses adapter._pending_messages (separate dict). Removed the dead writes to prevent unbounded growth.

Test plan

  • test_exec_command_does_not_leak_credentials — sets OPENROUTER_API_KEY in env, runs env, verifies key not in output
  • test_exec_command_output_is_redacted — command outputs API key pattern, verifies redacted
  • 19 existing quick command + gateway tests pass, 0 regressions

@0xbyt4
fix(security): sanitize env and redact output in quick commands + rem…
479813c 
…ove write-only _pending_messages

1. Quick command exec ran in the gateway process's full environment
   without env sanitization or output redaction. A quick command like
   "env" or "printenv" would leak all API keys, OAuth tokens, and
   bot credentials to the messaging user.

   Fix: apply _sanitize_subprocess_env() before exec and
   redact_sensitive_text() on output before returning.

2. GatewayRunner._pending_messages was written on every interrupt
   (lines 1331-1334) but never read or consumed anywhere. The actual
   interrupt delivery uses adapter._pending_messages (a separate dict).
   Removed the write-only accumulation to prevent unbounded growth.
@nidhishgajjar

nidhishgajjar commented Apr 15, 2026

Copy link
Copy Markdown

Orb Code Review (powered by GLM 5.1 on Orb Cloud)

Summary

This PR fixes two issues:

  1. Security: Quick command credential leakage — exec-type quick commands ran subprocesses with the gateway process's full environment (containing all API keys/bot tokens) and returned raw output. Now uses _sanitize_subprocess_env() and redact_sensitive_text().
  2. Memory leak: Write-only _pending_messages — GatewayRunner._pending_messages was appended on every interrupt in this code path but never consumed. The actual delivery uses adapter._pending_messages. Removed the dead writes.

Architecture

The fix plugs a genuine security hole. Quick commands run in the gateway process which holds all provider API keys. Without sanitization, a command like env would dump credentials to chat. The chosen approach reuses two existing, well-tested utilities:

  • _sanitize_subprocess_env() from tools.environments.local — strips the Hermes-managed secret blocklist
  • redact_sensitive_text() from agent.redact — catches patterns that slip through env filtering

Defense-in-depth: env sanitization prevents credential injection into child processes; output redaction catches any secrets the command itself generates.

Issues

Warning — _sanitize_subprocess_env is a private function (underscore-prefixed) from tools.environments.local. Importing it from gateway/run.py creates a cross-module coupling to an internal API. Consider either:

  • Making it public (sanitize_subprocess_env) if it's intended for reuse, or
  • Adding a brief comment acknowledging the intentional coupling

This is minor — the function is stable and the import is pragmatic. Not a blocker.

Suggestion — The imports are done inside the function body (from tools.environments.local import ... / from agent.redact import ...). This is fine for quick commands (not hot-path), but if this pattern grows, consider top-level imports or a shared security utilities module.

Cross-file impact

  • gateway/run.py — main changes
  • tests/test_quick_commands.py — two new test cases
  • tools/environments/local.py — imported but not modified
  • agent/redact.py — imported but not modified

No changes to the imported modules. No impact on other callers.

Assessment

approve ✅ — Important security fix with good test coverage. The credential leakage was a real vulnerability. The _pending_messages cleanup is a clean bonus fix.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery labels May 3, 2026
@teknium1 teknium1 mentioned this pull request May 11, 2026
Merged
@teknium1

teknium1 commented May 11, 2026

Copy link
Copy Markdown
Contributor

Merged via PR #23584. Your commits were cherry-picked onto current main with your authorship preserved (f6736ced8). Added one small follow-up pinning _REDACT_ENABLED=True via monkeypatch in the redaction test, since agent.redact._REDACT_ENABLED is snapshotted at import and xdist ordering could flip it. Thanks for both fixes — the credential-leak path was real, and the dead-write _pending_messages cleanup was a clean catch.

@AhmetArif0 AhmetArif0 mentioned this pull request May 11, 2026
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround type/security Security vulnerability or hardening

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@0xbyt4 @nidhishgajjar @teknium1 @alt-glitch