From 56d4bfe4ba839e38820d36b6402e0fe607819eea Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 30 Jun 2026 23:40:45 -0700 Subject: [PATCH] fix(approval): honour tirith_fail_open in cron-deny tirith path + tests Follow-up to the salvaged #22070. The cron-deny tirith ImportError branch was unconditionally fail-open; now it honours security.tirith_fail_open: false by blocking (a cron session has no user to approve), mirroring the main flow's fail-closed synthesis (#20733). Adds regression tests: tirith-only content threat blocked in cron-deny, plus fail-closed/fail-open ImportError behavior. --- tests/tools/test_cron_approval_mode.py | 93 ++++++++++++++++++++++++++ tools/approval.py | 28 +++++++- 2 files changed, 120 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_cron_approval_mode.py b/tests/tools/test_cron_approval_mode.py index 007c777e267..9264d108cff 100644 --- a/tests/tools/test_cron_approval_mode.py +++ b/tests/tools/test_cron_approval_mode.py @@ -212,6 +212,99 @@ class TestCronDenyModeAllGuards: result = check_all_command_guards("rm -rf /tmp/stuff", "local") assert result["approved"] + def test_tirith_content_threat_blocked_in_cron_deny(self, monkeypatch): + """Content-level threats caught only by tirith (not the regex patterns) + are blocked in cron-deny mode. Regression for #22070: previously the + cron-deny early return ran only detect_dangerous_command and returned + before reaching the tirith check, so these were silently approved.""" + monkeypatch.setenv("HERMES_CRON_SESSION", "1") + monkeypatch.delenv("HERMES_INTERACTIVE", raising=False) + monkeypatch.delenv("HERMES_GATEWAY_SESSION", raising=False) + monkeypatch.delenv("HERMES_EXEC_ASK", raising=False) + monkeypatch.delenv("HERMES_YOLO_MODE", raising=False) + + from unittest.mock import patch as mock_patch + # A tirith "block" result while detect_dangerous_command reports safe: + # proves the block comes from the tirith path, not the regex path. + fake_tirith = { + "action": "block", + "findings": [{"severity": "HIGH", "title": "Homograph URL", + "description": "URL contains Cyrillic lookalike chars"}], + "summary": "homograph url", + } + with ( + mock_patch("tools.approval._get_cron_approval_mode", return_value="deny"), + mock_patch("tools.approval.detect_dangerous_command", + return_value=(False, None, None)), + mock_patch("tools.tirith_security.check_command_security", + return_value=fake_tirith), + ): + result = check_all_command_guards("curl http://xn--e1afmkfd.example/x", "local") + assert not result["approved"] + assert "BLOCKED" in result["message"] + + def test_tirith_import_error_fail_closed_blocks_in_cron_deny(self, monkeypatch): + """When tirith is unavailable and security.tirith_fail_open is false, + cron-deny mode blocks rather than silently allowing (a cron session has + no user to approve). Mirrors the fail-closed handling in the main flow.""" + monkeypatch.setenv("HERMES_CRON_SESSION", "1") + monkeypatch.delenv("HERMES_INTERACTIVE", raising=False) + monkeypatch.delenv("HERMES_GATEWAY_SESSION", raising=False) + monkeypatch.delenv("HERMES_EXEC_ASK", raising=False) + monkeypatch.delenv("HERMES_YOLO_MODE", raising=False) + + from unittest.mock import patch as mock_patch + import builtins + _real_import = builtins.__import__ + + def _blocked_import(name, *a, **k): + if name.endswith("tirith_security"): + raise ImportError("simulated missing tirith") + return _real_import(name, *a, **k) + + with ( + mock_patch("tools.approval._get_cron_approval_mode", return_value="deny"), + mock_patch("tools.approval.detect_dangerous_command", + return_value=(False, None, None)), + mock_patch("hermes_cli.config.load_config", + return_value={"security": {"tirith_enabled": True, + "tirith_fail_open": False}}), + mock_patch.object(builtins, "__import__", _blocked_import), + ): + result = check_all_command_guards("echo hi", "local") + assert not result["approved"] + assert "tirith_fail_open" in result["message"] + + def test_tirith_import_error_fail_open_allows_in_cron_deny(self, monkeypatch): + """When tirith is unavailable and tirith_fail_open is true (default), + cron-deny mode allows safe commands — preserving pre-#22070 behavior.""" + monkeypatch.setenv("HERMES_CRON_SESSION", "1") + monkeypatch.delenv("HERMES_INTERACTIVE", raising=False) + monkeypatch.delenv("HERMES_GATEWAY_SESSION", raising=False) + monkeypatch.delenv("HERMES_EXEC_ASK", raising=False) + monkeypatch.delenv("HERMES_YOLO_MODE", raising=False) + + from unittest.mock import patch as mock_patch + import builtins + _real_import = builtins.__import__ + + def _blocked_import(name, *a, **k): + if name.endswith("tirith_security"): + raise ImportError("simulated missing tirith") + return _real_import(name, *a, **k) + + with ( + mock_patch("tools.approval._get_cron_approval_mode", return_value="deny"), + mock_patch("tools.approval.detect_dangerous_command", + return_value=(False, None, None)), + mock_patch("hermes_cli.config.load_config", + return_value={"security": {"tirith_enabled": True, + "tirith_fail_open": True}}), + mock_patch.object(builtins, "__import__", _blocked_import), + ): + result = check_all_command_guards("echo hi", "local") + assert result["approved"] + # --------------------------------------------------------------------------- # Edge cases: cron mode interaction with other approval mechanisms diff --git a/tools/approval.py b/tools/approval.py index 92585abf5c8..e5cb744420c 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -1704,7 +1704,33 @@ def check_all_command_guards(command: str, env_type: str, ), } except ImportError: - pass # tirith not installed — allow + # Tirith not installed. Honour security.tirith_fail_open: + # the default (True) allows as before, but when an operator + # has explicitly opted into fail-closed the command cannot + # be silently allowed — and a cron session has no user to + # approve it, so fail-closed means block (mirrors the + # fail-closed synthesis in the main flow below; see #20733). + _cron_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 {} + if _sec.get("tirith_enabled", True): + _cron_fail_open = _sec.get("tirith_fail_open", True) + except Exception: + pass + if not _cron_fail_open: + return { + "approved": False, + "message": ( + "BLOCKED: the Tirith security scanner could not be " + "imported and security.tirith_fail_open is false, " + "so this command cannot be silently allowed — and " + "cron jobs run without a user present to approve it. " + "Find an alternative approach, install tirith, or set " + "approvals.cron_mode: approve in config.yaml." + ), + } + # else: tirith_fail_open is True — allow as before return {"approved": True, "message": None} # --- Phase 1: Gather findings from both checks ---