From 41f03b9c1908becbcad81c2c9451913b93f32772 Mon Sep 17 00:00:00 2001 From: Deepanshu Date: Thu, 27 Aug 2026 17:19:43 -0400 Subject: [PATCH] fix(guardrails): stop scan_raw_request from silently no-op'ing on real requests _independent_snapshot did one whole-dict copy.deepcopy and returned None on any failure. Every real proxy request carries data["litellm_logging_obj"] (a Logging instance nesting a live OTel span with a real lock) by the time pre_call_hook runs, which can never be deep-copied, so the snapshot failed on every real request and silently fell back to the live, unisolated data with no warning -- defeating the entire feature in production while every existing test (none of which set litellm_logging_obj) kept passing. Rework the helper to deep-copy each top-level key independently, falling back to the original reference only for the specific key that fails, same crash tolerance as safe_deep_copy's own per-key fallback. It never returns None now; only the keys scan_raw_request actually depends on (messages/ input, metadata/litellm_metadata) need to be genuinely independent. --- litellm/proxy/utils.py | 64 ++++++++++--------- .../utils/proxy_logging/test_pre_call_hook.py | 31 +++++++++ 2 files changed, 66 insertions(+), 29 deletions(-) diff --git a/litellm/proxy/utils.py b/litellm/proxy/utils.py index 0e5d418f5c5..42e94df6a64 100644 --- a/litellm/proxy/utils.py +++ b/litellm/proxy/utils.py @@ -456,18 +456,25 @@ 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 +) -> dict: # 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. + A copy of ``data`` whose top-level keys are deep-copied independently + where possible -- always attempted, regardless of + ``litellm.safe_memory_mode``. Unlike ``safe_deep_copy``, which can return + the *original* object outright under that mode (defeating any isolation + guarantee for every key, not just the ones that need it), this never + skips copying wholesale. - ``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. + Real proxy requests carry ``data["litellm_logging_obj"]`` (a ``Logging`` + instance nesting a live OTel span with a real lock) by the time + ``pre_call_hook`` runs, which can never be deep-copied -- and + scan_raw_request doesn't need it to be. Any individual key that fails to + deep-copy falls back to sharing its original reference, same crash + tolerance as ``safe_deep_copy``'s own per-key fallback; only the keys + that scan_raw_request actually reads for its block decision or writes + for bookkeeping (``messages``/``input``, ``metadata``/``litellm_metadata``) + need to be genuinely independent, and those are plain, cleanly-copyable + structures. """ sanitized: Final = { key: ( @@ -480,20 +487,23 @@ def _independent_snapshot( ) 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) + + def _copied_value(key: str, sanitized_value: object) -> object: + try: + copied_value: Final = copy.deepcopy(sanitized_value) + except Exception: # noqa: BLE001 # any unpicklable value falls back to the original reference for this key only + return data.get(key) + original_value: Final = data.get(key) if ( - isinstance(original_meta, dict) - and isinstance(copied_meta, dict) - and "litellm_parent_otel_span" in original_meta + key in ("metadata", "litellm_metadata") + and isinstance(copied_value, dict) + and isinstance(original_value, dict) + and "litellm_parent_otel_span" in original_value ): - copied_meta["litellm_parent_otel_span"] = original_meta["litellm_parent_otel_span"] - return copied + return {**copied_value, "litellm_parent_otel_span": original_value["litellm_parent_otel_span"]} + return copied_value + + return {key: _copied_value(key, value) for key, value in sanitized.items()} def _prompt_block_text(block: object) -> str: @@ -1459,10 +1469,9 @@ class ProxyLogging: """ scans_raw_request: Final = getattr(callback, "scan_raw_request", False) 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 = ( # mutable-ok: same request-payload shape as data + _independent_snapshot(raw_request_snapshot) if should_use_raw_snapshot else data ) - 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 @@ -1964,10 +1973,7 @@ class ProxyLogging: 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 + return _independent_snapshot(raw_request_snapshot) results: Final = await asyncio.gather( *( 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 6c1f43db567..e2b976264f6 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 @@ -612,6 +612,37 @@ async def test_scan_raw_request_snapshot_survives_unpicklable_metadata( assert out is not None +@pytest.mark.asyncio +async def test_scan_raw_request_isolation_survives_unpicklable_top_level_field( + proxy_logging, make_user_api_key_auth, monkeypatch +): + """ + Bugbot finding on BerriAI/litellm#34940: real proxy requests carry + data["litellm_logging_obj"] (a Logging instance nesting a live OTel span + with a real lock) by the time pre_call_hook runs -- a top-level field, not + inside metadata, so the otel-span placeholder substitution never touches + it. A whole-dict copy.deepcopy over the entire payload (the previous + _independent_snapshot) fails on that field on every real request and + silently falls back to the live, unisolated data with no warning, + defeating the entire feature in production even though every test above + passes (none of them set litellm_logging_obj). The isolation guarantee + (blocking order-independence) must hold even when such a field is + present. + """ + monkeypatch.setattr( + litellm, "callbacks", [_RedactingGuardrail(), _BlockOnSecretGuardrail(scan_raw_request=True)] + ) + proxy_logging.slack_alerting_instance = MagicMock(alerting=None) + data = _secret_request() + data["litellm_logging_obj"] = _Unpicklable() + with pytest.raises(HTTPException, match="blocked"): + await proxy_logging.pre_call_hook( + user_api_key_dict=make_user_api_key_auth(), + data=data, + call_type="completion", + ) + + @pytest.mark.asyncio async def test_scan_raw_request_snapshot_taken_before_pipelines( proxy_logging, make_user_api_key_auth, monkeypatch