-
Notifications
You must be signed in to change notification settings - Fork 134
feat(ai): constrain ai task to allowed_targets scope (AI-hardening round 1) #1217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -525,18 +525,39 @@ class PermissionEngine: | |||||||||||||||||||||||||||||||||
| Two-step validation: (1) action type check, (2) target/path check. | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def __init__(self, config: Dict, targets: List[str] = None, workspace: str = ""): | ||||||||||||||||||||||||||||||||||
| def __init__(self, config: Dict, targets: List[str] = None, workspace: str = "", allowed_targets: List[str] = None): | ||||||||||||||||||||||||||||||||||
| self.targets = targets or [] | ||||||||||||||||||||||||||||||||||
| self.workspace = str(workspace) | ||||||||||||||||||||||||||||||||||
| self.rules = {"allow": [], "deny": [], "ask": []} | ||||||||||||||||||||||||||||||||||
| self.runtime_allow: List[Tuple[str, List[str]]] = [] | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Platform-supplied allow-list of target regexes (e.g. validated workspace | ||||||||||||||||||||||||||||||||||
| # mandates). When set, a `target(...)` action is allowed only if it matches | ||||||||||||||||||||||||||||||||||
| # one of these regexes — this constrains the AI to the authorized scope. | ||||||||||||||||||||||||||||||||||
| # Each entry is matched as a regex (full-match), falling back to a literal | ||||||||||||||||||||||||||||||||||
| # match if the pattern is not valid regex. | ||||||||||||||||||||||||||||||||||
| self.allowed_targets: List = [] | ||||||||||||||||||||||||||||||||||
| for pat in (allowed_targets or []): | ||||||||||||||||||||||||||||||||||
| if not pat: | ||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||
| self.allowed_targets.append(re.compile(pat)) | ||||||||||||||||||||||||||||||||||
| except re.error: | ||||||||||||||||||||||||||||||||||
| self.allowed_targets.append(re.compile(re.escape(pat))) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| for category in ("allow", "deny", "ask"): | ||||||||||||||||||||||||||||||||||
| for rule_str in config.get(category, []): | ||||||||||||||||||||||||||||||||||
| resolved = self._resolve_variables(rule_str) | ||||||||||||||||||||||||||||||||||
| rule_type, patterns = parse_rule(resolved) | ||||||||||||||||||||||||||||||||||
| self.rules[category].append((rule_type, patterns)) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def _resolve_variables(self, rule: str) -> str: | ||||||||||||||||||||||||||||||||||
| """Replace {workspace} and {targets} variables in a rule string.""" | ||||||||||||||||||||||||||||||||||
| result = rule.replace("{workspace}", self.workspace) | ||||||||||||||||||||||||||||||||||
|
|
@@ -621,6 +642,10 @@ def check_action(self, action: Dict) -> PermissionResult: | |||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def _has_rules_for(self, rule_type: str) -> bool: | ||||||||||||||||||||||||||||||||||
| """Check if any rules exist for the given rule type.""" | ||||||||||||||||||||||||||||||||||
| # Platform-supplied allowed_targets act as a target allow-list: their presence | ||||||||||||||||||||||||||||||||||
| # forces the target-check step to run so out-of-scope targets get constrained. | ||||||||||||||||||||||||||||||||||
| if rule_type == "target" and self.allowed_targets: | ||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||
| for category in ("allow", "deny", "ask"): | ||||||||||||||||||||||||||||||||||
| for rt, _ in self.rules[category]: | ||||||||||||||||||||||||||||||||||
| if rt == rule_type: | ||||||||||||||||||||||||||||||||||
|
|
@@ -701,6 +726,13 @@ def _check_value(self, rule_type: str, value: str) -> PermissionResult: | |||||||||||||||||||||||||||||||||
| if match_rule(v, patterns): | ||||||||||||||||||||||||||||||||||
| return PermissionResult(decision="deny", reason=f"Denied by rule: {rule_type}({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})") | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
Comment on lines
+762
to
+768
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win Reject out-of-scope targets before consulting other allow paths. When 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
| for rt, patterns in self.rules["allow"]: | ||||||||||||||||||||||||||||||||||
| if rt == rule_type: | ||||||||||||||||||||||||||||||||||
| for v in values_to_check: | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
match()widens literal allow-list entries into prefix matches.Line 557 lets
allowed_targets=["example.com"]matchexample.com.evil.com, and the escaped-literal fallback inherits the same bug._check_value()already expands URLs intohostandhost:port, sofullmatch()alone preserves the intended scope boundary.Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents