security(tui-gateway): fail-closed dangerous-command check in shell.exec / command.dispatch (#16560) - #17045
Closed
0xsir0000 wants to merge 1 commit into
Closed
Conversation
…xec / command.dispatch (NousResearch#16560) The ``shell.exec`` JSON-RPC handler in ``tui_gateway/server.py`` wrapped the ``detect_dangerous_command`` import in ``except ImportError: pass``, so a caller able to make ``tools.approval`` un-importable (deleted, shadowed, broken venv) bypassed the entire safety gate and got a free ``subprocess.run(cmd, shell=True, ...)`` against a JSON-RPC parameter. The ``command.dispatch`` quick-commands path skipped the safety check entirely — quick commands ran via ``shell=True`` straight from ``_load_cfg()`` with no normalization or filtering at all. Hoist the danger check into ``_check_dangerous_shell_command()``, which treats both pattern matches and import failures as a refusal (returns a JSON-RPC error instead of executing). Both call sites now share the same fail-closed gate, so neither one can run shell input that the rest of the codebase already classifies as dangerous, and an environment that breaks the safety module fails loudly instead of silently dropping protection. Three regression tests cover (a) ``shell.exec`` blocking a flagged command, (b) ``shell.exec`` refusing to run when ``tools.approval`` is unimportable, and (c) ``command.dispatch`` blocking a flagged ``quick_commands`` entry. Fixes NousResearch#16560
Contributor
Author
|
Closing as duplicate — I missed @alt-glitch's earlier comment that #15542 and #15881 already cover both fail-closed shell.exec and quick-command hardening. Sorry for the noise. |
alt-glitch
added
type/security
Collaborator
Closed
6 tasks
This was referenced May 27, 2026
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
2 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.
Summary
Two
subprocess.run(..., shell=True)call sites intui_gateway/server.pyaccept input that is not gated by the danger-pattern check:shell.exec(JSON-RPC method,cmdfrom request params) wrapped thedetect_dangerous_commandimport inexcept ImportError: pass— any caller able to maketools.approvalun-importable (deleted, shadowed, broken venv) bypassed the entire safety gate.command.dispatchforquick_commandsran the configured command viashell=Truestraight from_load_cfg()with no danger check at all.Fix
Hoist the danger check into
_check_dangerous_shell_command(), which treats both pattern matches and import failures as a refusal (returns a JSON-RPC error instead of executing). Both call sites now share the same fail-closed gate, so an environment that breaks the safety module fails loudly instead of silently dropping protection, and quick commands match the same standard asshell.exec.Test plan
test_shell_exec_blocks_dangerous_command— flagged command is refused (existing pattern path).test_shell_exec_fails_closed_when_safety_module_missing—ImportErrorno longer falls through tosubprocess.run.test_command_dispatch_blocks_dangerous_quick_command— flaggedquick_commandsentry is refused.test_command_dispatch_exec_nonzero_surfaces_error(existing) still passes — non-flagged quick commands run unchanged.Fixes #16560