From 81c48f810edf9412165539f278e51131dffb348d Mon Sep 17 00:00:00 2001 From: Yuneng Jiang Date: Mon, 31 Aug 2026 22:09:21 -0700 Subject: [PATCH] fix(key_management): keep read-only keys read-only in non-admin preset transitions A non-admin could widen a read-only (info_routes) key to llm_api or full access through the preset carve-out. Read-only keys now stay read-only unless a proxy admin widens them; the other preset transitions, including the LIT-4891 llm_api to full access switch, still work. Also converts the transition tests to assert on a returned outcome so the no-403 cases carry real assertions. --- .../key_management_endpoints.py | 18 ++-- .../test_key_management_endpoints.py | 94 ++++++++++++++----- 2 files changed, 81 insertions(+), 31 deletions(-) diff --git a/litellm/proxy/management_endpoints/key_management_endpoints.py b/litellm/proxy/management_endpoints/key_management_endpoints.py index 1fc2850c4ae..5b237cccecf 100644 --- a/litellm/proxy/management_endpoints/key_management_endpoints.py +++ b/litellm/proxy/management_endpoints/key_management_endpoints.py @@ -736,6 +736,9 @@ def _check_allowed_routes_caller_permission( ) +_READ_ONLY_ALLOWED_ROUTES_PRESET: Final = frozenset(("info_routes",)) + + def _is_safe_preset_route_transition( incoming_allowed_routes: Sequence[str] | None, existing_allowed_routes: Sequence[str] | None, @@ -743,13 +746,16 @@ def _is_safe_preset_route_transition( """ True when every route on BOTH sides is a safe `key_type` preset bucket (empty = full access, which non-admins already get from a default - `/key/generate`). Requiring the existing side too keeps an owner from - clearing an admin-set custom route restriction (LIT-4139). + `/key/generate`), with one carve-out: a read-only (`info_routes`) key + stays read-only, so widening it needs an admin. Requiring the existing + side to be a safe preset keeps an owner from clearing an admin-set + custom route restriction (LIT-4139). """ - return all( - route in _NON_ADMIN_SAFE_ALLOWED_ROUTES_PRESETS - for route in (*(incoming_allowed_routes or ()), *(existing_allowed_routes or ())) - ) + incoming: Final = frozenset(incoming_allowed_routes or ()) + existing: Final = frozenset(existing_allowed_routes or ()) + if not (incoming | existing) <= _NON_ADMIN_SAFE_ALLOWED_ROUTES_PRESETS: + return False + return existing != _READ_ONLY_ALLOWED_ROUTES_PRESET or incoming == existing def _enforce_allowed_routes_update_permission( 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 0ddc8c5a822..ff555d893f5 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 @@ -11054,56 +11054,100 @@ class TestLIT4891SafePresetKeyTypeTransition: ) async def _run_update(self, data, existing_key_row): - await _validate_update_key_data( - data=data, - existing_key_row=existing_key_row, - user_api_key_dict=self._make_auth(), - llm_router=None, - premium_user=False, - prisma_client=AsyncMock(), - user_api_key_cache=MagicMock(), - ) + try: + await _validate_update_key_data( + data=data, + existing_key_row=existing_key_row, + user_api_key_dict=self._make_auth(), + llm_router=None, + premium_user=False, + prisma_client=AsyncMock(), + user_api_key_cache=MagicMock(), + ) + except HTTPException as exc: + return exc + return None + + def _assert_routes_403(self, exc): + assert exc is not None + assert exc.status_code == 403 + assert "Only proxy admins can set" in str(exc.detail) @pytest.mark.asyncio async def test_non_admin_owner_can_clear_safe_preset_to_full_access(self): - await self._run_update( - data=UpdateKeyRequest(key="sk-test", allowed_routes=[]), - existing_key_row=self._make_existing_key(allowed_routes=["llm_api_routes"]), + assert ( + await self._run_update( + data=UpdateKeyRequest(key="sk-test", allowed_routes=[]), + existing_key_row=self._make_existing_key(allowed_routes=["llm_api_routes"]), + ) + is None ) @pytest.mark.asyncio async def test_non_admin_owner_can_switch_full_access_to_safe_preset(self): - await self._run_update( - data=UpdateKeyRequest(key="sk-test", allowed_routes=["llm_api_routes"]), - existing_key_row=self._make_existing_key(allowed_routes=[]), + assert ( + await self._run_update( + data=UpdateKeyRequest(key="sk-test", allowed_routes=["llm_api_routes"]), + existing_key_row=self._make_existing_key(allowed_routes=[]), + ) + is None ) @pytest.mark.asyncio - async def test_non_admin_owner_can_switch_between_safe_presets(self): - await self._run_update( - data=UpdateKeyRequest(key="sk-test", allowed_routes=["info_routes"]), - existing_key_row=self._make_existing_key(allowed_routes=["llm_api_routes"]), + async def test_non_admin_owner_can_narrow_to_read_only_preset(self): + assert ( + await self._run_update( + data=UpdateKeyRequest(key="sk-test", allowed_routes=["info_routes"]), + existing_key_row=self._make_existing_key(allowed_routes=["llm_api_routes"]), + ) + is None + ) + + @pytest.mark.asyncio + async def test_non_admin_can_resend_read_only_preset_unchanged(self): + assert ( + await self._run_update( + data=UpdateKeyRequest(key="sk-test", allowed_routes=["info_routes"]), + existing_key_row=self._make_existing_key(allowed_routes=["info_routes"]), + ) + is None + ) + + @pytest.mark.asyncio + async def test_non_admin_cannot_widen_read_only_key_to_full_access(self): + self._assert_routes_403( + await self._run_update( + data=UpdateKeyRequest(key="sk-test", allowed_routes=[]), + existing_key_row=self._make_existing_key(allowed_routes=["info_routes"]), + ) + ) + + @pytest.mark.asyncio + async def test_non_admin_cannot_widen_read_only_key_to_llm_api(self): + self._assert_routes_403( + await self._run_update( + data=UpdateKeyRequest(key="sk-test", allowed_routes=["llm_api_routes"]), + existing_key_row=self._make_existing_key(allowed_routes=["info_routes"]), + ) ) @pytest.mark.asyncio async def test_non_admin_cannot_clear_custom_route_restriction(self): - with pytest.raises(HTTPException) as exc_info: + self._assert_routes_403( await self._run_update( data=UpdateKeyRequest(key="sk-test", allowed_routes=[]), existing_key_row=self._make_existing_key(allowed_routes=["/chat/completions"]), ) - assert exc_info.value.status_code == 403 - assert "Only proxy admins can set" in str(exc_info.value.detail) + ) @pytest.mark.asyncio async def test_non_admin_cannot_set_non_preset_routes(self): - with pytest.raises(HTTPException) as exc_info: + self._assert_routes_403( await self._run_update( data=UpdateKeyRequest(key="sk-test", allowed_routes=["management_routes"]), existing_key_row=self._make_existing_key(allowed_routes=["llm_api_routes"]), ) - assert exc_info.value.status_code == 403 - assert "Only proxy admins can set" in str(exc_info.value.detail) + ) class TestKeyOwnerPrivilegeEscalation: