From 22b04d0f678e0dd48c387a58e44f0ea4eabbe1dc Mon Sep 17 00:00:00 2001 From: Yucheng Zhu Date: Wed, 29 Jul 2026 21:56:22 -0700 Subject: [PATCH] fix(credentials): scope access validation and credential_info merge to logging destinations Three collateral regressions surfaced while reviewing this PR, each narrowing feature-added behavior back toward base for non-destination paths. validate_credential_access now runs only for logging destinations, so a provider credential carrying an unrelated access key is no longer rejected with a 400. update_db_credential and _sync_in_memory_credential subfield-merge credential_info only for logging destinations; provider credentials keep base replace semantics. regenerate_key_fn no longer swallows the team-lookup 404, so regenerating a key whose team was deleted fails like base instead of silently proceeding against a dangling team The logging-destination merge preserves credential_type, description, and the untouched access subfields, which the Edit-access UI modal depends on since it patches only access. The gate keys off the credential_type tag alone (not a full parse of access) so a destination with a malformed access is still routed into validation and rejected. Follow-up LIT-5004 tracks the general /credentials PATCH replace-vs-merge decision for all credential types --- .../proxy/credential_endpoints/endpoints.py | 31 ++++--- .../key_management_endpoints.py | 15 ++-- .../logging_exporter_access.py | 27 ++++++- .../credential_endpoints/test_endpoints.py | 74 +++++++++++++++++ .../test_key_management_endpoints.py | 81 +++++++++++++++++++ .../test_logging_exporter_access.py | 22 +++++ 6 files changed, 229 insertions(+), 21 deletions(-) diff --git a/litellm/proxy/credential_endpoints/endpoints.py b/litellm/proxy/credential_endpoints/endpoints.py index 838f4b7daca..2769d991904 100644 --- a/litellm/proxy/credential_endpoints/endpoints.py +++ b/litellm/proxy/credential_endpoints/endpoints.py @@ -14,6 +14,7 @@ from litellm.litellm_core_utils.litellm_logging import _get_masked_values from litellm.proxy._types import CommonProxyErrors, UserAPIKeyAuth from litellm.proxy.auth.user_api_key_auth import user_api_key_auth from litellm.proxy.common_utils.encrypt_decrypt_utils import encrypt_value_helper +from litellm.proxy.management_endpoints.logging_exporter_access import is_logging_credential from litellm.proxy.management_endpoints.logging_exporter_validation import ( validate_credential_access, ) @@ -65,7 +66,8 @@ async def create_credential( """ from litellm.proxy.proxy_server import llm_router, prisma_client - validate_credential_access(credential.credential_info) + if is_logging_credential(credential.credential_info): + validate_credential_access(credential.credential_info) try: if prisma_client is None: @@ -285,9 +287,12 @@ def update_db_credential( merged_credential.credential_values.update(encrypted_params) if encrypted_credential.credential_info: - if merged_credential.credential_info is None: - merged_credential.credential_info = {} - _merge_credential_info(merged_credential.credential_info, encrypted_credential.credential_info) + if is_logging_credential(db_credential.credential_info): + if merged_credential.credential_info is None: + merged_credential.credential_info = {} + _merge_credential_info(merged_credential.credential_info, encrypted_credential.credential_info) + else: + merged_credential.credential_info = encrypted_credential.credential_info return merged_credential @@ -344,8 +349,6 @@ async def update_credential( """ from litellm.proxy.proxy_server import prisma_client - validate_credential_access(credential.credential_info) - try: if prisma_client is None: raise HTTPException( @@ -356,6 +359,8 @@ async def update_credential( db_credential = await credentials_repository.find_by_name(credential_name) if db_credential is None: raise HTTPException(status_code=404, detail="Credential not found in DB.") + if is_logging_credential(db_credential.credential_info) or is_logging_credential(credential.credential_info): + validate_credential_access(credential.credential_info) merged_credential = update_db_credential(db_credential, _patch_to_credential_item(credential, credential_name)) credential_object_jsonified = jsonify_object(merged_credential.model_dump()) await credentials_repository.update_by_name( @@ -384,10 +389,11 @@ def _sync_in_memory_credential( """Mirror the DB write into ``litellm.credential_list``. Skips when the credential isn't resident in memory (e.g. created on - another scaled instance, restored from DB on the next reload). The - in-memory ``credential_info`` is merged subfield-by-subfield via - ``_merge_credential_info`` so a partial patch can't clobber stored - ``access`` subfields it didn't touch. + another scaled instance, restored from DB on the next reload). For a + logging destination the in-memory ``credential_info`` is merged + subfield-by-subfield via ``_merge_credential_info`` so a partial patch + can't clobber stored ``access`` subfields it didn't touch; a provider + credential keeps the base replace semantics. """ existing_in_memory: CredentialItem | None = None for cred in litellm.credential_list: @@ -402,7 +408,10 @@ def _sync_in_memory_credential( in_memory_values.update(patch.credential_values) in_memory_info = dict(existing_in_memory.credential_info or {}) if patch.credential_info: - _merge_credential_info(in_memory_info, patch.credential_info) + if is_logging_credential(existing_in_memory.credential_info): + _merge_credential_info(in_memory_info, patch.credential_info) + else: + in_memory_info = dict(patch.credential_info) updated_in_memory = CredentialItem( credential_name=merged.credential_name, credential_values=in_memory_values, diff --git a/litellm/proxy/management_endpoints/key_management_endpoints.py b/litellm/proxy/management_endpoints/key_management_endpoints.py index 44425734dca..ef7236aa200 100644 --- a/litellm/proxy/management_endpoints/key_management_endpoints.py +++ b/litellm/proxy/management_endpoints/key_management_endpoints.py @@ -4805,15 +4805,12 @@ async def regenerate_key_fn( # noqa: C901 # single endpoint handling many opti if data is not None and (data.access_group_ids or data.object_permission is not None): regenerate_team_table: LiteLLM_TeamTableCachedObj | None = None if _key_in_db.team_id is not None: - try: - regenerate_team_table = await get_team_object( - team_id=_key_in_db.team_id, - prisma_client=prisma_client, - user_api_key_cache=user_api_key_cache, - check_db_only=True, - ) - except HTTPException: - regenerate_team_table = None + regenerate_team_table = await get_team_object( + team_id=_key_in_db.team_id, + prisma_client=prisma_client, + user_api_key_cache=user_api_key_cache, + check_db_only=True, + ) _regen_is_proxy_admin = user_api_key_dict.user_role == LitellmUserRoles.PROXY_ADMIN.value TeamMemberPermissionChecks.enforce_member_can_assign_access_groups( user_api_key_dict=user_api_key_dict, diff --git a/litellm/proxy/management_endpoints/logging_exporter_access.py b/litellm/proxy/management_endpoints/logging_exporter_access.py index 93d65295be4..5eaffa55773 100644 --- a/litellm/proxy/management_endpoints/logging_exporter_access.py +++ b/litellm/proxy/management_endpoints/logging_exporter_access.py @@ -10,12 +10,20 @@ scope is the given set of team ids and org ids. The resolver passes a one-element scope built with ``identity_scope``. """ -from pydantic import ValidationError +from pydantic import BaseModel, ConfigDict, ValidationError import litellm from litellm.models.credentials import CredentialAccess, CredentialInfo +class _LoggingDestinationTag(BaseModel): + """Lenient read of just the ``credential_type`` tag, ignoring the rest of + ``credential_info`` (including a possibly-malformed ``access``).""" + + model_config = ConfigDict(extra="ignore") + credential_type: str | None = None + + def parse_credential_info(raw: object) -> CredentialInfo | None: """Parse stored ``credential_info`` into the typed model, or ``None`` when it is absent or malformed. @@ -32,6 +40,23 @@ def parse_credential_info(raw: object) -> CredentialInfo | None: return None +def is_logging_credential(raw: object) -> bool: + """Whether ``credential_info`` is tagged as an admin-owned logging destination. + + The ``access`` shape validation and the ``credential_info`` subfield merge are + scoped to these; a provider credential is left on its base replace-and-accept path. + + This keys off the ``credential_type`` tag alone and does not parse ``access``: a + destination carrying a malformed ``access`` is still a logging destination, and the + point of the gate is to route it into ``validate_credential_access`` so that bad + ``access`` is rejected rather than stored. + """ + try: + return _LoggingDestinationTag.model_validate(raw).credential_type == "logging" + except ValidationError: + return False + + def identity_scope(team_id: str | None, org_id: str | None) -> tuple[frozenset[str], frozenset[str]]: """A single request identity's scope as ``(team_ids, org_ids)`` for ``access_grants``.""" diff --git a/tests/test_litellm/proxy/credential_endpoints/test_endpoints.py b/tests/test_litellm/proxy/credential_endpoints/test_endpoints.py index eefa90bf932..fe0edeb47ce 100644 --- a/tests/test_litellm/proxy/credential_endpoints/test_endpoints.py +++ b/tests/test_litellm/proxy/credential_endpoints/test_endpoints.py @@ -97,6 +97,80 @@ def test_update_db_credential_preserves_untouched_access_subfields(): } +# --- provider credentials keep base replace semantics (merge is logging-only) --- + +def test_update_db_credential_replaces_info_for_provider_credential(): + """The subfield merge is scoped to logging destinations. A provider credential + patch replaces credential_info wholesale (base behavior): omitted keys drop, so a + partial patch is a full replace, not a merge -- merging non-destination creds would + silently resurrect stale provider metadata the caller meant to remove.""" + from litellm.proxy.credential_endpoints.endpoints import update_db_credential + + db = CredentialItem( + credential_name="openai", + credential_values={}, + credential_info={"custom_llm_provider": "openai", "stale": "keepout"}, + ) + patch = CredentialItem( + credential_name="openai", + credential_values={}, + credential_info={"custom_llm_provider": "azure"}, + ) + + merged = update_db_credential(db, patch) + + assert merged.credential_info == {"custom_llm_provider": "azure"} + + +# --- access-shape validation is scoped to logging destinations --------------- + +@pytest.mark.asyncio +async def test_create_credential_validates_access_only_for_logging(monkeypatch): + """validate_credential_access runs for a logging destination but never for a + provider credential. A provider cred carrying an unrelated `access` key must not be + rejected by the destination access-shape validator (that would 400 a valid + provider credential the validator was never meant to see).""" + import litellm.proxy.proxy_server as proxy_server + from litellm.types.utils import CreateCredentialItem + + monkeypatch.setattr(proxy_server, "prisma_client", None, raising=False) + + class _Validated(Exception): + pass + + def _spy(_info): + raise _Validated() + + monkeypatch.setattr(endpoints, "validate_credential_access", _spy) + + async def _create(credential): + return await endpoints.create_credential( + request=MagicMock(), + fastapi_response=MagicMock(), + credential=credential, + user_api_key_dict=_admin(), + ) + + logging_cred = CreateCredentialItem( + credential_name="dest", + credential_values={"otel_endpoint": "http://collector:4318"}, + credential_info={"credential_type": "logging", "description": "generic", "access": {"global": True}}, + ) + with pytest.raises(_Validated): + await _create(logging_cred) + + provider_cred = CreateCredentialItem( + credential_name="openai", + credential_values={"api_key": "sk"}, + credential_info={"custom_llm_provider": "openai", "access": {"bogus": True}}, + ) + # Validator is skipped; the handler proceeds and fails on the (None) prisma client, + # a 500 -- never the _Validated sentinel. + with pytest.raises(Exception) as excinfo: + await _create(provider_cred) + assert not isinstance(excinfo.value, _Validated) + + # --- PATCH routing regression ------------------------------------------------ def test_patch_credentials_route_targets_update_credential(): 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 867ef759fb3..64c013f0f9b 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 @@ -13162,6 +13162,87 @@ async def test_regenerate_applies_normalized_mcp_object_permission(): assert regenerated_data.object_permission.mcp_servers == ["server-id"] +@pytest.mark.asyncio +async def test_regenerate_propagates_team_not_found(): + """Regenerating a key whose team no longer exists must surface the team-lookup + 404 rather than swallowing it and regenerating against a dangling team. The + access_group_ids/object_permission gate depends on the resolved team, so a missing + team must abort the regenerate (matching /key/generate and /key/update) instead of + silently continuing with team_table=None.""" + from litellm.proxy._types import RegenerateKeyRequest + from litellm.proxy.management_endpoints.key_management_endpoints import ( + regenerate_key_fn, + ) + + data = RegenerateKeyRequest(key="sk-old", access_group_ids=["ag-1"]) + existing_key = LiteLLM_VerificationToken( + token="abc123", + user_id="user-1", + models=["gpt-4"], + team_id="dangling-team", + max_budget=None, + tags=None, + ) + mock_prisma_client = AsyncMock() + mock_repo = MagicMock() + mock_repo.table.find_unique = AsyncMock(return_value=existing_key) + execute_mock = AsyncMock(return_value=MagicMock()) + enforce_mock = MagicMock() + + async def _raise_team_not_found(*args, **kwargs): + raise HTTPException(status_code=404, detail={"error": "Team not found"}) + + with ( + patch("litellm.proxy.proxy_server.premium_user", True), + patch("litellm.proxy.proxy_server.master_key", None), + patch("litellm.proxy.proxy_server.prisma_client", mock_prisma_client), + patch("litellm.proxy.proxy_server.proxy_logging_obj", MagicMock()), + patch("litellm.proxy.proxy_server.user_api_key_cache", MagicMock()), + patch("litellm.proxy.proxy_server.hash_token", lambda token: "hashed-old"), + patch( + "litellm.proxy.management_endpoints.key_management_endpoints.VerificationTokenRepository", + return_value=mock_repo, + ), + patch( + "litellm.proxy.management_endpoints.key_management_endpoints.TeamMemberPermissionChecks.can_team_member_execute_key_management_endpoint", + new_callable=AsyncMock, + ), + patch( + "litellm.proxy.management_endpoints.key_management_endpoints.TeamMemberPermissionChecks.enforce_member_can_assign_access_groups", + enforce_mock, + ), + patch( + "litellm.proxy.management_endpoints.key_management_endpoints.can_modify_verification_token", + new_callable=AsyncMock, + return_value=True, + ), + patch( + "litellm.proxy.management_endpoints.key_management_endpoints.get_team_object", + _raise_team_not_found, + ), + patch( + "litellm.proxy.management_endpoints.key_management_endpoints._execute_virtual_key_regeneration", + execute_mock, + ), + ): + with pytest.raises((HTTPException, ProxyException)) as exc_info: + await regenerate_key_fn( + key="sk-old", + data=data, + user_api_key_dict=UserAPIKeyAuth( + user_role=LitellmUserRoles.PROXY_ADMIN.value, + api_key="sk-admin", + user_id="admin", + ), + ) + + err = exc_info.value + code = getattr(err, "status_code", None) or getattr(err, "code", None) + assert str(code) == "404" + enforce_mock.assert_not_called() + execute_mock.assert_not_awaited() + + # --------------------------------------------------------------------------- # Regression tests for GHSA-q775-qw9r-2r4g: budget escalation via key/generate # --------------------------------------------------------------------------- diff --git a/tests/test_litellm/proxy/management_endpoints/test_logging_exporter_access.py b/tests/test_litellm/proxy/management_endpoints/test_logging_exporter_access.py index 71a276fe447..e5bab96e9f0 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_logging_exporter_access.py +++ b/tests/test_litellm/proxy/management_endpoints/test_logging_exporter_access.py @@ -15,11 +15,33 @@ from litellm.models.credentials import CredentialAccess, CredentialInfo, Credent from litellm.proxy.management_endpoints.logging_exporter_access import ( access_grants, identity_scope, + is_logging_credential, parse_credential_info, resolved_logging_exporter_names, ) +# --- is_logging_credential: the access-validation + merge gate --------------- + + +def test_is_logging_credential_true_for_logging_type(): + assert is_logging_credential({"credential_type": "logging", "description": "arize"}) is True + + +def test_is_logging_credential_true_even_with_malformed_access(): + """A destination carrying an invalid access shape is still a logging destination. + The gate must route it into validate_credential_access (which rejects it), not skip + validation because the strict access model can't parse it.""" + assert is_logging_credential({"credential_type": "logging", "access": {"nonsense_field": True}}) is True + + +def test_is_logging_credential_false_for_provider_and_malformed(): + assert is_logging_credential({"custom_llm_provider": "openai"}) is False + assert is_logging_credential({"custom_llm_provider": "openai", "access": {"global": True}}) is False + assert is_logging_credential(None) is False + assert is_logging_credential("nope") is False + + # --- parse_credential_info: fail closed on bad input -----------------------