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.
This commit is contained in:
teknium1 2026-06-30 23:40:45 -07:00 committed by Teknium
parent c50f517bff
commit 56d4bfe4ba
2 changed files with 120 additions and 1 deletions

View file

@ -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

View file

@ -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 ---