fix(presidio): reverting stream SSE output incrementally instead of buffering the whole response - #31756
Conversation
…fering t…" This reverts commit 94936a3.
|
Issue reopened: LIT-3222 Presidio guardrails buffer SSE streaming responses |
Greptile SummaryThis PR reverts #31503, which had replaced the full-response-buffering approach in Presidio's streaming PII guardrail with an incremental per-sentence masking strategy. The revert restores buffering all
Confidence Score: 2/5Not safe to merge as-is: a guardrail blocking exception raised during streaming masking is swallowed and unmasked content is forwarded to the client. Both _stream_apply_output_masking and _stream_pii_unmasking now wrap _process_response_for_pii in a bare except Exception that logs and yields buffered chunks on any error. BlockedPiiEntityError and GuardrailRaisedException fall into that handler, so a streaming request that should be blocked instead delivers unmasked content silently. The deleted test_mask_streaming_propagates_guardrail_interventions test was the only CI guard for this contract. Both changed files need attention: presidio.py for the exception-handling regression in the two streaming methods, and test_presidio.py for the missing guardrail-blocking propagation coverage.
|
| Filename | Overview |
|---|---|
| litellm/proxy/guardrails/guardrail_hooks/presidio.py | Reverts incremental streaming to full-response buffering; introduces a broad exception handler that swallows guardrail blocking exceptions (BlockedPiiEntityError / GuardrailRaisedException), causing unmasked PII to pass through to the client instead of blocking the request. |
| tests/test_litellm/proxy/guardrails/guardrail_hooks/test_presidio.py | Removes 678 lines of incremental-streaming tests including the only test that verified guardrail blocking exceptions propagate during streaming; the deletion leaves the regression undetected. |
Comments Outside Diff (1)
-
tests/test_litellm/proxy/guardrails/guardrail_hooks/test_presidio.py, line 612 (link)Removal of guardrail-intervention propagation test
BlockedPiiEntityErroris no longer imported here becausetest_mask_streaming_propagates_guardrail_interventionswas deleted as part of this revert. That test was the only coverage verifying thatBlockedPiiEntityErrorraised by Presidio during streaming causes the request to be blocked rather than passing unmasked content through. Without it, the regression described in_stream_apply_output_maskinghas no automated detector.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "Revert "fix(presidio): stream SSE output..." | Re-trigger Greptile
| except Exception as e: | ||
| if self._is_guardrail_intervention(e): | ||
| raise | ||
| verbose_proxy_logger.error(f"Error masking streaming PII output: {str(e)}") | ||
| for chunk in all_chunks: | ||
| yield chunk |
There was a problem hiding this comment.
Guardrail blocking exceptions silently swallowed
The broad except Exception catch covers _process_response_for_pii (line 1112), which raises BlockedPiiEntityError or GuardrailRaisedException when the guardrail's action is BLOCK. With this handler, those exceptions are logged and discarded, and the original unmasked chunks buffered in all_chunks are then yielded directly to the client. The removed test test_mask_streaming_propagates_guardrail_interventions specifically validated this contract. A minimal fix is to check self._is_guardrail_intervention(e) and re-raise before falling back to passthrough.
|
|
||
| await self._process_response_for_pii( | ||
| response=assembled_model_response, | ||
| request_data=request_data, |
There was a problem hiding this comment.
Same guardrail-intervention bypass in unmasking path
_stream_pii_unmasking has the same broad except Exception wrapping _process_response_for_pii (mode="unmask"). A GuardrailRaisedException raised there is caught, logged, and the loop exits via for chunk in remaining_chunks: yield chunk, silently passing through content without re-raising. Guardrail interventions from the unmask path should propagate just like those from the mask path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Pull request was closed
| mock_response_stream = convert_model_response_to_streaming(assembled_model_response) | ||
| yield mock_response_stream | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
Medium: Blocked PII is replayed on streaming masking errors
A user can now receive streamed content that Presidio was configured to block: _process_response_for_pii raises BlockedPiiEntityError/GuardrailRaisedException, but this catch logs the error and replays all_chunks unmodified. Re-raise guardrail interventions here and avoid falling back to the original chunks when output masking fails.
| if passthrough_due_to_unknown_stream_shape: | ||
| yield chunk | ||
| else: | ||
| all_chunks.append(chunk) |
There was a problem hiding this comment.
Medium: Unbounded buffering of streaming output
An authenticated user can keep the proxy accumulating every ModelResponseStream chunk in memory by requesting a large streaming completion, because this path appends until the upstream stream ends before masking or yielding. Keep a bounded buffer and process incrementally, or enforce a hard cap that fails closed instead of holding the entire streamed response.
PR overviewThis PR changes the Presidio guardrail streaming response path for SSE/model stream output in the LiteLLM proxy, aiming to handle streamed chunks incrementally instead of waiting for the full response before returning data. There are still two open issues in the streaming Presidio handling. The current path can fall back to replaying unmodified streamed chunks when masking or blocking fails, which can expose content that the guardrail was configured to stop. It also still buffers streamed output without a hard bound, allowing an authenticated caller to drive excessive memory use with a large streaming response. No issues have been fixed or addressed yet, so the PR still carries meaningful security risk. Open issues (2)
Fixed/addressed: 0 · PR risk: 7/10 |
Reverts #31503