diff --git a/litellm/proxy/management_endpoints/team_callback_endpoints.py b/litellm/proxy/management_endpoints/team_callback_endpoints.py index 834d4e8b73b..f04422925a5 100644 --- a/litellm/proxy/management_endpoints/team_callback_endpoints.py +++ b/litellm/proxy/management_endpoints/team_callback_endpoints.py @@ -76,21 +76,28 @@ def _redact_callback_secrets(metadata: Any) -> Any: return redacted -def _mask_sensitive_callback_vars(callbacks: TeamCallbackMetadata) -> None: - """Mask credential-bearing callback vars in place, keeping the rest readable. +def _drop_sensitive_callback_vars(callbacks: TeamCallbackMetadata) -> None: + """Remove credential-bearing callback vars in place, keeping the rest readable. ``callback_vars`` mixes credentials (``langsmith_api_key``, ``langfuse_secret_key``, ``gcs_path_service_account``) with plain configuration (project names, bucket names, hosts). The configuration is what makes a read of this endpoint useful, so only the sensitive keys are - replaced, using the same marker as the audit-log redaction above. + dropped. A value that still carries the encrypted prefix here failed to decrypt, so - it is masked too. Handing back a ciphertext blob under a key that is not + it is dropped too. Handing back a ciphertext blob under a key that is not classified as sensitive would give the caller something it cannot use and cannot tell apart from a real value. - Masking in place rather than rebuilding the mapping keeps this under the + Keys are omitted rather than replaced with the ``***REDACTED***`` marker + used for audit logs, because a marker survives a read-modify-write and is + then stored as the credential itself. Omitting does not make the response a + safe round trip: the metadata write paths replace ``logging`` wholesale, so + echoing this payload back still drops the stored credential. It only stops + a fabricated one being stored in its place. + + Deleting in place rather than rebuilding the mapping keeps this under the LIT002 mutable-collection-construction budget. It is safe because the only caller passes an object it just built from a decrypted deep copy of the row, so nothing here is reachable from the team's stored metadata. @@ -100,7 +107,7 @@ def _mask_sensitive_callback_vars(callbacks: TeamCallbackMetadata) -> None: for key in tuple(callbacks.callback_vars): value = callbacks.callback_vars[key] if is_sensitive_callback_key(key) or str(value).startswith(_CALLBACK_VAR_ENCRYPTED_PREFIX): - callbacks.callback_vars[key] = _CALLBACK_VARS_REDACTED + del callbacks.callback_vars[key] def _resolve_team_callbacks(team_metadata: object) -> TeamCallbackMetadata: @@ -142,7 +149,7 @@ def _resolve_team_callbacks(team_metadata: object) -> TeamCallbackMetadata: TeamCallbackMetadata(**callback_settings) if isinstance(callback_settings, dict) else TeamCallbackMetadata() ) - _mask_sensitive_callback_vars(resolved) + _drop_sensitive_callback_vars(resolved) return resolved @@ -530,8 +537,10 @@ async def get_team_callbacks( Covers callbacks registered through POST /team/{team_id}/callback and the Admin UI as well as teams still on the deprecated callback_settings shape, resolved from the team's stored metadata with the same precedence used at request time. A key-level logging config overrides the team's - at request time and is not reflected here. Credential-bearing callback_vars are returned masked - as `***REDACTED***` + at request time and is not reflected here. Credential-bearing callback_vars are omitted from the + response rather than masked, so a marker can never be posted back and stored as a credential. The + response is therefore a partial config; credentials must be supplied again before it is written + anywhere, and a team left without one falls back to the proxy-level credential for that callback Returns { "status": "success", diff --git a/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py index 21e25d30b82..cc669c080d4 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py @@ -19,6 +19,10 @@ from litellm.proxy._types import ( LitellmUserRoles, UserAPIKeyAuth, ) +from litellm.proxy.common_utils.callback_utils import ( + decrypt_callback_vars, + encrypt_callback_vars, +) from litellm.proxy.management_endpoints.team_callback_endpoints import ( add_team_callbacks, disable_team_logging, @@ -526,14 +530,87 @@ async def test_get_team_callbacks_returns_callbacks_registered_via_post(monkeypa assert response["data"]["success_callbacks"] == ["langsmith"] assert response["data"]["failure_callbacks"] == [] - # Non-secret vars come back usable, the credential is masked, and the + # Non-secret vars come back usable, the credential key is absent, and the # ciphertext that is stored on the row never reaches the response. assert response["data"]["callback_vars"]["langsmith_project"] == "tenant-project" - assert response["data"]["callback_vars"]["langsmith_api_key"] == "***REDACTED***" + assert "langsmith_api_key" not in response["data"]["callback_vars"] assert "lsv2-real-secret" not in json.dumps(response) assert "litellm_enc::" not in json.dumps(response) +@pytest.mark.asyncio +async def test_get_team_callbacks_response_does_not_write_a_marker_as_a_credential_when_cloned(monkeypatch): + """The read response must be safe to post back into a write path. + + Cloning one team's logging config onto another is the obvious use of this + endpoint. When the response carried a ``***REDACTED***`` marker in place of + the credential, that replay stored the marker itself as the credential: + it is encrypted at rest like any real secret, so the row looks legitimate + while the target team's logging silently does nothing. Omitting the key + instead means the replay writes no credential at all, which is a visibly + incomplete config rather than a plausible broken one. + """ + monkeypatch.setenv("LITELLM_SALT_KEY", "test-salt-32-bytes-aaaaaaaaaaaaaa") + source_metadata = { + "logging": [ + { + "callback_name": "langsmith", + "callback_type": "success", + "callback_vars": { + "langsmith_api_key": "lsv2-source-secret", + "langsmith_project": "source-project", + }, + } + ] + } + source_row = _team_row(team_id="team-source", metadata=encrypt_callback_vars(source_metadata)) + source_prisma = _patch_prisma(source_row) + + with ( + patch("litellm.proxy.proxy_server.prisma_client", source_prisma), + patch("litellm.proxy.proxy_server.master_key", None), + ): + read = await get_team_callbacks( + http_request=MagicMock(spec=Request), + team_id="team-source", + user_api_key_dict=_admin_auth(), + ) + + # Nothing a caller could echo into any write path is a marker, so this + # holds whichever of the metadata write routes the clone goes through. + assert "***REDACTED***" not in json.dumps(read) + + target_row = _team_row(team_id="team-target", metadata={}) + target_prisma = _patch_prisma(target_row) + + with ( + patch("litellm.proxy.proxy_server.prisma_client", target_prisma), + patch("litellm.proxy.proxy_server.litellm_proxy_admin_name", "admin"), + patch("litellm.proxy.proxy_server.master_key", None), + ): + await add_team_callbacks( + data=AddTeamCallback( + callback_name="langsmith", + callback_type="success", + callback_vars=read["data"]["callback_vars"], + ), + http_request=MagicMock(spec=Request), + team_id="team-target", + user_api_key_dict=_admin_auth(), + litellm_changed_by=None, + ) + + persisted = decrypt_callback_vars( + json.loads(target_prisma.db.litellm_teamtable.update.await_args.kwargs["data"]["metadata"]) + ) + cloned_vars = persisted["logging"][0]["callback_vars"] + # The non-secret config clones, the credential does not follow, and no + # marker is ever stored where a credential belongs. + assert cloned_vars["langsmith_project"] == "source-project" + assert "langsmith_api_key" not in cloned_vars + assert "***REDACTED***" not in json.dumps(persisted) + + @pytest.mark.asyncio async def test_get_team_callbacks_prefers_logging_over_deprecated_callback_settings(): """A team carrying both shapes must report only the one that actually fires. @@ -646,7 +723,7 @@ async def test_get_team_callbacks_decrypts_vars_stored_under_non_sensitive_keys( @pytest.mark.asyncio -async def test_get_team_callbacks_masks_values_that_fail_to_decrypt(monkeypatch): +async def test_get_team_callbacks_drops_values_that_fail_to_decrypt(monkeypatch): """A value that cannot be decrypted must never leave as ciphertext. After a salt-key rotation an existing value no longer decrypts, and the @@ -683,7 +760,7 @@ async def test_get_team_callbacks_masks_values_that_fail_to_decrypt(monkeypatch) ) assert response["data"]["success_callbacks"] == ["langsmith"] - assert response["data"]["callback_vars"]["langsmith_project"] == "***REDACTED***" + assert "langsmith_project" not in response["data"]["callback_vars"] assert _CALLBACK_VAR_ENCRYPTED_PREFIX not in json.dumps(response) @@ -716,7 +793,7 @@ async def test_get_team_callbacks_falls_back_to_deprecated_callback_settings(): assert response["data"]["callback_vars"]["gcs_bucket_name"] == "legacy-bucket" # Legacy rows predate encryption at rest, so this endpoint is where the # plaintext secret would otherwise escape. - assert response["data"]["callback_vars"]["langfuse_secret_key"] == "***REDACTED***" + assert "langfuse_secret_key" not in response["data"]["callback_vars"] assert "sk-lf-legacy-plaintext" not in json.dumps(response) diff --git a/ui/litellm-dashboard/src/lib/http/schema.d.ts b/ui/litellm-dashboard/src/lib/http/schema.d.ts index a3a461edca0..bcf67d27dd8 100644 --- a/ui/litellm-dashboard/src/lib/http/schema.d.ts +++ b/ui/litellm-dashboard/src/lib/http/schema.d.ts @@ -14750,8 +14750,10 @@ export interface paths { * Covers callbacks registered through POST /team/{team_id}/callback and the Admin UI as well as * teams still on the deprecated callback_settings shape, resolved from the team's stored metadata * with the same precedence used at request time. A key-level logging config overrides the team's - * at request time and is not reflected here. Credential-bearing callback_vars are returned masked - * as `***REDACTED***` + * at request time and is not reflected here. Credential-bearing callback_vars are omitted from the + * response rather than masked, so a marker can never be posted back and stored as a credential. The + * response is therefore a partial config; credentials must be supplied again before it is written + * anywhere, and a team left without one falls back to the proxy-level credential for that callback * * Returns { * "status": "success",