Skip to content

Commit f8a7aa2

Browse files
ocervellclaude
andauthored
fix(ai): explicit allow-list for ask-loop approval (refactor-safety) (#1265)
Round-2 backlog item #2 from the AI-task review. The permission ask-loop in `actions.py` approved on *any non-`deny` answer* (deny-list shape). Both backends normalize to exactly `allow`/`deny` today, so this is **behavior-neutral now** — but a new backend or a refactored token could slip through the gap. Flips the three prompt sites (shell/target/path) to an explicit allow-list via a small `_is_approved()` helper: only a normalized `allow` proceeds; `None`, `deny`, or any unexpected token denies (fail closed). Tests: `test_ai_actions.py` 75→79 (+4: allow proceeds, deny denies, unexpected token denies, None denies). No regressions. Folds into the aggregate #1241. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 772f5cc commit f8a7aa2

2 files changed

Lines changed: 42 additions & 3 deletions

File tree

secator/ai/actions.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,13 @@ def _build_action_display(action: Dict) -> str:
171171
return ""
172172

173173

174+
def _is_approved(response) -> bool:
175+
# Explicit allow-list: only a normalized "allow" answer approves. None, "deny",
176+
# or any unexpected token denies (fail closed) — so a new backend or a refactored
177+
# answer vocabulary can't silently approve via a "not deny" gap.
178+
return bool(response) and response.get("answer") == "allow"
179+
180+
174181
def check_guardrails_sync(action: Dict, ctx: ActionContext) -> Tuple[Optional[str], List]:
175182
"""Non-generator wrapper for check_guardrails.
176183
@@ -255,7 +262,7 @@ def check_guardrails(action: Dict, ctx: ActionContext):
255262
if is_remote:
256263
yield ctx.backend.build_pending_prompt(**ask_kwargs)
257264
response = ctx.backend.ask_user(**ask_kwargs) if ctx.backend else None
258-
if response is None or response.get("answer") == "deny":
265+
if not _is_approved(response):
259266
return "Action denied: shell command not approved"
260267
if parse_failed:
261268
return None
@@ -279,7 +286,7 @@ def check_guardrails(action: Dict, ctx: ActionContext):
279286
if is_remote:
280287
yield ctx.backend.build_pending_prompt(**ask_kwargs)
281288
response = ctx.backend.ask_user(**ask_kwargs) if ctx.backend else None
282-
if response is None or response.get("answer") == "deny":
289+
if not _is_approved(response):
283290
return f"Action denied: target {target} not approved"
284291

285292
# Handle path prompts
@@ -302,7 +309,7 @@ def check_guardrails(action: Dict, ctx: ActionContext):
302309
if is_remote:
303310
yield ctx.backend.build_pending_prompt(**ask_kwargs)
304311
response = ctx.backend.ask_user(**ask_kwargs) if ctx.backend else None
305-
if response is None or response.get("answer") == "deny":
312+
if not _is_approved(response):
306313
return f"Action denied: {access_type} access to {path} not approved"
307314

308315
# Re-check to see if more layers need prompting

tests/unit/test_ai_actions.py

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1129,6 +1129,38 @@ def test_unresolved_after_max_rounds_denies(self):
11291129
self.assertIn("unresolved", denial)
11301130

11311131

1132+
class TestApprovalAllowList(unittest.TestCase):
1133+
"""Ask-loop approval must be an explicit allow-list: only "allow" proceeds."""
1134+
1135+
def _run(self, answer):
1136+
from secator.ai.actions import check_guardrails_sync
1137+
ask = MagicMock(decision="ask", shell_command="somecmd", targets=[], paths=[], reason="needs approval")
1138+
allow = MagicMock(decision="allow", shell_command="", targets=[], paths=[], reason="")
1139+
engine = MagicMock()
1140+
engine.check_action.side_effect = [ask, allow]
1141+
backend = MagicMock()
1142+
backend.ask_user.return_value = None if answer is None else {"answer": answer}
1143+
ctx = ActionContext(targets=['t.com'], model='m')
1144+
ctx.permission_engine = engine
1145+
ctx.backend = backend
1146+
denial, _items = check_guardrails_sync({"action": "shell", "command": "somecmd"}, ctx)
1147+
return denial
1148+
1149+
def test_allow_proceeds(self):
1150+
self.assertIsNone(self._run("allow"))
1151+
1152+
def test_deny_denies(self):
1153+
self.assertIsNotNone(self._run("deny"))
1154+
1155+
def test_unexpected_answer_denies(self):
1156+
# an out-of-vocabulary token must NOT be treated as approval (fail closed)
1157+
self.assertIsNotNone(self._run("sure"))
1158+
self.assertIsNotNone(self._run("allow_all_typo"))
1159+
1160+
def test_none_response_denies(self):
1161+
self.assertIsNotNone(self._run(None))
1162+
1163+
11321164
@unittest.skipUnless(ADDONS_ENABLED['ai'], 'ai addon not installed')
11331165
class TestSubagentFanoutCap(unittest.TestCase):
11341166
"""H4: recursion depth + per-turn fan-out caps on AI-subagent spawns."""

0 commit comments

Comments
 (0)