[Fix] Extend caller-permission checks to service-account + tighten raw-body acceptance#26493
Conversation
Low: Security hardening of key management authorizationThis PR tightens authorization checks in three key management code paths: it prevents non-admins from directly setting Status: 0 open Posted by Veria AI · 2026-04-25T06:19:47.763Z |
Greptile SummaryThis PR tightens permission checks on three key-management endpoints by introducing an Confidence Score: 5/5Safe to merge — the three security gaps are correctly closed, existing non-admin preset flows continue to work, and the implementation is internally consistent across all call sites. No P0 or P1 findings. All raw-body call sites consistently use allow_safe_presets=False; only the post-handle_key_type rechecks pass True. RegenerateKeyRequest inherits from GenerateKeyRequest so the handle_key_type(data, {}) call is type-safe. The intent is well-documented and the test evidence covers the patched surfaces. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds allow_safe_presets flag to _check_allowed_routes_caller_permission, extends permission guards to /key/service-account/generate, and adds a post-handle_key_type re-check in regenerate_key_fn — all three security fixes appear correct and internally consistent |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Non-admin request body] --> B{allowed_routes set?}
B -- Yes --> C[_check_allowed_routes\nallow_safe_presets=False]
C --> D[403 Forbidden]
B -- No --> E{key_type set?}
E -- No --> F[Continue normally]
E -- Yes --> G[handle_key_type derives routes]
G --> H[_check_allowed_routes\nallow_safe_presets=True]
H --> I{Routes in safe presets?}
I -- Yes llm_api or read_only --> J[200 Key created]
I -- No management_routes etc --> K[403 Forbidden]
A --> L{passthrough routes set?}
L -- Yes --> M[_check_passthrough_routes]
M --> N[403 Forbidden]
L -- No --> E
Reviews (2): Last reviewed commit: "docs: clarify intent of empty-dict handl..." | Re-trigger Greptile
| _check_allowed_routes_caller_permission( | ||
| allowed_routes=handle_key_type(data, {}).get("allowed_routes"), | ||
| user_api_key_dict=user_api_key_dict, | ||
| allow_safe_presets=True, | ||
| ) |
There was a problem hiding this comment.
handle_key_type invoked with empty dict as side-effect-free lookup
handle_key_type(data, {}) is called with a fresh empty dict purely to extract the key-type-derived allowed_routes. handle_key_type was designed to transform an existing data_json dict, so this usage is non-obvious and could silently break if someone later adds side effects on the passed dict or on data inside that function. Since the function's mapping logic is simple, consider an inline helper or at least a comment explaining why an empty dict is intentionally passed, to prevent a future contributor from "fixing" this into handle_key_type(data, data_json) which would have actual side effects here.
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!
bd914a6 to
f7f77d0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Relevant issues
Follow-up to #26492 (already merged) addressing review notes on the same area.
Summary
Three follow-ups in
key_management_endpoints.py:_check_allowed_routes_caller_permissionnow opts into the safe-presetcarve-out only at the post-
handle_key_typere-check inside_common_key_generation_helper. The raw-body call sites(
/key/generateentry,_validate_update_key_data,regenerate_key_fn,and now
/key/service-account/generate) keep the default strictness, sonon-admins can no longer write
allowed_routes=[\"llm_api_routes\"]directly in the request body and must go through
key_typeinstead./key/service-account/generate(generate_service_account_key_fn) nowruns
_check_allowed_routes_caller_permissionand_check_passthrough_routes_caller_permissionat entry, matching/key/generate. Previously a non-admin team admin could create aservice-account key with
metadata.allowed_passthrough_routesandreach admin-only routes through the passthrough gate.
/key/regeneratemirrors/key/generate's post-handle_key_typere-check, so a
key_typepreset submitted via the regenerate body isrouted through the same caller-permission helper before the request
continues. Functionally a no-op today (the request fails downstream at
the prisma layer because the column does not exist) but closes the
surface against future schema or routing changes.
The legitimate
key_type=llm_apiandkey_type=read_onlyflows fornon-admin callers continue to work, since the post-
handle_key_typecall site is the only one that opts into the safe-preset carve-out.
Testing
End-to-end against a live proxy with the master key and a fresh
internal-user key:
allowed_routes=[\"llm_api_routes\"]on/key/generate,/key/update,/key/regenerate→ 403 (was 200 after the previous PR)key_type=managementon/key/regenerate→ 403 (was 500 from a downstream column miss)/key/service-account/generatewithmetadata.allowed_passthrough_routes→ 403 (was 200)key_type=llm_apiandkey_type=read_onlyon/key/generate→ 200 (preset path still works)/key/infoon own key) → 200Type
🐛 Bug Fix
Screenshots