mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-17 23:51:30 +00:00
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.
This commit is contained in:
parent
1b738d6189
commit
41f03b9c19
2 changed files with 66 additions and 29 deletions
|
|
@ -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(
|
||||
*(
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue