diff --git a/litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/credentials.py b/litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/credentials.py index 48ceeedd73d..adcbf3b748e 100644 --- a/litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/credentials.py +++ b/litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/credentials.py @@ -18,7 +18,7 @@ The stash bridges pre_call resolution into post_call where request metadata is gone — see ``stash_resolved`` for the full rationale. """ -from typing import TYPE_CHECKING, Dict, Literal, Optional, Tuple +from typing import TYPE_CHECKING, Literal, Optional, Tuple from litellm._logging import verbose_proxy_logger @@ -33,9 +33,15 @@ if TYPE_CHECKING: logger = verbose_proxy_logger.getChild("alice_wonderfence") -# Key used to stash per-request resolved (api_key, app_id) on -# logging_obj.model_call_details so post_call can recover it. -_LOGGING_OBJ_STASH_KEY = "alice_wonderfence_resolved" +# Attribute on the request-scoped logging_obj where the resolved +# (api_key, app_id) is stashed so post_call can recover it. This is a private +# instance attribute, NOT a model_call_details key, because model_call_details +# is forwarded verbatim as ``kwargs`` to every logging callback / exporter +# (litellm_logging.py) — stashing the resolved WonderFence api_key there would +# leak a tenant secret into logs. A private attribute on the same object gives +# the identical cross-hook / cross-task visibility (see stash_resolved) without +# entering the logged payload. +_STASH_ATTR = "_alice_wonderfence_resolved" def get_metadata(request_data: dict) -> dict: @@ -149,32 +155,35 @@ def stash_resolved( lives) is dropped. Without a bridge, post_call resolution fails even though the request explicitly supplied the value. - Why logging_obj.model_call_details (and not a ContextVar): + Why a private attribute on logging_obj (and not model_call_details): + ``model_call_details`` is forwarded verbatim as ``kwargs`` to every + logging callback / exporter (``litellm_logging.py`` passes + ``kwargs=self.model_call_details`` to success/failure handlers and + logging hooks), and the redaction layer only scrubs message + input/output and known StandardLoggingPayload fields, not arbitrary + custom keys. Stashing the resolved WonderFence ``api_key`` there leaks + a tenant secret into logs. A private instance attribute is request + scoped on the same object but is not part of the logged ``kwargs``. + + Why an attribute on logging_obj (and not a ContextVar): during_call hooks run via ``asyncio.gather`` in ``litellm/proxy/utils.py:1500``, which wraps each coroutine in its own asyncio Task with a *copied* context. ContextVar writes in a child Task are not visible to the parent Task that runs post_call, so a ContextVar bridge silently fails. ``logging_obj`` is passed through every hook by reference (same object across pre_call, during_call, - and post_call), so mutations to its ``model_call_details`` dict are - visible regardless of task boundary. - - Why this isn't a layering hack: - Despite the name, ``model_call_details`` is used throughout LiteLLM - as a generic request-scoped state bag (see ``main.py:6444``, - ``proxy/utils.py:1885-1895``, every passthrough handler under - ``proxy/pass_through_endpoints/``). It stores things like ``model``, - ``custom_llm_provider``, ``response_cost``, ``messages``, ``client``, - ``litellm_call_id`` — well beyond log payload material. + and post_call), so mutations to it are visible regardless of task + boundary. Keyed by ``guardrail_name`` so multiple alice_wonderfence instances configured on the same proxy don't collide. """ if logging_obj is None: return - container: Dict[str, Tuple[str, str]] = logging_obj.model_call_details.setdefault( - _LOGGING_OBJ_STASH_KEY, {} - ) + container = getattr(logging_obj, _STASH_ATTR, None) + if not isinstance(container, dict): + container = {} + setattr(logging_obj, _STASH_ATTR, container) container[guardrail_name] = (api_key, app_id) @@ -203,8 +212,8 @@ def recover_resolved( """ if logging_obj is None: return None - container = logging_obj.model_call_details.get(_LOGGING_OBJ_STASH_KEY) - if not container: + container = getattr(logging_obj, _STASH_ATTR, None) + if not isinstance(container, dict) or not container: return None own = container.get(guardrail_name) if own is not None: diff --git a/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/conftest.py b/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/conftest.py index a5c7e531224..ced0a4d3de2 100644 --- a/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/conftest.py +++ b/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/conftest.py @@ -72,11 +72,25 @@ def _request_data(**overrides): return base -def _make_logging_obj() -> Mock: - """Mock the LiteLLMLoggingObj surface we use: only ``model_call_details``.""" - obj = Mock() - obj.model_call_details = {} - return obj +def _make_logging_obj(): + """Build a real ``LiteLLMLoggingObj``. + + The post_call bridge stashes resolved credentials on a private attribute of + this object, so tests must use the real class (not a Mock, whose attribute + auto-creation would mask whether the attribute is genuinely settable and + readable) to validate that the stash survives request -> response. + """ + from litellm.litellm_core_utils.litellm_logging import Logging + + return Logging( + model="gpt-4", + messages=[{"role": "user", "content": "hi"}], + stream=False, + call_type="completion", + start_time=0, + litellm_call_id="alice-wonderfence-test", + function_id="alice-wonderfence-test", + ) @pytest.fixture diff --git a/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/test_credentials.py b/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/test_credentials.py index f5aaf6cf37c..96e5dd45b6b 100644 --- a/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/test_credentials.py +++ b/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/test_credentials.py @@ -6,10 +6,14 @@ directly with explicit args instead of constructing a guardrail instance. import pytest +import json + from litellm.proxy.guardrails.guardrail_hooks.alice_wonderfence.credentials import ( get_metadata, + recover_resolved, resolve_api_key, resolve_app_id, + stash_resolved, ) from litellm.proxy.guardrails.guardrail_hooks.alice_wonderfence.exceptions import ( WonderFenceMissingSecrets, @@ -301,3 +305,43 @@ def test_responses_route_admin_pin_beats_caller_metadata_api_key(): ) == "admin-pinned" ) + + +# --------------- stash storage: secret must not leak to logged payload --------------- + + +def test_logging_obj_allows_private_stash_attr_off_model_call_details(make_logging_obj): + """Guard: the real LiteLLMLoggingObj must accept a private attribute that is + NOT part of model_call_details. Fails if Logging becomes slotted/pydantic or + the stash is moved back into the logged dict.""" + obj = make_logging_obj() + obj._alice_wonderfence_resolved = {"g": ("k", "a")} + assert obj._alice_wonderfence_resolved == {"g": ("k", "a")} + assert "_alice_wonderfence_resolved" not in obj.model_call_details + + +def test_stash_round_trips_on_real_logging_obj(make_logging_obj): + obj = make_logging_obj() + stash_resolved(obj, "guard-1", "wf-key-abc", "app-1") + assert recover_resolved(obj, "guard-1") == ("wf-key-abc", "app-1") + + +def test_stashed_api_key_not_present_in_model_call_details(make_logging_obj): + """Regression: model_call_details is forwarded verbatim as kwargs to logging + callbacks/exporters, so a resolved tenant api_key stashed there leaks. The + stash must live off model_call_details. Fails on the prior implementation + that stored it under model_call_details["alice_wonderfence_resolved"].""" + secret = "wf-super-secret-key-9f3a" + obj = make_logging_obj() + stash_resolved(obj, "guard-1", secret, "app-1") + + dumped = json.dumps(obj.model_call_details, default=str) + assert secret not in dumped + assert "alice_wonderfence_resolved" not in obj.model_call_details + # recovery still works from the private attribute + assert recover_resolved(obj, "guard-1") == (secret, "app-1") + + +def test_recover_returns_none_when_nothing_stashed(make_logging_obj): + obj = make_logging_obj() + assert recover_resolved(obj, "guard-1") is None