[Fix] Privilege Escalation on /key/block, /key/unblock, and /key/update max_budget#23781
Conversation
…x_budget updates to admins Non-admin users (INTERNAL_USER) could call /key/block and /key/unblock on arbitrary keys, and modify max_budget on their own keys via /key/update. These endpoints are now restricted to proxy admins, team admins, or org admins. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes a privilege-escalation vulnerability where any The fix introduces a shared Key observations:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds _check_key_admin_access helper and enforces it in block_key, unblock_key, and _validate_update_key_data; the helper has a silent fall-through to 403 when the key's team no longer exists, which can mislead legitimate team admins. |
| tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py | Adds 6 unit tests for the new admin access checks; missing coverage for unblock_key allowed paths (proxy admin, team admin) and the org admin branch of _check_key_admin_access. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Request: block_key / unblock_key / update_key max_budget] --> B[Compute hashed_token]
B --> C[_check_key_admin_access]
C --> D{Is PROXY_ADMIN?}
D -- Yes --> E[Allow - return]
D -- No --> F[Fetch key row from DB]
F --> G{Key found?}
G -- No --> H[Raise 404]
G -- Yes --> I{Has team_id?}
I -- No --> J[Raise 403]
I -- Yes --> K[get_team_object]
K --> L{team_obj is None?}
L -- Yes --> J
L -- No --> M{_is_user_team_admin?}
M -- Yes --> E
M -- No --> N{_is_user_org_admin_for_team?}
N -- Yes --> E
N -- No --> J[Raise 403]
E --> O[Proceed with DB mutation]
Last reviewed commit: 55c7ba9
| team_obj = await get_team_object( | ||
| team_id=target_key_row.team_id, | ||
| prisma_client=prisma_client, | ||
| user_api_key_cache=user_api_key_cache, | ||
| check_db_only=True, | ||
| ) |
There was a problem hiding this comment.
check_db_only=True bypasses cache for team lookup
Passing check_db_only=True forces a DB hit every time this function runs, even when the team object is already warm in user_api_key_cache. For the block/unblock path this means at minimum two sequential DB queries per request (key lookup + team lookup).
Consider dropping check_db_only=True to allow the cache to serve the team object when available — the same pattern used elsewhere in the file for management-endpoint team lookups.
| if data.max_budget is not None and data.max_budget != existing_key_row.max_budget: | ||
| if prisma_client is not None: | ||
| hashed_key = existing_key_row.token | ||
| await _check_key_admin_access( | ||
| user_api_key_dict=user_api_key_dict, | ||
| hashed_token=hashed_key, | ||
| prisma_client=prisma_client, | ||
| user_api_key_cache=user_api_key_cache, | ||
| route="/key/update (max_budget)", | ||
| ) |
There was a problem hiding this comment.
Admin check silently skipped when prisma_client is None
The if prisma_client is not None: guard means that if the DB is somehow unavailable at this call site, the max_budget privilege check is bypassed entirely and a non-admin user can silently raise their own budget.
While update_key_fn raises earlier when prisma_client is None, _validate_update_key_data is a reusable helper that could be called from other call sites without that guarantee. The safer approach is to fail loudly rather than silently permit the mutation — for example, raising an HTTPException(status_code=500) when prisma_client is None and a budget change is requested, so the absence of DB connectivity is an explicit error rather than a hidden privilege bypass.
| # Look up the target key to find its team | ||
| target_key_row = await prisma_client.db.litellm_verificationtoken.find_unique( | ||
| where={"token": hashed_token} | ||
| ) |
There was a problem hiding this comment.
Redundant DB fetch in the /key/update max_budget path
When _check_key_admin_access is called from _validate_update_key_data, the target key was already fetched as existing_key_row (which carries team_id). This find_unique call adds a redundant DB round-trip on every max_budget update request.
A simple improvement would be to add an optional team_id parameter to _check_key_admin_access that callers can provide when the team is already known, skipping this lookup. For _validate_update_key_data, you could pass getattr(existing_key_row, "team_id", None) directly.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
| user_api_key_dict = UserAPIKeyAuth( | ||
| user_role=LitellmUserRoles.PROXY_ADMIN, | ||
| api_key="sk-admin", | ||
| user_id="admin_user", | ||
| ) | ||
|
|
||
| result = await block_key( | ||
| data=BlockKeyRequest(key="sk-test123456789"), | ||
| http_request=mock_request, | ||
| user_api_key_dict=user_api_key_dict, | ||
| litellm_changed_by=None, | ||
| ) | ||
| assert result is not None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
Missing unblock_key allowed-case tests
The PR tests unblock_key being rejected for internal users, but there are no counterpart tests verifying that the unblock operation succeeds for proxy admins or team admins. Since the security fix is symmetric across block and unblock, the gap leaves the unblock allow-path untested in this suite.
By contrast, block_key has both test_block_key_allowed_for_proxy_admin and test_block_key_allowed_for_team_admin. Consider adding equivalent tests for unblock_key:
@pytest.mark.asyncio
async def test_unblock_key_allowed_for_proxy_admin(monkeypatch):
...
@pytest.mark.asyncio
async def test_unblock_key_allowed_for_team_admin(monkeypatch):
...| mock_existing_key.project_id = None | ||
| mock_existing_key.max_budget = 10.0 | ||
| mock_existing_key.models = [] | ||
| mock_existing_key.model_dump.return_value = { | ||
| "token": test_hashed_token, | ||
| "user_id": "internal_user", | ||
| "team_id": None, | ||
| "max_budget": 10.0, | ||
| } | ||
|
|
||
| mock_prisma_client.get_data = AsyncMock(return_value=mock_existing_key) | ||
| mock_prisma_client.db.litellm_verificationtoken.find_unique = AsyncMock( | ||
| return_value=mock_existing_key | ||
| ) | ||
|
|
||
| monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma_client) |
There was a problem hiding this comment.
Org admin path in _check_key_admin_access has no test coverage
_check_key_admin_access contains three allow-paths: proxy admin, team admin, and org admin (via _is_user_org_admin_for_team). The six new tests cover the first two paths but there is no test exercising the org admin branch. If _is_user_org_admin_for_team has a defect, org admins would be silently blocked (or—depending on the defect—incorrectly allowed), and neither case would be caught by the new test suite.
Consider adding:
@pytest.mark.asyncio
async def test_block_key_allowed_for_org_admin(monkeypatch):
"""Org admins should be able to block keys belonging to their org's teams."""
...| # If the key belongs to a team, check team admin / org admin | ||
| if target_key_row.team_id: | ||
| team_obj = await get_team_object( | ||
| team_id=target_key_row.team_id, | ||
| prisma_client=prisma_client, | ||
| user_api_key_cache=user_api_key_cache, | ||
| check_db_only=True, | ||
| ) | ||
| if team_obj is not None: | ||
| if _is_user_team_admin( | ||
| user_api_key_dict=user_api_key_dict, team_obj=team_obj | ||
| ): | ||
| return | ||
| if await _is_user_org_admin_for_team( | ||
| user_api_key_dict=user_api_key_dict, team_obj=team_obj | ||
| ): | ||
| return |
There was a problem hiding this comment.
Team admin silently denied when team lookup returns None
When the target key has a team_id but get_team_object returns None (e.g., the team record was deleted after the key was created), the entire if team_obj is not None: block is skipped and execution falls through to the unconditional raise HTTPException(403). This means a legitimate team admin operating on an orphaned-team key receives a confusing 403 with no indication that the team itself is missing.
The safer approach is to check team_obj is None explicitly inside the if target_key_row.team_id: branch and raise a 404 for a missing team rather than silently falling through to the generic authorization error. This makes the failure mode observable and distinguishable from a genuine authorization denial.
Relevant issues
Reported via security disclosure email.
Summary
Failure Path (Before Fix)
Any virtual key tied to a non-admin user (
INTERNAL_USERrole) could call/key/block,/key/unblock, and/key/updateon arbitrary keys. This allowed:/key/updatewithout admin privilegesRoot cause:
/key/blockand/key/unblockare included inkey_management_routes, which is part ofinternal_user_routes. The route-level check passes forINTERNAL_USER, and neither handler enforced an admin or ownership check. For/key/update,max_budgetwas not restricted to admin-only callers.Fix
Added
_check_key_admin_access()helper that verifies the caller is a proxy admin, team admin (for the target key's team), or org admin (for the team's org). This check is now enforced in:block_key— before any DB mutationunblock_key— before any DB mutationupdate_key_fn— whenmax_budgetis being changedNon-admin users can still update non-budget fields on their own keys.
Testing
6 new unit tests:
test_block_key_rejected_for_internal_usertest_unblock_key_rejected_for_internal_usertest_block_key_allowed_for_proxy_admintest_block_key_allowed_for_team_admintest_update_key_max_budget_rejected_for_internal_usertest_update_key_non_budget_fields_allowed_for_internal_userType
🐛 Bug Fix
✅ Test