fix(proxy): allow non-admin key_type preset transitions on /key/update - #35006
fix(proxy): allow non-admin key_type preset transitions on /key/update#35006yucheng-berri wants to merge 2 commits into
Conversation
…e (LIT-4891) The LIT-4139 hardening (#31987) rejects any explicit allowed_routes value from a non-admin on /key/update, including the allowed_routes: [] the Admin UI sends when a key owner switches Key Type from AI APIs to Full access. That blocked a self-service flow that worked before v1.92.0. _check_allowed_routes_caller_permission gains an existing_allowed_routes input that only the /key/update call site provides: when both the requested and the existing value consist solely of safe preset tokens (either may be empty), the write is a key_type preset transition and is allowed. A key whose existing allowed_routes contains anything outside the safe presets was custom-restricted by an admin, so every non-admin write on it, including clearing, still returns 403. Generate, service-account generate, regenerate, and both bulk paths are unchanged
| if "allowed_routes" in data_json and data_json["allowed_routes"] is None: | ||
| # The allowed_routes DB column is a non-nullable String[]. | ||
| data_json["allowed_routes"] = [] |
There was a problem hiding this comment.
🟡 New code comment added despite repository rule forbidding comments
A new inline code comment is introduced (key_management_endpoints.py:1914) even though the repository's mandatory coding guidelines forbid writing any new comments unless explicitly requested in a user prompt.
Impact: The change does not comply with the repository's stated contribution rules.
Rule source in CLAUDE.md
AGENTS.md points to CLAUDE.md, whose first rule states: "Do not write any comments (existing comments can stay) unless explicitly asked to in a user (not system) prompt". The added line # The allowed_routes DB column is a non-nullable String[]. is a newly introduced comment. The behavior it documents can be conveyed via the commit message or left uncommented per the rule.
| if "allowed_routes" in data_json and data_json["allowed_routes"] is None: | |
| # The allowed_routes DB column is a non-nullable String[]. | |
| data_json["allowed_routes"] = [] | |
| if "allowed_routes" in data_json and data_json["allowed_routes"] is None: | |
| data_json["allowed_routes"] = [] |
Was this helpful? React with 👍 or 👎 to provide feedback.
Greptile SummaryThis PR restores non-admin key-type preset transitions on
Confidence Score: 3/5The PR is not yet safe to merge because an authorized non-admin owner can still remove an administrator-assigned safe-preset restriction. Safe preset assignments carry no provenance, so the new value-based carve-out permits a creator-owner to replace an administrator-assigned preset with an empty route list, which route authorization interprets as unrestricted access. Files Needing Attention: litellm/proxy/management_endpoints/key_management_endpoints.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds existing-route-aware authorization for preset transitions and normalizes explicit null route lists. |
| tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py | Expands coverage for permitted preset transitions and rejected custom-route or unauthorized updates. |
Reviews (2): Last reviewed commit: "docs(proxy): state the preset-provenance..." | Re-trigger Greptile
| ): | ||
| return | ||
| if ( | ||
| existing_allowed_routes is not None |
There was a problem hiding this comment.
High: Administrator route restrictions can be removed
The stored value does not indicate who selected the preset, so this branch also accepts transitions away from an administrator-enforced info_routes or llm_api_routes restriction. A regular team member holding the /key/update grant can change an admin-created team key from info_routes to llm_api_routes, enabling billable inference, or clear llm_api_routes to [], which disables the virtual-key route gate and exposes management routes allowed by the key holder's role. Keep widening transitions proxy-admin-only unless the database records that the restriction is explicitly user-managed; non-admins can safely be permitted to narrow an unrestricted key.
PR overviewThis pull request changes the One significant authorization issue remains open. A team member with the key-update grant can loosen administrator-selected route restrictions on an existing team key, potentially enabling billable inference or access to role-permitted management routes. No issues have yet been addressed. Open issues (1)
Fixed/addressed: 0 · PR risk: 7/10 |
…es gate docstring
|
On the Security Review concern (admin-set safe preset indistinguishable from a self-service preset, so the creator-owner can later clear it): this is a deliberate product decision, not an oversight, and the PR now documents it in the gate's docstring
Please re-review the current head 696f6ee |
|
@greptileai please review the current head 696f6ee |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Relevant issues
Linear ticket
Resolves LIT-4891
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Repro rig: proxy from this branch on
localhost:4091, fresh Postgres, master key set.priyais a non-admin internal user;priya-own-keyis a key she created herself withkey_type: llm_api(allowed_routes: ["llm_api_routes"]), which is what the Admin UI's Create Key flow producesBefore the fix (staging), the exact payload the Admin UI sends when switching Key Type from "AI APIs" to "Full access":
After the fix, the same requests:
LIT-4139 attack surface, verified still closed on the same live proxy:
Type
🐛 Bug Fix
Changes
Since v1.92.0 (#31987, LIT-4139),
/key/updaterejects any explicitly providedallowed_routesvalue from a non-admin, including the empty list. The Admin UI expresses "switch Key Type from AI APIs to Full access" by clearingallowed_routesto[], so a non-admin key owner or team admin doing that self-service change gets a 403 whose message tells them to usekey_type, which is exactly what they are doing. The hardening was correct for its target (a key owner erasing an admin-set custom route restriction) but the legitimate preset transition and the attack are byte-identical requests, so the gate needs the key's existing state to tell them apart_check_allowed_routes_caller_permissiongains anexisting_allowed_routesinput that only the/key/updatecall site provides. When both the requested value and the key's existing value consist solely of safe preset tokens (llm_api_routes,info_routes; either side may be empty, meaning unrestricted), the write is a key_type preset transition and this field-level gate lets it through. Who may perform it is still enforced by the downstream ownership rules: the key's creator-owner, a team admin, or a team member holding the/key/updategrant. A key whose existing value contains anything outside the safe presets was custom-restricted by an admin, and every non-admin write on it, including clearing, still returns 403. Element values are type-checked so unhashable garbage in the request gets the same 403 instead of a 500, and a non-list existing value falls back strict (403), never openprepare_key_update_datanow coerces an explicitallowed_routes: nullto[]; the DB column is a non-nullableString[], so the previous behavior on that shape (once past the gate) was a Prisma write errorCall sites for
/key/generate,/key/service_account/generate, and/key/regeneratedo not passexisting_allowed_routesand keep the strict LIT-4139 behavior, pinned by test. Bulk update paths cannot carryallowed_routesand are unchangedBehavior changes
/key/update: a non-admin explicitallowed_routeswrite that previously returned 403 now succeeds when the caller passes the ownership rules and both the requested and existing values are safe presets or empty. This restores the pre-v1.92.0 self-service key_type switching for owner-created keys and team keys (the ticket's scenario). Escaping an admin-set custom restriction still returns 403/key/updategrant can again move a team key between safe presets and full access, as on v1.91.x. Keys an admin wants hard-restricted should use a customallowed_routeslist, which remains admin-only to modify/key/updatewith explicitallowed_routes: nullfrom an authorized caller now clears the restriction (stores[]) instead of failing/key/updatemay now see the ownership-check 403 message instead of the allowed_routes 403 message, since the field gate no longer fires first on preset-shaped keysKnown follow-ups (will file tickets): the stored
key_typecolumn is not updated on route transitions, so key lists can display a stale type label until the key is re-saved (pre-existing for admin edits, now reachable by non-admins); the UI still omitsallowed_routeswhen unchanged (LIT-2681 workaround) which can be simplified now that the server accepts the preset caseQA runbook
/chat/completions, then repeat step 4 as the non-admin user. Expect a 403Final Attestation