mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-28 01:21:43 +00:00
fix(skills): honor policy table for dangerous verdicts
Salvaged from PR #1007 by stablegenius49. - let INSTALL_POLICY decide dangerous verdict handling for builtin skills - allow --force to override blocked dangerous decisions for trusted and community sources - accept --yes / -y as aliases for --force in /skills install - update regression tests to match the intended policy precedence
This commit is contained in:
parent
6d8286f396
commit
3325e51e53
5 changed files with 40 additions and 49 deletions
|
|
@ -1,11 +1,8 @@
|
|||
"""Tests for the --force flag dangerous verdict bypass fix in skills_guard.py.
|
||||
"""Regression tests for skills guard policy precedence.
|
||||
|
||||
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".
|
||||
Official/builtin skills should follow the INSTALL_POLICY table even when their
|
||||
scan verdict is dangerous, and --force should override blocked verdicts for
|
||||
non-builtin sources.
|
||||
"""
|
||||
|
||||
|
||||
|
|
@ -44,10 +41,6 @@ def _new_should_allow(verdict, trust_level, force):
|
|||
}
|
||||
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]
|
||||
|
|
@ -61,35 +54,28 @@ def _new_should_allow(verdict, trust_level, force):
|
|||
return False
|
||||
|
||||
|
||||
class TestForceNeverOverridesDangerous:
|
||||
"""The core bug: --force bypassed the dangerous verdict block."""
|
||||
class TestPolicyPrecedenceForDangerousVerdicts:
|
||||
def test_builtin_dangerous_is_allowed_by_policy(self):
|
||||
assert _new_should_allow("dangerous", "builtin", force=False) is True
|
||||
|
||||
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_trusted_dangerous_is_blocked_without_force(self):
|
||||
assert _new_should_allow("dangerous", "trusted", force=False) is False
|
||||
|
||||
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_force_overrides_dangerous_for_community(self):
|
||||
assert _new_should_allow("dangerous", "community", force=True) is True
|
||||
|
||||
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_overrides_dangerous_for_trusted(self):
|
||||
assert _new_should_allow("dangerous", "trusted", force=True) is True
|
||||
|
||||
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
|
||||
def test_old_code_happened_to_allow_forced_dangerous_community(self):
|
||||
assert _old_should_allow("dangerous", "community", force=True) is True
|
||||
|
|
|
|||
|
|
@ -46,9 +46,9 @@ from tools.skills_guard import (
|
|||
|
||||
|
||||
class TestResolveTrustLevel:
|
||||
def test_builtin_not_exposed(self):
|
||||
# builtin is only used internally, not resolved from source string
|
||||
assert _resolve_trust_level("openai/skills") == "trusted"
|
||||
def test_official_sources_resolve_to_builtin(self):
|
||||
assert _resolve_trust_level("official") == "builtin"
|
||||
assert _resolve_trust_level("official/email/agentmail") == "builtin"
|
||||
|
||||
def test_trusted_repos(self):
|
||||
assert _resolve_trust_level("openai/skills") == "trusted"
|
||||
|
|
@ -116,11 +116,17 @@ class TestShouldAllowInstall:
|
|||
allowed, _ = should_allow_install(self._result("trusted", "caution", f))
|
||||
assert allowed is True
|
||||
|
||||
def test_dangerous_blocked_even_trusted(self):
|
||||
def test_trusted_dangerous_blocked_without_force(self):
|
||||
f = [Finding("x", "critical", "c", "f", 1, "m", "d")]
|
||||
allowed, _ = should_allow_install(self._result("trusted", "dangerous", f))
|
||||
assert allowed is False
|
||||
|
||||
def test_builtin_dangerous_allowed_without_force(self):
|
||||
f = [Finding("x", "critical", "c", "f", 1, "m", "d")]
|
||||
allowed, reason = should_allow_install(self._result("builtin", "dangerous", f))
|
||||
assert allowed is True
|
||||
assert "builtin source" in reason
|
||||
|
||||
def test_force_overrides_caution(self):
|
||||
f = [Finding("x", "high", "c", "f", 1, "m", "d")]
|
||||
allowed, reason = should_allow_install(self._result("community", "caution", f), force=True)
|
||||
|
|
@ -132,22 +138,21 @@ 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)."""
|
||||
def test_force_overrides_dangerous_for_community(self):
|
||||
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
|
||||
assert allowed is True
|
||||
assert "Force-installed" in reason
|
||||
|
||||
def test_force_never_overrides_dangerous_trusted(self):
|
||||
"""--force must not bypass dangerous even for trusted sources."""
|
||||
def test_force_overrides_dangerous_for_trusted(self):
|
||||
f = [Finding("x", "critical", "c", "f", 1, "m", "d")]
|
||||
allowed, _ = should_allow_install(
|
||||
allowed, reason = should_allow_install(
|
||||
self._result("trusted", "dangerous", f), force=True
|
||||
)
|
||||
assert allowed is False
|
||||
assert allowed is True
|
||||
assert "Force-installed" in reason
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue