fix(proxy): preserve multipart/form-data boundary in passthrough endpoints - #23338
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes multipart file upload failures in passthrough endpoints by skipping body parsing for Key changes and concerns:
Confidence Score: 1/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/pass_through_endpoints/pass_through_endpoints.py | Core fix for multipart passthrough: skips body parsing for multipart requests and removes custom_body FastAPI parameter, but the approach is fragile (relies on _parsed_body being empty as a sentinel that only holds if all litellm-internal keys are stripped), and the custom_body removal is backwards-incompatible. |
| tests/test_litellm/proxy/pass_through_endpoints/test_pass_through_endpoints.py | Two new tests added: test_multipart_passthrough_preserves_boundary (unit test that only exercises the leaf function, not the full pipeline) and test_mapped_pass_through_routes_with_server_root_path (assertions are inverted relative to the actual implementation — will fail CI). |
Sequence Diagram
sequenceDiagram
participant Client
participant FastAPI as FastAPI endpoint_func
participant Parser as _parse_request_data_by_content_type
participant PTR as pass_through_request
participant NSH as non_streaming_http_request_handler
participant MMR as make_multipart_http_request
participant Upstream
Client->>FastAPI: POST /passthrough (multipart/form-data)
FastAPI->>Parser: parse content-type
alt Actual multipart (not JSON)
Parser->>Parser: request.json() raises → skip (body cached by Starlette)
Parser-->>FastAPI: custom_body_data=None
else JSON with multipart content-type
Parser->>Parser: request.json() succeeds → custom_body_data=entire body
Parser-->>FastAPI: custom_body_data=<full JSON dict>
end
FastAPI->>PTR: pass_through_request(custom_body=final_custom_body)
PTR->>PTR: is_multipart = is_multipart(request) AND NOT custom_body
alt is_multipart=True (actual multipart)
PTR->>PTR: _parsed_body = {}
PTR->>PTR: inject litellm_logging_obj → _parsed_body non-empty
PTR->>PTR: _init_kwargs strips litellm params → _parsed_body = {} again
PTR->>NSH: non_streaming_http_request_handler(_parsed_body={})
NSH->>NSH: is_multipart AND NOT _parsed_body → True
NSH->>MMR: make_multipart_http_request(request)
MMR->>MMR: request.form() → build files dict
MMR->>Upstream: POST with multipart files (httpx sets boundary)
Upstream-->>Client: 200 OK
else is_multipart=False (JSON body)
PTR->>PTR: _parsed_body = custom_body (JSON dict)
PTR->>NSH: non_streaming_http_request_handler(_parsed_body=JSON)
NSH->>Upstream: POST json=_parsed_body
Upstream-->>Client: 200 OK
end
Comments Outside Diff (1)
-
litellm/proxy/pass_through_endpoints/pass_through_endpoints.py, line 1131-1136 (link)Backwards-incompatible removal of
custom_bodyparameterThe
custom_body: Optional[dict] = Noneparameter has been removed from the URL-basedendpoint_funcincreate_pass_through_route. Because this parameter was part of the FastAPI route signature, any external client that was passingcustom_bodyin a JSON/form body to a passthrough endpoint will now receive a validation error or have the field silently ignored, breaking existing integrations.Per the project's policy, backwards-incompatible changes should be gated behind a user-controlled flag rather than removed outright. For example:
# Keep accepting the field but warn / ignore it, or gate removal behind a flag: if litellm.preserve_passthrough_custom_body_param: custom_body: Optional[dict] = None
Alternatively, the parameter could be kept in the signature but marked as deprecated and explicitly ignored, preserving compatibility while preventing FastAPI from auto-parsing multipart bodies into it.
Rule Used: What: avoid backwards-incompatible changes without... (source)
Last reviewed commit: 8e32919
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async_client.request = AsyncMock(side_effect=mock_httpx_request) | ||
|
|
||
| # Create mock request | ||
| request = MagicMock(spec=Request) | ||
| request.method = "POST" |
There was a problem hiding this comment.
content-type assertion passes trivially
The test passes headers={} (empty dict) into make_multipart_http_request. Inside the implementation, headers_copy.pop("content-type", None) is a no-op on an empty dict, so the assertion assert "content-type" not in headers will always be True regardless of whether the stripping logic is actually working.
To give this assertion real coverage, the headers argument should be populated with a "content-type" key before it is passed in, so the test actually verifies that the header is removed by make_multipart_http_request.
…oints Fixes issue where multipart file uploads through passthrough endpoints failed with RequestValidationError. The proxy was consuming the request body stream and FastAPI was trying to parse multipart bodies as JSON dicts. Changes: - Try JSON parsing first for multipart content-type (handles misconfigured clients) - Skip multipart parsing if JSON succeeds to avoid stream consumption - Remove custom_body parameter from endpoint_func to prevent FastAPI auto-parsing - Check for parsed body before using multipart handler - Add regression test for multipart boundary preservation Handles both actual multipart uploads and JSON bodies with incorrect multipart content-type headers. Made-with: Cursor
a3e4058 to
c2fca11
Compare
|
@greptile-apps re review |
| except (json.JSONDecodeError, Exception): | ||
| # Not JSON - this is actual multipart data | ||
| # Skip parsing here to avoid consuming the request body stream | ||
| # make_multipart_http_request will handle it | ||
| pass |
There was a problem hiding this comment.
Overly-broad exception clause silences unexpected errors
except (json.JSONDecodeError, Exception) is redundant — Exception already catches json.JSONDecodeError — and it swallows every possible exception (e.g. MemoryError, RuntimeError, network-level errors on request.body()). Any real error reading the body will silently be treated as "not JSON, skip", making failures invisible.
| except (json.JSONDecodeError, Exception): | |
| # Not JSON - this is actual multipart data | |
| # Skip parsing here to avoid consuming the request body stream | |
| # make_multipart_http_request will handle it | |
| pass | |
| except (json.JSONDecodeError, ValueError): | |
| # Not JSON - this is actual multipart data | |
| # Skip parsing here to avoid consuming the request body stream | |
| # make_multipart_http_request will handle it | |
| pass |
| # Test the multipart handler directly | ||
| response = await HttpPassThroughEndpointHelpers.make_multipart_http_request( | ||
| request=request, | ||
| async_client=async_client, | ||
| url=httpx.URL("http://test.com/upload"), | ||
| headers={}, | ||
| requested_query_params=None, | ||
| ) | ||
|
|
||
| # Verify the response | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
Test bypasses the broken code path — pass_through_request flow is untested
The test calls make_multipart_http_request directly, skipping the entire pass_through_request → non_streaming_http_request_handler pipeline where the routing bug lives. A test that exercises only the leaf function will pass even when the top-level fix is broken (as is the case here).
To provide real regression coverage, the test should either:
- Call
pass_through_requestend-to-end (mocking outproxy_logging_obj,user_api_key_auth, etc.), or - At minimum, exercise
non_streaming_http_request_handlerwith a_parsed_bodythat has already been mutated the waypass_through_requestmutates it (i.e. withlitellm_logging_objpresent), and assert thatmake_multipart_http_requestis still dispatched.
| elif is_multipart: | ||
| # Don't parse multipart body here - it will be handled by make_multipart_http_request | ||
| _parsed_body = {} |
There was a problem hiding this comment.
Empty dict sentinel for multipart is fragile and leaks into the downstream request
Setting _parsed_body = {} as a sentinel value for "don't send a body" is unreliable because:
- Lines 707–713 will inject
metadata.guardrailsinto this dict when guardrails are configured. - Line 736 always injects
litellm_logging_obj, making the dict non-empty. - If the
not _parsed_bodycheck innon_streaming_http_request_handleris ever bypassed (see the comment on line 736), these internal keys will be forwarded to the upstream service as JSON.
A safer approach is to keep _parsed_body = None for multipart and propagate an explicit boolean:
elif is_multipart:
_parsed_body = None # leave body untouched; make_multipart_http_request reads itThen guard the litellm_logging_obj injection against None, and pass an explicit _is_multipart flag to non_streaming_http_request_handler rather than relying on the emptiness of _parsed_body.
| # ✅ Try to parse as JSON first (handles misconfigured clients sending JSON with multipart content-type) | ||
| # If that fails, skip parsing - pass_through_request will handle actual multipart | ||
| try: | ||
| body = await request.json() | ||
| # Successfully parsed as JSON - treat as JSON body | ||
| query_params_data = body.get("query_params") | ||
| custom_body_data = body.get("custom_body") | ||
| stream = body.get("stream") | ||
| # If custom_body is not set, use the entire body | ||
| if custom_body_data is None and body: | ||
| custom_body_data = body | ||
| except (json.JSONDecodeError, Exception): | ||
| # Not JSON - this is actual multipart data | ||
| # Skip parsing here to avoid consuming the request body stream | ||
| # make_multipart_http_request will handle it | ||
| pass |
There was a problem hiding this comment.
Attempting request.json() on real multipart bodies is semantically wrong and still consumes the body
The comment says "Try to parse as JSON first (handles misconfigured clients sending JSON with multipart content-type)", but this approach has two problems:
-
Stream consumption:
request.json()callsawait request.body()internally. Even though Starlette caches the raw bytes, the call still reads the entire body into memory beforemake_multipart_http_requestcan stream it viarequest.form(). For large file uploads this causes a full in-memory copy of the file regardless of the code path taken. -
Incorrect fallback assignment: When JSON parsing succeeds, line 1064 sets
custom_body_data = body(the entire parsed JSON object). This means any JSON request arriving with amultipart/form-datacontent-type header will have its entire body treated ascustom_body, overriding whatever the caller intended. The original code isolated specific keys (query_params,custom_body,stream); this new path does not.
A safer approach: only run the JSON-fallback path if the body starts with { or [ (fast check without full parse), or require that clients send the correct content-type rather than trying to paper over misconfigured clients in the proxy layer.
|
@greptile re-review with new commits |
| # prefixed route should match mapped routes like /vertex_ai | ||
| assert ( | ||
| InitPassThroughEndpointHelpers.is_registered_pass_through_route( | ||
| "/litellm/vertex_ai/v1/projects/foo" | ||
| ) | ||
| is True | ||
| ) | ||
| assert ( | ||
| InitPassThroughEndpointHelpers.is_registered_pass_through_route( | ||
| "/litellm/bedrock/model/invoke" | ||
| ) | ||
| is True | ||
| ) | ||
|
|
||
| # bare route without prefix should not match when root is set | ||
| assert ( | ||
| InitPassThroughEndpointHelpers.is_registered_pass_through_route( | ||
| "/vertex_ai/v1/projects/foo" | ||
| ) | ||
| is False | ||
| ) |
There was a problem hiding this comment.
Test assertions are inverted — test will fail CI
The assertions in test_mapped_pass_through_routes_with_server_root_path are testing behaviour that doesn't exist in the current implementation of is_registered_pass_through_route.
Looking at is_registered_pass_through_route (line 2061–2063 in the production file):
for mapped_route in LiteLLMRoutes.mapped_pass_through_routes.value:
if route.startswith(mapped_route): # NO root-path prefix applied here
return TrueThe mapped-route check uses raw startswith(mapped_route) without ever calling _build_full_path_with_root. This means:
"/litellm/vertex_ai/v1/projects/foo".startswith("/vertex_ai")→False— but the test assertsTrue"/litellm/bedrock/model/invoke".startswith("/bedrock")→False— but the test assertsTrue"/vertex_ai/v1/projects/foo".startswith("/vertex_ai")→True— but the test assertsFalse
All three assertions are wrong relative to the actual implementation. The test would fail on every CI run.
The fix would need to either:
- Update
is_registered_pass_through_routeto prefix mapped routes with_build_full_path_with_root(implementing the intended behaviour), or - Correct the assertions to match what the code actually does (if the test was written against the wrong spec).
…assthrough fix(proxy): preserve multipart/form-data boundary in passthrough endpoints
2 participants
Summary
Fixes LIT-2181
Fixes multipart file upload failures in passthrough endpoints where requests were failing with
RequestValidationError: Input should be a valid dictionary.Root causes:
custom_body: Optional[dict]parameter_parse_request_data_by_content_typewas consuming the request body stream beforemake_multipart_http_requestcould process itChanges:
_parse_request_data_by_content_typeto preserve request streamcustom_bodyparameter fromendpoint_functo prevent FastAPI auto-parsingis_multipartcheck inpass_through_requestto skip_read_request_bodytest_multipart_passthrough_preserves_boundaryTesting:
test_multipart_passthrough_preserves_boundaryCloses issue where file uploads through passthrough API failed with common frameworks (FastAPI, Django, Express).
Made with Cursor