fix(proxy): run pre-call guardrails on batch input file uploads - #37519
Conversation
Greptile SummaryThis PR scans batch-upload records using configured pre-call guardrails before sending files to providers
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains
|
| Filename | Overview |
|---|---|
| litellm/proxy/openai_files_endpoints/batch_guardrails.py | Implements bounded batch-record scanning, replacement detection, failure selection, and file rewinding |
| litellm/proxy/openai_files_endpoints/files_endpoints.py | Invokes batch scanning before provider upload when an applicable pre-call guardrail is configured |
| litellm/proxy/utils.py | Adds guardrail availability detection and a guardrail-only pre-call dispatch path |
| tests/test_litellm/proxy/openai_files_endpoint/test_batch_guardrails.py | Covers scanning, replacement dictionaries, metadata isolation, classification, exceptions, and rewind behavior |
| tests/test_litellm/proxy/openai_files_endpoint/test_files_endpoint.py | Verifies batch scanning is integrated into file creation without affecting unrelated uploads |
| tests/test_litellm/proxy/utils/proxy_logging/test_pre_call_hook.py | Verifies guardrail-only dispatch excludes unrelated callbacks while preserving guardrail and pipeline behavior |
Reviews (9): Last reviewed commit: "fix(proxy): give each batch record its o..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
|
|
||
| def _iter_records(source: BinaryIO) -> Iterator[_ParsedRecord | UnparseableRecord]: | ||
| """Yield one record per line. JSONL is one object per line, so a line that does not parse is bad.""" | ||
| for line_number, raw_line in enumerate(source, start=1): |
There was a problem hiding this comment.
Low: Unbounded batch records can exhaust proxy resources
An authenticated user can upload a JSONL file with one extremely large line; iterating the file reads that whole line into memory, after which decoding, json.loads, deepcopy, and fingerprint serialization create additional copies on the async request worker. The endpoint explicitly supports gigabyte-scale files, while the global request-size limit is optional, so _SCAN_WINDOW does not prevent memory exhaustion or prolonged event-loop blocking. Read each line with a byte limit and reject oversized records before decoding or parsing; CPU-heavy parsing and comparison should also run outside the event loop.
PR overviewThis PR updates the proxy’s batch file upload flow to run pre-call guardrails against JSONL input records before processing them. One issue has been addressed, but an authenticated user can still submit an oversized JSONL record that causes excessive memory use and prolonged request-worker blocking. This could degrade proxy availability unless per-record size limits are enforced before decoding and parsing. Open issues (1)
Fixed/addressed: 1 · PR risk: 5/10 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Fixed: the dict a guardrail returns is now what gets compared. The unscannable-url rejection is deliberate and documented under Caveats. @greptileai please review head 85ff920 |
| if ( | ||
| purpose == "batch" | ||
| and not isinstance(file_source, bytes) | ||
| and proxy_logging_obj.has_pre_call_guardrails(request_metadata) | ||
| ): | ||
| scan_failure: Final = await scan_batch_input_file( | ||
| file_source=file_source, | ||
| request_metadata=request_metadata, | ||
| user_api_key_dict=user_api_key_dict, | ||
| proxy_logging_obj=proxy_logging_obj, | ||
| ) | ||
| if scan_failure is not None: | ||
| raise_public(scan_failure) |
There was a problem hiding this comment.
🟨 Batch guardrail scan is only applied when purpose=batch, letting a caller relabel the upload to skip it
The new pre-call guardrail scan in litellm/proxy/openai_files_endpoints/files_endpoints.py:465-477 is gated on the caller-supplied purpose form field being exactly "batch". purpose is untrusted client input, and nothing revalidates it when the resulting file id is later handed to POST /v1/batches. On the LiteLLM-managed files, Bedrock and Vertex paths (where LiteLLM stores the JSONL itself rather than handing it to OpenAI, which independently rejects .jsonl under purpose=assistants), a caller can upload the identical JSONL under a different purpose to bypass the guardrail scan entirely and then run it as a batch. The added test case with purpose="assistants" explicitly asserts a 200 for content that trips the guardrail, confirming the bypass path.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 85ff920. Configure here.
|
Validated live against real OpenAI on two proxies: rejections fire only when a pre-call guardrail exists, clean files still upload and create batches. Added 19b908d so a dropped null-valued key no longer reads as unchanged Guardrail-affected record rejected, nothing stored
Clean upload, real batch created, blocking guardrail error propagates
Backward compat: no guardrails configured, and non-batch purposes
|
| # `metadata` is dropped rather than diffed: guardrail dispatch writes its bookkeeping into it | ||
| # whenever the payload has one, and a record's own metadata is not scanned content on the | ||
| # online path either. | ||
| _INJECTED_KEYS: Final = frozenset({_SCAN_METADATA_KEY, "metadata"}) | ||
|
|
||
| # Only what guardrail dispatch reads. The parent OTel span is deliberately left out: parenting one | ||
| # guardrail span per record would put tens of thousands of spans on a single upload's trace. |
There was a problem hiding this comment.
🟡 New code adds explanatory prose comments that the project's coding rules disallow
The new batch-scanning module carries several multi-line prose comments explaining logic (for example at litellm/proxy/openai_files_endpoints/batch_guardrails.py:29-32), which the repository's rules only permit for genuinely complex business logic, tool directives, or TODOs.
Impact: The change violates an explicit repository convention meant to keep verbose explanatory comments out of the codebase.
Which comments and which rule
CLAUDE.md (mandatory via AGENTS.md) opens with "Do not write comments unless they are any of: absolutely necessary to explain some very complex business logic ... used as an input for tools ... a TODO or FIXME". The new file's non-tool comments include the injected-keys rationale (batch_guardrails.py:29-31), the OTel-span rationale (batch_guardrails.py:34-35), the Bedrock-classifier mirror note (batch_guardrails.py:57-58), and the replacement-dict note (batch_guardrails.py:226-227); litellm/proxy/openai_files_endpoints/files_endpoints.py:463 adds another. The # mutable-ok: suppressions are allowed tool comments and are not part of this finding. Most of this prose duplicates what the adjacent docstrings already state and should be folded into them or dropped.
Was this helpful? React with 👍 or 👎 to provide feedback.
45b549c to
abfb3d7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| scan_input: Final[dict] = copy.deepcopy(body) # mutable-ok: pre_call_hook mutates the dict it is given | ||
| scan_input.pop("metadata", None) | ||
| scan_input[_SCAN_METADATA_KEY] = dict(scan_metadata) # mutable-ok: guardrails write bookkeeping here | ||
|
|
||
| returned: Final = await proxy_logging_obj.pre_call_hook( | ||
| user_api_key_dict=user_api_key_dict, | ||
| data=scan_input, | ||
| call_type=call_type, | ||
| guardrails_only=True, | ||
| ) |
There was a problem hiding this comment.
🟨 Guardrail selection inside a batch record body can suppress key/team-configured guardrails during the scan
Each batch record's raw body is handed straight to guardrail dispatch as the request payload (litellm/proxy/openai_files_endpoints/batch_guardrails.py:214-223). Guardrail selection reads a root-level guardrails key from the payload first (litellm/integrations/custom_guardrail.py:558-575, if "guardrails" in data: return data["guardrails"]), before falling back to the proxy-injected metadata. On the online request path this root key is never trusted verbatim: move_guardrails_to_metadata (litellm/proxy/litellm_pre_call_utils.py:2675) pops it and merges it with key/team/policy configuration. The scan skips that step, so an uploader can put "guardrails": [] (or an arbitrary dynamic-guardrail dict) inside each record body and make non-default_on guardrails configured on the key/team fail should_run_guardrail for that record, letting content through the new pre-call scan unchecked. default_on=True guardrails are unaffected because that branch is evaluated before the requested list.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Rebased onto staging and dropped the checks the new batch upload validation already makes. @greptileai please review head abfb3d7 |
|
bugbot run |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@greptileai please re-review, new head pushed with the latest round of fixes |
|
bugbot run |
tin-berri
left a comment
tin-berri
left a comment
There was a problem hiding this comment.
done — #37519 approved. Real security gap closed: guardrails previously never ran on batch file uploads (/v1/files, purpose=batch), so a caller could bypass an org's PII/content guardrail entirely by routing the same content through batch instead of chat completions. Reviewed the dispatch wiring directly: scanning is correctly gated on purpose == "batch" and proxy_logging_obj.has_pre_call_guardrails(...), so it's a true no-op when no pre-call guardrail is configured (matches the PR's backward-compat claim), and fails closed — any guardrail that changes a record rejects the whole upload with a 400 naming the offending line, nothing is stored at the provider. Confirmed file_source is never bytes on the batch path (only non-batch purposes hit that branch), so the not isinstance(file_source, bytes) check isn't an accidental bypass. Devin's review includes live end-to-end validation against real OpenAI (dirty batch rejected/nothing stored, clean batch created normally, no-guardrail backward compat) rather than just mocked assertions.
One open item, flagged by veria-ai and worth a fast follow-up rather than blocking: _iter_records in batch_guardrails.py parses each JSONL line with no per-record size cap before json.loads, so an authenticated caller could submit an oversized line to spike memory/CPU on the request worker (low severity, requires a valid key, no data exposure). CI green.
|
Deliberate. Staging's batch file validation already rejects the whole upload for a line it cannot process, so this matches the behavior one function earlier in the same endpoint. Per-record handling covers policy violations, not records we cannot read. |
|
@greptileai review latest head, rescore based on the above comment: |
|
@greptileai please re-review at 8b2f6d6 |
|
bugbot run |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Parallel guardrails are block-only by contract: pre_call_hook discards their returned data online too, so batch matches the online path exactly |
|
@greptileai please re-review, new head |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 142045e. Configure here.
POST /v1/files with purpose=batch was the only route in files_endpoints that never reached pre_call_hook, so guardrails did not see batch content at all and records reached the provider unscanned. Stream the uploaded JSONL a record at a time and run each record's body through the existing pre_call_hook dispatch under the call type its url maps to, so guardrail resolution, key and team config, and the per-endpoint translations are reused rather than reimplemented. The hook gains a guardrails_only mode for this, since the same callback loop also drives rate limiters, budget hooks, prompt templates and hanging-request alerting, none of which should fire once per record. A guardrail that blocks raises its own exception, which propagates untouched so its status code survives. A record a guardrail would rewrite, a record that cannot be parsed, and a record whose url cannot be scanned all reject the upload, since silently skipping any of them is the bypass this is meant to close. Per-record redaction lands separately. The scan only runs when a guardrail that actually runs pre_call, or a guardrail pipeline, is configured, so deployments without one are byte for byte unchanged.
…t was given async_pre_call_hook may return a replacement dict instead of mutating its input, and process_pre_call_hook_response then makes that replacement the request. The scan only inspected the dict it passed in, so a guardrail that redacts by returning a copy was treated as a no-op and its record uploaded unchanged.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… url is unfamiliar The scanner only accepted five exact urls, but callers write that field by hand and the provider transformers are far more permissive: bedrock treats any non-empty url as chat and vertex strips query strings and trailing slashes. Uploads that work today would have started failing the moment a pre-call guardrail was configured. Normalize the url before lookup and fall back to the body shape when it is unfamiliar, so a record we can still read is a record we still scan. Only a body with no messages, prompt or input is now refused, and the error says so instead of listing urls that were never the whole set. Also pins the default side of the guardrails_only gate: the hanging-request alert and prompt templating are asserted to still fire when the flag is absent.
…ready makes check_batch_file_upload now runs first and rejects a line that does not parse, a line that is not an object, and a line missing custom_id, method, url or body, so the guardrail scan can rely on all four. Its own parse handling was unreachable through the endpoint and is gone, along with the tests for it. What is left is the case that validation does not cover, a body whose value is not an object, since it only checks that the key is present.
… the whole url A record naming its route in full, which is how callers actually write batch files, matched no known route, so it fell through to the body shape. A Responses record carries `input`, and that reads as an embedding, so the record was scanned as the wrong call type and any guardrail scoped to chat or Responses skipped it while the upload was accepted. Chat records survived only because their body shape happens to map back to the same call type. The url is now reduced to its path before matching. Guardrails that pick their policy from a request header, such as noma choosing an application id, saw no headers at all during the scan and fell back to a default, so a batch record could be evaluated under a different policy than the same content sent online. The sanitized headers the proxy already stores in request metadata now travel with the scan. Also drops the bare `dict` annotation, the unreachable non-dict branch on the guardrail chain's own return, and the type alias that was missing its `TypeAlias`, which together were failing the lint gate.
The narrowed metadata was handed to every record as a shallow copy, so `headers` and `tags` stayed shared with the upload request and with the other records in the same window. A guardrail that writes into one of those in place, which several do to record their own bookkeeping, would have its write show up in every record scanned after it and in the request itself. The narrowing already removed the values that cannot be copied, so each record now gets a deep copy.
|
@greptileai please re-review, new head |
142045e to
d2d124e
Compare
|
Rebased onto current staging, which is where the missing test-quality gate script came from |
|
@greptileai please re-review, rebased head |
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d2d124e. Configure here.
| # `metadata` is dropped rather than diffed: guardrail dispatch writes its bookkeeping into it | ||
| # whenever the payload has one, and a record's own metadata is not scanned content on the | ||
| # online path either. | ||
| _INJECTED_KEYS: Final = frozenset({_SCAN_METADATA_KEY, "metadata"}) |
There was a problem hiding this comment.
Scan metadata uses wrong request key
High Severity
Each record is scanned as chat, completions, embeddings, or Responses, but request headers, tags, and guardrail lists are injected only under litellm_metadata. Online those call types put the same fields on metadata, and several pre-call guardrails (noma, AIM, Cisco, Akto) read only metadata. Header-selected policies such as x-noma-application-id therefore miss the upload headers and fall back to a default application, so batch records can be scanned under the wrong policy while the file is still accepted.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d2d124e. Configure here.
2 participants
TLDR
Problem this solves:
How it solves it:
User Flow
Before: a platform team with a PII guardrail turned on for every request finds that batch jobs bypass it entirely
purpose=batch, where one row's prompt contains a customer email addressfile-9sR7kQopg8iZ6bGpp3Yk9GAfter: the same upload is refused, and the response names the row that tripped
purpose=batchA guardrail changed batch input line 2 (custom_id row-2)Relevant issues
Linear ticket
Resolves LIT-2026
Pre-Submission checklist
uv run pytest tests/test_litellm/<your_test_file>.py -vScreenshots / Proof of Fix
Shared setup.
config.yaml:custom_guardrail.myCustomGuardrailis the example guardrail shipped inlitellm/proxy/example_config_yaml/custom_guardrail.py, which masks the literallitellmin any message. Uploads go to the real OpenAI files API.batch_input.jsonlhas three records, and line 2 trips the guardrail:clean.jsonlholds two records the guardrail does not touch.unreadable.jsonlholds one record whoseurlis/v1/rerankand whose body carries onlymodelanddocuments, so there is no message, prompt or input to scan.risk_input.jsonlholds one record naming its route in full,https://api.openai.com/v1/responses, with a Responses body carryinginput, and one ordinary chat record. It is uploaded with-H "x-noma-application-id: app-1", the header noma reads to pick its application. A probe guardrail logs the call type and the headers it was handed for each record, which is what cases 4 and 5 read.Before (a0f367f)
1. clean batch file
curl -sS http://localhost:4000/v1/files -H "Authorization: Bearer sk-..." -F purpose=batch -F file=@clean.jsonl{"id":"file-8ZKvsc1GV4w62AqvHVf2dD","bytes":372,"object":"file","purpose":"batch","status":"processed"}2. batch file whose line 2 trips the guardrail
curl -sS http://localhost:4000/v1/files -H "Authorization: Bearer sk-..." -F purpose=batch -F file=@batch_input.jsonlHTTP 200and{"id":"file-9sR7kQopg8iZ6bGpp3Yk9G","bytes":590,"object":"file","purpose":"batch","status":"processed"}curl -sS http://localhost:4000/v1/files/file-9sR7kQopg8iZ6bGpp3Yk9G/content -H "Authorization: Bearer sk-..."{"custom_id": "row-2", ... "content": "this row mentions litellm and should be redacted"}3. record whose body has nothing a guardrail can read
curl -sS http://localhost:4000/v1/files -H "Authorization: Bearer sk-..." -F purpose=batch -F file=@unreadable.jsonlHTTP 200and{"id":"file-4QZuAkQgkPP465TWiHUHtP","bytes":117,"object":"file","purpose":"batch","status":"processed"}4. record naming its route in full
risk_input.jsonlas above5. guardrail that picks its policy from a request header
risk_input.jsonlas above, with-H "x-noma-application-id: app-1"After (8b2f6d6)
1. clean batch file
curl -sS http://localhost:4000/v1/files -H "Authorization: Bearer sk-..." -F purpose=batch -F file=@clean.jsonl{"id":"file-PGmbVim7grkmM24vYia2nr","bytes":372,"object":"file","purpose":"batch","status":"processed"}2. batch file whose line 2 trips the guardrail
curl -sS http://localhost:4000/v1/files -H "Authorization: Bearer sk-..." -F purpose=batch -F file=@batch_input.jsonlHTTP 400and{"error":{"message":"{'error': 'A guardrail changed batch input line 2 (custom_id row-2). Per-record redaction is not enabled, so the file was rejected rather than modified'}","code":"400"}}3. record whose body has nothing a guardrail can read
curl -sS http://localhost:4000/v1/files -H "Authorization: Bearer sk-..." -F purpose=batch -F file=@unreadable.jsonlHTTP 400and{"error":{"message":"{'error': 'Batch input line 1 (custom_id u-1) targets /v1/rerank and its body has no messages, prompt or input, so guardrails cannot read it. Give the record a chat, completion, embedding, responses or messages body'}","code":"400"}}4. record naming its route in full
risk_input.jsonlas aboveHTTP 200, and the probe records each call type:aresponses. Run against the same file atabfb3d7ddc, before the url was reduced to its path, it read asaembedding, so a guardrail scoped to chat or Responses would have skipped it while the upload was accepted5. guardrail that picks its policy from a request header
risk_input.jsonlas above, with-H "x-noma-application-id: app-1"abfb3d7ddcthe same run loggedheaders=NoneType
🐛 Bug Fix
Caveats (if any)
Sits on top of the batch upload validation already on staging, which rejects a line that does not parse, a line that is not an object, and a line missing
custom_id,method,urlorbody. The guardrail scan relies on all four rather than re-checking them, so case 3 above is the remaining gap it covers: validation checks thatbodyis present, not that its value is an object with readable content.Only
purpose=batchuploads are scanned, andpurposecomes from the caller. Against OpenAI this cannot be used to skip the scan, since the provider rejects a.jsonlupload underpurpose=assistantswithInvalid extension jsonl, verified live. It is unverified for the managed files, Bedrock and Vertex paths, where LiteLLM stores the file itself and nothing revalidatespurposeat batch creation. Worth closing on those paths as follow-up work.Final Attestation
Note
Medium Risk
Touches proxy guardrail dispatch and
/v1/filesbatch uploads, so a scan bug could reject valid jobs or still miss content. Behavior is reject-only and skipped when no pre-call guardrails are configured.Overview
Closes a silent bypass where
purpose=batchfile uploads skipped pre-call guardrails entirely.When pre-call guardrails (or pipelines) are configured, each JSONL record is scanned through
pre_call_hookwith a newguardrails_onlypath so rate limits, budgets, prompt templates, and hanging-request alerts do not fire per row. Records are scanned in windows of 32; the file is rewound and never rewritten. A blocking guardrail exception is re-raised as-is; redaction or an unreadable body (no messages/prompt/input) returns 400 naming the line/custom_id. Call type comes from the record URL (including absolute URLs) with body-shape fallback.Request metadata is narrowed to what guardrails actually read (headers, tags, guardrail config) so OTel spans and secrets are not copied per record. Scanning is skipped when no pre-call guardrail would run.
Reviewed by Cursor Bugbot for commit d2d124e. Bugbot is set up for automated code reviews on this repo. Configure here.