fix(proxy): improve input validation on management endpoints - #25445
Conversation
…loyment best practices (BerriAI#25439) - New doc page covering all signed image variants, verification commands, CI/CD enforcement (K8s Sigstore Policy Controller, GCP Binary Authorization, AWS/EKS, GitHub Actions), digest pinning, and safe upgrade patterns - Added to sidebar under Setup & Deployment - Cross-linked from the existing deploy.md cosign section Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Krrish Dholakia <krrish-berri-2@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR closes three distinct security gaps in the LiteLLM proxy: it sandboxes the Jinja2 environment used to render user-supplied dotprompt templates ( Confidence Score: 4/5Safe to merge pending review of the two P2 findings around org-admin role scope and the silent clear-via-empty-list edge case on key update. All three security fixes are well-implemented and tested. The only remaining concerns are P2: the litellm/proxy/management_endpoints/key_management_endpoints.py — specifically the
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds _check_allowed_routes_caller_permission to block non-admins from setting allowed_routes on generate and update key paths; check is correctly placed in both flows; minor formatting-only changes elsewhere. |
| litellm/integrations/dotprompt/prompt_manager.py | Replaces jinja2.Environment with ImmutableSandboxedEnvironment to prevent SSTI when user-supplied templates are rendered via /prompts/test; no other logic changes. |
| litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py | Adds _validate_public_image_url helper and applies it to both logo_url and favicon_url in update_ui_theme_settings, rejecting any non-HTTP/HTTPS value to prevent path traversal via the unauthenticated /get_image endpoint. |
| tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py | Adds TestAllowedRoutesCallerPermission class (4 async tests), test_jinja_prompt_manager_is_sandboxed, and two _validate_public_image_url tests — all mock-only, no real network calls. |
Sequence Diagram
sequenceDiagram
participant Client
participant generate_key_fn
participant _check_allowed_routes_caller_permission
participant _common_key_generation_helper
Client->>generate_key_fn: POST /key/generate {allowed_routes: [...]}
generate_key_fn->>_check_allowed_routes_caller_permission: allowed_routes, user_role
alt user_role == PROXY_ADMIN
_check_allowed_routes_caller_permission-->>generate_key_fn: OK
generate_key_fn->>_common_key_generation_helper: proceed
_common_key_generation_helper-->>Client: 200 key created
else non-admin with non-empty allowed_routes
_check_allowed_routes_caller_permission-->>generate_key_fn: raise HTTPException(403)
generate_key_fn-->>Client: 403 Forbidden
else allowed_routes is None or []
_check_allowed_routes_caller_permission-->>generate_key_fn: return (no-op)
generate_key_fn->>_common_key_generation_helper: proceed
_common_key_generation_helper-->>Client: 200 key created
end
Reviews (1): Last reviewed commit: "fix(proxy): improve input validation on ..." | Re-trigger Greptile
| # Empty list is the default on GenerateKeyRequest — treat as "not set". | ||
| if not allowed_routes: | ||
| return | ||
| if user_api_key_dict.user_role == LitellmUserRoles.PROXY_ADMIN.value: |
There was a problem hiding this comment.
ORG_ADMIN role is implicitly blocked from setting allowed_routes
The check only permits PROXY_ADMIN. Org admins (LitellmUserRoles.ORG_ADMIN) who previously could set allowed_routes on keys they manage will now receive a 403. If org admins should retain this capability, the guard needs to include that role:
if user_api_key_dict.user_role in (
LitellmUserRoles.PROXY_ADMIN.value,
LitellmUserRoles.ORG_ADMIN.value,
):
returnIf restricting org admins is intentional, a comment explaining the reasoning would help avoid future confusion.
| # Empty list is the default on GenerateKeyRequest — treat as "not set". | ||
| if not allowed_routes: | ||
| return |
There was a problem hiding this comment.
Non-admin can silently clear existing
allowed_routes via update
UpdateKeyRequest.allowed_routes inherits = [] from KeyRequestBase, so when a non-admin sends an update request that omits allowed_routes, data.allowed_routes is []. The if not allowed_routes: return guard passes, and the update proceeds — potentially writing [] over a previously-restricted allowed_routes set by an admin. If prepare_key_update_data includes fields that differ from None, a non-admin could inadvertently (or deliberately) wipe the allow-list.
Consider checking data.allowed_routes is None instead of truthiness to distinguish "caller did not set this field" from "caller explicitly set empty list", or explicitly exclude [] from the update payload if unchanged from the inherited default.
|
Hey @jaydns Are you facing the same issues with Lint Check |
9b33d9d
into
BerriAI:litellm_yj_04_09_2026
- Discovery context: intended Pwn2Own Berlin 2026 entry under Local Inference category, dropped after silent fix landed in BerriAI/litellm#25445 (commit d910a95661, 2026-04-09). - Vulnerable sink: litellm/integrations/dotprompt/prompt_manager.py:62 used a non-sandboxed jinja2.Environment for templates received via POST /prompts/test (`dotprompt_content`). - Reachability: zero-auth in default config (no master_key); internal_user role sufficient when master_key is set. - Patch: Environment -> ImmutableSandboxedEnvironment, blocks __globals__/__class__/__init__/__mro__ access. - Affected: <= v1.83.4 (verified v1.82.3, v1.82.6, v1.83.4). Patched: v1.83.5+; re-verified safe on v1.83.14 (2026-05-03). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix(proxy): improve input validation on management endpoints
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes