From 5b402dc0252143ed0c438336e9f7eed08c7a9250 Mon Sep 17 00:00:00 2001 From: Olivier Cervello Date: Thu, 2 Jul 2026 18:54:18 +0200 Subject: [PATCH] fix(ai): make ask-loop approval an explicit allow-list (refactor-safety) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The permission ask-loop treated any non-"deny" answer as approval (`response.get("answer") == "deny"` → deny, else proceed). Both backends currently normalize to exactly "allow"/"deny", so this is behavior-neutral today — but it's deny-list shaped: a new backend or a refactored answer vocabulary could silently approve through a "not deny" gap. Flip the three prompt sites (shell/target/path) to an explicit allow-list via `_is_approved()`: only a normalized "allow" answer proceeds; None, "deny", or any unexpected token denies (fail closed). Surfaced during the AI-task review (round 2 backlog, item 2). Co-Authored-By: Claude Opus 4.8 --- secator/ai/actions.py | 13 ++++++++++--- tests/unit/test_ai_actions.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/secator/ai/actions.py b/secator/ai/actions.py index 98fdcb82c..5c722f0f9 100644 --- a/secator/ai/actions.py +++ b/secator/ai/actions.py @@ -171,6 +171,13 @@ def _build_action_display(action: Dict) -> str: return "" +def _is_approved(response) -> bool: + # Explicit allow-list: only a normalized "allow" answer approves. None, "deny", + # or any unexpected token denies (fail closed) — so a new backend or a refactored + # answer vocabulary can't silently approve via a "not deny" gap. + return bool(response) and response.get("answer") == "allow" + + def check_guardrails_sync(action: Dict, ctx: ActionContext) -> Tuple[Optional[str], List]: """Non-generator wrapper for check_guardrails. @@ -255,7 +262,7 @@ def check_guardrails(action: Dict, ctx: ActionContext): if is_remote: yield ctx.backend.build_pending_prompt(**ask_kwargs) response = ctx.backend.ask_user(**ask_kwargs) if ctx.backend else None - if response is None or response.get("answer") == "deny": + if not _is_approved(response): return "Action denied: shell command not approved" if parse_failed: return None @@ -279,7 +286,7 @@ def check_guardrails(action: Dict, ctx: ActionContext): if is_remote: yield ctx.backend.build_pending_prompt(**ask_kwargs) response = ctx.backend.ask_user(**ask_kwargs) if ctx.backend else None - if response is None or response.get("answer") == "deny": + if not _is_approved(response): return f"Action denied: target {target} not approved" # Handle path prompts @@ -302,7 +309,7 @@ def check_guardrails(action: Dict, ctx: ActionContext): if is_remote: yield ctx.backend.build_pending_prompt(**ask_kwargs) response = ctx.backend.ask_user(**ask_kwargs) if ctx.backend else None - if response is None or response.get("answer") == "deny": + if not _is_approved(response): return f"Action denied: {access_type} access to {path} not approved" # Re-check to see if more layers need prompting diff --git a/tests/unit/test_ai_actions.py b/tests/unit/test_ai_actions.py index 2fffec603..07b5ed64b 100644 --- a/tests/unit/test_ai_actions.py +++ b/tests/unit/test_ai_actions.py @@ -1129,6 +1129,38 @@ def test_unresolved_after_max_rounds_denies(self): self.assertIn("unresolved", denial) +class TestApprovalAllowList(unittest.TestCase): + """Ask-loop approval must be an explicit allow-list: only "allow" proceeds.""" + + def _run(self, answer): + from secator.ai.actions import check_guardrails_sync + ask = MagicMock(decision="ask", shell_command="somecmd", targets=[], paths=[], reason="needs approval") + allow = MagicMock(decision="allow", shell_command="", targets=[], paths=[], reason="") + engine = MagicMock() + engine.check_action.side_effect = [ask, allow] + backend = MagicMock() + backend.ask_user.return_value = None if answer is None else {"answer": answer} + ctx = ActionContext(targets=['t.com'], model='m') + ctx.permission_engine = engine + ctx.backend = backend + denial, _items = check_guardrails_sync({"action": "shell", "command": "somecmd"}, ctx) + return denial + + def test_allow_proceeds(self): + self.assertIsNone(self._run("allow")) + + def test_deny_denies(self): + self.assertIsNotNone(self._run("deny")) + + def test_unexpected_answer_denies(self): + # an out-of-vocabulary token must NOT be treated as approval (fail closed) + self.assertIsNotNone(self._run("sure")) + self.assertIsNotNone(self._run("allow_all_typo")) + + def test_none_response_denies(self): + self.assertIsNotNone(self._run(None)) + + @unittest.skipUnless(ADDONS_ENABLED['ai'], 'ai addon not installed') class TestSubagentFanoutCap(unittest.TestCase): """H4: recursion depth + per-turn fan-out caps on AI-subagent spawns."""