feat(ai): constrain ai task to allowed_targets scope (AI-hardening round 1) - #1217
feat(ai): constrain ai task to allowed_targets scope (AI-hardening round 1)#1217ocervell wants to merge 2 commits into
Conversation
Add an internal `allowed_targets` opt to the `ai` task — a platform-set allow-list of target strings/regexes (e.g. validated workspace mandates). It is marked internal (set by the platform, not the user, like `context`). Wire it into PermissionEngine: when `allowed_targets` is set it forces the target-check step to run and a proposed `target(...)` action is allowed only if the value (or its URL host/host:port components) matches one of the regexes. Deny rules still take precedence. Invalid regexes fall back to a literal (escaped) match. Tests cover literal/regex/url-host matches, out-of-scope constraint, deny precedence, and the invalid-regex fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5vSjfkBuGAAHdKxHS3ySm
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough
Changesallowed_targets Guardrail Constraint
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/test_ai_guardrails.py (1)
353-373: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winStrengthen the negative mandate assertions.
The suite still misses two important regressions: a literal allow-list entry should reject suffix hosts like
example.com.evil.com, and the out-of-scope cases should assert"deny"rather than just"not allow"so anaskpath cannot slip through unnoticed.Suggested test additions
def test_allowed_target_literal_match(self): engine = self._make_engine(allow=["shell(nmap)"], allowed_targets=["example.com"]) result = engine.check_action({"action": "shell", "command": "nmap example.com"}) self.assertEqual(result.decision, "allow") + def test_allowed_target_literal_does_not_prefix_match(self): + engine = self._make_engine(allow=["shell(nmap)"], allowed_targets=["example.com"]) + result = engine.check_action({"action": "shell", "command": "nmap example.com.evil.com"}) + self.assertEqual(result.decision, "deny") + def test_target_outside_allowed_targets_is_constrained(self): """A target not matching any allowed_targets regex must NOT be silently allowed.""" engine = self._make_engine(allow=["shell(nmap)"], allowed_targets=[r".*\.example\.com"]) result = engine.check_action({"action": "shell", "command": "nmap evil.attacker.com"}) - self.assertNotEqual(result.decision, "allow") + self.assertEqual(result.decision, "deny") def test_allowed_targets_presence_forces_target_check(self): """Even with no config target rules, allowed_targets makes the target step run.""" engine = self._make_engine(allow=["task(*)"], allowed_targets=["10.0.0.1"]) result = engine.check_action({"action": "task", "name": "nmap", "targets": ["8.8.8.8"]}) - self.assertNotEqual(result.decision, "allow") + self.assertEqual(result.decision, "deny")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_ai_guardrails.py` around lines 353 - 373, Strengthen the guardrails tests in test_ai_guardrails by adding a negative case for literal allowed_targets so the check in test_allowed_target_literal_match rejects suffix lookalikes like example.com.evil.com, and update the out-of-scope assertions in test_target_outside_allowed_targets_is_constrained and test_allowed_targets_presence_forces_target_check to assert a deny decision instead of only not allow. Use the existing _make_engine and check_action flow to verify the target-validation path enforces denial rather than falling back to ask.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@secator/ai/guardrails.py`:
- Around line 554-558: The target allow-list check in _matches_allowed_targets
is too permissive because rx.match() turns literal entries into prefix matches;
remove the fallback and rely on rx.fullmatch() only so allow-list patterns
remain exact. Keep the change localized to _matches_allowed_targets in
guardrails.py, since _check_value() already normalizes URL targets into host and
host:port before this comparison.
- Around line 729-735: The target allow-list handling in
`Guardrails._check_values()` currently only allows matching `allowed_targets`
but does not explicitly reject non-matching targets, letting later
config/runtime paths in `_check_values()`, `prompt_target()`, and the workspace
auto-approve flow in `secator/tasks/ai.py` override the mandate. Update the
`rule_type == "target"` branch to return a deny/forbid `PermissionResult` when
`allowed_targets` is set and none of `values_to_check` match, so out-of-scope
targets are blocked before any other allow path is consulted.
---
Nitpick comments:
In `@tests/unit/test_ai_guardrails.py`:
- Around line 353-373: Strengthen the guardrails tests in test_ai_guardrails by
adding a negative case for literal allowed_targets so the check in
test_allowed_target_literal_match rejects suffix lookalikes like
example.com.evil.com, and update the out-of-scope assertions in
test_target_outside_allowed_targets_is_constrained and
test_allowed_targets_presence_forces_target_check to assert a deny decision
instead of only not allow. Use the existing _make_engine and check_action flow
to verify the target-validation path enforces denial rather than falling back to
ask.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f6a9905-698a-4824-bd86-fef33867a5d7
📒 Files selected for processing (3)
secator/ai/guardrails.pysecator/tasks/ai.pytests/unit/test_ai_guardrails.py
| def _matches_allowed_targets(self, value: str) -> bool: | ||
| """Check if a target value matches any platform-supplied allowed_targets regex.""" | ||
| for rx in self.allowed_targets: | ||
| if rx.fullmatch(value) or rx.match(value): | ||
| return True |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
match() widens literal allow-list entries into prefix matches.
Line 557 lets allowed_targets=["example.com"] match example.com.evil.com, and the escaped-literal fallback inherits the same bug. _check_value() already expands URLs into host and host:port, so fullmatch() alone preserves the intended scope boundary.
Suggested fix
def _matches_allowed_targets(self, value: str) -> bool:
"""Check if a target value matches any platform-supplied allowed_targets regex."""
for rx in self.allowed_targets:
- if rx.fullmatch(value) or rx.match(value):
+ if rx.fullmatch(value):
return True
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _matches_allowed_targets(self, value: str) -> bool: | |
| """Check if a target value matches any platform-supplied allowed_targets regex.""" | |
| for rx in self.allowed_targets: | |
| if rx.fullmatch(value) or rx.match(value): | |
| return True | |
| def _matches_allowed_targets(self, value: str) -> bool: | |
| """Check if a target value matches any platform-supplied allowed_targets regex.""" | |
| for rx in self.allowed_targets: | |
| if rx.fullmatch(value): | |
| return True |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/ai/guardrails.py` around lines 554 - 558, The target allow-list check
in _matches_allowed_targets is too permissive because rx.match() turns literal
entries into prefix matches; remove the fallback and rely on rx.fullmatch() only
so allow-list patterns remain exact. Keep the change localized to
_matches_allowed_targets in guardrails.py, since _check_value() already
normalizes URL targets into host and host:port before this comparison.
| # Platform-supplied allowed_targets (regex) allow-list — checked after deny | ||
| # (deny still wins) but before config/runtime allow rules. | ||
| if rule_type == "target" and self.allowed_targets: | ||
| for v in values_to_check: | ||
| if self._matches_allowed_targets(v): | ||
| return PermissionResult(decision="allow", reason=f"Allowed by mandate: target({v})") | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Reject out-of-scope targets before consulting other allow paths.
When allowed_targets is present and none of values_to_check match, this code falls through to config/runtime target(...) rules. In this file, that also means _check_values() can downgrade the miss into ask, and prompt_target() or the workspace auto-approve path in secator/tasks/ai.py can add a runtime allow for an out-of-scope target. The mandate needs an explicit deny on mismatch.
Suggested fix
# Platform-supplied allowed_targets (regex) allow-list — checked after deny
# (deny still wins) but before config/runtime allow rules.
if rule_type == "target" and self.allowed_targets:
for v in values_to_check:
if self._matches_allowed_targets(v):
return PermissionResult(decision="allow", reason=f"Allowed by mandate: target({v})")
+ return PermissionResult(
+ decision="deny",
+ reason=f"Outside allowed_targets mandate: target({value})",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Platform-supplied allowed_targets (regex) allow-list — checked after deny | |
| # (deny still wins) but before config/runtime allow rules. | |
| if rule_type == "target" and self.allowed_targets: | |
| for v in values_to_check: | |
| if self._matches_allowed_targets(v): | |
| return PermissionResult(decision="allow", reason=f"Allowed by mandate: target({v})") | |
| # Platform-supplied allowed_targets (regex) allow-list — checked after deny | |
| # (deny still wins) but before config/runtime allow rules. | |
| if rule_type == "target" and self.allowed_targets: | |
| for v in values_to_check: | |
| if self._matches_allowed_targets(v): | |
| return PermissionResult(decision="allow", reason=f"Allowed by mandate: target({v})") | |
| return PermissionResult( | |
| decision="deny", | |
| reason=f"Outside allowed_targets mandate: target({value})", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/ai/guardrails.py` around lines 729 - 735, The target allow-list
handling in `Guardrails._check_values()` currently only allows matching
`allowed_targets` but does not explicitly reject non-matching targets, letting
later config/runtime paths in `_check_values()`, `prompt_target()`, and the
workspace auto-approve flow in `secator/tasks/ai.py` override the mandate.
Update the `rule_type == "target"` branch to return a deny/forbid
`PermissionResult` when `allowed_targets` is set and none of `values_to_check`
match, so out-of-scope targets are blocked before any other allow path is
consulted.
Symmetric to allowed_targets: PermissionEngine now accepts a platform-set denied_targets list (single/regex patterns). A target(...) value (or its URL host / host:port) matching a denied_targets entry is DENIED, and deny takes precedence over allowed_targets (a target matching both is denied), mirroring the mandate scope matcher's deny-wins. Presence of denied_targets also forces the target-check step on. The ai task gains an internal denied_targets opt that flows to the engine. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5vSjfkBuGAAHdKxHS3ySm
1 participant
AI-hardening round 1 — secator core (NOT yet deployed)
Stops the
aitask (and the runners it spawns) from drifting outside the authorized workspace scope by adding a platform-supplied target allow-list.What's done
allowed_targetsopt on theaitask (secator/tasks/ai.py): a list of allowed target strings/regexes, markedinternal: True(set by the platform, not the user — likecontext). Read in_init_optionsand passed to the PermissionEngine.secator/ai/guardrails.py):PermissionEngine.__init__now takesallowed_targets. Entries are compiled as regexes (invalid regex falls back to an escaped literal). When set, they:_has_rules_for("target")returns True), so out-of-scope targets are constrained rather than silently allowed;target(...)value (or its URL host / host:port components) when it matches an entry — checked after deny rules (deny still wins) and before config/runtime allow rules.tests/unit/test_ai_guardrails.py, newTestAllowedTargets): literal/regex/URL-host matches, out-of-scope constraint, deny precedence, task-target scope, invalid-regex fallback.Verification
tests/unit/test_ai_guardrails.py: 126 passed (9 new).flake8(project.flake8): clean on all changed lines (the 5 pre-existing E501/F841/E30x in the test file are onorigin/maintoo, outside this change).Companion PR
Pairs with freelabz/secator-api#200 (attaches validated mandates as
allowed_targets, addscontext.allowed_targets, andDISALLOWED_RUNNER_OPTIONS). This is AI-hardening round 1, not yet deployed.🤖 Generated with Claude Code
https://claude.ai/code/session_01P5vSjfkBuGAAHdKxHS3ySm
Summary by CodeRabbit
New Features
Bug Fixes