fix(proxy): admin-gate permissions on /user/new and /user/update (LIT-4138) - #31998
Conversation
Greptile SummaryThis PR wires the existing
Confidence Score: 5/5Safe to merge — the gate is inserted at the correct positions in both user-creation and user-update paths, relies on the already-tested The change is narrowly scoped: two call sites added, one error string trimmed. Both No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/internal_user_endpoints.py | Adds _check_permissions_caller_permission at the correct points in new_user and _update_single_user_helper; placement and logic are sound. |
| litellm/proxy/management_endpoints/key_management_endpoints.py | One-line error-message change: removes "on a key" locative so the message is correct when raised from user endpoints. No logic change. |
| tests/test_litellm/proxy/management_endpoints/test_internal_user_endpoints.py | Six new properly-mocked tests covering attack vectors (non-empty, explicit-empty) and controls (omit, admin) for both new_user and _update_single_user_helper. |
Reviews (3): Last reviewed commit: "fix(proxy): admin-gate `permissions` on ..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR wires an existing presence-based permissions gate (
Confidence Score: 4/5Safe to merge; the change is narrowly scoped to two call sites and delegates entirely to a helper already proven in the key-management flow. The production change is two identical five-line call-site additions. The helper itself is unchanged and covered by existing key-endpoint tests. The new tests are mock-only, exercise all four attack vectors, and distinguish the omit-default from the explicit-empty case via model_fields_set. The only open item is a cosmetic mismatch in the error message wording. litellm/proxy/management_endpoints/key_management_endpoints.py — the _check_permissions_caller_permission error message says 'on a key' and is now surfaced to callers of user-management endpoints.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/internal_user_endpoints.py | Adds _check_permissions_caller_permission to new_user (after the admin-role gate) and _update_single_user_helper (before the Prisma write); both placements are correct and the shared helper covers /user/update and /user/bulk_update. |
| tests/test_litellm/proxy/management_endpoints/test_internal_user_endpoints.py | Six new async mock-only tests cover non-admin rejection for non-empty and explicit-empty permissions, pass-through for omitted permissions, and admin bypass on both new_user and _update_single_user_helper. |
Reviews (2): Last reviewed commit: "fix(proxy): admin-gate `permissions` on ..." | Re-trigger Greptile
…IT-4138)
`NewUserRequest` and `UpdateUserRequest` inherit `permissions` from
`GenerateRequestBase`. `/user/new` passes the field into
`generate_key_helper_fn` which persists it on the auto-created key,
so an org admin who lands on `/user/new` (the route allowlist accepts
org_admin callers when the request body names an org where they hold
that membership) can mint a key with proxy-wide capabilities such as
`get_spend_routes`.
This wires the existing `_check_permissions_caller_permission` helper
into `new_user` and `_update_single_user_helper`. The helper's presence
check keys on `data.model_fields_set`, so an omitted field flows
through untouched and an explicit `{}` / `null` from a non-admin is
rejected 403 the same as any other value.
`_update_single_user_helper` is shared by `/user/update` and
`/user/bulk_update`, so both paths inherit the gate.
Tests in
`tests/test_litellm/proxy/management_endpoints/test_internal_user_endpoints.py`:
- test_new_user_non_admin_permissions_non_empty_rejected
- test_new_user_non_admin_permissions_explicit_empty_rejected
- test_new_user_non_admin_omits_permissions_succeeds (control)
- test_new_user_admin_can_set_permissions (control)
- test_update_single_user_non_admin_permissions_rejected
- test_update_single_user_non_admin_permissions_explicit_empty_rejected
The four attack-vector tests fail on the pre-fix HEAD and pass on this
commit. Full mapped test file (79 tests) green.
5bf71a4 to
c78170b
Compare
|
@greptileai re-review, amended commit (c78170b) drops the "on a key" locative from the shared error message per your comment. The helper is now called from both key and user endpoints, so "Only proxy admins can set |
|
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 c78170b. Configure here.
Relevant issues
Linear ticket
Resolves LIT-4138
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
NewUserRequestandUpdateUserRequestinheritpermissionsfromGenerateRequestBase./user/newpasses the field through_update_internal_new_user_paramsand intogenerate_key_helper_fn, which persists it on the auto-created key atkey_management_endpoints.py:3654. The route allowlist accepts an org admin caller on/user/newwhen the request body names an organization the caller holdsorg_adminmembership in (_user_is_org_admininauth_checks_organization.py:135), so a legitimate org admin can mint a key with proxy-wide capabilities such asget_spend_routesthat go far beyond their orgThis PR wires the existing
_check_permissions_caller_permissionhelper intonew_user(after the admin-role check and beforegenerate_key_helper_fn) and into_update_single_user_helper(after the DB-connected check and before the Prisma write). The helper is the same presence-based check introduced in LIT-4092; it keys ondata.model_fields_set, so an omittedpermissionsflows through the model default and an explicit{}/nullfrom a non-admin trips the gate_update_single_user_helperis shared by/user/updateand/user/bulk_update, so both paths inherit the gate./user/updatetoday rejectspermissionsat the Prisma layer (theLiteLLM_UserTableschema has nopermissionscolumn, so the DB errors before persistence); the gate now returns a consistent 403 upstream and forecloses a future migration silently reopening the classSix new tests in
test_internal_user_endpoints.pycover non-admin explicit-non-empty and explicit-empty on bothnew_userand_update_single_user_helper, plus the omit-default and admin controls. The four attack-vector tests fail on the pre-fix HEAD and pass on this commit. Full mapped test file (79 tests) greenScreenshots / Proof of Fix
Live proxy on
localhost:4010against Postgres,LITELLM_LICENSEset soget_spend_routespropagation is available. Setup: admin creates userrca-user-b, mints them a key, creates an organization, and adds the user to it withrole=org_adminAttack path on the unfixed HEAD
{"key":"sk-...fXNkFA","user_id":"rca-victim-b","permissions":{"get_spend_routes":true}, ...}200 OK; the newly-minted key has
get_spend_routes: truewritten to the DB, granting it proxy-wide/global/spend/*read accessSame call on this commit
{"error":{"message":"{'error': 'Only proxy admins can set `permissions`.'}","code":"403"}}Explicit empty on this commit
{"error":{"message":"{'error': 'Only proxy admins can set `permissions`.'}","code":"403"}}Control: org admin
/user/newwithout apermissionsfield still succeeds on the fixed proxyProxy admin (master key) can still write any
permissionsvalue on/user/newOut of scope, tracked separately
The
/user/updatepath has a pre-existing behavior where an explicitpermissionsvalue returns 400 at the Prisma layer because theLiteLLM_UserTableschema has nopermissionscolumn. That is not changed here. The helper is still wired into_update_single_user_helperso a future schema addition (or an alternate persistence path forpermissionsat the user level) cannot silently reopen the classNote
High Risk
This is an authorization fix in user/key management: incorrect behavior previously allowed elevated key capabilities; the change is narrow but touches security-sensitive proxy admin boundaries.
Overview
Closes a privilege-escalation path where org admins (and other non–proxy-admins) could pass
permissionson/user/newand have those values land on the auto-created API key (e.g.get_spend_routes)./user/newand_update_single_user_helper(shared by/user/updateand/user/bulk_update) now call the existing_check_permissions_caller_permissionhelper before user/key persistence. The check is presence-based (model_fields_set): omittingpermissionsis allowed; sending any explicit value (including{}) as a non-admin returns 403. Proxy admins are unchanged.The key-management error text is generalized to “set
permissions” (not only “on a key”). Six tests cover reject/allow cases for create and update paths.Reviewed by Cursor Bugbot for commit c78170b. Bugbot is set up for automated code reviews on this repo. Configure here.