From 986cdedd4fae90ba6ae674ce56cec22094bc5047 Mon Sep 17 00:00:00 2001 From: user <70670632+stuxf@users.noreply.github.com> Date: Thu, 30 Apr 2026 05:18:29 +0000 Subject: [PATCH] fix(audit): redact callback secrets and surface fire-and-forget failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Greptile P2s addressed: 1. (security) The audit-log row for an ``add_team_callbacks`` call would serialize the entire ``callback_vars`` block — including ``langfuse_secret_key``, ``langsmith_api_key``, and the GCS service account path — verbatim into ``LiteLLM_AuditLogs``. Anyone with read access to the audit table could harvest team callback credentials. Same risk for ``disable_team_logging`` when the team's existing row has populated ``callback_settings.callback_vars``. Add ``_redact_callback_secrets``: deep-copies the metadata snapshot and replaces every ``callback_vars`` value with ``***REDACTED***``. The keys are kept so an auditor can still see *which* fields changed. Applied to both before and after snapshots. 2. ``asyncio.create_task`` is fire-and-forget; if the audit-log write raises (transient DB error etc.) the exception is silently discarded by the event loop and the audit row is just missing — the exact gap this PR is closing. Attach a ``done_callback`` that logs the exception at warning level via ``verbose_proxy_logger`` so the operator sees there's a gap. Tests assert that callback values are not present in the serialized audit payload (both for ``add_team_callbacks`` and for ``disable_team_logging`` when the team's existing row has populated secrets). --- .../team_callback_endpoints.py | 63 ++++++++++++++++++- .../test_team_callback_endpoints.py | 63 +++++++++++++++++++ 2 files changed, 123 insertions(+), 3 deletions(-) diff --git a/litellm/proxy/management_endpoints/team_callback_endpoints.py b/litellm/proxy/management_endpoints/team_callback_endpoints.py index cd1d8363691..a6c0c7dcc0e 100644 --- a/litellm/proxy/management_endpoints/team_callback_endpoints.py +++ b/litellm/proxy/management_endpoints/team_callback_endpoints.py @@ -31,6 +31,56 @@ from litellm.proxy.management_helpers.utils import management_endpoint_wrapper router = APIRouter() +_CALLBACK_VARS_REDACTED = "***REDACTED***" + + +def _redact_callback_secrets(metadata: Any) -> Any: + """Strip secret values out of a team-metadata snapshot before audit logging. + + Both ``team_metadata["logging"]`` (list of ``AddTeamCallback`` dicts) and + ``team_metadata["callback_settings"]["callback_vars"]`` carry provider + credentials such as ``langfuse_secret_key``, ``langsmith_api_key``, and + ``gcs_path_service_account``. Persisting them verbatim into + ``LiteLLM_AuditLogs`` would let anyone with read access to the audit + table harvest team callback credentials, so we replace each value with + a fixed marker. The keys themselves are kept so the audit reader can + still see *which* fields changed. + """ + if not isinstance(metadata, dict): + return metadata + redacted = copy.deepcopy(metadata) + logging_entries = redacted.get("logging") + if isinstance(logging_entries, list): + for entry in logging_entries: + if isinstance(entry, dict) and isinstance(entry.get("callback_vars"), dict): + entry["callback_vars"] = { + k: _CALLBACK_VARS_REDACTED for k in entry["callback_vars"] + } + callback_settings = redacted.get("callback_settings") + if isinstance(callback_settings, dict) and isinstance( + callback_settings.get("callback_vars"), dict + ): + callback_settings["callback_vars"] = { + k: _CALLBACK_VARS_REDACTED for k in callback_settings["callback_vars"] + } + return redacted + + +def _log_audit_task_exception(task: "asyncio.Task[None]") -> None: + """Surface a fire-and-forget audit-log task failure. + + ``asyncio.create_task`` swallows exceptions silently — if the audit + write fails (transient DB error etc.) we'd otherwise lose the row + without any signal. Log at warning level so the operator sees there's + a gap in the audit trail. + """ + if task.cancelled(): + return + exc = task.exception() + if exc is not None: + verbose_proxy_logger.warning("Failed to write team-callback audit log: %s", exc) + + async def _emit_team_callback_audit_log( *, team_id: str, @@ -46,6 +96,9 @@ async def _emit_team_callback_audit_log( when audit logging is not enabled on the proxy. Captured under ``LitellmTableNames.TEAM_TABLE_NAME`` so the row co-locates with other team mutations in the audit table. + + Callback secrets are redacted before serialization so the audit table + cannot itself become a credential-harvest sink. """ if litellm.store_audit_logs is not True: return @@ -55,7 +108,10 @@ async def _emit_team_callback_audit_log( ) from litellm.proxy.proxy_server import litellm_proxy_admin_name - asyncio.create_task( + redacted_before = _redact_callback_secrets(before_metadata) + redacted_after = _redact_callback_secrets(after_metadata) + + task = asyncio.create_task( create_audit_log_for_update( request_data=LiteLLM_AuditLogs( id=str(uuid.uuid4()), @@ -67,11 +123,12 @@ async def _emit_team_callback_audit_log( table_name=LitellmTableNames.TEAM_TABLE_NAME, object_id=team_id, action="updated", - updated_values=json.dumps({"metadata": after_metadata}, default=str), - before_value=json.dumps({"metadata": before_metadata}, default=str), + updated_values=json.dumps({"metadata": redacted_after}, default=str), + before_value=json.dumps({"metadata": redacted_before}, default=str), ) ) ) + task.add_done_callback(_log_audit_task_exception) @router.post( 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 547c092ee3f..6c4d1be7831 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 @@ -195,6 +195,69 @@ async def test_add_team_callbacks_emits_audit_log_when_enabled(monkeypatch): assert len(after["metadata"]["logging"]) == 1 assert after["metadata"]["logging"][0]["callback_name"] == "langfuse" + # Callback secrets MUST NOT leak into the audit log payload. + callback_vars = after["metadata"]["logging"][0]["callback_vars"] + assert callback_vars["langfuse_public_key"] != "pk" + assert callback_vars["langfuse_secret_key"] != "sk" + # Key names are preserved so the auditor can see which fields changed. + assert "langfuse_public_key" in callback_vars + assert "langfuse_secret_key" in callback_vars + # And no plaintext secret should appear anywhere in the serialized row. + assert "sk" not in log.updated_values.replace("sk-", "") # crude leak check + assert "pk" not in (log.updated_values.replace("pk-", "").replace("public_key", "")) + + +@pytest.mark.asyncio +async def test_disable_team_logging_redacts_existing_callback_secrets(monkeypatch): + monkeypatch.setattr(litellm, "store_audit_logs", True) + # Existing team has populated callback_vars containing secrets — redaction + # must apply to the BEFORE snapshot too. + mock_prisma = _patch_prisma( + { + "callback_settings": { + "success_callback": ["langfuse"], + "failure_callback": [], + "callback_vars": { + "langfuse_public_key": "pk-real", + "langfuse_secret_key": "sk-real-secret", + }, + } + } + ) + + audit_calls = [] + + async def capture(request_data): + audit_calls.append(request_data) + + with ( + patch("litellm.proxy.proxy_server.prisma_client", mock_prisma), + patch("litellm.proxy.proxy_server.litellm_proxy_admin_name", "admin"), + patch( + "litellm.proxy.management_helpers.audit_logs.create_audit_log_for_update", + new=capture, + ), + ): + await disable_team_logging( + http_request=MagicMock(spec=Request), + team_id="team-1", + user_api_key_dict=_admin_auth(), + litellm_changed_by=None, + ) + import asyncio + + for _ in range(3): + await asyncio.sleep(0) + + assert len(audit_calls) == 1 + log = audit_calls[0] + # The pre-existing secret_key value must NOT appear in the serialized + # before_value or updated_values. + assert "sk-real-secret" not in log.before_value + assert "sk-real-secret" not in log.updated_values + assert "pk-real" not in log.before_value + assert "pk-real" not in log.updated_values + @pytest.mark.asyncio async def test_add_team_callbacks_no_audit_when_disabled(monkeypatch):