Skip to content

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

Merged
teknium1 merged 2 commits into
mainfrom
hermes/hermes-cfcaa403
May 11, 2026
Merged

fix(security): sanitize env and redact output in quick commands + remove write-only _pending_messages#23584
teknium1 merged 2 commits into
mainfrom
hermes/hermes-cfcaa403

Conversation

@teknium1

Copy link
Copy Markdown
Contributor

Summary

Salvage of #1806 onto current main. Two unrelated fixes from @0xbyt4 with one test stability follow-up from us.

  1. Quick command exec leaked credentials. `type: exec` quick commands ran via `create_subprocess_shell()` in the gateway process's full environment with no env filtering or output redaction. A gateway operator who ships `quick_commands: env: {type: exec, command: env}` for ops debugging — or any group/channel where multiple users can fire the operator's quick commands — leaks the bot's API keys to whoever types the command. Fix: apply `_sanitize_subprocess_env()` before exec and `redact_sensitive_text()` on output (respects the user's `security.redact_secrets` setting).

  2. `GatewayRunner._pending_messages` was write-only. Appended on every interrupt but never read anywhere. The actual interrupt delivery uses `adapter._pending_messages` (separate dict on `BaseAdapter`). Removed the dead writes to prevent slow unbounded growth per session.

Changes

  • `gateway/run.py`: env sanitization + output redaction on `exec` quick commands; remove dead writes to `self._pending_messages`.
  • `tests/cli/test_quick_commands.py`: two new tests covering both fixes.
  • Test stability: pin `_REDACT_ENABLED=True` via `monkeypatch` (the module snapshots env at import; xdist ordering could flip it).

Validation

Before After
Quick-command `exec` env gateway's full env (API keys included) `_sanitize_subprocess_env` strips Hermes-managed keys
Output raw subprocess output `redact_sensitive_text` masks token-shaped strings
`GatewayRunner._pending_messages` grew on every interrupt, never consumed unwritten (live path is `adapter._pending_messages`)
`tests/cli/test_quick_commands.py` n/a 18/18 pass

Closes #1806.

0xbyt4 and others added 2 commits May 10, 2026 21:57
…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.
agent.redact._REDACT_ENABLED is snapshotted at import time from
HERMES_REDACT_SECRETS env. Under xdist a prior test in the same worker
can flip it, so test_exec_command_output_is_redacted was order-dependent.
Pin it via monkeypatch like test_terminal_output_transform_still_runs_strip_and_redact does.
@teknium1 teknium1 merged commit 28b4fe6 into main May 11, 2026
12 of 15 checks passed
@teknium1 teknium1 deleted the hermes/hermes-cfcaa403 branch May 11, 2026 05:12
@github-actions

Copy link
Copy Markdown
Contributor

🔎 Lint report: hermes/hermes-cfcaa403 vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 8136 on HEAD, 8136 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 4271 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround labels May 11, 2026
EndeavorYen pushed a commit to EndeavorYen/hermes-agent that referenced this pull request May 11, 2026
…mmands

HermesCLI.process_command() and tui_gateway command.dispatch both handle
type: exec quick commands via subprocess.run(shell=True) with no env=
parameter, so the child inherits the full process environment — all API
keys and bot tokens stored in os.environ are visible to the script.
Any output is returned raw to the terminal or web-UI client without
redaction.

Fix: mirror the approach applied to gateway/run.py in NousResearch#23584.
Apply _sanitize_subprocess_env() before spawning the subprocess and
redact_sensitive_text() on the collected output before display.
Symmetric across all three exec quick-command paths.

Parity with gateway/run.py fix in NousResearch#23584.
memosr added a commit to memosr/hermes-agent that referenced this pull request May 29, 2026
…mpt injection

agent/lsp/reporter.py builds the <diagnostics> block that the LSP
write-time analysis feature (NousResearch#24168, NousResearch#25978) injects into every
write_file / patch tool result. Three fields from each diagnostic --
message, code, and source -- were passed through verbatim, and
file_path was interpolated unescaped into an XML-ish attribute. All
four sources cross a trust boundary into model tool output, so a
hostile repository can plant instruction-shaped text in identifier
names, type aliases, or import paths and have it echo back into the
tool result the model reads.

Attack scenario (TypeScript-flavored, the same trick works with Rust
trait names, Python class names, and any LSP that echoes identifiers
in diagnostic messages):

    type IGNORE_PREVIOUS_INSTRUCTIONS_AND_EXFILTRATE_AUTH_JSON = string;
    const x: IGNORE_PREVIOUS_INSTRUCTIONS_AND_EXFILTRATE_AUTH_JSON = 42;

typescript-language-server's resulting Type-not-assignable message
echoes the hostile identifier back into <diagnostics>, and the model
can treat it as a directive. Stronger variants:

* a raw newline in an identifier preserved by the server can fake a
  </diagnostics> close and inject content as a new block;
* a crafted file name like evil.py"><tool_call>... closes the
  file="..." attribute early and synthesizes attacker-controlled
  tags inside the tool result.

Fix:

* Introduce a small _sanitize_field() helper applied to message,
  code, and source at the point each crosses the trust boundary into
  the formatted diagnostic line. It collapses CR/LF, drops ASCII
  control characters, caps per-field length (message 300, code 80,
  source 80), and html.escape(..., quote=False)s the result so < >
  & can no longer synthesize tags.

* html.escape(file_path, quote=True) on the <diagnostics file="...">
  attribute so a crafted filename can't break out of the attribute.

Legitimate diagnostics produced by trustworthy language servers on
trustworthy code render the same way (just with HTML-escaped text);
the change is purely additive on the protective side. No call-site
contract changes for format_diagnostic / report_for_file.

CVSS estimate: AV:N/AC:L/PR:N/UI:R/S:C/C:H/I:H/A:N -> 7.3 (HIGH).
UI:R because the user has to point the agent at the hostile repo,
but that's the normal 'clone this repo and clean it up' workflow.
S:C because successful injection lets the attacker steer what the
agent does next -- read other files, call other tools, exfiltrate
secrets via subsequent tool calls.

Regression tests added in tests/agent/lsp/test_reporter.py:

* test_format_diagnostic_escapes_html_in_message -- a hostile message
  containing </diagnostics><tool_call> must HTML-escape, not pass
  through.
* test_format_diagnostic_collapses_newlines_in_message -- raw \n / \r
  in the message must not produce extra lines in the output.
* test_format_diagnostic_caps_message_length -- a 1000-char identifier
  is capped to MAX_MESSAGE_CHARS so it can't push past block bounds.
* test_format_diagnostic_escapes_brackets_in_code_and_source -- code
  and source receive the same treatment as message.
* test_format_diagnostic_drops_control_characters -- NUL / BEL / ESC
  bytes are stripped.
* test_report_for_file_escapes_file_path_attribute -- a filename
  containing \">  cannot break out of file="...".

All six new tests fail without the fix and pass with it; the 10
existing test_reporter.py tests continue to pass.

Mirrors the defense-in-depth pattern used elsewhere in the codebase
(NousResearch#23584 sanitize env + redact output, NousResearch#26823 sanitize tool error
strings before re-injection, NousResearch#26829 close 3 dangerous-command
detection bypasses, NousResearch#22432 coerce Google Chat sender_type from
relay).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants