From 9c81c938d3cbce83f519534516a97482aebaec96 Mon Sep 17 00:00:00 2001 From: Brandon Zarnitz Date: Sat, 27 Jun 2026 04:23:09 -0700 Subject: [PATCH] fix(approval): honour tirith_fail_open=false on Tirith ImportError (#20733) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check_all_command_guards() swallowed ImportError from tools.tirith_security with an unconditional pass, leaving tirith_result["action"] as "allow" regardless of security.tirith_fail_open. When an operator sets tirith_fail_open: false they have explicitly opted into fail-closed behaviour; a missing or broken Tirith module must not silently permit command execution. Inside the except ImportError handler, read the live security config. When tirith_enabled is true and tirith_fail_open is false, synthesise a "warn"-action Tirith result so the command flows through the normal approval path (prompt the user, or block in cron/gateway contexts) instead of bypassing it. The default tirith_fail_open: true behaviour is unchanged. Adds three regression tests to tests/tools/test_approval.py: - fail_open=true + ImportError → silently allowed (no regression) - fail_open=false + ImportError → approval callback invoked, command denied - tirith_enabled=false → always allowed regardless of fail_open Fixes #20733 Co-Authored-By: Claude Sonnet 4.6 # Conflicts: # tests/tools/test_approval.py --- tests/tools/test_approval.py | 95 ++++++++++++++++++++++++++++++++++++ tools/approval.py | 35 ++++++++++++- 2 files changed, 129 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index a4d0c7af22c..37438550379 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1826,3 +1826,98 @@ class TestApprovalTimeoutIsNotConsent: assert last_post.get("choice") == "timeout", ( f"hook choice should be 'timeout' on no-response, got {last_post.get('choice')!r}" ) + + +class TestTirithImportErrorFailOpenPolicy: + """Regression guard for #20733. + + When ``tools.tirith_security`` cannot be imported, ``check_all_command_guards`` + must honour the ``security.tirith_fail_open`` config knob: + + * ``tirith_fail_open: true`` (default) → allow, no approval prompt. + * ``tirith_fail_open: false`` → surface a Tirith-style warning through + the normal approval flow so the command is not silently permitted. + """ + + def _make_failing_import(self, real_import): + """Return a builtins.__import__ replacement that raises for tirith.""" + def _fake(name, *args, **kwargs): + if name == "tools.tirith_security": + raise ImportError("simulated tirith import failure") + return real_import(name, *args, **kwargs) + return _fake + + def test_fail_open_true_allows_silently_on_import_error(self): + """Default fail-open: ImportError is silently swallowed, command allowed.""" + import builtins + from unittest.mock import patch as _patch + from tools.approval import check_all_command_guards + + cfg = { + "approvals": {"mode": "manual"}, + "security": {"tirith_enabled": True, "tirith_fail_open": True}, + } + real_import = builtins.__import__ + with _patch("builtins.__import__", side_effect=self._make_failing_import(real_import)): + with _patch("hermes_cli.config.load_config", return_value=cfg): + with _patch("tools.approval.detect_dangerous_command", return_value=(False, None, None)): + with mock_patch.dict("os.environ", {"HERMES_INTERACTIVE": "1"}, clear=False): + result = check_all_command_guards("echo hello", "local") + + assert result.get("approved") is True + + def test_fail_open_false_escalates_to_approval_on_import_error(self): + """Fail-closed: ImportError must NOT silently allow when tirith_fail_open=false.""" + import builtins + from unittest.mock import patch as _patch + from tools.approval import check_all_command_guards + + cfg = { + "approvals": {"mode": "manual"}, + "security": {"tirith_enabled": True, "tirith_fail_open": False}, + } + calls = [] + + def approval_callback(command, description, **kwargs): + calls.append({"command": command, "description": description}) + return "deny" + + real_import = builtins.__import__ + with _patch("builtins.__import__", side_effect=self._make_failing_import(real_import)): + with _patch("hermes_cli.config.load_config", return_value=cfg): + with _patch("tools.approval.detect_dangerous_command", return_value=(False, None, None)): + with mock_patch.dict("os.environ", {"HERMES_INTERACTIVE": "1"}, clear=False): + result = check_all_command_guards( + "echo hello", + "local", + approval_callback=approval_callback, + ) + + # The user must have been consulted — the command should NOT be silently allowed. + assert result.get("approved") is not True or calls, ( + "Command was silently allowed despite tirith_fail_open=false and Tirith import failure. " + "This is the bug described in issue #20733." + ) + # Specifically: user denied via callback, so approved must be False. + assert result.get("approved") is False + assert calls, "Approval callback was never invoked — command slipped through silently" + assert "tirith" in calls[0]["description"].lower() or "unavailable" in calls[0]["description"].lower() + + def test_tirith_disabled_skips_fail_open_check(self): + """When tirith_enabled=false, ImportError is irrelevant — allow without prompt.""" + import builtins + from unittest.mock import patch as _patch + from tools.approval import check_all_command_guards + + cfg = { + "approvals": {"mode": "manual"}, + "security": {"tirith_enabled": False, "tirith_fail_open": False}, + } + real_import = builtins.__import__ + with _patch("builtins.__import__", side_effect=self._make_failing_import(real_import)): + with _patch("hermes_cli.config.load_config", return_value=cfg): + with _patch("tools.approval.detect_dangerous_command", return_value=(False, None, None)): + with mock_patch.dict("os.environ", {"HERMES_INTERACTIVE": "1"}, clear=False): + result = check_all_command_guards("echo hello", "local") + + assert result.get("approved") is True diff --git a/tools/approval.py b/tools/approval.py index e444e089dce..e9942efc9df 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -1600,7 +1600,40 @@ def check_all_command_guards(command: str, env_type: str, from tools.tirith_security import check_command_security tirith_result = check_command_security(command) except ImportError: - pass # tirith module not installed — allow + # Tirith module not installed. When tirith_fail_open is True (the + # default) we silently allow, matching the pre-existing behaviour. + # When tirith_fail_open is False the operator has explicitly opted into + # fail-closed; an import failure must not silently grant access, so we + # synthesize a warn result that will be surfaced to the user through the + # normal approval flow. Fixes #20733. + _tirith_fail_open = True # safe default if config is unreadable + try: + from hermes_cli.config import load_config as _load_cfg + _sec = (_load_cfg() or {}).get("security", {}) or {} + _tirith_enabled = _sec.get("tirith_enabled", True) + if _tirith_enabled: + _tirith_fail_open = _sec.get("tirith_fail_open", True) + except Exception: + pass + if not _tirith_fail_open: + tirith_result = { + "action": "warn", + "findings": [ + { + "rule_id": "tirith-import-error", + "severity": "HIGH", + "title": "Tirith security module unavailable", + "description": ( + "The Tirith security scanner could not be imported. " + "Because security.tirith_fail_open is false, this " + "command cannot be silently allowed. Approve only if " + "you have verified the command is safe." + ), + } + ], + "summary": "Tirith unavailable (fail-closed)", + } + # else: tirith_fail_open is True — allow as before (tirith_result stays "allow") # Dangerous command check (detection only, no approval) is_dangerous, pattern_key, description = detect_dangerous_command(command)