From f8849178fc4692f3121899b93af8d4e94e11da59 Mon Sep 17 00:00:00 2001 From: Deepanshu Date: Thu, 27 Aug 2026 16:11:16 -0400 Subject: [PATCH] fix(guardrails): make scan_raw_request snapshots independent of safe_memory_mode safe_deep_copy can return the original object under litellm.safe_memory_mode, or alias a per-key reference on copy failure. Under that mode, the scan_raw_request comparison baseline aliased raw_request_snapshot (and therefore the live request), letting mark_pre_call_hook_ran write a premature execution marker that a deployment-level guardrail sharing the same name would read as "already ran" and skip. Also affected the feature's core isolation guarantee: input_data itself could alias the live request under the same mode. Replace every scan_raw_request snapshot with _independent_snapshot, which never returns an alias, only a genuine copy or None. --- litellm/proxy/utils.py | 84 +++++++++++++++---- .../utils/proxy_logging/test_pre_call_hook.py | 32 +++++++ 2 files changed, 101 insertions(+), 15 deletions(-) diff --git a/litellm/proxy/utils.py b/litellm/proxy/utils.py index 2535b35e810..0e5d418f5c5 100644 --- a/litellm/proxy/utils.py +++ b/litellm/proxy/utils.py @@ -94,7 +94,6 @@ from litellm.integrations.SlackAlerting.utils import _add_langfuse_trace_id_to_a from litellm.litellm_core_utils.core_helpers import ( coerce_token_limit, is_expected_client_error, - safe_deep_copy, ) from litellm.litellm_core_utils.litellm_logging import Logging from litellm.litellm_core_utils.safe_json_dumps import safe_dumps @@ -455,6 +454,48 @@ def _pipeline_managed_guardrail_names(data: Mapping[str, object]) -> frozenset[s ) +def _independent_snapshot( + data: dict, # mutable-ok: same request-payload shape as every other guardrail snapshot in this file +) -> dict | None: # mutable-ok: same request-payload shape as every other guardrail snapshot in this file + """ + A guaranteed-independent copy of ``data``, or None if one couldn't be + made -- never the original object or an aliased sub-value. + + ``safe_deep_copy`` is allowed to return the original object outright + under ``litellm.safe_memory_mode``, and to fall back to the original + reference for any individual key that fails to deep-copy. Both are fine + for its usual callers, but scan_raw_request's isolation guarantee (a + guardrail's raw-request view must never be mutable-shared with the live + request or with another guardrail's view) depends on the copy actually + being independent, so it can't reuse that helper. + """ + sanitized: Final = { + key: ( + { + inner_key: ("placeholder" if inner_key == "litellm_parent_otel_span" else inner_value) + for inner_key, inner_value in value.items() + } + if key in ("metadata", "litellm_metadata") and isinstance(value, dict) + else value + ) + for key, value in data.items() + } + try: + copied: Final = copy.deepcopy(sanitized) + except Exception: # noqa: BLE001 # any unpicklable value anywhere in the payload should degrade to None, not crash + return None + for meta_key in ("metadata", "litellm_metadata"): + original_meta = data.get(meta_key) + copied_meta = copied.get(meta_key) + if ( + isinstance(original_meta, dict) + and isinstance(copied_meta, dict) + and "litellm_parent_otel_span" in original_meta + ): + copied_meta["litellm_parent_otel_span"] = original_meta["litellm_parent_otel_span"] + return copied + + def _prompt_block_text(block: object) -> str: if isinstance(block, str): return block @@ -1417,17 +1458,23 @@ class ProxyLogging: is visible instead of silently forwarding unredacted content. """ scans_raw_request: Final = getattr(callback, "scan_raw_request", False) - input_data: Final = ( - safe_deep_copy(raw_request_snapshot) if scans_raw_request and raw_request_snapshot is not None else data + should_use_raw_snapshot: Final = scans_raw_request and raw_request_snapshot is not None + raw_input_copy: Final[dict | None] = ( # mutable-ok: same request-payload shape as data + _independent_snapshot(raw_request_snapshot) if should_use_raw_snapshot else None ) + input_data: Final = raw_input_copy if raw_input_copy is not None else data # _process_guardrail_callback always calls mark_pre_call_hook_ran on a # successful run, which unconditionally stamps bookkeeping metadata onto # the dict regardless of whether the guardrail's own hook mutated # anything -- so comparing `result` straight against `input_data` would # warn on every single scan_raw_request call. Apply that same stamp to a - # throwaway copy first so the comparison isolates the guardrail's own - # content mutation from this bookkeeping noise. - expected_if_unmutated: Final[dict | None] = safe_deep_copy(input_data) if scans_raw_request else None + # throwaway, guaranteed-independent copy first (never the live request or + # raw_request_snapshot itself) so the comparison isolates the guardrail's + # own content mutation from this bookkeeping noise without risking a + # premature marker write into shared state. + expected_if_unmutated: Final[dict | None] = ( # mutable-ok: same request-payload shape as data + _independent_snapshot(input_data) if scans_raw_request else None + ) if expected_if_unmutated is not None: callback.mark_pre_call_hook_ran(expected_if_unmutated) result: Final = await self._process_guardrail_callback( @@ -1752,14 +1799,16 @@ class ProxyLogging: # not) that masks/rewrites content can't hide a violation from a later # one that opted into scanning the original request. Only computed # when at least one registered guardrail actually opted in, and via - # safe_deep_copy (not a bare deepcopy) since the payload commonly - # carries unpicklable objects (e.g. an otel span in metadata) that - # would otherwise raise here on every guarded request. + # _independent_snapshot (not safe_deep_copy) since this isolation + # guarantee must hold even under litellm.safe_memory_mode, which + # otherwise makes deep copies return the original object. needs_raw_request_snapshot: Final = any( isinstance(cb, CustomGuardrail) and getattr(cb, "scan_raw_request", False) for cb in ProxyLogging._callback_capabilities().resolved_callbacks ) - raw_request_snapshot: Final[dict | None] = safe_deep_copy(data) if needs_raw_request_snapshot else None + raw_request_snapshot: Final[dict | None] = ( # mutable-ok: same request-payload shape as data + _independent_snapshot(data) if needs_raw_request_snapshot else None + ) try: # Execute guardrail pipelines before the normal callback loop @@ -1911,15 +1960,20 @@ class ProxyLogging: sequential branch does: its block decision must not depend on what a sequential guardrail already masked or rewrote. """ + + def _input_for(callback: CustomGuardrail) -> dict: # mutable-ok: same request-payload shape as data + if not getattr(callback, "scan_raw_request", False) or raw_request_snapshot is None: + return data + snapshot_copy: Final[dict | None] = _independent_snapshot( # mutable-ok: same request-payload shape + raw_request_snapshot + ) + return snapshot_copy if snapshot_copy is not None else data + results: Final = await asyncio.gather( *( self._process_guardrail_callback( callback=callback, - data=( - safe_deep_copy(raw_request_snapshot) - if getattr(callback, "scan_raw_request", False) and raw_request_snapshot is not None - else data - ), + data=_input_for(callback), user_api_key_dict=user_api_key_dict, call_type=call_type, event_type=GuardrailEventHooks.pre_call, diff --git a/tests/test_litellm/proxy/utils/proxy_logging/test_pre_call_hook.py b/tests/test_litellm/proxy/utils/proxy_logging/test_pre_call_hook.py index 353297acbb2..6c1f43db567 100644 --- a/tests/test_litellm/proxy/utils/proxy_logging/test_pre_call_hook.py +++ b/tests/test_litellm/proxy/utils/proxy_logging/test_pre_call_hook.py @@ -681,6 +681,38 @@ async def test_scan_raw_request_warns_when_guardrail_mutation_discarded( assert "scan_raw_request" in str(mock_logger.warning.call_args) +@pytest.mark.asyncio +async def test_scan_raw_request_baseline_does_not_leak_marker_under_safe_memory_mode( + proxy_logging, make_user_api_key_auth, monkeypatch +): + """ + veria-ai finding on BerriAI/litellm#34940: safe_deep_copy returns the + original object unchanged when litellm.safe_memory_mode is True, so + calling the mutating mark_pre_call_hook_ran on the "expected baseline" + copy actually mutates the shared raw_request_snapshot -- writing this + guardrail's execution marker into metadata even when should_run_guardrail + says the guardrail should be skipped for this event. A deployment-level + guardrail sharing the same guardrail_name would then see the marker via + _pre_call_hook_already_ran and skip real inspection, a security bypass. + """ + monkeypatch.setattr(litellm, "safe_memory_mode", True) + + class _SkippedScanner(_BlockOnSecretGuardrail): + def __init__(self, **kwargs): + kwargs["default_on"] = False + super().__init__(scan_raw_request=True, **kwargs) + + callback = _SkippedScanner() + monkeypatch.setattr(litellm, "callbacks", [callback]) + proxy_logging.slack_alerting_instance = MagicMock(alerting=None) + out = await proxy_logging.pre_call_hook( + user_api_key_dict=make_user_api_key_auth(), + data=_secret_request(), + call_type="completion", + ) + assert callback._pre_call_hook_already_ran(out) is False + + @pytest.mark.asyncio async def test_scan_raw_request_does_not_warn_when_guardrail_only_blocks( proxy_logging, make_user_api_key_auth, monkeypatch