diff --git a/litellm/proxy/utils.py b/litellm/proxy/utils.py index 7d6c34ba697..2535b35e810 100644 --- a/litellm/proxy/utils.py +++ b/litellm/proxy/utils.py @@ -1420,6 +1420,16 @@ class ProxyLogging: input_data: Final = ( safe_deep_copy(raw_request_snapshot) if scans_raw_request and raw_request_snapshot 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 + if expected_if_unmutated is not None: + callback.mark_pre_call_hook_ran(expected_if_unmutated) result: Final = await self._process_guardrail_callback( callback=callback, data=input_data, @@ -1427,7 +1437,12 @@ class ProxyLogging: call_type=call_type, event_type=GuardrailEventHooks.pre_call, ) - if scans_raw_request and result is not None: + if ( + scans_raw_request + and expected_if_unmutated is not None + and result is not None + and result != expected_if_unmutated + ): 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 " 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 eff5fc8a9fc..353297acbb2 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 @@ -663,7 +663,7 @@ async def test_scan_raw_request_warns_when_guardrail_mutation_discarded( 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]") + msg["content"] = msg["content"].replace("SECRET", "[REDACTED]") return data from litellm.proxy import utils as proxy_utils_module @@ -679,3 +679,61 @@ async def test_scan_raw_request_warns_when_guardrail_mutation_discarded( ) mock_logger.warning.assert_called_once() assert "scan_raw_request" in str(mock_logger.warning.call_args) + + +@pytest.mark.asyncio +async def test_scan_raw_request_does_not_warn_when_guardrail_only_blocks( + proxy_logging, make_user_api_key_auth, monkeypatch +): + """ + Bugbot finding on BerriAI/litellm#34940: _process_guardrail_callback always + returns a dict once a guardrail actually runs (it only returns None when + should_run_guardrail is False), so checking `result is not None` is true on + every single request -- a correctly configured, non-mutating scan_raw_request + blocker (like _BlockOnSecretGuardrail here) would warn on every call, not just + when it actually mutates something. + """ + 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", [_BlockOnSecretGuardrail(scan_raw_request=True)]) + proxy_logging.slack_alerting_instance = MagicMock(alerting=None) + await proxy_logging.pre_call_hook( + user_api_key_dict=make_user_api_key_auth(), + data={"messages": [{"role": "user", "content": "nothing flagged here"}], "model": "m"}, + call_type="completion", + ) + mock_logger.warning.assert_not_called() + + +@pytest.mark.asyncio +async def test_scan_raw_request_warns_on_in_place_mutation_returning_none( + proxy_logging, make_user_api_key_auth, monkeypatch +): + """ + _RedactingGuardrail mirrors the common in-place-mutate-and-return-None + guardrail contract (e.g. real masking integrations). Detecting this case + correctly requires comparing dict *content*, not object identity: the + mutated dict is still the exact same object reference the guardrail was + given, so an identity check (`result is input_data`) would wrongly say + nothing changed. + """ + from litellm.proxy import utils as proxy_utils_module + + class _ScanningRedactor(_RedactingGuardrail): + def __init__(self, **kwargs): + super().__init__(**kwargs) + self.scan_raw_request = True + + mock_logger = MagicMock() + monkeypatch.setattr(proxy_utils_module, "verbose_proxy_logger", mock_logger) + monkeypatch.setattr(litellm, "callbacks", [_ScanningRedactor()]) + 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)