mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-07 02:59:05 +00:00
fix(guardrails): stop scan_raw_request warning from firing on every call
_process_guardrail_callback always returns a dict once a guardrail runs (mark_pre_call_hook_ran unconditionally stamps bookkeeping metadata), so comparing the result to non-None warned on every request even when the guardrail never touched the payload. Compare against a bookkeeping-only baseline instead, so only an actual content mutation triggers the warning.
This commit is contained in:
parent
1e1db8944c
commit
36b7584d53
2 changed files with 75 additions and 2 deletions
|
|
@ -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 "
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue