mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-19 00:01:29 +00:00
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.
This commit is contained in:
parent
36b7584d53
commit
f8849178fc
2 changed files with 101 additions and 15 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue