fix(key_management): enforce upperbound_key_generate_params on /key/regenerate#26340
Conversation
Low: Security hardening — upperbound enforcement on key regenerationThis PR adds a call to Status: 0 open Posted by Veria AI · 2026-04-23T18:46:56.065Z |
Greptile SummaryThis PR fixes a security bypass where Prior review threads have flagged two open points: (1) Confidence Score: 4/5Safe to merge from a correctness standpoint; two open concerns from prior threads (token-before-validation ordering and backwards-incompatibility without a feature flag per repo rule b48b7341) are unresolved. The production fix is minimal and correct — two lines that mirror the existing update-path precedent. All five new tests are properly mocked and verify the key enforcement scenarios. No new P0/P1 issues were found in this review pass. Score is 4 rather than 5 because the backwards-incompatible behaviour change (previously flagged) has not been addressed with a feature flag as required by the repo's own rule, and the token-before-validation ordering remains. litellm/proxy/management_endpoints/key_management_endpoints.py — specifically the ordering of get_new_token vs _enforce_upperbound_key_params and the missing feature flag for the backwards-incompatible enforcement change.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/key_management_endpoints.py | Two-line fix correctly inserts _enforce_upperbound_key_params(data, fill_defaults=False) before prepare_key_update_data; enforcement fires after get_new_token() (flagged in prior thread) and the change is backwards-incompatible without a feature flag (also flagged). |
| tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py | Adds 5 new async unit tests covering over-limit duration, within-limit duration, over-limit max_budget, None-value inheritance, and no-config no-op; all mock network/DB calls correctly per the no-real-network-calls rule. The existing test change (parenthesis removal on "cafebabe" * 8) is cosmetic only. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Client: POST /key/regenerate] --> B[_execute_virtual_key_regeneration]
B --> C[get_new_token - allocates new key string]
C --> D{data is not None?}
D -- Yes --> E[_enforce_upperbound_key_params\nfill_defaults=False]
D -- No --> G[Build update_data with defaults only]
E --> F{Value exceeds\nupperbound?}
F -- Yes --> H[Raise HTTPException 400]
F -- No / None --> I[prepare_key_update_data]
I --> J[DB update LiteLLM_VerificationToken]
G --> J
J --> K[Invalidate cache]
K --> L[Return GenerateKeyResponse 200]
Reviews (2): Last reviewed commit: "fix(key_management): enforce upperbound_..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| non_default_values = {} | ||
| if data is not None: | ||
| # Enforce upperbound key params on regenerate (don't fill defaults) | ||
| _enforce_upperbound_key_params(data, fill_defaults=False) | ||
| non_default_values = await prepare_key_update_data( | ||
| data=data, existing_key_row=key_in_db | ||
| ) |
There was a problem hiding this comment.
get_new_token() is called on line 3758, allocating a new token string, before _enforce_upperbound_key_params raises on an over-cap request. The generated token is never persisted, so there is no security impact, but every rejected regenerate call wastes a call to get_new_token. Moving the enforcement call above get_new_token would be cleaner:
non_default_values = {}
if data is not None:
# Enforce upperbound key params on regenerate (don't fill defaults)
_enforce_upperbound_key_params(data, fill_defaults=False)
new_token = await get_new_token(data=data)
new_token_hash = hash_token(new_token)
...| @@ -3762,6 +3762,8 @@ async def _execute_virtual_key_regeneration( | |||
|
|
|||
| non_default_values = {} | |||
| if data is not None: | |||
| # Enforce upperbound key params on regenerate (don't fill defaults) | |||
| _enforce_upperbound_key_params(data, fill_defaults=False) | |||
There was a problem hiding this comment.
Per the repo's review rules, behaviour changes that can break existing callers should be gated behind a user-controlled flag. Any operator who currently uses /key/regenerate with values above the cap (even inadvertently) will start receiving HTTP 400s after this deploy with no opt-out path. The PR description acknowledges this explicitly ("Operators who were relying on the bypass would need to remove the cap or raise it").
While the existing docs describe upperbound_key_generate_params as a hard ceiling and the bypass was unintended, a flag such as enforce_upperbound_on_regenerate: true (defaulting to true) would let operators stage the rollout and avoid unexpected breakage during upgrades.
Rule Used: What: avoid backwards-incompatible changes without... (source)
michelligabriele
force-pushed
the
litellm_fix_upperbound_regenerate_enforcement
branch
from
40c9cc0 to
ea6b029
Compare
michelligabriele
temporarily deployed
to
integration-postgres
GitHub Actions
Inactive
michelligabriele
temporarily deployed
to
integration-postgres
GitHub Actions
Inactive
michelligabriele
temporarily deployed
to
integration-postgres
GitHub Actions
Inactive
michelligabriele
temporarily deployed
to
integration-postgres
GitHub Actions
Inactive
michelligabriele
temporarily deployed
to
integration-postgres
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…nt endpoints (#28620) * test(proxy): add create_scratch_actor harness helper Adds create_scratch_actor() to the management behavior-suite conftest and extends create_scratch_team() with team_member_permissions / models kwargs, needed by the PR3 team-key-permission and team-model matrices. The new helper mints a scratch-prefixed user + verification token (+ org memberships), all reclaimed by the existing scratch-prefix teardown. * test(proxy): pin /key block, unblock, health, aliases behavior Adds behavior-pinning matrices for POST /key/block, POST /key/unblock, POST /key/health, and GET /key/aliases. Pins that the management-route gate 401s ORG_ADMIN-role callers before _check_key_admin_access runs, the block/unblock round-trip on the blocked column, missing-key 404, and the _apply_non_admin_alias_scope visibility rules for /key/aliases. * test(proxy): pin /key/bulk_update + /team/key/bulk_update behavior Adds behavior-pinning matrices for POST /key/bulk_update (PROXY_ADMIN-only; ORG_ADMIN stopped 401 at the route gate, INTERNAL_USER-role 403 at the handler) and POST /team/key/bulk_update (team-member-permission gate keyed on KEY_UPDATE). Pins batch semantics: empty/over-cap 400, per-key failure isolation into failed_updates, all_keys_in_team broadcast, and no-keys 404. Adds an optional key_alias arg to create_scratch_key for multi-key scenarios. * test(proxy): pin /key SA-generate, v2-info, reset-spend behavior Adds behavior-pinning matrices for POST /key/service-account/generate (team-membership + team-member-permission gating; SA keys carry no user_id), POST /v2/key/info (per-key _can_user_query_key_info silently drops invisible keys), and POST /key/{key}/reset_spend (PROXY_ADMIN or team admin only; missing key 404, reset-value 400). Pins that ORG_ADMIN-role callers are stopped 401 at the management-route gate on the two non-info routes. * test(proxy): close PR1/PR2 key-side deferred coverage gaps Closes the four key-side gaps deferred from PR1/PR2: - 404 on missing key for /key/update and /key/delete (not 401/403) - denied /key/update leaves max_budget/tpm_limit/rpm_limit untouched - /key/regenerate enforces litellm.upperbound_key_generate_params (#26340) - /key/list key_alias substring vs exact (admin-only) + team_id filter, and a non-admin filtering a foreign team is 403 * test(proxy): pin /team block, unblock, available, filter/ui, members/me Adds behavior-pinning matrices for POST /team/block + /team/unblock (management-route gate fronts _verify_team_access; reachable only by PROXY_ADMIN and an org admin of the team's own org), GET /team/available (default empty path), GET /team/filter/ui (route-gated PROXY-ADMIN-only despite the handler having no gate), and GET /team/{team_id}/members/me (caller resolves its own membership; non-member 404, no-user_id key 400). * test(proxy): pin /team model add/delete + permissions endpoints Adds behavior-pinning matrices for POST /team/model/add + /team/model/delete (route-gated PROXY-ADMIN-only; missing team 404), GET /team/permissions_list + POST /team/permissions_update (self-managed; proxy/team/org admin pass), and POST /team/permissions_bulk_update (PROXY_ADMIN-only). Pins the deliberate divergence that the available-team self-join grants read access via permissions_list but never write access via permissions_update. * test(proxy): pin /team delete, bulk_member_add, v2/list, daily/activity Adds behavior-pinning matrices for POST /team/delete (per-team _verify_team_access; batch aborts whole on a missing id), POST /team/bulk_member_add (route-gated PROXY-ADMIN-only; empty/over-cap 400), GET /v2/team/list (_enforce_list_team_v2_access — bare query 401s regular users, org-scoped for org admins) and GET /team/daily/activity (non-member team_ids filter 404, the VERIA-43 fix). * test(proxy): add route-coverage gate + close team org-relocation gap Adds test_route_coverage.py (PR3.M1): parses every @router route literal from the two management-endpoint source files and asserts each is exercised by >=1 behavior-suite scenario — a permanent regression guard for future routes. Closes the last PR1/PR2 deferred gap: the /team/update org-relocation allowed branch, exercised by a dual-org-admin minted via create_scratch_actor. test_team_model uses literal route URLs so the coverage parser resolves them. * test(proxy): bound plain route params to one path segment in coverage gate Plain path params ({team_id}) now compile to [^/?]+ instead of [^?]+, so a parameter cannot span '/'. Starlette ':path' params still match across '/'. Keeps the route-coverage guard from falsely reporting a future multi-segment route as covered. All 37 routes remain covered.
…nt endpoints (BerriAI#28620) * test(proxy): add create_scratch_actor harness helper Adds create_scratch_actor() to the management behavior-suite conftest and extends create_scratch_team() with team_member_permissions / models kwargs, needed by the PR3 team-key-permission and team-model matrices. The new helper mints a scratch-prefixed user + verification token (+ org memberships), all reclaimed by the existing scratch-prefix teardown. * test(proxy): pin /key block, unblock, health, aliases behavior Adds behavior-pinning matrices for POST /key/block, POST /key/unblock, POST /key/health, and GET /key/aliases. Pins that the management-route gate 401s ORG_ADMIN-role callers before _check_key_admin_access runs, the block/unblock round-trip on the blocked column, missing-key 404, and the _apply_non_admin_alias_scope visibility rules for /key/aliases. * test(proxy): pin /key/bulk_update + /team/key/bulk_update behavior Adds behavior-pinning matrices for POST /key/bulk_update (PROXY_ADMIN-only; ORG_ADMIN stopped 401 at the route gate, INTERNAL_USER-role 403 at the handler) and POST /team/key/bulk_update (team-member-permission gate keyed on KEY_UPDATE). Pins batch semantics: empty/over-cap 400, per-key failure isolation into failed_updates, all_keys_in_team broadcast, and no-keys 404. Adds an optional key_alias arg to create_scratch_key for multi-key scenarios. * test(proxy): pin /key SA-generate, v2-info, reset-spend behavior Adds behavior-pinning matrices for POST /key/service-account/generate (team-membership + team-member-permission gating; SA keys carry no user_id), POST /v2/key/info (per-key _can_user_query_key_info silently drops invisible keys), and POST /key/{key}/reset_spend (PROXY_ADMIN or team admin only; missing key 404, reset-value 400). Pins that ORG_ADMIN-role callers are stopped 401 at the management-route gate on the two non-info routes. * test(proxy): close PR1/PR2 key-side deferred coverage gaps Closes the four key-side gaps deferred from PR1/PR2: - 404 on missing key for /key/update and /key/delete (not 401/403) - denied /key/update leaves max_budget/tpm_limit/rpm_limit untouched - /key/regenerate enforces litellm.upperbound_key_generate_params (BerriAI#26340) - /key/list key_alias substring vs exact (admin-only) + team_id filter, and a non-admin filtering a foreign team is 403 * test(proxy): pin /team block, unblock, available, filter/ui, members/me Adds behavior-pinning matrices for POST /team/block + /team/unblock (management-route gate fronts _verify_team_access; reachable only by PROXY_ADMIN and an org admin of the team's own org), GET /team/available (default empty path), GET /team/filter/ui (route-gated PROXY-ADMIN-only despite the handler having no gate), and GET /team/{team_id}/members/me (caller resolves its own membership; non-member 404, no-user_id key 400). * test(proxy): pin /team model add/delete + permissions endpoints Adds behavior-pinning matrices for POST /team/model/add + /team/model/delete (route-gated PROXY-ADMIN-only; missing team 404), GET /team/permissions_list + POST /team/permissions_update (self-managed; proxy/team/org admin pass), and POST /team/permissions_bulk_update (PROXY_ADMIN-only). Pins the deliberate divergence that the available-team self-join grants read access via permissions_list but never write access via permissions_update. * test(proxy): pin /team delete, bulk_member_add, v2/list, daily/activity Adds behavior-pinning matrices for POST /team/delete (per-team _verify_team_access; batch aborts whole on a missing id), POST /team/bulk_member_add (route-gated PROXY-ADMIN-only; empty/over-cap 400), GET /v2/team/list (_enforce_list_team_v2_access — bare query 401s regular users, org-scoped for org admins) and GET /team/daily/activity (non-member team_ids filter 404, the VERIA-43 fix). * test(proxy): add route-coverage gate + close team org-relocation gap Adds test_route_coverage.py (PR3.M1): parses every @router route literal from the two management-endpoint source files and asserts each is exercised by >=1 behavior-suite scenario — a permanent regression guard for future routes. Closes the last PR1/PR2 deferred gap: the /team/update org-relocation allowed branch, exercised by a dual-org-admin minted via create_scratch_actor. test_team_model uses literal route URLs so the coverage parser resolves them. * test(proxy): bound plain route params to one path segment in coverage gate Plain path params ({team_id}) now compile to [^/?]+ instead of [^?]+, so a parameter cannot span '/'. Starlette ':path' params still match across '/'. Keeps the route-coverage guard from falsely reporting a future multi-segment route as covered. All 37 routes remain covered.
2 participants
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:
Screenshots / Proof of Fix
Reproduced against a local proxy configured with:
Before the fix —
/key/regenerateignored the cap:durationexpiresPOST /key/generate1hPOST /key/generate2hduration is over max limit set in config - user_value=2h; max_value=1hPOST /key/{key}/regenerate2hGET /key/infoAfter the fix — regenerate surfaces the same 400 as generate, and within-cap regenerates still succeed:
The 400 response body from
/key/regeneratepost-fix matches the existing generate error shape:{ "error": { "message": "{'error': 'duration is over max limit set in config - user_value=2h; max_value=1h'}", "type": "internal_server_error", "code": "400" } }Type
🐛 Bug Fix
Changes
Issue
POST /key/{key}/regeneratewas not calling_enforce_upperbound_key_params, so the proxy-wideupperbound_key_generate_paramscap (documented indocs/my-website/docs/proxy/self_serve.mdas a hard ceiling) was only enforced on/key/generateand/key/update. Any caller with regenerate permission could sidestep it by generating a short-lived key and immediately regenerating it with a largerduration,max_budget,budget_duration,max_parallel_requests,tpm_limit, orrpm_limit. The over-cap values were persisted toLiteLLM_VerificationToken, so this was not a response-layer artifact.Fix
Added a single call to the existing helper inside
_execute_virtual_key_regeneration, mirroring the precedent already inupdate_key_fn:fill_defaults=Falseis the update-style mode:Nonefields inRegenerateKeyRequestmean "inherit from the existing key", and the helper already skips them in this mode, so there's no risk of overwriting existing values with upperbound defaults.RegenerateKeyRequestextendsGenerateKeyRequest, so it satisfies the helper'sUnion[GenerateKeyRequest, UpdateKeyRequest]signature via inheritance — no annotation change needed.Same shape as #22712, which earlier fixed the analogous asymmetry for the team-scoped
team_member_key_durationcap. This one covers the proxy-wide cap.Behaviour change
Regenerate calls that previously succeeded with over-cap values will now return HTTP 400. This makes runtime behaviour match what the self-serve docs already describe. Operators who were relying on the bypass would need to remove the cap or raise it.
Tests
Added 5 async tests in
tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.pythat exercise_execute_virtual_key_regenerationdirectly (the same seam used by the existing cache-invalidation test):test_execute_virtual_key_regeneration_rejects_over_limit_duration— over-capduration→ 400test_execute_virtual_key_regeneration_allows_within_limit_duration— within-capduration→ 200, DB update reachedtest_execute_virtual_key_regeneration_rejects_over_limit_max_budget— proves the fix covers non-duration fieldstest_execute_virtual_key_regeneration_skips_none_values—fill_defaults=Falsesemantic:Nonemeans "inherit", no raisetest_execute_virtual_key_regeneration_no_upperbound_config_is_noop— no cap configured → any duration acceptedOut of scope (follow-up candidates)
default_key_generate_paramshas the same asymmetry on regenerate (applied on generate, not on regenerate). Not included here to keep the PR scope to the reported security issue; happy to open a follow-up if useful.docs/my-website/docs/proxy/self_serve.md:408already describeupperbound_key_generate_paramsas a hard ceiling — no doc change needed, this PR just makes the behaviour match.