fix(guardrails): recompile ToolPermissionGuardrail rules on update_in_memory_litellm_params#29613
Conversation
…_memory_litellm_params
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes silent stale-state enforcement in
Confidence Score: 3/5The change introduces a regression in the override: when The core refactor is sound and the intent is correct, but the ordering of litellm/proxy/guardrails/guardrail_hooks/tool_permission.py — specifically the
|
| Filename | Overview |
|---|---|
| litellm/proxy/guardrails/guardrail_hooks/tool_permission.py | Extracts rule compilation into _recompile_rules and adds update_in_memory_litellm_params override, but the override has a defect: calling super() first lets the base class set self.rules = None (the Pydantic default) when no rules are provided, and the subsequent guard skips _recompile_rules, leaving self.rules as None and causing a TypeError on the next tool-permission check. |
Reviews (1): Last reviewed commit: "fix(guardrails): recompile ToolPermissio..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| super().update_in_memory_litellm_params(litellm_params) | ||
| if litellm_params.rules is not None: | ||
| self._recompile_rules(litellm_params.rules) |
There was a problem hiding this comment.
self.rules left as None when litellm_params.rules is not provided
The base-class super().update_in_memory_litellm_params(litellm_params) iterates vars(litellm_params) and calls setattr(self, key, value) for every field — including rules, which defaults to None in ToolPermissionGuardrailConfigModel. When a PUT /guardrails call does not include rules, litellm_params.rules is None, so super() sets self.rules = None and then the guard if litellm_params.rules is not None: skips _recompile_rules. The next incoming request hits for rule in self.rules: in _check_tool_permission or _get_permission_for_tool_call and raises TypeError: 'NoneType' object is not iterable, crashing enforcement entirely. A minimal safeguard like if not isinstance(self.rules, list): self.rules = [] after the super call (or calling _recompile_rules unconditionally with litellm_params.rules or []) would prevent this.
| self.rules: List[ToolPermissionRule] = [] | ||
| self._compiled_rule_patterns: Dict[str, Dict[str, re.Pattern]] = {} | ||
| self._compiled_rule_targets: Dict[str, Dict[str, Optional[re.Pattern]]] = {} | ||
| if rules: | ||
| for rule_item in rules: | ||
| if isinstance(rule_item, ToolPermissionRule): | ||
| rule = rule_item | ||
| else: | ||
| rule = ToolPermissionRule(**rule_item) | ||
| self.rules.append(rule) | ||
|
|
||
| compiled_target_patterns: Dict[str, Optional[re.Pattern]] = { | ||
| "tool_name": None, | ||
| "tool_type": None, | ||
| } | ||
| if rule.tool_name is not None: | ||
| try: | ||
| compiled_target_patterns["tool_name"] = re.compile( | ||
| rule.tool_name | ||
| ) | ||
| except re.error as exc: | ||
| raise ValueError( | ||
| f"Invalid regex for tool_name in rule '{rule.id}': {exc}" | ||
| ) from exc | ||
| if rule.tool_type is not None: | ||
| try: | ||
| compiled_target_patterns["tool_type"] = re.compile( | ||
| rule.tool_type | ||
| ) | ||
| except re.error as exc: | ||
| raise ValueError( | ||
| f"Invalid regex for tool_type in rule '{rule.id}': {exc}" | ||
| ) from exc | ||
| self._compiled_rule_targets[rule.id] = compiled_target_patterns | ||
|
|
||
| if rule.allowed_param_patterns: | ||
| compiled_patterns: Dict[str, re.Pattern] = {} | ||
| for path, pattern in rule.allowed_param_patterns.items(): | ||
| try: | ||
| compiled_patterns[path] = re.compile(pattern) | ||
| except re.error as exc: | ||
| raise ValueError( | ||
| f"Invalid regex in allowed_param_patterns for rule '{rule.id}': {exc}" | ||
| ) from exc | ||
|
|
||
| if compiled_patterns: | ||
| self._compiled_rule_patterns[rule.id] = compiled_patterns | ||
| self._recompile_rules(rules) |
There was a problem hiding this comment.
_recompile_rules call that follows — that helper already resets all three attributes at its top. These lines are redundant.
| self.rules: List[ToolPermissionRule] = [] | |
| self._compiled_rule_patterns: Dict[str, Dict[str, re.Pattern]] = {} | |
| self._compiled_rule_targets: Dict[str, Dict[str, Optional[re.Pattern]]] = {} | |
| if rules: | |
| for rule_item in rules: | |
| if isinstance(rule_item, ToolPermissionRule): | |
| rule = rule_item | |
| else: | |
| rule = ToolPermissionRule(**rule_item) | |
| self.rules.append(rule) | |
| compiled_target_patterns: Dict[str, Optional[re.Pattern]] = { | |
| "tool_name": None, | |
| "tool_type": None, | |
| } | |
| if rule.tool_name is not None: | |
| try: | |
| compiled_target_patterns["tool_name"] = re.compile( | |
| rule.tool_name | |
| ) | |
| except re.error as exc: | |
| raise ValueError( | |
| f"Invalid regex for tool_name in rule '{rule.id}': {exc}" | |
| ) from exc | |
| if rule.tool_type is not None: | |
| try: | |
| compiled_target_patterns["tool_type"] = re.compile( | |
| rule.tool_type | |
| ) | |
| except re.error as exc: | |
| raise ValueError( | |
| f"Invalid regex for tool_type in rule '{rule.id}': {exc}" | |
| ) from exc | |
| self._compiled_rule_targets[rule.id] = compiled_target_patterns | |
| if rule.allowed_param_patterns: | |
| compiled_patterns: Dict[str, re.Pattern] = {} | |
| for path, pattern in rule.allowed_param_patterns.items(): | |
| try: | |
| compiled_patterns[path] = re.compile(pattern) | |
| except re.error as exc: | |
| raise ValueError( | |
| f"Invalid regex in allowed_param_patterns for rule '{rule.id}': {exc}" | |
| ) from exc | |
| if compiled_patterns: | |
| self._compiled_rule_patterns[rule.id] = compiled_patterns | |
| self._recompile_rules(rules) | |
| self._recompile_rules(rules) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Heads-up to avoid duplicated effort: this targets the same bug as #29592. I opened #29655 against
Also note the "Verify PR source branch" check requires external/fork PRs to target |
3 participants
Summary
Fixes #29592.
ToolPermissionGuardrailcompiles_compiled_rule_targetsand_compiled_rule_patternsonly inside__init__. The base-classupdate_in_memory_litellm_paramsupdatesself.rulesviasetattrbut never rebuilds the compiled maps. Any rule change sent throughPUT /guardrailsis accepted but silently ignored at enforcement time until the process reinitializes the guardrail (restart, PATCH path, or DB poll).PresidioGuardrailhas the same derived-state problem and already overridesupdate_in_memory_litellm_paramsto re-apply its state. This PR mirrors that pattern forToolPermissionGuardrail.Changes
__init__into a private_recompile_rules(self, rules)helper.__init__now callsself._recompile_rules(rules)(behaviour unchanged).update_in_memory_litellm_paramsoverride: callssuper(), then callsself._recompile_rules(litellm_params.rules)whenever rules are updated, and also re-normalisesdefault_action/on_disallowed_actionfrom the incoming params.Test plan
update_in_memory_litellm_params,_compiled_rule_patternsshould contain the new pattern and_get_permission_for_tool_callshould enforce itToolPermissionGuardraildirectly with rules still works (rules compiled in__init__)update_in_memory_litellm_paramswithrules=Nonedoes not wipe existing compiled state