chore(guardrails): tighten tool permission checks#26969
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR tightens the Confidence Score: 4/5Safe to merge with awareness of the fail-closed behavior change for existing users using default_action=allow with param-rule tools. All findings are P2. The logic is well-reasoned and thoroughly tested. The two flagged items — a narrow edge case in the operation-ID normalizer and the backwards-incompatible fail-closed argument check — are unlikely to affect most deployments but are worth discussing before merging. litellm/proxy/guardrails/guardrail_hooks/tool_permission.py (fail-closed change) and litellm/proxy/_lazy_openapi_snapshot.py (custom-ID edge case)
|
| Filename | Overview |
|---|---|
| litellm/proxy/guardrails/guardrail_hooks/tool_permission.py | Core guardrail logic extended with legacy function-call normalization (pre- and post-call), fail-closed argument parsing, and new helper methods for extracting tool names from modern and legacy request shapes. Logic is sound and well-tested; one backwards-incompatible behavior change worth noting. |
| tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tool_permission.py | Extensive new tests covering legacy function-call pre/post hooks, rewrite-mode filtering, and fail-closed argument parsing; one test expectation was deliberately flipped from allow to block (previously discussed in thread). |
| litellm/proxy/_lazy_openapi_snapshot.py | Adds _normalize_operation_ids to stabilize FastAPI-generated operation IDs for multi-method routes; works correctly for its stated purpose but could inadvertently rewrite custom IDs that happen to end with an HTTP method token. |
| tests/test_litellm/proxy/test_lazy_openapi_snapshot.py | New unit tests for _normalize_operation_ids; covers the common case and custom ID preservation, but misses the edge case of custom IDs that end with an HTTP method token. |
| litellm/proxy/_lazy_openapi_snapshot.json | Snapshot JSON updated to reflect the stabilized operation IDs produced by the new normalization logic. |
Reviews (3): Last reviewed commit: "chore(proxy): stabilize lazy openapi sna..." | Re-trigger Greptile
| for forced_tool_name in ( | ||
| self._get_named_tool_choice(data), | ||
| self._get_named_function_call(data), | ||
| ): | ||
| if forced_tool_name is not None: | ||
| request_tools.append((forced_tool_name, "function")) |
There was a problem hiding this comment.
When a tool (e.g., "Read") appears in both tools/functions AND as a named tool_choice/function_call, _collect_request_tools adds it to request_tools more than once. In block mode this is harmless (raises on first denial), and in rewrite mode _modify_request_with_permission_errors converts denied_tool_names to a set, so no duplicate filtering occurs. However, _check_tool_permission is still called redundantly for the same tool, and if the permission check is ever stateful or expensive this could matter. Deduplicating the returned list (e.g., using dict.fromkeys) would make the contract cleaner.
| arguments, parse_error = self._parse_tool_call_arguments(tool_call) | ||
| if parse_error: | ||
| default_message = f"Tool '{tool_identifier}' {parse_error} required by rule '{rule.id}'" | ||
| message = self.render_violation_message( | ||
| default=default_message, | ||
| context={"tool_name": tool_identifier, "rule_id": rule.id}, | ||
| ) | ||
| return False, rule.id, message | ||
| if not arguments: | ||
| last_pattern_failure_msg = f"Tool '{tool_identifier}' is missing arguments required by rule '{rule.id}'" | ||
| continue | ||
| default_message = f"Tool '{tool_identifier}' is missing arguments required by rule '{rule.id}'" | ||
| message = self.render_violation_message( | ||
| default=default_message, | ||
| context={"tool_name": tool_identifier, "rule_id": rule.id}, | ||
| ) | ||
| return False, rule.id, message |
There was a problem hiding this comment.
if not arguments: branch reachable only for empty-dict arguments
After if parse_error: returns for all (None, error_msg) outcomes, the subsequent if not arguments: is only reached when _parse_tool_call_arguments returns ({}, None) — i.e., when the JSON decoded to an empty dict. The guard is correct and intentional (fail closed), but the message "is missing arguments required by rule" is slightly misleading for this case since arguments are present but empty. A message like "has no argument values required by rule" would distinguish this from the actual missing-arguments path and aid debugging.
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!
- BerriAI/litellm#26969: tool-permission guardrail tightening (merge-after-nits) - BerriAI/litellm#26967: VCR Redis observability (merge-as-is) - google-gemini/gemini-cli#26303: brain/critique role split + iteration (needs-discussion) - google-gemini/gemini-cli#26287: voice transcription cursor-position insert (merge-after-nits) - google-gemini/gemini-cli#26274: ssh:// extension install scheme (merge-as-is)
42122b8
into
BerriAI:litellm_internal_staging
Relevant issues
Veria: VERIA-52
What changed
Tests
uv run pytest tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tool_permission.py -quv run pytest tests/test_litellm/proxy/guardrails/guardrail_hooks -quv run --python 3.12 pytest tests/test_litellm/proxy/test_lazy_openapi_snapshot.py tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tool_permission.py -quv run ruff check litellm/proxy/guardrails/guardrail_hooks/tool_permission.py tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tool_permission.pyuv run --python 3.12 ruff check litellm/proxy/_lazy_openapi_snapshot.py tests/test_litellm/proxy/test_lazy_openapi_snapshot.py litellm/proxy/guardrails/guardrail_hooks/tool_permission.py tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tool_permission.pyuv run black .uv run --python 3.12 black --check litellm/proxy/_lazy_openapi_snapshot.py tests/test_litellm/proxy/test_lazy_openapi_snapshot.py litellm/proxy/guardrails/guardrail_hooks/tool_permission.py tests/test_litellm/proxy/guardrails/guardrail_hooks/test_tool_permission.pyuv run --python 3.12 --no-sync mypy --no-incremental .uv run --python 3.12 python -m litellm.proxy._lazy_openapi_snapshotstability check