diff --git a/tests/tools/test_force_dangerous_override.py b/tests/tools/test_force_dangerous_override.py new file mode 100644 index 000000000..ab9600f20 --- /dev/null +++ b/tests/tools/test_force_dangerous_override.py @@ -0,0 +1,95 @@ +"""Tests for the --force flag dangerous verdict bypass fix in skills_guard.py. + +Regression test: the old code had `if result.verdict == "dangerous" and not force:` +which meant force=True would skip the early return, fall through the policy +lookup, and hit `if force: return True` - allowing installation of skills +flagged as dangerous (reverse shells, data exfiltration, etc). + +The docstring explicitly states: "never overrides dangerous". +""" + + +def _old_should_allow(verdict, trust_level, force): + """Simulate the BROKEN old logic.""" + INSTALL_POLICY = { + "builtin": ("allow", "allow", "allow"), + "trusted": ("allow", "allow", "block"), + "community": ("allow", "block", "block"), + } + VERDICT_INDEX = {"safe": 0, "caution": 1, "dangerous": 2} + + # Old buggy check: `and not force` + if verdict == "dangerous" and not force: + return False + + policy = INSTALL_POLICY.get(trust_level, INSTALL_POLICY["community"]) + vi = VERDICT_INDEX.get(verdict, 2) + decision = policy[vi] + + if decision == "allow": + return True + + if force: + return True # Bug: this line is reached for dangerous + force=True + + return False + + +def _new_should_allow(verdict, trust_level, force): + """Simulate the FIXED logic.""" + INSTALL_POLICY = { + "builtin": ("allow", "allow", "allow"), + "trusted": ("allow", "allow", "block"), + "community": ("allow", "block", "block"), + } + VERDICT_INDEX = {"safe": 0, "caution": 1, "dangerous": 2} + + # Fixed: no `and not force` - dangerous is always blocked + if verdict == "dangerous": + return False + + policy = INSTALL_POLICY.get(trust_level, INSTALL_POLICY["community"]) + vi = VERDICT_INDEX.get(verdict, 2) + decision = policy[vi] + + if decision == "allow": + return True + + if force: + return True + + return False + + +class TestForceNeverOverridesDangerous: + """The core bug: --force bypassed the dangerous verdict block.""" + + def test_old_code_allows_dangerous_with_force(self): + """Old code: force=True lets dangerous skills through.""" + assert _old_should_allow("dangerous", "community", force=True) is True + + def test_new_code_blocks_dangerous_with_force(self): + """Fixed code: force=True still blocks dangerous skills.""" + assert _new_should_allow("dangerous", "community", force=True) is False + + def test_new_code_blocks_dangerous_trusted_with_force(self): + """Fixed code: even trusted + force cannot install dangerous.""" + assert _new_should_allow("dangerous", "trusted", force=True) is False + + def test_force_still_overrides_caution(self): + """force=True should still work for caution verdicts.""" + assert _new_should_allow("caution", "community", force=True) is True + + def test_caution_community_blocked_without_force(self): + """Caution + community is blocked without force (unchanged).""" + assert _new_should_allow("caution", "community", force=False) is False + + def test_safe_always_allowed(self): + """Safe verdict is always allowed regardless of force.""" + assert _new_should_allow("safe", "community", force=False) is True + assert _new_should_allow("safe", "community", force=True) is True + + def test_dangerous_blocked_without_force(self): + """Dangerous is blocked without force (both old and new agree).""" + assert _old_should_allow("dangerous", "community", force=False) is False + assert _new_should_allow("dangerous", "community", force=False) is False diff --git a/tests/tools/test_skills_guard.py b/tests/tools/test_skills_guard.py index 85430169e..70eb9fc69 100644 --- a/tests/tools/test_skills_guard.py +++ b/tests/tools/test_skills_guard.py @@ -132,6 +132,23 @@ class TestShouldAllowInstall: allowed, _ = should_allow_install(self._result("community", "dangerous", f), force=False) assert allowed is False + def test_force_never_overrides_dangerous(self): + """--force must not bypass dangerous verdict (regression test).""" + f = [Finding("x", "critical", "c", "f", 1, "m", "d")] + allowed, reason = should_allow_install( + self._result("community", "dangerous", f), force=True + ) + assert allowed is False + assert "DANGEROUS" in reason + + def test_force_never_overrides_dangerous_trusted(self): + """--force must not bypass dangerous even for trusted sources.""" + f = [Finding("x", "critical", "c", "f", 1, "m", "d")] + allowed, _ = should_allow_install( + self._result("trusted", "dangerous", f), force=True + ) + assert allowed is False + # --------------------------------------------------------------------------- # scan_file — pattern detection diff --git a/tools/skills_guard.py b/tools/skills_guard.py index 353b4d9ba..a20a9b753 100644 --- a/tools/skills_guard.py +++ b/tools/skills_guard.py @@ -650,7 +650,7 @@ def should_allow_install(result: ScanResult, force: bool = False) -> Tuple[bool, Returns: (allowed, reason) tuple """ - if result.verdict == "dangerous" and not force: + if result.verdict == "dangerous": return False, f"Scan verdict is DANGEROUS ({len(result.findings)} findings). Blocked." policy = INSTALL_POLICY.get(result.trust_level, INSTALL_POLICY["community"])