From 80d48a41e40730ea0adf0faf72d8273cb5ef882e Mon Sep 17 00:00:00 2001 From: Ryan Crabbe Date: Fri, 17 Apr 2026 23:21:20 -0700 Subject: [PATCH 1/4] [Fix] preserve service_account_id in metadata on /key/update /key/update and /user/update wholesale-replaced the metadata JSON column whenever a caller passed a `metadata` field, silently dropping service_account_id. The pre-call check in litellm_pre_call_utils.py then stopped treating the key as a service account and bypassed service_account_settings.enforced_params. Add LiteLLM_Reserved_Metadata_Fields and have prepare_metadata_fields preserve these keys from the existing row when the caller omits them. Reject attempts to change an already-set value (400) since rebinding service_account_id would break spend attribution. --- litellm/proxy/_types.py | 5 ++ .../key_management_endpoints.py | 14 +++ .../test_key_management_endpoints.py | 90 +++++++++++++++++++ 3 files changed, 109 insertions(+) diff --git a/litellm/proxy/_types.py b/litellm/proxy/_types.py index 7fff640c497..0bcd57119d9 100644 --- a/litellm/proxy/_types.py +++ b/litellm/proxy/_types.py @@ -4047,6 +4047,11 @@ LiteLLM_ManagementEndpoint_MetadataFields_Premium = [ "allowed_passthrough_routes", ] +# Metadata keys preserved from existing rows when an update omits them. +LiteLLM_Reserved_Metadata_Fields = [ + "service_account_id", +] + class ProviderBudgetResponseObject(LiteLLMPydanticObjectBase): """ diff --git a/litellm/proxy/management_endpoints/key_management_endpoints.py b/litellm/proxy/management_endpoints/key_management_endpoints.py index f69d9d2f8d4..2d9fc4e89b8 100644 --- a/litellm/proxy/management_endpoints/key_management_endpoints.py +++ b/litellm/proxy/management_endpoints/key_management_endpoints.py @@ -1517,6 +1517,20 @@ def prepare_metadata_fields( casted_metadata = cast(dict, non_default_values["metadata"]) + # Reserved metadata fields are immutable once set. Preserve the existing value + # when omitted, reject attempts to change it. + for reserved_field in LiteLLM_Reserved_Metadata_Fields: + existing_value = existing_metadata.get(reserved_field) + if existing_value is None: + continue + incoming_value = casted_metadata.get(reserved_field) + if incoming_value is not None and incoming_value != existing_value: + raise HTTPException( + status_code=400, + detail=f"{reserved_field} is immutable once set and cannot be changed via update.", + ) + casted_metadata[reserved_field] = existing_value + data_json = data.model_dump(exclude_unset=True, exclude_none=True) try: 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 479defbff5c..bece11aa668 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 @@ -1206,6 +1206,95 @@ async def test_update_service_account_works_with_team_id(): await prepare_key_update_data(data=data, existing_key_row=existing_key) +@pytest.mark.asyncio +async def test_update_preserves_service_account_id_when_metadata_replaced(): + """ + Regression: /key/update wholesale-replaced metadata, silently dropping + service_account_id. The pre-call check then treated the key as a regular + key and bypassed service_account_settings.enforced_params. + """ + data = UpdateKeyRequest( + key="sk-1", + metadata={"unrelated": "value"}, + team_id="IJ", + ) + existing_key = LiteLLM_VerificationToken( + token="hashed", + team_id="IJ", + metadata={"service_account_id": "sa-123"}, + ) + + result = await prepare_key_update_data( + data=data, existing_key_row=existing_key + ) + + assert result["metadata"]["service_account_id"] == "sa-123" + assert result["metadata"]["unrelated"] == "value" + + +@pytest.mark.asyncio +async def test_update_rejects_service_account_id_overwrite(): + """ + Once assigned, a key's service_account_id is an identity marker — rebinding + it via update would break spend attribution. Reject rather than silently + ignore so scripted callers surface the bug. + """ + data = UpdateKeyRequest( + key="sk-1", + metadata={"service_account_id": "sa-new"}, + team_id="IJ", + ) + existing_key = LiteLLM_VerificationToken( + token="hashed", + team_id="IJ", + metadata={"service_account_id": "sa-old"}, + ) + + with pytest.raises(HTTPException) as exc_info: + await prepare_key_update_data(data=data, existing_key_row=existing_key) + assert exc_info.value.status_code == 400 + + +@pytest.mark.asyncio +async def test_update_allows_matching_service_account_id(): + """Resending the same value (e.g. UI round-trip) is a no-op, not a conflict.""" + data = UpdateKeyRequest( + key="sk-1", + metadata={"service_account_id": "sa-123", "other": "value"}, + team_id="IJ", + ) + existing_key = LiteLLM_VerificationToken( + token="hashed", + team_id="IJ", + metadata={"service_account_id": "sa-123"}, + ) + + result = await prepare_key_update_data( + data=data, existing_key_row=existing_key + ) + + assert result["metadata"]["service_account_id"] == "sa-123" + assert result["metadata"]["other"] == "value" + + +@pytest.mark.asyncio +async def test_update_without_metadata_still_preserves_existing(): + """Omitting metadata entirely must not drop existing metadata fields.""" + data = UpdateKeyRequest(key="sk-1", max_budget=100) + existing_key = LiteLLM_VerificationToken( + token="hashed", + team_id="IJ", + metadata={"service_account_id": "sa-123", "other": "kept"}, + ) + + result = await prepare_key_update_data( + data=data, existing_key_row=existing_key + ) + + assert result["metadata"]["service_account_id"] == "sa-123" + assert result["metadata"]["other"] == "kept" + + @pytest.mark.asyncio async def test_prepare_key_update_data_duration_never_expires(): """Test that duration="-1" sets expires to None (never expires).""" @@ -7961,6 +8050,7 @@ async def test_update_key_non_budget_fields_allowed_for_internal_user(monkeypatc mock_existing_key.max_budget = 10.0 mock_existing_key.key_alias = None mock_existing_key.models = [] + mock_existing_key.metadata = {} mock_existing_key.model_dump.return_value = { "token": test_hashed_token, "user_id": "internal_user", From 01acbb8d3da2ed426771139261fa0135029ae243 Mon Sep 17 00:00:00 2001 From: Ryan Crabbe Date: Sat, 18 Apr 2026 11:06:22 -0700 Subject: [PATCH 2/4] [Fix] reject explicit null when clearing reserved metadata field Addresses Greptile review feedback: - Clarify LiteLLM_Reserved_Metadata_Fields comment to describe both preserve-on-omit and reject-on-change behaviors. - Treat explicit null as a change attempt so callers trying to clear service_account_id get a 400 instead of a silent no-op. --- litellm/proxy/_types.py | 3 ++- .../key_management_endpoints.py | 8 ++++--- .../test_key_management_endpoints.py | 23 +++++++++++++++++++ 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/litellm/proxy/_types.py b/litellm/proxy/_types.py index 0bcd57119d9..973086b2838 100644 --- a/litellm/proxy/_types.py +++ b/litellm/proxy/_types.py @@ -4047,7 +4047,8 @@ LiteLLM_ManagementEndpoint_MetadataFields_Premium = [ "allowed_passthrough_routes", ] -# Metadata keys preserved from existing rows when an update omits them. +# Metadata keys that are immutable once set: preserved when an update omits them, +# and rejected (400) when an update tries to change them. LiteLLM_Reserved_Metadata_Fields = [ "service_account_id", ] diff --git a/litellm/proxy/management_endpoints/key_management_endpoints.py b/litellm/proxy/management_endpoints/key_management_endpoints.py index 2d9fc4e89b8..dd38ff162b0 100644 --- a/litellm/proxy/management_endpoints/key_management_endpoints.py +++ b/litellm/proxy/management_endpoints/key_management_endpoints.py @@ -1518,13 +1518,15 @@ def prepare_metadata_fields( casted_metadata = cast(dict, non_default_values["metadata"]) # Reserved metadata fields are immutable once set. Preserve the existing value - # when omitted, reject attempts to change it. + # when omitted, reject any explicit attempt to change it (including null). for reserved_field in LiteLLM_Reserved_Metadata_Fields: existing_value = existing_metadata.get(reserved_field) if existing_value is None: continue - incoming_value = casted_metadata.get(reserved_field) - if incoming_value is not None and incoming_value != existing_value: + if ( + reserved_field in casted_metadata + and casted_metadata[reserved_field] != existing_value + ): raise HTTPException( status_code=400, detail=f"{reserved_field} is immutable once set and cannot be changed via update.", 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 bece11aa668..cdf453fe3ee 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 @@ -1277,6 +1277,29 @@ async def test_update_allows_matching_service_account_id(): assert result["metadata"]["other"] == "value" +@pytest.mark.asyncio +async def test_update_rejects_explicit_null_service_account_id(): + """ + Explicit null is an attempt to clear — not an omission. Silently ignoring + it would let a caller think they cleared the field when they didn't, so + treat it the same as any other rebind attempt and return 400. + """ + data = UpdateKeyRequest( + key="sk-1", + metadata={"service_account_id": None}, + team_id="IJ", + ) + existing_key = LiteLLM_VerificationToken( + token="hashed", + team_id="IJ", + metadata={"service_account_id": "sa-old"}, + ) + + with pytest.raises(HTTPException) as exc_info: + await prepare_key_update_data(data=data, existing_key_row=existing_key) + assert exc_info.value.status_code == 400 + + @pytest.mark.asyncio async def test_update_without_metadata_still_preserves_existing(): """Omitting metadata entirely must not drop existing metadata fields.""" From 208d583a3301a055a02b1ca7b895cc881a064c99 Mon Sep 17 00:00:00 2001 From: Ryan Crabbe Date: Sat, 18 Apr 2026 11:52:54 -0700 Subject: [PATCH 3/4] [Fix] handle metadata=null on service-account keys Addresses Greptile P1: `metadata: null` on a key with a reserved field was crashing inside prepare_metadata_fields because cast(dict, None) is a runtime no-op, so the loop hit `reserved_field in None` and returned 500 instead of 400. Extend the rejection condition to cover `casted_metadata is None`, so attempts to clear an immutable field via `metadata: null` return 400 consistently with the overwrite path. Non-service-account keys still fall through to the existing "clear all metadata" behavior. --- .../key_management_endpoints.py | 2 +- .../test_key_management_endpoints.py | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/litellm/proxy/management_endpoints/key_management_endpoints.py b/litellm/proxy/management_endpoints/key_management_endpoints.py index dd38ff162b0..7404d635bdd 100644 --- a/litellm/proxy/management_endpoints/key_management_endpoints.py +++ b/litellm/proxy/management_endpoints/key_management_endpoints.py @@ -1523,7 +1523,7 @@ def prepare_metadata_fields( existing_value = existing_metadata.get(reserved_field) if existing_value is None: continue - if ( + if casted_metadata is None or ( reserved_field in casted_metadata and casted_metadata[reserved_field] != existing_value ): 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 cdf453fe3ee..82a8dc1108f 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 @@ -1300,6 +1300,25 @@ async def test_update_rejects_explicit_null_service_account_id(): assert exc_info.value.status_code == 400 +@pytest.mark.asyncio +async def test_update_rejects_whole_metadata_null_on_service_account_key(): + """ + `metadata: null` on a key with a reserved field would have dereferenced None + inside the reserved-field loop and returned a 500. Must surface as a 400 + since the effect would be to clear an immutable field. + """ + data = UpdateKeyRequest(key="sk-1", metadata=None, team_id="IJ") + existing_key = LiteLLM_VerificationToken( + token="hashed", + team_id="IJ", + metadata={"service_account_id": "sa-123"}, + ) + + with pytest.raises(HTTPException) as exc_info: + await prepare_key_update_data(data=data, existing_key_row=existing_key) + assert exc_info.value.status_code == 400 + + @pytest.mark.asyncio async def test_update_without_metadata_still_preserves_existing(): """Omitting metadata entirely must not drop existing metadata fields.""" From 7eab549190ec18455c48e864bbb2446ecf034e2b Mon Sep 17 00:00:00 2001 From: Ryan Crabbe Date: Sat, 25 Apr 2026 09:08:55 -0700 Subject: [PATCH 4/4] test: tighten prepare_key_update_data mock and apply black - test_prepare_key_update_data: replace bare MagicMock with MagicMock(spec=LiteLLM_VerificationToken) and explicitly set existing_key_row.metadata = {}, so reserved-field reads return real values instead of MagicMock-returning-MagicMock. Fixes a regression surfaced by the new reserved-metadata preservation logic. - test_key_management_endpoints.py: black-format-only changes from recent edits. --- tests/proxy_unit_tests/test_proxy_utils.py | 5 +++-- .../test_key_management_endpoints.py | 16 ++++------------ 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/tests/proxy_unit_tests/test_proxy_utils.py b/tests/proxy_unit_tests/test_proxy_utils.py index 8703331ea83..feca4251a45 100644 --- a/tests/proxy_unit_tests/test_proxy_utils.py +++ b/tests/proxy_unit_tests/test_proxy_utils.py @@ -686,12 +686,13 @@ async def test_proxy_config_update_from_db(): @pytest.mark.asyncio async def test_prepare_key_update_data(): - from litellm.proxy._types import UpdateKeyRequest + from litellm.proxy._types import LiteLLM_VerificationToken, UpdateKeyRequest from litellm.proxy.management_endpoints.key_management_endpoints import ( prepare_key_update_data, ) - existing_key_row = MagicMock() + existing_key_row = MagicMock(spec=LiteLLM_VerificationToken) + existing_key_row.metadata = {} data = UpdateKeyRequest(key="test_key", models=["gpt-4"], duration="120s") updated_data = await prepare_key_update_data(data, existing_key_row) assert "expires" in updated_data 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 705284b80e9..647be49e0d1 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 @@ -1229,9 +1229,7 @@ async def test_update_preserves_service_account_id_when_metadata_replaced(): metadata={"service_account_id": "sa-123"}, ) - result = await prepare_key_update_data( - data=data, existing_key_row=existing_key - ) + result = await prepare_key_update_data(data=data, existing_key_row=existing_key) assert result["metadata"]["service_account_id"] == "sa-123" assert result["metadata"]["unrelated"] == "value" @@ -1274,9 +1272,7 @@ async def test_update_allows_matching_service_account_id(): metadata={"service_account_id": "sa-123"}, ) - result = await prepare_key_update_data( - data=data, existing_key_row=existing_key - ) + result = await prepare_key_update_data(data=data, existing_key_row=existing_key) assert result["metadata"]["service_account_id"] == "sa-123" assert result["metadata"]["other"] == "value" @@ -1334,9 +1330,7 @@ async def test_update_without_metadata_still_preserves_existing(): metadata={"service_account_id": "sa-123", "other": "kept"}, ) - result = await prepare_key_update_data( - data=data, existing_key_row=existing_key - ) + result = await prepare_key_update_data(data=data, existing_key_row=existing_key) assert result["metadata"]["service_account_id"] == "sa-123" assert result["metadata"]["other"] == "kept" @@ -8221,9 +8215,7 @@ async def test_update_key_non_budget_rejects_cross_user_modification(monkeypatch ) mock_prisma_client = AsyncMock() - test_hashed_token = ( - "cafebabe" * 8 - ) + test_hashed_token = "cafebabe" * 8 mock_existing_key = MagicMock() mock_existing_key.token = test_hashed_token