fix(proxy): admin-gate allowed_routes presence on /key/update and /key/regenerate (LIT-4139) - #31987
Conversation
Greptile SummaryCloses a proxy key authorization bypass where non-admins could send explicit
Confidence Score: 5/5Safe to merge — the change is a minimal, targeted tightening of a single helper and its four call sites, with no behaviour change for well-formed requests that omit the field. The helper logic is straightforward, the four updated call sites are consistent, the two intentionally unchanged post-handle_key_type rechecks are correctly reasoned about in both the code comments and the PR description, and the eight new tests are mutation-killed against the exact lines they guard. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds allowed_routes_was_provided: bool = False kwarg to _check_allowed_routes_caller_permission and passes the presence flag at the four raw-body call sites; the early-return guard is tightened from not allowed_routes to not allowed_routes_was_provided and not allowed_routes, correctly closing the bypass for explicit-empty/null values; the allow_safe_presets branch gets an and allowed_routes null-guard to prevent TypeError. |
| tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py | Eight new tests added to TestAllowedRoutesCallerPermission: explicit-empty and explicit-null on update, explicit-empty on regenerate and generate, enterprise-gate ordering on regenerate, helper acceptance of safe presets, helper rejection of unsafe presets, and TypeError guard for allowed_routes_was_provided=True + None; no existing tests are weakened. |
Reviews (4): Last reviewed commit: "fix(proxy): admin-gate `allowed_routes` ..." | Re-trigger Greptile
Greptile SummaryThis PR tightens the
Confidence Score: 4/5The changes are narrowly scoped to the permission helper and its four call sites; the logic correctly threads model_fields_set presence through without touching the safe-presets path used by key_type flows. The core fix is well-constructed and the five new tests cover the critical newly-handled cases. The only gap is that generate_service_account_key_fn has no new explicit-presence test; a regression there would go undetected by this test suite. tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py needs a test for the explicit-presence case on generate_service_account_key_fn to match coverage on the other three endpoints.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds is_explicit: bool param to _check_allowed_routes_caller_permission and threads "allowed_routes" in data.model_fields_set through all four raw-body call sites; logic is correct and backward-compatible with the two post-handle_key_type call sites that keep defaults. |
| tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py | Adds five focused tests for explicit-empty and explicit-null allowed_routes on generate/update/regenerate; generate_service_account_key_fn is the one changed call site without a new explicit-presence test. |
Reviews (2): Last reviewed commit: "fix(proxy): admin-gate `allowed_routes` ..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
4f3ba8d to
96991c0
Compare
|
@greptileai re-review, amended commit (96991c0) adds three helper-level tests addressing the internal reviewer's Minor + Nit: (1) positive safe-preset accept test ( |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 96991c0. Configure here.
…key/regenerate (LIT-4139) `_check_allowed_routes_caller_permission` previously keyed its admin-only rule on truthiness. The refactor adds an `allowed_routes_was_provided` keyword param that raw-body call sites populate from `"allowed_routes" in data.model_fields_set`, so a caller that omits the field (default flows through) is distinct from one that sends any explicit value. Four raw-body call sites now pass `allowed_routes_was_provided=...`: `_common_key_generation_helper`, `generate_service_account_key_fn`, `_validate_update_key_data`, and `regenerate_key_fn`. Two derived-value call sites keep the pre-fix shape: the post-`handle_key_type` recheck at `_common_key_generation_helper` and the mirror in `regenerate_key_fn`. Both pass values produced by `handle_key_type` (not by the request body), so `allowed_routes_was_provided` stays False and the `allow_safe_presets=True` carve-out continues to accept the `llm_api_routes` / `info_routes` presets. In `regenerate_key_fn` the gate runs before the `premium_user` license check, matching the LIT-4092 ordering. `test_non_admin_regenerate_key_allowed_routes_rejected_before_enterprise_gate` pins the ordering; it fails on a swap of the two gates. Tests in `tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py` under `TestAllowedRoutesCallerPermission`: - test_non_admin_generate_key_explicit_empty_allowed_routes_rejected - test_non_admin_update_key_explicit_empty_allowed_routes_rejected - test_non_admin_update_key_explicit_null_allowed_routes_rejected - test_non_admin_regenerate_key_explicit_empty_allowed_routes_rejected - test_non_admin_regenerate_key_allowed_routes_rejected_before_enterprise_gate - test_helper_accepts_derived_safe_preset_for_non_admin - test_helper_rejects_derived_unsafe_preset_for_non_admin - test_helper_rejects_when_provided_and_none_without_typeerror The four attack-vector tests fail on the pre-fix HEAD and pass on this commit. Three helper-level tests pin the derived-value branch and the load-bearing None guard; each is mutation-killed against a targeted change to the frozenset or the guard. Full mapped test file (347 tests) green.
96991c0 to
067c20a
Compare
|
@greptileai re-review, amended commit (067c20a) renames the |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 067c20a. Configure here.
Relevant issues
Linear ticket
Resolves LIT-4139
Pre-Submission checklist
@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).
Type
Bug Fix
Changes
_check_allowed_routes_caller_permissionpreviously used truthiness (if not allowed_routes: return) to distinguish "field not set" from "field explicitly set to a truthy value". The refactor adds anallowed_routes_was_provided: bool = Falsekeyword param. Raw-body call sites pass"allowed_routes" in data.model_fields_set; the omit-default carve-out remains fornot allowed_routes_was_provided and not allowed_routesFour raw-body call sites updated to pass the presence flag from
model_fields_set:_common_key_generation_helper(line 1571),generate_service_account_key_fn(line 1742),_validate_update_key_data(line 2237), andregenerate_key_fn(line 4559). Two derived-value call sites left unchanged (the post-handle_key_typerecheck withallow_safe_presets=Trueat line 914 and its regenerate mirror at line 4580) — those pass values derived byhandle_key_typefromdata.key_type, not fromdata.model_fields_set, and are handled by the_NON_ADMIN_SAFE_ALLOWED_ROUTES_PRESETSallowlist inside the helperIn
regenerate_key_fnthe gate runs before thepremium_userlicense check so the rejection is consistent across premium and non-premium deployments. That ordering is pinned bytest_non_admin_regenerate_key_allowed_routes_rejected_before_enterprise_gateEight new tests in
TestAllowedRoutesCallerPermissionintests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py. Five cover the non-admin explicit-empty case on generate / update / regenerate, explicit-null on update, and the enterprise-gate ordering on regenerate. Three helper-level tests pin the derived-value acceptance path (llm_api_routesaccepted,management_routesrejected) and the load-bearingand allowed_routesguard against a future caller that pairsallowed_routes_was_provided=Truewith aNonevalue. Every added attack-vector or contract test is mutation-killed against a targeted change to the code it locks. Full mapped file (347 tests) greenScreenshots / Proof of Fix
Live proxy on
localhost:4010against Postgres,LITELLM_LICENSEset so/key/regenerateis available. Admin creates alice as aninternal_userand setsallowed_routes: ["/chat/completions"]on alice's personal key/key/updateexplicit empty/key/updateexplicit null/key/regenerateexplicit emptyDB state after three rejected attacks
The admin-set value survived. Controls: admin still writes
allowed_routesfreely, admin still clears with[], alice still updates unrelated fields (tpm_limit=42) on her key, alice's/key/generate {"key_type":"llm_api"}still resolves toallowed_routes: ["llm_api_routes"]through the preset flow, alice's/key/generate {}still mints keys with the defaultallowed_routes: []Boundary conditions
Same fixed premium proxy, verified after the internal review
Explicit
nullon generate and regenerate as a non-admin. The presence check keys onmodel_fields_setsonullis caught the same as[]or any other explicit valueRaw-body
["llm_api_routes"]from a non-admin on all three endpoints. Raw-body call sites leaveallow_safe_presets=False, so a non-admin cannot hand-invoke the safe-preset carve-out; only thekey_typepreset flow reaches the derivation call site withallow_safe_presets=True/key/updateand/key/regeneratereturn the same 403 for the same payloadAdmin
[]accepted on all three endpoints (admin fast path)Note on admin
null(pre-existing, unrelated to this fix):UpdateKeyRequest.allowed_routesandRegenerateKeyRequest.allowed_routesare typedOptional[list] = []but the Pydantic validator on these routes rejects an explicitnullat request-parse time (400 on/key/update, 500 on/key/regenerateafter the wrapper)./key/generateaccepts and coerces to[]. Neither behavior changed here. The unit tests for the helper-levelNonecase bypass Pydantic parsing (constructing the model in Python directly), so they exercise the hypothetical future where Pydantic loosens and confirm the gate still behavesOut of scope, tracked separately
/key/bulk_update,/team/key/bulk_update), which route through_process_single_key_updateand do not invoke this helper.KeyUpdateFields(extra="forbid") omitsallowed_routes, so the field is currently unreachable through the team-bulk path; single-key bulk is admin-only. Latent trap if the allowlist widensNote
Medium Risk
Tightens authorization on proxy key management endpoints; behavior change for non-admins who explicitly send
allowed_routes, with broad test coverage to limit regressions on thekey_typepreset flow.Overview
Non-admins could bypass the
allowed_routesadmin gate by sending[]ornullin/key/generate,/key/update, and/key/regenerate, because_check_allowed_routes_caller_permissiontreated empty/falsy values as “not set.”The helper now takes
allowed_routes_was_provided, set from"allowed_routes" in data.model_fields_setat raw-body entry points, so any explicit body value (including empty list or null) requires PROXY_ADMIN. Omitted fields still skip the gate when the value is the model default. Thekey_typepreset path is unchanged: post-handle_key_typerechecks useallow_safe_presets=Truewithout the presence flag.regenerate_key_fnruns this check before the enterprisepremium_usergate so non-admins get a consistent 403. Tests cover explicit empty/null, preset allowlist behavior, and the safe-presetNoneguard.Reviewed by Cursor Bugbot for commit 067c20a. Bugbot is set up for automated code reviews on this repo. Configure here.