[WIP] Reformat request parameters#26289
[WIP] Reformat request parameters#26289Michael-RZ-Berri wants to merge 5 commits intolitellm_internal_stagingfrom
Conversation
|
Michael Riad Zaky seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Greptile SummaryThis PR introduces Confidence Score: 4/5Safe to merge pending resolution of the backwards-incompatible default noted in a prior review thread. All three previously-identified P1 bypass paths (JSON ingress, form-data via get_request_body, bracket-notation metadata via extract_nested_form_metadata) are now covered by the sanitization logic. Tests are comprehensive, mock-only, and cover idempotency, unicode round-tripping, opt-in behaviour, and end-to-end cache safety. No new P0/P1 issues were found in this pass. Score is capped at 4 rather than 5 because an unresolved backwards-incompatible default change (flagged in the previous review thread) remains — existing callers sending mock_response in a shared staging proxy will silently hit the live model without any log warning unless ops teams proactively set the flag. litellm/proxy/common_utils/http_parsing_utils.py — specifically the interaction between _read_request_body's own form-data branch (which does not delegate to get_form_data) and the get_form_data code path, to ensure no duplicate or inconsistent sanitization occurs for the same request across both paths.
|
| Filename | Overview |
|---|---|
| litellm/proxy/common_utils/http_parsing_utils.py | Adds strip_internal_control_fields and supporting helpers that sanitize proxy-internal fields (mock_response, applied_guardrails, user_api_key_*, etc.) from incoming request bodies at three ingress points: _read_request_body, get_form_data, and extract_nested_form_metadata. JSON-string metadata is parsed, stripped, and re-serialized with ensure_ascii=False. |
| litellm/proxy/example_config_yaml/otel_test_config.yaml | Adds general_settings.allow_client_side_mock_response: true so the existing OTEL test suite (which relies on mock_response via extra_body) continues to work after the new default-off behaviour. |
| tests/test_litellm/proxy/common_utils/test_http_parsing_utils.py | Adds a comprehensive TestStripInternalControlFields suite: covers mock-response default stripping, opt-in preservation, restricted top-level and metadata fields, user_api_key_* prefix stripping, JSON-string metadata sanitisation (including unicode round-trip), idempotency, form-data paths, and the extract_nested_form_metadata bracket-notation path. All tests mock rather than make real network calls, satisfying the repo's test-isolation rule. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming HTTP Request] --> B{Content-Type?}
B -->|application/json| C[_read_request_body\norjson.loads body]
B -->|multipart / form-data\napplication/x-www-form-urlencoded| D[get_form_data\nrequest.form]
B -->|files endpoint\nrequest.form direct| E[extract_nested_form_metadata\nbracket-notation assembly]
C --> F[strip_internal_control_fields]
D --> F
E --> G[_strip_internal_metadata_keys\nmetadata-level only]
F --> H{_client_mock_response_allowed?}
H -->|No| I[pop mock_response\npop mock_tool_calls]
H -->|Yes| J[keep mock fields]
I --> K[pop _RESTRICTED_TOP_LEVEL_FIELDS]
J --> K
K --> L[_strip_internal_metadata_keys\non metadata / litellm_metadata]
L --> M[strip JSON-string\nmetadata containers]
M --> N[Cache sanitized body\nrequest.scope.parsed_body]
G --> O[Return sanitized metadata dict]
N --> P[Return sanitized body to handler]
Reviews (6): Last reviewed commit: "extend reformatting to bracket-notation ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if not _client_mock_response_allowed(): | ||
| for key in _MOCK_RESPONSE_FIELDS: | ||
| data.pop(key, None) |
There was a problem hiding this comment.
mock_response
Any client currently sending mock_response in a request body — a common pattern in staging/test environments that point at a shared proxy — will have the field silently dropped and the real LLM called instead. There is no log line at the call site explaining the strip, so the failure mode is opaque: callers see unexpected live-model responses with no indication that their field was ignored.
The opt-in flag exists, but the migration path requires ops teams to discover and set general_settings.allow_client_side_mock_response: true before their test workloads are disrupted. Per the team's rule about backwards-incompatible changes, a safer rollout would be: log a prominent warning when the field is stripped in this release, and default to true (preserve existing behavior) until a future major version flips the default.
if not _client_mock_response_allowed():
for key in _MOCK_RESPONSE_FIELDS:
if key in data:
verbose_proxy_logger.warning(
"Stripping client-supplied '%s' from request body. "
"Set general_settings.allow_client_side_mock_response: true "
"in config.yaml to allow this field.",
key,
)
data.pop(key)Rule Used: What: avoid backwards-incompatible changes without... (source)
| elif isinstance(container, str): | ||
| try: | ||
| parsed = json.loads(container) | ||
| except (json.JSONDecodeError, ValueError): | ||
| continue | ||
| if isinstance(parsed, dict) and _strip_internal_metadata_keys(parsed): | ||
| data[container_key] = json.dumps(parsed) |
There was a problem hiding this comment.
When _strip_internal_metadata_keys returns True (something was removed), the sanitized dict is re-serialized with json.dumps. This changes the representation of the original string — key ordering, Unicode escaping, and whitespace may all differ from the client's original. While the data is semantically equivalent, any downstream code that does a raw-string equality check on the metadata value (e.g., a cache key or an HMAC) will see a different string than the client sent.
Consider using ensure_ascii=False to keep the round-trip closer to the original.
Michael-RZ-Berri
changed the title
Michael-RZ-Berri
force-pushed
the
litellm_reformatRequestParameters
branch
from
ea70fe7 to
42a2b1f
Compare
Medium: Sanitization bypass via multipart/form-data content typeThis PR adds ingress sanitization of proxy-internal control fields in The secondary defense in
Status: 0 open Posted by Veria AI · 2026-04-25T17:36:06.412Z |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
All review comments resolved — no active issues remaining. Status: 0 open Posted by Veria AI · 2026-04-25T19:40:00.358Z |
Low: Positive security hardening with incomplete endpoint coverageThis PR adds centralized ingress sanitization ( The new stripping covers fields not previously stripped at all ( Status: 0 open Posted by Veria AI · 2026-04-25T19:43:59.172Z |
Low: Security hardening for request parameter sanitizationThis PR adds The implementation is sound and well-tested. No new security issues are introduced. Status: 0 open Posted by Veria AI · 2026-04-25T19:48:26.781Z |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
|
|
||
| # Strip proxy-internal fields before anything downstream (including | ||
| # the cache) sees the body. | ||
| strip_internal_control_fields(parsed_body) |
There was a problem hiding this comment.
Medium: Incomplete coverage of ingress sanitization
This strip only runs when the body is parsed through _read_request_body or get_form_data. Five endpoints in proxy_server.py (moderations ~L7857, audio_speech ~L7983, create_assistant ~L8545, add_messages ~L8933, run_thread ~L9126) call orjson.loads(body) directly and then pass data to add_litellm_data_to_request without going through this path. Those endpoints remain vulnerable to injection of fields like applied_guardrails, mock_response, _guardrail_pipelines, etc. that add_litellm_data_to_request doesn't strip.
Consider either calling strip_internal_control_fields(data) inside add_litellm_data_to_request (which all endpoints already use), or refactoring those five endpoints to use _read_request_body.
Medium: Incomplete ingress sanitization — some endpoints bypass the new stripThis PR adds
Status: 1 new · 1 open Posted by Veria AI · 2026-04-25T19:50:09.235Z |
2 participants
Relevant issues
Cleans parameters in requests so functions down the line operate as expected.
This change also disables mock responses by default, users need to re-enable with general_settings.allow_client_side_mock_response: true in config.yaml during development.
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
The attached tests cover the cases where the sanitization is needed, and they pass now.
Type
🐛 Bug Fix
✅ Test
Changes
Importantly, this change turns off mock responses by default so that they can't be used for something similar in production. The mock endpoints need to be re-enabled with general_settings.allow_client_side_mock_response: true in config.yaml, which should be mentioned in the docs.
This touches _read_request_body in http_parsing_utils, also the related test file.