diff --git a/litellm/proxy/utils.py b/litellm/proxy/utils.py index 52f103d7c62..7d6c34ba697 100644 --- a/litellm/proxy/utils.py +++ b/litellm/proxy/utils.py @@ -91,7 +91,11 @@ from litellm.integrations.custom_logger import CustomLogger from litellm.integrations.prometheus import PrometheusLogger from litellm.integrations.SlackAlerting.slack_alerting import SlackAlerting from litellm.integrations.SlackAlerting.utils import _add_langfuse_trace_id_to_alert -from litellm.litellm_core_utils.core_helpers import coerce_token_limit, is_expected_client_error +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 from litellm.litellm_core_utils.safe_json_loads import safe_json_loads @@ -1407,11 +1411,14 @@ class ProxyLogging: declared block-only, same contract as ``run_in_parallel``: any data it returns is discarded, since applying its view on top of a stale snapshot would silently undo whatever a later guardrail already did to - the live request. + the live request. A guardrail that mutates content (e.g. PII masking) + should never set this flag -- if one does anyway, its returned + mutation is discarded and a warning is logged so the misconfiguration + is visible instead of silently forwarding unredacted content. """ scans_raw_request: Final = getattr(callback, "scan_raw_request", False) input_data: Final = ( - copy.deepcopy(raw_request_snapshot) if scans_raw_request and raw_request_snapshot is not None else data + safe_deep_copy(raw_request_snapshot) if scans_raw_request and raw_request_snapshot is not None else data ) result: Final = await self._process_guardrail_callback( callback=callback, @@ -1420,6 +1427,14 @@ class ProxyLogging: call_type=call_type, event_type=GuardrailEventHooks.pre_call, ) + if scans_raw_request and result is not None: + verbose_proxy_logger.warning( + "Guardrail '%s' has scan_raw_request=True but returned a modified payload; " + "scan_raw_request is for block-only guardrails and this mutation is being " + "discarded. Remove scan_raw_request from this guardrail's config if it needs " + "to mask/rewrite content.", + getattr(callback, "guardrail_name", None) or callback.__class__.__name__, + ) if result is None or scans_raw_request: return data return result @@ -1715,6 +1730,22 @@ class ProxyLogging: call_type=call_type, ) + # Snapshotted here, before _maybe_execute_pipelines or any guardrail in + # this hook has run, so a scan_raw_request guardrail's block/pass + # decision never depends on its position in the guardrails list or on + # a pipeline that runs ahead of it: an earlier guardrail (pipelined or + # 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. + 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 + try: # Execute guardrail pipelines before the normal callback loop data = await self._maybe_execute_pipelines( @@ -1749,14 +1780,6 @@ class ProxyLogging: and getattr(cb, "run_in_parallel", False) and not (cb.guardrail_name and cb.guardrail_name in pipeline_managed) ) - # Snapshotted once, before any guardrail in this hook has run, so a - # scan_raw_request guardrail's block/pass decision never depends on - # its position in the guardrails list: an earlier guardrail that - # masks/rewrites content (e.g. PII redaction) can't hide a violation - # from a later one that opted into scanning the original request. - raw_request_snapshot: Final[dict | None] = ( # mutable-ok: same request-payload shape as data - copy.deepcopy(data) if data is not None else None - ) deferred_route_exc: SensitiveDataRouteException | None = None for _callback in caps.resolved_callbacks: @@ -1878,7 +1901,7 @@ class ProxyLogging: self._process_guardrail_callback( callback=callback, data=( - copy.deepcopy(raw_request_snapshot) + safe_deep_copy(raw_request_snapshot) if getattr(callback, "scan_raw_request", False) and raw_request_snapshot is not None else data ), 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 9218b69f1a0..eff5fc8a9fc 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 @@ -576,3 +576,106 @@ async def test_scan_raw_request_guardrail_does_not_undo_later_masking( call_type="completion", ) assert "[REDACTED]" in out["messages"][0]["content"] + + +class _Unpicklable: + """Mirrors a real otel span: deepcopy always raises, matching what + safe_deep_copy exists to handle (see litellm_core_utils/core_helpers.py).""" + + def __deepcopy__(self, memo): + raise TypeError("cannot deepcopy this object") + + +@pytest.mark.asyncio +async def test_scan_raw_request_snapshot_survives_unpicklable_metadata( + proxy_logging, make_user_api_key_auth, monkeypatch +): + """ + Bugbot finding on BerriAI/litellm#34940: the scan_raw_request snapshot + used a bare copy.deepcopy, which raises on request payloads carrying + unpicklable objects (e.g. metadata["litellm_parent_otel_span"] when + tracing is enabled) -- failing every guarded request, not just ones + that actually use scan_raw_request. Must use safe_deep_copy instead. + """ + monkeypatch.setattr(litellm, "callbacks", [_BlockOnSecretGuardrail(scan_raw_request=True)]) + proxy_logging.slack_alerting_instance = MagicMock(alerting=None) + data = { + "messages": [{"role": "user", "content": "hello, nothing flagged here"}], + "model": "m", + "metadata": {"litellm_parent_otel_span": _Unpicklable()}, + } + out = await proxy_logging.pre_call_hook( + user_api_key_dict=make_user_api_key_auth(), + data=data, + call_type="completion", + ) + assert out is not None + + +@pytest.mark.asyncio +async def test_scan_raw_request_snapshot_taken_before_pipelines( + proxy_logging, make_user_api_key_auth, monkeypatch +): + """ + veria-ai finding on BerriAI/litellm#34940: the raw snapshot was taken + after _maybe_execute_pipelines ran, so a pipeline that masks content + ahead of a non-pipelined scan_raw_request guardrail could still hide + the violation from it. Simulates a pipeline-style rewrite by having + _maybe_execute_pipelines itself return redacted data, and confirms the + scan_raw_request blocker still sees the pre-pipeline raw content. + """ + + async def fake_pipelines(self, data, user_api_key_dict, call_type, event_hook): + for msg in data.get("messages", []): + if "SECRET" in msg.get("content", ""): + msg["content"] = msg["content"].replace("SECRET", "[REDACTED]") + return data + + monkeypatch.setattr(ProxyLogging, "_maybe_execute_pipelines", fake_pipelines) + monkeypatch.setattr(litellm, "callbacks", [_BlockOnSecretGuardrail(scan_raw_request=True)]) + proxy_logging.slack_alerting_instance = MagicMock(alerting=None) + with pytest.raises(HTTPException, match="blocked"): + await proxy_logging.pre_call_hook( + user_api_key_dict=make_user_api_key_auth(), + data=_secret_request(), + call_type="completion", + ) + + +@pytest.mark.asyncio +async def test_scan_raw_request_warns_when_guardrail_mutation_discarded( + proxy_logging, make_user_api_key_auth, monkeypatch +): + """ + veria-ai finding on BerriAI/litellm#34940: scan_raw_request is accepted + even for a guardrail that mutates the request (e.g. a masking + integration), silently discarding its redaction and forwarding raw + content. Config-time rejection isn't generically possible (no marker + exists for "this guardrail mutates"), so a loud runtime warning is the + mitigation: confirm it fires when a scan_raw_request guardrail returns + a modified payload. + """ + + class _MutatingScanner(_RedactingGuardrail): + def __init__(self, **kwargs): + super().__init__(**kwargs) + self.scan_raw_request = True + + async def async_pre_call_hook(self, user_api_key_dict, cache, data, call_type): # type: ignore[override] + for msg in data.get("messages", []): + msg["content"] = msg["content"].replace("SECRET-VALUE-123", "[REDACTED]") + return data + + from litellm.proxy import utils as proxy_utils_module + + mock_logger = MagicMock() + monkeypatch.setattr(proxy_utils_module, "verbose_proxy_logger", mock_logger) + monkeypatch.setattr(litellm, "callbacks", [_MutatingScanner()]) + proxy_logging.slack_alerting_instance = MagicMock(alerting=None) + await proxy_logging.pre_call_hook( + user_api_key_dict=make_user_api_key_auth(), + data=_secret_request(), + call_type="completion", + ) + mock_logger.warning.assert_called_once() + assert "scan_raw_request" in str(mock_logger.warning.call_args)