fix(security): fail-closed when dangerous-command guard cannot be loaded in TUI shell.exec#15542
Open
memosr wants to merge 1 commit into
Open
fix(security): fail-closed when dangerous-command guard cannot be loaded in TUI shell.exec#15542memosr wants to merge 1 commit into
memosr wants to merge 1 commit into
Conversation
4 tasks
Closed
6 tasks
10 tasks
This was referenced May 27, 2026
Contributor
|
Security behavior looks correct; I would merge once the required CI I validated this against current Evidence:
GitHub currently reports the PR as Signed: GPT-5.5-xhigh in Codex |
…ded in TUI shell.exec
tui_gateway/server.py exposes a shell.exec JSON-RPC method that
runs shell commands via subprocess.run(shell=True). It tries to
block dangerous commands using tools.approval.detect_dangerous_command,
but on ImportError silently falls through to executing the command:
try:
from tools.approval import detect_dangerous_command
is_dangerous, _, desc = detect_dangerous_command(cmd)
if is_dangerous:
return _err(rid, 4005, f'blocked: {desc}...')
except ImportError:
pass # <-- silently disables the guard, then runs anyway
If tools.approval cannot be imported (missing module, circular
import, syntax error in a dependency, partial deploy), the screener
is skipped and arbitrary shell commands reach subprocess.run.
Fix:
* Replace the silent ImportError pass with a fail-closed return:
error code 4006 with message 'dangerous-command guard unavailable;
refusing to execute'. Consistent with the fail-closed pattern
applied to the cron SSRF check in NousResearch#10180 and the WebSocket
empty-peer guard in NousResearch#15544.
No behavior change when tools.approval is available -- the existing
detect_dangerous_command path runs unchanged, dangerous commands
still return 4005, and safe commands still flow through to
subprocess.run.
Regression tests added in tests/test_tui_gateway_server.py:
* test_shell_exec_fails_closed_when_guard_unavailable -- injects an
ImportError for tools.approval via a patched builtins.__import__
and a tripwire on subprocess.run; asserts the response is error
4006 with 'guard unavailable' in the message and that
subprocess.run is never reached.
* test_shell_exec_still_runs_when_guard_available -- guards against
over-reach: with tools.approval present and the command not
flagged dangerous, shell.exec must still execute. Without this
the fail-closed change could regress into 'refuses everything'.
Verified locally that the fail-closed test fails without this
patch (the response carries error code 5003 from the tripwire-raised
AssertionError inside subprocess.run, proving the silent-skip path
was reached) and passes with the fix in place. The 205 pre-existing
tests in test_tui_gateway_server.py are unaffected by this change.
Per @egilewski's audit on this PR (NousResearch#15542), the original April patch
targeted the same fail-open path; the file has refactored heavily
since (tui_gateway/server.py is now ~8k lines vs ~4.6k at the
original PR head), so this commit re-applies the fail-closed
behavior at the current shell.exec call site and adds the
regression coverage that the original PR was missing.
0a55150 to
0e8648c
Compare
magnus919
added a commit
to magnus919/hermes-agent-1
that referenced
this pull request
Jun 7, 2026
…guarded_command The quick-commands exec path (command.dispatch) ran bare subprocess.run(..., shell=True) with no dangerous-command detection. This was already partially addressed upstream for shell.exec (fail-closed ImportError pattern). This change: 1. Adds _exec_guarded_command() — a reusable helper that blocks known dangerous patterns, fails closed on ImportError, and returns structured result dicts with uniform error handling 2. Refactors the quick-commands exec path to use it instead of bare subprocess.run(shell=True) The shell.exec handler was already fixed upstream and is not re-touched. Tested: 5/6 local proof tests pass (6th is pre-existing CI issue). 169 tests pass in test_tui_gateway_server.py (1 pre-existing failure in browser_manage unrelated to this change). All 9 command/shell/exec/dispatch tests pass. Refs NousResearch#16560 Supersedes NousResearch#15542 (still open)
|
Closing as superseded by upstream changes + PR #28214.
No action needed on this PR. |
|
Closing — superseded by upstream changes to shell.exec + PR #28214 for quick-commands hardening. |
Contributor
|
Recommendation: close as superseded by current upstream changes. I rechecked this against current Evidence:
The regression test coverage added here is still useful, but the implementation is stale and conflicts with the newer upstream handler. I would close this PR as superseded rather than merge it as-is. Signed: GPT-5.5-xhigh in Codex |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
tui_gateway/server.pyexposes ashell.execJSON-RPC method thatruns shell commands via
subprocess.run(shell=True). It tries toblock dangerous commands using
tools.approval.detect_dangerous_command,but on
ImportErrorit silently falls through to executing the command:If
tools.approvalcannot be imported (missing module, circular import,SyntaxError in a dependency, partial deployment), the dangerous-command
check is silently skipped and arbitrary shell commands are executed.
Fix
Made the handler fail-closed: if the guard cannot be loaded, refuse
to execute the command rather than silently bypassing the check:
This is consistent with the fail-closed pattern applied to the cron
SSRF check in #10180.
Type of Change
Checklist
tools.approvalis available