From 433aef58e560844fe0707def248263da4c98a6ff Mon Sep 17 00:00:00 2001 From: Yucheng Zhu Date: Tue, 18 Aug 2026 11:11:53 -0700 Subject: [PATCH] fix(team_callback_endpoints): omit credential callback_vars instead of masking them GET /team/{team_id}/callback masked credential-bearing callback_vars as the literal ***REDACTED***. No write path recognises that marker, so a caller that cloned the response into POST /team/{team_id}/callback or POST /team/update stored the marker itself as the credential. It is encrypted at rest like any other secret, so the row looks legitimate while the team's logging silently stops working. Drop the credential keys from the response instead, so no marker exists for a caller to echo back. The response is a partial config rather than a safe round trip: the metadata write paths replace logging wholesale, so echoing it back still drops the stored credential, it just cannot substitute a fabricated one. Resolves LIT-5110 --- .../team_callback_endpoints.py | 27 ++++-- .../test_team_callback_endpoints.py | 87 +++++++++++++++++-- ui/litellm-dashboard/src/lib/http/schema.d.ts | 6 +- 3 files changed, 104 insertions(+), 16 deletions(-) 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",