Tirith fail-open flag ignored on ImportError

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #53604 fix(approval): honour tirith_fail_open=false on Tirith ImportError (#20733)

  merged to main

  files: tests/tools/test_approval.py, tools/approval.py

  Show the diff

  • tests/tools/test_approval.py

    @@ -1826,3 +1826,98 @@ def _capture(event_name, **kwargs):
             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

  • 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)