fix(proxy): stop x-litellm-tags header tags leaking into Bedrock passthrough body - #30994
Conversation
…through body
apply_client_tag_policy_pre_auth merged x-litellm-tags into the request
body's metadata dict in place. That dict is shared by reference with the
cached parsed body, which passthrough routes forward verbatim, so the
LiteLLM-internal billing tags ended up in the upstream Bedrock payload and
Bedrock rejected the request with HTTP 400 ("Extra inputs are not permitted").
Build a new metadata dict and reassign request_data[metadata_var] instead of
mutating the shared object. The tag-budget check still sees the merged tags on
request_data, but the cached body the route re-reads stays untouched.
This is the header-tag counterpart to the key-level metadata.tags leak in
GH#30629; the key-tag path is addressed separately by routing Bedrock through
litellm_metadata.
|
Superseded by the latest commit on #30985 (dbd1600), which pre-seeds litellm_metadata before apply_client_tag_policy_pre_auth. For Bedrock and the other LITELLM_METADATA_ROUTES that routes header-supplied x-litellm-tags into litellm_metadata instead of the provider-facing metadata field, which closes the same header-tag leak this PR targeted, and it does so as part of the complete #30629 fix covering both the key-tag and header-tag vectors. Closing this as redundant. One thing worth keeping in mind as a possible follow-up: that pre-seed is scoped to LITELLM_METADATA_ROUTES, so a passthrough provider not on that list would still forward header tags in its body. If that ever becomes a real case, hardening apply_client_tag_policy_pre_auth to never mutate the shared cached request body dict would close it generally |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR attempts to fix
Confidence Score: 2/5The fix avoids in-place mutation of the shared metadata dict, but the reassignment still propagates into the cached parsed body when auth is the first reader — which is the standard request flow — leaving tags visible in the body forwarded to Bedrock. The new dict assignment works only when Both changed files need attention:
|
| Filename | Overview |
|---|---|
| litellm/proxy/litellm_pre_call_utils.py | apply_client_tag_policy_pre_auth now builds a new metadata dict instead of mutating in-place, but the assignment request_data[key] = new_dict still modifies parsed_body when auth is the first caller of _read_request_body (the production case), so tags can still leak into the forwarded Bedrock body. |
| tests/test_litellm/proxy/test_litellm_pre_call_utils.py | New regression test pre-caches the parsed body via _safe_set_request_parsed_body before calling _read_request_body, which puts the test into a different code path (cache-hit returns a shallow copy) than the production flow (first-ever call returns parsed_body directly). The test passes but does not verify the actual production scenario. |
Reviews (1): Last reviewed commit: "fix(proxy): stop x-litellm-tags header t..." | Re-trigger Greptile
| request_mock.scope = {} | ||
| _safe_set_request_parsed_body( | ||
| request=request_mock, | ||
| parsed_body={ | ||
| "anthropic_version": "bedrock-2023-05-31", | ||
| "messages": [{"role": "user", "content": "hi"}], | ||
| "metadata": {"user_id": "end-user-1"}, | ||
| }, | ||
| ) | ||
|
|
||
| auth_body = await _read_request_body(request_mock) | ||
| LiteLLMProxyRequestSetup.apply_client_tag_policy_pre_auth( | ||
| request=request_mock, | ||
| request_data=auth_body, | ||
| user_api_key_dict=UserAPIKeyAuth(api_key="hashed-key", metadata={}), | ||
| ) | ||
|
|
||
| # _tag_max_budget_check reads the same dict the merge wrote to. | ||
| assert auth_body["metadata"]["tags"] == ["billing:cost-center-1"] | ||
|
|
||
| # Passthrough re-reads the body from cache and forwards it to Bedrock. | ||
| forwarded_body = await _read_request_body(request_mock) | ||
| assert "tags" not in forwarded_body["metadata"] | ||
| assert forwarded_body["metadata"] == {"user_id": "end-user-1"} |
There was a problem hiding this comment.
Test pre-caches the body, masking the production failure path
The test calls _safe_set_request_parsed_body before _read_request_body, so _read_request_body hits the cache path and returns a new shallow-copy dict ({key: parsed_body[key] for key in accepted_keys}). This means auth_body IS NOT original_body, so assigning request_data["metadata"] = new_dict only updates the shallow copy — original_body["metadata"] is untouched and the second _read_request_body call returns a clean body.
In the actual Bedrock passthrough production flow, user_api_key_auth (a FastAPI Depends) is the first caller of _read_request_body. Because no cache exists yet, _read_request_body parses the body, stores (tuple(parsed_body.keys()), parsed_body) in request.scope["parsed_body"], and returns parsed_body directly — not a copy. At that point request_data IS parsed_body. When apply_client_tag_policy_pre_auth then does request_data["metadata"] = new_dict_with_tags, it also sets parsed_body["metadata"] = new_dict_with_tags. When bedrock_llm_proxy_route later calls _read_request_body, it retrieves the cache and reconstructs {key: parsed_body[key] for key in accepted_keys} — where "metadata" now maps to new_dict_with_tags. Tags still leak.
To reproduce the actual production path, the test should not pre-populate the cache; it should let _read_request_body read raw bytes on the first call (as auth does), then assert on the second read.
| # Build a new dict instead of mutating existing_metadata: it may be the | ||
| # request body's metadata object, shared by reference with the cached | ||
| # body that passthrough routes forward verbatim. An in-place write would | ||
| # leak these LiteLLM-internal tags into the upstream provider payload. | ||
| existing_tags = existing_metadata.get("tags") | ||
| request_data[_metadata_variable_name] = { | ||
| **existing_metadata, | ||
| "tags": LiteLLMProxyRequestSetup._merge_tags( | ||
| request_tags=existing_tags if isinstance(existing_tags, list) else None, | ||
| tags_to_add=header_tags, | ||
| ), | ||
| } |
There was a problem hiding this comment.
New dict assignment still leaks when
request_data IS parsed_body
The fix avoids mutating the existing metadata dict in-place, but the assignment request_data[_metadata_variable_name] = new_dict still changes parsed_body["metadata"] whenever request_data and parsed_body are the same object — which is the case in the standard production flow.
user_api_key_auth is the first code to call _read_request_body on a Bedrock passthrough request. Because the cache is empty at that point, _read_request_body parses the raw body, caches (tuple(keys), parsed_body), and returns parsed_body directly (not a copy). Consequently request_data IS parsed_body. The fix then does request_data["metadata"] = new_dict_with_tags, which is identical to parsed_body["metadata"] = new_dict_with_tags. When bedrock_llm_proxy_route later calls _read_request_body, _safe_get_request_parsed_body returns {key: parsed_body[key] for key in accepted_keys}, and parsed_body["metadata"] is now the new dict with tags — so they are forwarded to Bedrock unchanged.
A robust fix would need to either (a) save a snapshot of the original body in request.scope before auth-time mutations (separate from the live parsed_body entry the route handler reads), or (b) strip LiteLLM-internal keys from data["data"] in the passthrough handler before serialising and forwarding the body.
Relevant issues
Follow-up to #30629. That issue and its fix (#30985) cover key-level
metadata.tagsleaking into the Bedrock passthrough body. This PR closes the sibling vector: tags supplied via thex-litellm-tagsrequest header.Linear ticket
N/A
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
make test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
apply_client_tag_policy_pre_authmergesx-litellm-tagsinto the request body'smetadatadict before auth so_tag_max_budget_checkcan see them. It did this with an in-place write. Thatmetadatadict is shared by reference with the cached parsed body (request.scope["parsed_body"]), and the Bedrock passthrough route re-reads that same cached body and forwards it verbatim to AWS. So the LiteLLM-internal billing tags ended up undermetadatain the upstream payload, and Bedrock's Anthropic schema rejects any non-user_idkey there withHTTP 400 "Extra inputs are not permitted".The fix builds a new metadata dict and reassigns
request_data[metadata_var]rather than mutating the shared object. The budget check still sees the merged tags onrequest_data; the cached body the route forwards stays clean.To reproduce against a live proxy (real Bedrock call, costs real money), with a Bedrock passthrough model configured and the proxy running on
localhost:4000:Before this change the call returns
HTTP 400with a validation error onmetadata.tags; after it returnsHTTP 200, and theteam-a/cost-center-7tags are still recorded for spend tracking (visible at http://localhost:4000/ui/?page=logs).Type
🐛 Bug Fix
Changes
litellm/proxy/litellm_pre_call_utils.py:apply_client_tag_policy_pre_authnow constructs a fresh metadata dict instead of mutating the request body's metadata object in place, so header tags can no longer leak through the shared cached body into a passthrough provider payload.tests/test_litellm/proxy/test_litellm_pre_call_utils.py: regression test driving the merge through the real request-body cache and asserting the re-read (forwarded) body carries notagswhile the auth-time body still exposes them to_tag_max_budget_check. It fails before the fix and passes after.