diff --git a/litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/credentials.py b/litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/credentials.py index adcbf3b748e..802d0188ca6 100644 --- a/litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/credentials.py +++ b/litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/credentials.py @@ -20,8 +20,6 @@ gone — see ``stash_resolved`` for the full rationale. from typing import TYPE_CHECKING, Literal, Optional, Tuple -from litellm._logging import verbose_proxy_logger - from .exceptions import WonderFenceMissingSecrets if TYPE_CHECKING: @@ -30,18 +28,22 @@ if TYPE_CHECKING: ) -logger = verbose_proxy_logger.getChild("alice_wonderfence") +# Prefix for the per-guardrail attribute on the request-scoped logging_obj where +# the resolved (api_key, app_id) is stashed so post_call can recover it. The +# guardrail name is baked into the attribute so each instance has a physically +# separate slot — one instance can never read another's credentials. +# +# These are private instance attributes, NOT model_call_details keys, 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_PREFIX = "_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 _stash_attr(guardrail_name: str) -> str: + return _STASH_ATTR_PREFIX + guardrail_name def get_metadata(request_data: dict) -> dict: @@ -175,58 +177,31 @@ def stash_resolved( 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. + The attribute is per-guardrail (see ``_stash_attr``) so multiple + alice_wonderfence instances on the same request each get an isolated slot. """ if logging_obj is None: return - 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) + setattr(logging_obj, _stash_attr(guardrail_name), (api_key, app_id)) def recover_resolved( logging_obj: Optional["LiteLLMLoggingObj"], guardrail_name: str ) -> Optional[Tuple[str, str]]: - """Look up (api_key, app_id) stashed earlier in this request. + """Look up the (api_key, app_id) this guardrail stashed earlier in this + request, or ``None``. - Prefer this instance's own stash. If absent, fall back to any sibling - alice_wonderfence instance's stash on the same request. - - Why the sibling fallback exists: - LiteLLM serializes parallel during_call hooks through a single shared - slot ``data["guardrail_to_apply"]`` (``proxy/utils.py:1483``). That - slot is overwritten in a loop *before* any gather() task runs, so - only the last-registered guardrail callback actually executes its - during_call — the others see ``None`` and bail. Post_call, by - contrast, iterates sequentially and *all* registered guardrails run. - Net effect when a single request lists multiple alice_wonderfence - guardrails (e.g. ``guardrails: ["wonderfence", "alice-wonderfence"]`` - against a config that defines both): only one writes a stash, but - every one tries to read one in post_call. Since every - alice_wonderfence instance resolves api_key / app_id from the same - request-body / key / team metadata fields, sibling stashes carry - equivalent values. + Returns only this instance's own stash. It deliberately does NOT fall back + to another alice_wonderfence instance's stash: a sibling may have resolved + under a different policy (e.g. ``allow_request_metadata_override=True``, + carrying caller-supplied request-body credentials) that a stricter instance + must not inherit. When this instance has no own stash, the caller fails + closed rather than borrowing. """ if logging_obj is None: return None - 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: - return own - sibling_name, sibling_value = next(iter(container.items())) - logger.warning( - "Alice WonderFence: post_call recovering stash from sibling " - "guardrail '%s' (own name '%s' not in stash). See recover_resolved " - "docstring for why.", - sibling_name, - guardrail_name, - ) - return sibling_value + value = getattr(logging_obj, _stash_attr(guardrail_name), None) + return value if isinstance(value, tuple) else None def resolve_credentials( 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 96e5dd45b6b..a1df1ee7cb6 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 @@ -310,22 +310,25 @@ def test_responses_route_admin_pin_beats_caller_metadata_api_key(): # --------------- 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): + """Guard: the real LiteLLMLoggingObj must accept the per-guardrail stash + attribute. Fails if Logging becomes slotted/pydantic.""" 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_recover_does_not_borrow_a_sibling_guardrails_stash(make_logging_obj): + """Each guardrail's stash is isolated: a name that never stashed recovers + None even when a sibling stashed on the same logging_obj. This is what makes + a stricter instance fail closed instead of inheriting a permissive sibling's + caller-supplied credentials.""" + obj = make_logging_obj() + stash_resolved(obj, "writer", "writer-key", "writer-app") + assert recover_resolved(obj, "reader") is None + assert recover_resolved(obj, "writer") == ("writer-key", "writer-app") + + 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 diff --git a/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/test_post_call_bridge.py b/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/test_post_call_bridge.py index 4d9916c46a5..5281282f572 100644 --- a/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/test_post_call_bridge.py +++ b/tests/test_litellm/proxy/guardrails/guardrail_hooks/alice_wonderfence/test_post_call_bridge.py @@ -122,13 +122,16 @@ async def test_post_call_without_prior_stash_raises(make_guardrail, make_logging @pytest.mark.asyncio -async def test_post_call_recovers_via_sibling_stash( +async def test_post_call_does_not_borrow_sibling_stash( make_guardrail, make_request_data, make_logging_obj ): - """When two alice_wonderfence instances are listed in one request's - ``guardrails`` array, LiteLLM only invokes one's during_call — but every - instance runs post_call. The instance whose during_call did NOT fire - must recover the stash written by the sibling that did.""" + """A stricter instance must NOT inherit a sibling's stashed credentials. + + Exploit being closed: a permissive writer (allow_request_metadata_override + =True) stashes caller-supplied request-body credentials; a stricter reader + (allow_request_metadata_override=False) that has no own stash must fail + closed in post_call rather than scan with the writer's caller-controlled + creds.""" g_writer, c_writer = make_guardrail( guardrail_name="writer", allow_request_metadata_override=True, @@ -136,7 +139,7 @@ async def test_post_call_recovers_via_sibling_stash( g_writer._client_cache["default-api-key"] = c_writer g_reader, c_reader = make_guardrail( guardrail_name="reader", - allow_request_metadata_override=True, + allow_request_metadata_override=False, ) g_reader._client_cache["default-api-key"] = c_reader for c in (c_writer, c_reader): @@ -149,25 +152,28 @@ async def test_post_call_recovers_via_sibling_stash( logging_obj = make_logging_obj() - # Only the writer's during_call fires (simulating LiteLLM's - # data["guardrail_to_apply"] last-write-wins behavior). + # Writer stashes caller-supplied request-body app_id (override allowed). await g_writer.apply_guardrail( inputs={"texts": ["hi"]}, request_data=make_request_data( - metadata={"alice_wonderfence_app_id": "shared-app"} + metadata={"alice_wonderfence_app_id": "caller-supplied-app"} ), input_type="request", logging_obj=logging_obj, ) - # Reader's post_call: own name not in stash, must fall back to writer's. - await g_reader.apply_guardrail( - inputs={"texts": ["resp"]}, - request_data={"model": "gpt-4", "metadata": {}}, - input_type="response", - logging_obj=logging_obj, - ) - assert c_reader.evaluate_response.call_args.kwargs["app_id"] == "shared-app" + # Reader's post_call: no own stash, request_data resolves nothing, and it + # must NOT borrow the writer's stash -> fail closed. + with pytest.raises(HTTPException) as exc: + await g_reader.apply_guardrail( + inputs={"texts": ["resp"]}, + request_data={"model": "gpt-4", "metadata": {}}, + input_type="response", + logging_obj=logging_obj, + ) + assert exc.value.status_code == 500 + assert "alice_wonderfence_app_id" in exc.value.detail["exception"] + c_reader.evaluate_response.assert_not_awaited() @pytest.mark.asyncio