diff --git a/litellm/proxy/management_endpoints/key_management_endpoints.py b/litellm/proxy/management_endpoints/key_management_endpoints.py index f69d9d2f8d..32227b3dd6 100644 --- a/litellm/proxy/management_endpoints/key_management_endpoints.py +++ b/litellm/proxy/management_endpoints/key_management_endpoints.py @@ -1961,22 +1961,48 @@ async def _validate_update_key_data( user_api_key_cache=user_api_key_cache, ) - # Admin-only: only proxy admins, team admins, or org admins can modify max_budget or spend - if ( - data.max_budget is not None and data.max_budget != existing_key_row.max_budget + # Cross-key authorization. Previously only gated on max_budget/spend + # changes, which let a non-admin blanket-rewrite any OTHER field on + # any key (models, alias, metadata, tpm_limit, rpm_limit, + # allowed_routes, guardrails, blocked, duration, permissions, …) as + # long as they avoided budget/spend. + # + # Policy: + # - Key owner (same user_id): may update non-budget fields on their + # own key without the admin check. + # - Anyone else (non-PROXY_ADMIN, not the owner): must pass + # _check_key_admin_access (PROXY_ADMIN / key-owner / team-admin / + # org-admin of the key). + # - max_budget / spend: always require the admin check, even for the + # key owner (matches the existing admin-only budget semantics). + is_key_owner = ( + user_api_key_dict.user_id is not None + and existing_key_row.user_id == user_api_key_dict.user_id + ) + _is_budget_change = ( + data.max_budget is not None + and data.max_budget != existing_key_row.max_budget ) or ( data.spend is not None and data.spend != getattr(existing_key_row, "spend", None) + ) + if ( + (not _is_proxy_admin) + and prisma_client is not None + and (not is_key_owner or _is_budget_change) ): - 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/spend)", - ) + 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/spend)" + if _is_budget_change + else "/key/update" + ), + ) # Check team limits if key has a team_id (from request or existing key) team_obj: Optional[LiteLLM_TeamTableCachedObj] = None diff --git a/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py index 479defbff5..41ab60fcf6 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py @@ -8030,6 +8030,74 @@ async def test_update_key_non_budget_fields_allowed_for_internal_user(monkeypatc assert result is not None +@pytest.mark.asyncio +async def test_update_key_non_budget_rejects_cross_user_modification(monkeypatch): + """Regression: previously _check_key_admin_access was gated on + max_budget/spend changes only, so an internal user could rewrite any + OTHER field (alias, models, tpm_limit, blocked, metadata, …) on any + key they weren't admin of as long as they avoided budget/spend. This + confirms that a non-admin user updating a key that belongs to another + user fails with 403 even for non-budget fields.""" + from litellm.proxy.management_endpoints.key_management_endpoints import ( + update_key_fn, + ) + + mock_prisma_client = AsyncMock() + test_hashed_token = ( + "cafebabe" * 8 + ) + + mock_existing_key = MagicMock() + mock_existing_key.token = test_hashed_token + mock_existing_key.user_id = "victim_user" # owned by someone else + mock_existing_key.team_id = None + mock_existing_key.project_id = None + mock_existing_key.max_budget = 10.0 + mock_existing_key.key_alias = "original" + mock_existing_key.models = [] + mock_existing_key.model_dump.return_value = { + "token": test_hashed_token, + "user_id": "victim_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) + monkeypatch.setattr("litellm.proxy.proxy_server.user_api_key_cache", AsyncMock()) + monkeypatch.setattr("litellm.proxy.proxy_server.proxy_logging_obj", MagicMock()) + monkeypatch.setattr("litellm.proxy.proxy_server.llm_router", None) + monkeypatch.setattr("litellm.proxy.proxy_server.premium_user", True) + monkeypatch.setattr( + "litellm.proxy.proxy_server.hash_token", lambda t: test_hashed_token + ) + + mock_request = MagicMock() + mock_request.query_params = {} + attacker = UserAPIKeyAuth( + user_role=LitellmUserRoles.INTERNAL_USER, + api_key="sk-attacker", + user_id="attacker_user", # NOT the owner + ) + + # Trying to blanket-rewrite a non-budget field on someone else's key + # must now fail. + with pytest.raises(ProxyException) as exc: + await update_key_fn( + request=mock_request, + data=UpdateKeyRequest( + key=test_hashed_token, key_alias="pwned", blocked=True + ), + user_api_key_dict=attacker, + litellm_changed_by=None, + ) + assert str(exc.value.code) == "403" + + # ============================================================================ # LIT-1884: Internal users cannot create invalid keys # ============================================================================