fix(security): strip shell escapes in command denylist; fail-closed on missing approval module (#36846 #36847) by ashishpatel26 · Pull Request #40370 · NousResearch/hermes-agent

ashishpatel26 · 2026-06-06T08:02:07Z

Summary

Two related security fixes in the command-safety layer.

#36846 — Denylist bypass via shell escape sequences (`tools/approval.py`)

DANGEROUS_PATTERNS and HARDLINE_PATTERNS are matched against the output of _normalize_command_for_detection, which previously only stripped ANSI sequences, null bytes, and Unicode full-width variants. Two shell-level obfuscation techniques bypassed it:

Backslash-escape split: r\m — the shell removes the backslash and executes rm, but the pattern \brm\b never matched.
Empty-quote split: r''m or r""m — the shell concatenates the tokens into rm, but again the pattern didn't match.

Fix: after NFKC normalization, two re.sub calls now strip these constructs before pattern matching:

# Strip shell backslash-escapes: r\m → rm
command = re.sub(r'\([^\n])', r'\1', command)
# Strip empty-string literals that split tokens: r''m → rm, r""m → rm
command = re.sub(r"'' |\"\"", '', command)

re was already imported at the top of tools/approval.py.

Not fixed by this PR (out of scope): command-substitution bypasses such as $(echo rm) require a shell interpreter to evaluate and cannot be handled by static regex matching.

#36847 — Fail-open `except ImportError: pass` in `tui_gateway/server.py`

shell.exec wrapped the entire safety check in a try/except ImportError: pass block, meaning that if tools.approval was not importable for any reason (missing dep, import-time error, broken install), the safety gate was silently skipped and the command executed anyway.

Additionally, only detect_dangerous_command was called — the stricter detect_hardline_command check was absent.

Fix:

except ImportError: pass → except ImportError: return _err(rid, 5001, "shell.exec unavailable: approval safety module not importable") (fail-closed).
Added detect_hardline_command check that runs before detect_dangerous_command.

Files changed

File	Change
`tools/approval.py`	Add backslash-escape and empty-quote stripping to `_normalize_command_for_detection`
`tui_gateway/server.py`	Fail-closed on `ImportError`; add `detect_hardline_command` check

Test plan

Verify r\m -rf / is blocked by DANGEROUS_PATTERNS after normalization
Verify r''m -rf / is blocked after normalization
Verify r""m -rf / is blocked after normalization
Simulate ImportError from tools.approval in shell.exec and confirm a 5001 error is returned instead of the command executing
Verify a known hardline command (e.g. dd if=/dev/zero) is blocked via detect_hardline_command
Verify safe commands still pass through normally

tonydwb

Code Review Summary

Verdict: Approved (with a couple of non-blocking enhancements noted inline)

The implementation matches the stated intent:

tools/approval.py: _normalize_command_for_detection() now strips backslash-escapes and empty-string literals before dangerous-pattern matching, closing the bypass described in the PR. The two re.sub calls are scoped and safe.
tui_gateway/server.py: shell.exec now calls detect_hardline_command before detect_dangerous_command, and the except ImportError: pass has been replaced with a fail-closed 5001 error response, closing the second issue described in the PR.

Reviewed by Hermes Agent

tonydwb · 2026-06-06T08:04:24Z

+        if is_hardline:
+            return _err(
+                rid, 4005, f"blocked (hardline): {hardline_desc}. Use the agent for dangerous commands."
+            )
        is_dangerous, _, desc = detect_dangerous_command(cmd)


Non-blocking: consider logging a metric/warning when the fail-closed ImportError path triggers. A silent 5001 is difficult to distinguish from other 5xx failures in production observability. Worth a follow-up issue.

tonydwb · 2026-06-06T08:04:24Z

@@ -537,6 +537,10 @@ def _normalize_command_for_detection(command: str) -> str:
    command = command.replace('\x00', '')
    # Normalize Unicode (fullwidth Latin, halfwidth Katakana, etc.)
    command = unicodedata.normalize('NFKC', command)
+    # Strip shell backslash-escapes: r\m → rm. Prevents \-injection bypass.
+    command = re.sub(r'\\([^\n])', r'\1', command)


Non-blocking: add a short comment explaining why these two regexes exist (reference #36846) so a future reviewer who sees them in isolation does not mistake them for dead/obfuscated code.

liuhao1024 · 2026-06-06T08:38:27Z

Review: Mixed concerns — security fix bundled with unrelated compression fix

The security changes in tools/approval.py and tui_gateway/server.py look correct:

Backslash-escape stripping (r\m → rm) and empty-string literal stripping (r''m → rm) in _normalize_command_for_detection close real bypass vectors.
Hardline command detection in tui_gateway/server.py is a good defense-in-depth addition.
The ImportError → fail-closed (return _err(5001, ...)) is the right behavior — previously missing approval module silently allowed any command.

However, this PR also includes agent/context_compressor.py + tests/agent/test_context_compressor.py changes (the awaiting_real_usage_after_compression fix for #36718), which are unrelated to security. This is a separate concern that also appears in PR #40373.

The compression fix has a bug (missing __init__ initialization — see #40373 review comment) and should be removed from this PR to keep the security fix focused and mergeable independently.

…d on missing approval module DANGEROUS_PATTERNS and HARDLINE_PATTERNS are matched on the raw command string, so backslash-escape (r\m) and empty-quote split (r''m) bypass both lists. _normalize_command_for_detection now strips these before pattern matching. tui_gateway shell.exec had a bare 'except ImportError: pass' that silently disabled the entire safety gate if tools.approval wasn't importable. Changed to fail-closed (return 5001 error). Added detect_hardline_command check. Fixes NousResearch#36846, NousResearch#36847.

ashishpatel26 · 2026-06-06T10:27:02Z

Thanks @liuhao1024 — you're right. The context_compressor.py and test changes (the #36718 double-compression fix) were accidentally included because the branch was based on local HEAD rather than upstream/main directly.

Cleaned up: reset branch to upstream/main and cherry-picked only the security commit (tools/approval.py + tui_gateway/server.py). The PR now contains exactly the two files it should. Force-pushed.

teknium1 · 2026-06-06T15:28:00Z

Salvaged into #40591 — cherry-picked your commit (authorship preserved). Live-verified the escape bypass on main and after the fix, and added regression tests. Thanks for catching this!

#40591

tonydwb approved these changes Jun 6, 2026

View reviewed changes

alt-glitch added type/security Security vulnerability or hardening comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management P1 High — major feature broken, no workaround labels Jun 6, 2026

ashishpatel26 force-pushed the fix/denylist-bypass-36846-36847 branch from 22fcd15 to 8cade08 Compare June 6, 2026 10:22

ashishpatel26 mentioned this pull request Jun 6, 2026

fix(agent): preserve interleaved thinking+tool_use order to prevent HTTP 400 crash-loop #40373

Closed

4 tasks

teknium1 mentioned this pull request Jun 6, 2026

fix(security): close shell-escape denylist bypass + fail-closed on missing approval module (#36846, #36847) #40591

Merged

teknium1 closed this Jun 6, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(security): strip shell escapes in command denylist; fail-closed on missing approval module (#36846 #36847)#40370

fix(security): strip shell escapes in command denylist; fail-closed on missing approval module (#36846 #36847)#40370
ashishpatel26 wants to merge 1 commit into
NousResearch:mainfrom
ashishpatel26:fix/denylist-bypass-36846-36847

ashishpatel26 commented Jun 6, 2026

Uh oh!

tonydwb left a comment

Uh oh!

tonydwb Jun 6, 2026

Uh oh!

tonydwb Jun 6, 2026

Uh oh!

liuhao1024 commented Jun 6, 2026

Uh oh!

ashishpatel26 commented Jun 6, 2026

Uh oh!

teknium1 commented Jun 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ashishpatel26 commented Jun 6, 2026

Summary

#36846 — Denylist bypass via shell escape sequences (tools/approval.py)

#36847 — Fail-open except ImportError: pass in tui_gateway/server.py

Files changed

Test plan

Uh oh!

tonydwb left a comment

Choose a reason for hiding this comment

Code Review Summary

Uh oh!

tonydwb Jun 6, 2026

Choose a reason for hiding this comment

Uh oh!

tonydwb Jun 6, 2026

Choose a reason for hiding this comment

Uh oh!

liuhao1024 commented Jun 6, 2026

Review: Mixed concerns — security fix bundled with unrelated compression fix

Uh oh!

ashishpatel26 commented Jun 6, 2026

Uh oh!

teknium1 commented Jun 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

#36846 — Denylist bypass via shell escape sequences (`tools/approval.py`)

#36847 — Fail-open `except ImportError: pass` in `tui_gateway/server.py`