From 9860d93f2a8a36bd4d9c9fd554993795e7ba004e Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 28 Jun 2026 18:35:41 -0700 Subject: [PATCH] fix(terminal): require approval for host-bound Docker commands (#54483) * fix(terminal): require approval for host-bound Docker commands The Docker terminal backend blanket-skips dangerous-command approval on the assumption that the container is isolated from the host. That holds only when nothing is bind-mounted in. Once a host path is exposed (via TERMINAL_DOCKER_MOUNT_CWD_TO_WORKSPACE or a host-path entry in TERMINAL_DOCKER_VOLUMES), a command like `rm -rf /workspace` reaches real host files but is still auto-approved. Detect host bind mounts and route those sessions through the normal approval flow. Isolated Docker keeps the fast path. The same gating is applied to the execute_code guard, which had the identical blanket skip. Co-authored-by: Hermes Agent * chore: add AUTHOR_MAP entry for PR #6436 salvage (Kolektori) * test: accept has_host_access kwarg in _check_all_guards mocks The host-bound Docker approval fix adds a has_host_access kwarg to the _check_all_guards wrapper. Six pre-existing tests monkeypatch it with a fixed (command, env_type) / (cmd, env) lambda signature, which now raises TypeError when terminal_tool passes the new kwarg. Widen those mock signatures to accept **kwargs. --------- Co-authored-by: Kolektori <256073454+Kolektori@users.noreply.github.com> Co-authored-by: Hermes Agent --- scripts/release.py | 1 + tests/hermes_cli/test_gateway_restart_loop.py | 4 +- tests/tools/test_modal_sandbox_fixes.py | 95 +++++++++++++++++++ tests/tools/test_terminal_task_cwd.py | 8 +- tools/approval.py | 44 +++++++-- tools/code_execution_tool.py | 12 ++- tools/terminal_tool.py | 32 ++++++- 7 files changed, 176 insertions(+), 20 deletions(-) diff --git a/scripts/release.py b/scripts/release.py index eb23e2e53a2..7be6c05ddfc 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -45,6 +45,7 @@ ACP_REGISTRY_MANIFEST = REPO_ROOT / "acp_registry" / "agent.json" # Auto-extracted from noreply emails + manual overrides AUTHOR_MAP = { + "256073454+Kolektori@users.noreply.github.com": "Kolektori", # PR #6436 salvage (require approval for host-bound Docker commands; container guard fast-path) "carlosmcejas@gmail.com": "cmcejas", # PR #41188 salvage (early Telegram auth gate before event build/observe; #40863) "ha-agent@homelab.4410.us": "oreoluwa", # PR #49845 salvage (skip preflight content-type probe for OAuth MCP servers so OAuth discovery runs; Akiflow/Hospitable) "prathamesh290504@gmail.com": "PRATHAMESH75", # PR #37550 salvage (ExecStopPost cgroup-orphan reaper to unblock systemd restart; #37454) diff --git a/tests/hermes_cli/test_gateway_restart_loop.py b/tests/hermes_cli/test_gateway_restart_loop.py index 74ee9e4934e..239b8060024 100644 --- a/tests/hermes_cli/test_gateway_restart_loop.py +++ b/tests/hermes_cli/test_gateway_restart_loop.py @@ -329,7 +329,7 @@ class TestTerminalToolGatewayLifecycleGuard: return {"output": "Active: running", "returncode": 0} self._patch_env(monkeypatch, _FakeEnv(), inside_gateway=True) - monkeypatch.setattr(tt, "_check_all_guards", lambda cmd, env: {"approved": True}) + monkeypatch.setattr(tt, "_check_all_guards", lambda cmd, env, **kwargs: {"approved": True}) result = json.loads(tt.terminal_tool(command="systemctl status nginx")) @@ -349,7 +349,7 @@ class TestTerminalToolGatewayLifecycleGuard: return {"output": "restarting...", "returncode": 0} self._patch_env(monkeypatch, _FakeEnv(), inside_gateway=False) - monkeypatch.setattr(tt, "_check_all_guards", lambda cmd, env: {"approved": True}) + monkeypatch.setattr(tt, "_check_all_guards", lambda cmd, env, **kwargs: {"approved": True}) result = json.loads(tt.terminal_tool(command="systemctl restart hermes-gateway")) diff --git a/tests/tools/test_modal_sandbox_fixes.py b/tests/tools/test_modal_sandbox_fixes.py index dab7ec14758..3bccb3e07d1 100644 --- a/tests/tools/test_modal_sandbox_fixes.py +++ b/tests/tools/test_modal_sandbox_fixes.py @@ -301,3 +301,98 @@ class TestHostPrefixList: f"Host path {host_path!r} should be rejected as a container " "cwd but was accepted." ) + + +# ========================================================================= +# Test 7: Host-bound Docker sandboxes must not bypass dangerous-command +# approval. Isolated Docker keeps the container fast-path; once a host path +# is bind-mounted into the container, a command like `rm -rf /workspace` can +# reach real host files, so it goes through the normal approval flow. +# (PR #6436, @Kolektori) +# ========================================================================= + +class TestDockerHostBindApproval: + """Docker host bind mounts disable the container approval fast-path.""" + + def test_docker_host_access_detection(self): + """_docker_has_host_access flags bind-mounted host paths only.""" + # Isolated docker (no host binds) -> not host access. + assert _tt_mod._docker_has_host_access( + {"env_type": "docker", "docker_volumes": [], + "host_cwd": None, "docker_mount_cwd_to_workspace": False}) is False + # Host-path bind mount -> host access. + assert _tt_mod._docker_has_host_access( + {"env_type": "docker", "docker_volumes": ["/tmp:/hosttmp"]}) is True + # Named volume (not a host path) -> not host access. + assert _tt_mod._docker_has_host_access( + {"env_type": "docker", "docker_volumes": ["myvol:/data"]}) is False + # cwd auto-mount flag -> host access. + assert _tt_mod._docker_has_host_access( + {"env_type": "docker", "host_cwd": "/home/u/p", + "docker_mount_cwd_to_workspace": True}) is True + # Windows host path -> host access. + assert _tt_mod._docker_has_host_access( + {"env_type": "docker", "docker_volumes": ["C:\\Users:/data"]}) is True + # Other container backends never report host access. + assert _tt_mod._docker_has_host_access( + {"env_type": "modal", "docker_volumes": ["/tmp:/x"]}) is False + + def test_should_skip_container_guards(self): + """Docker skips only when isolated; other sandboxes always skip.""" + import tools.approval as A + assert A._should_skip_container_guards("docker", has_host_access=False) is True + assert A._should_skip_container_guards("docker", has_host_access=True) is False + assert A._should_skip_container_guards("modal", has_host_access=True) is True + assert A._should_skip_container_guards("singularity") is True + assert A._should_skip_container_guards("daytona") is True + assert A._should_skip_container_guards("local") is False + + def test_isolated_docker_keeps_fast_path(self, monkeypatch): + """Isolated Docker still bypasses dangerous-command approval.""" + import tools.approval as A + monkeypatch.setenv("HERMES_EXEC_ASK", "1") + monkeypatch.setattr( + "tools.tirith_security.check_command_security", + lambda _c: {"action": "allow", "findings": [], "summary": ""}) + res = A.check_all_command_guards("rm -rf /workspace", "docker", + has_host_access=False) + assert res["approved"] is True + + def test_host_bound_docker_requires_approval(self, monkeypatch): + """Host-bound Docker dangerous command escalates instead of bypassing.""" + import tools.approval as A + monkeypatch.setenv("HERMES_EXEC_ASK", "1") + monkeypatch.setattr( + "tools.tirith_security.check_command_security", + lambda _c: {"action": "allow", "findings": [], "summary": ""}) + res = A.check_all_command_guards("rm -rf /workspace", "docker", + has_host_access=True) + # Must NOT take the silent container fast-path. + assert res.get("approved") is not True + assert res.get("status") == "pending_approval" + + def test_execute_code_isolated_docker_keeps_fast_path(self, monkeypatch): + """Isolated Docker execute_code still bypasses the guard.""" + import tools.approval as A + monkeypatch.setenv("HERMES_EXEC_ASK", "1") + res = A.check_execute_code_guard("import os", "docker", + has_host_access=False) + assert res["approved"] is True + + def test_execute_code_host_bound_docker_requires_approval(self, monkeypatch): + """Host-bound Docker execute_code does not get the container fast-path.""" + import tools.approval as A + monkeypatch.setenv("HERMES_EXEC_ASK", "1") + res = A.check_execute_code_guard( + "import os; os.system('rm -rf /workspace')", "docker", + has_host_access=True) + assert res.get("approved") is not True + assert res.get("status") == "pending_approval" + + def test_execute_code_vercel_sandbox_always_skips(self, monkeypatch): + """vercel_sandbox has no host-bind concept and stays always-skipped.""" + import tools.approval as A + monkeypatch.setenv("HERMES_EXEC_ASK", "1") + res = A.check_execute_code_guard("import os", "vercel_sandbox", + has_host_access=True) + assert res["approved"] is True diff --git a/tests/tools/test_terminal_task_cwd.py b/tests/tools/test_terminal_task_cwd.py index 9d0388d8c94..85817fa5335 100644 --- a/tests/tools/test_terminal_task_cwd.py +++ b/tests/tools/test_terminal_task_cwd.py @@ -34,7 +34,7 @@ def test_foreground_command_uses_registered_task_cwd_for_existing_environment(mo monkeypatch.setattr( terminal_tool, "_check_all_guards", - lambda command, env_type: {"approved": True}, + lambda command, env_type, **kwargs: {"approved": True}, ) result = json.loads(terminal_tool.terminal_tool(command="pwd", task_id=task_id)) @@ -61,7 +61,7 @@ def test_explicit_workdir_still_wins_over_registered_task_cwd(monkeypatch): monkeypatch.setattr( terminal_tool, "_check_all_guards", - lambda command, env_type: {"approved": True}, + lambda command, env_type, **kwargs: {"approved": True}, ) result = json.loads( @@ -98,7 +98,7 @@ def test_foreground_command_prefers_live_env_cwd_over_init_time_cwd(monkeypatch) monkeypatch.setattr( terminal_tool, "_check_all_guards", - lambda command, env_type: {"approved": True}, + lambda command, env_type, **kwargs: {"approved": True}, ) result = json.loads(terminal_tool.terminal_tool(command="pwd", task_id=task_id)) @@ -136,7 +136,7 @@ def test_background_command_prefers_live_env_cwd_over_init_time_cwd(monkeypatch) monkeypatch.setattr( terminal_tool, "_check_all_guards", - lambda command, env_type: {"approved": True}, + lambda command, env_type, **kwargs: {"approved": True}, ) monkeypatch.setattr(process_registry_mod, "process_registry", registry) diff --git a/tools/approval.py b/tools/approval.py index e9942efc9df..ee49a6bceeb 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -1268,8 +1268,23 @@ def _smart_approve(command: str, description: str) -> str: return "escalate" +def _should_skip_container_guards(env_type: str, has_host_access: bool = False) -> bool: + """Return True when the backend is isolated enough to skip dangerous-command prompts. + + Isolated container backends sandbox the agent away from the host, so their + commands can't damage real files/services and we skip the approval layer. + Docker is the exception once host paths are bind-mounted into the container: + at that point a command like ``rm -rf /workspace`` reaches host files, so it + must go through the normal approval flow. + """ + if env_type == "docker": + return not has_host_access + return env_type in ("singularity", "modal", "daytona") + + def check_dangerous_command(command: str, env_type: str, - approval_callback=None) -> dict: + approval_callback=None, + has_host_access: bool = False) -> dict: """Check if a command is dangerous and handle approval. This is the main entry point called by terminal_tool before executing @@ -1279,11 +1294,13 @@ def check_dangerous_command(command: str, env_type: str, command: The shell command to check. env_type: Terminal backend type ('local', 'ssh', 'docker', etc.). approval_callback: Optional CLI callback for interactive prompts. + has_host_access: True when a Docker sandbox bind-mounts host paths, + so its commands can reach the host and must not skip approval. Returns: {"approved": True/False, "message": str or None, ...} """ - if env_type in {"docker", "singularity", "modal", "daytona"}: + if _should_skip_container_guards(env_type, has_host_access=has_host_access): return {"approved": True, "message": None} # Hardline floor: commands with no recovery path (rm -rf /, mkfs, dd @@ -1525,16 +1542,22 @@ def _await_gateway_decision(session_key: str, notify_cb, approval_data: dict, def check_all_command_guards(command: str, env_type: str, - approval_callback=None) -> dict: + approval_callback=None, + has_host_access: bool = False) -> dict: """Run all pre-exec security checks and return a single approval decision. Gathers findings from tirith and dangerous-command detection, then presents them as a single combined approval request. This prevents a gateway force=True replay from bypassing one check when only the other was shown to the user. + + ``has_host_access`` is True when a Docker sandbox bind-mounts host paths; + such a session is no longer isolated, so it goes through the normal flow + instead of the container fast-path. """ - # Skip containers for both checks - if env_type in {"docker", "singularity", "modal", "daytona"}: + # Skip isolated container backends for both checks. Docker stops skipping + # once host paths are bind-mounted into the sandbox. + if _should_skip_container_guards(env_type, has_host_access=has_host_access): return {"approved": True, "message": None} # Hardline floor: unconditional block for catastrophic commands @@ -1857,7 +1880,8 @@ def check_all_command_guards(command: str, env_type: str, "user_approved": True, "description": combined_desc} -def check_execute_code_guard(code: str, env_type: str) -> dict: +def check_execute_code_guard(code: str, env_type: str, + has_host_access: bool = False) -> dict: """Approve an execute_code script before its child process is spawned. execute_code runs arbitrary local Python — the script can call @@ -1883,8 +1907,12 @@ def check_execute_code_guard(code: str, env_type: str) -> dict: ) # Isolated backends already sandbox the child — matches the container skip - # in check_all_command_guards / check_dangerous_command. - if env_type in {"docker", "singularity", "modal", "daytona", "vercel_sandbox"}: + # in check_all_command_guards / check_dangerous_command. Docker stops + # skipping once host paths are bind-mounted into the sandbox; vercel_sandbox + # has no host-bind concept so it stays always-skipped. + if env_type == "vercel_sandbox": + return {"approved": True, "message": None} + if _should_skip_container_guards(env_type, has_host_access=has_host_access): return {"approved": True, "message": None} # --yolo or approvals.mode=off: bypass (session- or process-scoped). diff --git a/tools/code_execution_tool.py b/tools/code_execution_tool.py index 4d505fb1682..8946de73750 100644 --- a/tools/code_execution_tool.py +++ b/tools/code_execution_tool.py @@ -1104,15 +1104,21 @@ def execute_code( return tool_error("No code provided.") # Dispatch: remote backends use file-based RPC, local uses UDS - from tools.terminal_tool import _get_env_config - env_type = _get_env_config()["env_type"] + from tools.terminal_tool import _get_env_config, _docker_has_host_access + _env_config = _get_env_config() + env_type = _env_config["env_type"] # execute_code runs arbitrary Python (subprocess/os.system/...) that never # passes through terminal()/DANGEROUS_PATTERNS, so guard the whole script # here before either dispatch path spawns it. Runs synchronously in the # caller (tool-executor) thread, which holds the session context (#30882). + # A Docker sandbox with host bind mounts is no longer isolated, so its + # script does not get the container fast-path. from tools.approval import check_execute_code_guard - _guard = check_execute_code_guard(code, env_type) + _guard = check_execute_code_guard( + code, env_type, + has_host_access=_docker_has_host_access(_env_config), + ) if not _guard.get("approved", False): return json.dumps({ "status": "error", diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index e9ad5d9f970..69ef5dc8698 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -257,10 +257,33 @@ from tools.approval import ( ) -def _check_all_guards(command: str, env_type: str) -> dict: +def _docker_volume_uses_host_path(volume_spec: str) -> bool: + """Return True when a docker volume spec bind-mounts a host path.""" + if not isinstance(volume_spec, str): + return False + + vol = volume_spec.strip() + return bool(vol) and ( + vol.startswith(("/", "~", "./", "../")) or + (len(vol) >= 3 and vol[1] == ":" and vol[2] in ("/", "\\")) + ) + + +def _docker_has_host_access(config: Dict[str, Any]) -> bool: + """Return True when a Docker sandbox exposes host paths through bind mounts.""" + if config.get("env_type") != "docker": + return False + if config.get("host_cwd") and config.get("docker_mount_cwd_to_workspace"): + return True + return any(_docker_volume_uses_host_path(vol) for vol in config.get("docker_volumes", [])) + + +def _check_all_guards(command: str, env_type: str, + has_host_access: bool = False) -> dict: """Delegate to consolidated guard (tirith + dangerous cmd) with CLI callback.""" return _check_all_guards_impl(command, env_type, - approval_callback=_get_approval_callback()) + approval_callback=_get_approval_callback(), + has_host_access=has_host_access) # Allowlist: characters that can legitimately appear in directory paths. @@ -2231,7 +2254,10 @@ def terminal_tool( # Skip check if force=True (user has confirmed they want to run it) approval_note = None if not force: - approval = _check_all_guards(command, env_type) + approval = _check_all_guards( + command, env_type, + has_host_access=_docker_has_host_access(config), + ) if not approval["approved"]: # Check if this is an approval_required (gateway ask mode) if approval.get("status") == "pending_approval":