From b4e98d190a42c04c1e7cf3c44abf7524a92826d5 Mon Sep 17 00:00:00 2001 From: user <70670632+stuxf@users.noreply.github.com> Date: Fri, 17 Apr 2026 00:08:40 +0000 Subject: [PATCH] fix(proxy): close 6 more metadata/tag variant bypasses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge audit found 6 adjacent variants of the VERIA-28 class. All fixed here with regression tests: 1. Strip widened from 3 named keys to the full user_api_key_* prefix. The proxy writes a dozen user_api_key_* fields (user_id, alias, spend, team_id, request_route, end_user_id, …) into data[_metadata_variable_name]; the 3-key strip left the rest exploitable for identity/spend forgery in audit logs and guardrails. 2. proxy_server_request['body'] snapshot moved to AFTER the strip. Was captured at line ~990 before the strip ran, so standard_logging_object, lago, and spend_tracking readers saw the attacker-forged payload even though the live data dict was clean. 3. get_tags_from_request_body (auth-time) now coerces JSON-string metadata via safe_json_loads. Previously crashed with AttributeError on string metadata (DoS; potential RBAC bypass if a caller swallowed the exception). 4. get_end_user_id_from_request_body coerces JSON-string metadata/litellm_metadata. Previously isinstance(dict) guard caused end-user budget attribution to be silently skipped when the caller sent metadata as a JSON string. 5. Four hand-rolled 'if data.get("metadata") is None: data["metadata"] = {}' blocks in proxy_server.py (7160, 7341, 7590, 11375) now guard on isinstance(dict). They crashed with TypeError when metadata was a JSON string (DoS). 6. _get_admin_metadata defensively guards with isinstance(dict); previously AttributeError'd on any leaked string metadata. Also hoists the inline safe_json_loads import in _guardrail_modification_check to module level per CLAUDE.md style. --- litellm/integrations/custom_guardrail.py | 6 +- litellm/proxy/auth/auth_checks.py | 2 +- litellm/proxy/auth/auth_utils.py | 31 ++-- .../proxy/common_utils/http_parsing_utils.py | 19 +- litellm/proxy/litellm_pre_call_utils.py | 36 ++-- litellm/proxy/proxy_server.py | 15 +- .../common_utils/test_http_parsing_utils.py | 38 ++++ .../proxy/test_litellm_pre_call_utils.py | 166 ++++++++++++++++++ 8 files changed, 281 insertions(+), 32 deletions(-) diff --git a/litellm/integrations/custom_guardrail.py b/litellm/integrations/custom_guardrail.py index b0931964cc6..abf010e0d65 100644 --- a/litellm/integrations/custom_guardrail.py +++ b/litellm/integrations/custom_guardrail.py @@ -268,7 +268,11 @@ class CustomGuardrail(CustomLogger): team_meta: dict = {} key_meta: dict = {} for key in ("metadata", "litellm_metadata"): - meta = data.get(key) or {} + # Defensive: an unparsed JSON-string metadata could leak past the + # proxy's normal parse path; don't AttributeError on .get(). + meta = data.get(key) + if not isinstance(meta, dict): + continue team_meta = meta.get("user_api_key_team_metadata") or team_meta key_meta = meta.get("user_api_key_metadata") or key_meta return {**team_meta, **key_meta} diff --git a/litellm/proxy/auth/auth_checks.py b/litellm/proxy/auth/auth_checks.py index 846d61384b4..1621f04213f 100644 --- a/litellm/proxy/auth/auth_checks.py +++ b/litellm/proxy/auth/auth_checks.py @@ -31,6 +31,7 @@ from litellm.constants import ( ) from litellm.litellm_core_utils.dd_tracing import tracer from litellm.litellm_core_utils.get_llm_provider_logic import get_llm_provider +from litellm.litellm_core_utils.safe_json_loads import safe_json_loads from litellm.proxy._types import ( RBAC_ROLES, CallInfo, @@ -350,7 +351,6 @@ def _guardrail_modification_check( failing loudly at the auth layer so operators see an explicit 403 instead of a confusing silent-ignore. """ - from litellm.litellm_core_utils.safe_json_loads import safe_json_loads from litellm.proxy.guardrails.guardrail_helpers import can_modify_guardrails def _coerce_to_dict(container: Any) -> Optional[dict]: diff --git a/litellm/proxy/auth/auth_utils.py b/litellm/proxy/auth/auth_utils.py index 64766bbaadd..12d9bff91e0 100644 --- a/litellm/proxy/auth/auth_utils.py +++ b/litellm/proxy/auth/auth_utils.py @@ -823,19 +823,30 @@ def get_end_user_id_from_request_body( user_from_body_user_field = request_body["user"] return str(user_from_body_user_field) + def _as_dict(value: Any) -> dict: + # metadata / litellm_metadata can arrive as JSON strings from + # multipart/form-data or extra_body; coerce so string-encoded + # payloads can't evade end-user attribution. + if isinstance(value, dict): + return value + if isinstance(value, str): + from litellm.litellm_core_utils.safe_json_loads import safe_json_loads + + parsed = safe_json_loads(value) + return parsed if isinstance(parsed, dict) else {} + return {} + # Check 4: 'litellm_metadata.user' in request_body (commonly Anthropic) - litellm_metadata = request_body.get("litellm_metadata") - if isinstance(litellm_metadata, dict): - user_from_litellm_metadata = litellm_metadata.get("user") - if user_from_litellm_metadata is not None: - return str(user_from_litellm_metadata) + litellm_metadata = _as_dict(request_body.get("litellm_metadata")) + user_from_litellm_metadata = litellm_metadata.get("user") + if user_from_litellm_metadata is not None: + return str(user_from_litellm_metadata) # Check 5: 'metadata.user_id' in request_body (another common pattern) - metadata_dict = request_body.get("metadata") - if isinstance(metadata_dict, dict): - user_id_from_metadata_field = metadata_dict.get("user_id") - if user_id_from_metadata_field is not None: - return str(user_id_from_metadata_field) + metadata_dict = _as_dict(request_body.get("metadata")) + user_id_from_metadata_field = metadata_dict.get("user_id") + if user_id_from_metadata_field is not None: + return str(user_id_from_metadata_field) # Check 6: 'safety_identifier' in request body (OpenAI Responses API parameter) # SECURITY NOTE: safety_identifier can be set by any caller in the request body. diff --git a/litellm/proxy/common_utils/http_parsing_utils.py b/litellm/proxy/common_utils/http_parsing_utils.py index 1dd25262127..71abdfa5e9e 100644 --- a/litellm/proxy/common_utils/http_parsing_utils.py +++ b/litellm/proxy/common_utils/http_parsing_utils.py @@ -197,10 +197,10 @@ def check_file_size_under_limit( if llm_router is not None and request_data["model"] in router_model_names: try: - deployment: Optional[ - Deployment - ] = llm_router.get_deployment_by_model_group_name( - model_group_name=request_data["model"] + deployment: Optional[Deployment] = ( + llm_router.get_deployment_by_model_group_name( + model_group_name=request_data["model"] + ) ) if ( deployment @@ -426,7 +426,16 @@ def get_tags_from_request_body(request_body: dict) -> List[str]: List of tag names (strings), empty list if no valid tags found """ metadata_variable_name = get_metadata_variable_name_from_kwargs(request_body) - metadata = request_body.get(metadata_variable_name) or {} + metadata = request_body.get(metadata_variable_name) + # metadata can arrive as a JSON string from multipart/form-data or extra_body; + # coerce defensively so .get() below never raises AttributeError. + if isinstance(metadata, str): + from litellm.litellm_core_utils.safe_json_loads import safe_json_loads + + parsed = safe_json_loads(metadata) + metadata = parsed if isinstance(parsed, dict) else {} + elif not isinstance(metadata, dict): + metadata = {} tags_in_metadata: Any = metadata.get("tags", []) tags_in_request_body: Any = request_body.get("tags", []) combined_tags: List[str] = [] diff --git a/litellm/proxy/litellm_pre_call_utils.py b/litellm/proxy/litellm_pre_call_utils.py index 51f82b7c00d..aa3a29e4f2f 100644 --- a/litellm/proxy/litellm_pre_call_utils.py +++ b/litellm/proxy/litellm_pre_call_utils.py @@ -981,13 +981,16 @@ async def add_litellm_data_to_request( # noqa: PLR0915 # Init - Proxy Server Request # we do this as soon as entering so we track the original request ########################################################## - # Track arrival time for queue time metric + # Track arrival time for queue time metric. The body snapshot is filled + # in after the admin-injection strip below so the audit / spend-tracking + # consumers of proxy_server_request["body"] see the cleaned metadata + # rather than attacker-forged user_api_key_* fields. arrival_time = time.time() data["proxy_server_request"] = { "url": str(request.url), "method": request.method, "headers": _headers, - "body": copy.copy(data), # use copy instead of deepcopy + "body": None, # filled in post-strip; see below "arrival_time": arrival_time, # Track when request arrived at proxy } @@ -1087,19 +1090,24 @@ async def add_litellm_data_to_request( # noqa: PLR0915 # Strip internal pipeline state and admin-injection slots from user input. # Runs AFTER the string-to-dict parse above so JSON-string metadata (sent - # via multipart/form-data or extra_body) cannot smuggle `user_api_key_metadata` - # past the isinstance(dict) guard. + # via multipart/form-data or extra_body) cannot smuggle admin fields past + # the isinstance(dict) guard. # - # The proxy writes user_api_key_metadata / user_api_key_team_metadata into - # data[_metadata_variable_name] below; if a caller pre-populates either - # key on the OTHER metadata field, _get_admin_metadata lookups would treat - # the caller's payload as admin-configured. + # The proxy populates a family of ``user_api_key_*`` fields below + # (user_api_key_metadata, user_api_key_user_id, user_api_key_alias, + # user_api_key_spend, user_api_key_team_metadata, …) into + # data[_metadata_variable_name]. Because the proxy only writes to ONE of + # the two metadata dicts, a caller pre-populating any of these keys on + # the OTHER metadata dict would have their forged values surface in + # guardrails, spend tracking, audit logs, and identity resolution. Strip + # by prefix so new ``user_api_key_*`` fields added in the future are + # covered without per-key maintenance. for _meta_key in ("metadata", "litellm_metadata"): _user_meta = data.get(_meta_key) if isinstance(_user_meta, dict): _user_meta.pop("_pipeline_managed_guardrails", None) - _user_meta.pop("user_api_key_metadata", None) - _user_meta.pop("user_api_key_team_metadata", None) + for _k in [k for k in _user_meta if k.startswith("user_api_key_")]: + _user_meta.pop(_k, None) # Strip caller-supplied routing/budget tags unless the admin has opted # this key or team in via metadata.allow_client_tags=True. Tags drive @@ -1132,9 +1140,15 @@ async def add_litellm_data_to_request( # noqa: PLR0915 ", ".join(_stripped_from), ) + # Fill in the proxy_server_request body snapshot now that metadata has + # been parsed and stripped. Consumers (standard_logging_payload, lago, + # spend_tracking_utils, streaming_iterator) read `body` to audit the + # request; taking the snapshot here ensures they see cleaned metadata. + data["proxy_server_request"]["body"] = copy.copy(data) + # Snapshot the (now-cleaned) requester-supplied metadata for downstream # consumers. Taking the deepcopy AFTER the strip prevents attacker- - # injected admin slots (user_api_key_metadata, tags without opt-in, + # injected admin slots (user_api_key_*, tags without opt-in, # _pipeline_managed_guardrails) from surviving in requester_metadata # where guardrails and audit paths may read from it. if "metadata" in data and isinstance(data["metadata"], dict): diff --git a/litellm/proxy/proxy_server.py b/litellm/proxy/proxy_server.py index 2d789b982da..1d61bebee44 100644 --- a/litellm/proxy/proxy_server.py +++ b/litellm/proxy/proxy_server.py @@ -2201,9 +2201,11 @@ def run_ollama_serve(): with open(os.devnull, "w") as devnull: subprocess.Popen(command, stdout=devnull, stderr=devnull) except Exception as e: - verbose_proxy_logger.debug(f""" + verbose_proxy_logger.debug( + f""" LiteLLM Warning: proxy started with `ollama` model\n`ollama serve` failed with Exception{e}. \nEnsure you run `ollama serve` - """) + """ + ) def _get_process_rss_mb() -> Optional[float]: @@ -7157,7 +7159,10 @@ async def chat_completion( # noqa: PLR0915 global user_temperature, user_request_timeout, user_max_tokens, user_api_base data = await _read_request_body(request=request) if user_api_key_dict is not None: - if data.get("metadata") is None: + if not isinstance(data.get("metadata"), dict): + # Covers both missing and JSON-string metadata (multipart / + # extra_body); otherwise `data["metadata"][k] = v` below raises + # TypeError on a string value and 500s the request. data["metadata"] = {} if ( hasattr(user_api_key_dict, "user_id") @@ -11372,7 +11377,9 @@ async def async_queue_request( # if users are using user_api_key_auth, set `user` in `data` data["user"] = user_api_key_dict.user_id - if "metadata" not in data: + if not isinstance(data.get("metadata"), dict): + # Covers both missing and JSON-string metadata (multipart / + # extra_body); see above for the same guard upstream. data["metadata"] = {} data["metadata"]["user_api_key"] = user_api_key_dict.api_key data["metadata"]["user_api_key_metadata"] = user_api_key_dict.metadata diff --git a/tests/test_litellm/proxy/common_utils/test_http_parsing_utils.py b/tests/test_litellm/proxy/common_utils/test_http_parsing_utils.py index a1484bc263b..c9f595626ed 100644 --- a/tests/test_litellm/proxy/common_utils/test_http_parsing_utils.py +++ b/tests/test_litellm/proxy/common_utils/test_http_parsing_utils.py @@ -835,3 +835,41 @@ def test_safe_get_request_headers_state_unavailable(): result = _safe_get_request_headers(mock_request) assert result == {"content-type": "application/json"} + + +class TestGetTagsFromRequestBodyStringCoerce: + """Regression: the auth-time tag helper used `metadata.get("tags", ...)` + directly, which raised AttributeError when metadata arrived as a JSON + string (multipart/form-data or extra_body). That turned into a DoS at + auth time and potentially bypassed tag-based RBAC if the caller caught + the exception and fell through with empty tags. + """ + + def test_json_string_metadata_is_coerced_to_dict(self): + from litellm.proxy.common_utils.http_parsing_utils import ( + get_tags_from_request_body, + ) + + metadata_json = json.dumps({"tags": ["a", "b"]}) + # Must not raise + tags = get_tags_from_request_body({"metadata": metadata_json}) + assert tags == ["a", "b"] + + def test_unparseable_string_metadata_is_ignored(self): + from litellm.proxy.common_utils.http_parsing_utils import ( + get_tags_from_request_body, + ) + + # Must not raise; must yield no metadata tags but keep root tags + tags = get_tags_from_request_body( + {"metadata": "not-json", "tags": ["root-only"]} + ) + assert tags == ["root-only"] + + def test_dict_metadata_still_works(self): + from litellm.proxy.common_utils.http_parsing_utils import ( + get_tags_from_request_body, + ) + + tags = get_tags_from_request_body({"metadata": {"tags": ["x"]}}) + assert tags == ["x"] diff --git a/tests/test_litellm/proxy/test_litellm_pre_call_utils.py b/tests/test_litellm/proxy/test_litellm_pre_call_utils.py index 5f5ae0c64d9..664a936b540 100644 --- a/tests/test_litellm/proxy/test_litellm_pre_call_utils.py +++ b/tests/test_litellm/proxy/test_litellm_pre_call_utils.py @@ -280,6 +280,172 @@ async def test_add_litellm_data_to_request_strips_admin_injection_slots(): assert "_pipeline_managed_guardrails" not in other +@pytest.mark.asyncio +async def test_add_litellm_data_to_request_strips_all_user_api_key_prefix_keys(): + """Strip must cover the full user_api_key_* family, not a hand-maintained + list of 2-3 names. Proxy writes a dozen such fields (user_id, alias, + spend, team_id, request_route, …) and an attacker populating any of them + in the non-authoritative metadata key would otherwise forge identity / + spend in audit logs and guardrails.""" + from litellm.proxy.litellm_pre_call_utils import add_litellm_data_to_request + + request_mock = MagicMock(spec=Request) + request_mock.url.path = "/v1/chat/completions" + request_mock.url = MagicMock() + request_mock.url.__str__.return_value = "http://localhost/v1/chat/completions" + request_mock.method = "POST" + request_mock.query_params = {} + request_mock.headers = {"Content-Type": "application/json"} + request_mock.client = MagicMock() + request_mock.client.host = "127.0.0.1" + + attacker_injected = { + "user_api_key_user_id": "victim", + "user_api_key_alias": "admin-key", + "user_api_key_spend": 0.0, + "user_api_key_team_id": "victim-team", + "user_api_key_end_user_id": "victim-user", + "user_api_key_request_route": "/fake/route", + "user_api_key_hash": "fake-hash", + } + data = { + "model": "gpt-3.5-turbo", + "metadata": {**attacker_injected}, + "litellm_metadata": {**attacker_injected}, + } + + user_api_key_dict = UserAPIKeyAuth( + api_key="hashed-key", + user_id="real-user", + metadata={}, + team_metadata={}, + spend=42.0, + max_budget=100.0, + model_max_budget={}, + team_spend=0.0, + team_max_budget=200.0, + ) + + updated = await add_litellm_data_to_request( + data=data, + request=request_mock, + user_api_key_dict=user_api_key_dict, + proxy_config=MagicMock(), + general_settings={}, + version="test-version", + ) + + # The non-authoritative metadata dict must not retain ANY attacker-injected + # user_api_key_* key. + other = updated.get("litellm_metadata") or {} + attacker_leaks = [k for k in other if k.startswith("user_api_key_")] + assert attacker_leaks == [], f"Unexpected leaked keys: {attacker_leaks}" + + +@pytest.mark.asyncio +async def test_add_litellm_data_to_request_string_metadata_does_not_crash(): + """Regression: pre-strip code that pre-populated data['metadata'][k]=v + before the string-to-dict parse would crash on JSON-string metadata. + The snapshot / strip / admin-population pipeline must survive metadata + arriving as a string.""" + import json as _json + + from litellm.proxy.litellm_pre_call_utils import add_litellm_data_to_request + + request_mock = MagicMock(spec=Request) + request_mock.url.path = "/v1/chat/completions" + request_mock.url = MagicMock() + request_mock.url.__str__.return_value = "http://localhost/v1/chat/completions" + request_mock.method = "POST" + request_mock.query_params = {} + request_mock.headers = {"Content-Type": "multipart/form-data"} + request_mock.client = MagicMock() + request_mock.client.host = "127.0.0.1" + + data = { + "model": "gpt-3.5-turbo", + "metadata": _json.dumps({"generation_name": "test"}), + } + + user_api_key_dict = UserAPIKeyAuth( + api_key="hashed-key", + metadata={}, + team_metadata={}, + spend=0.0, + max_budget=100.0, + model_max_budget={}, + team_spend=0.0, + team_max_budget=200.0, + ) + + # Must not raise TypeError / AttributeError. + updated = await add_litellm_data_to_request( + data=data, + request=request_mock, + user_api_key_dict=user_api_key_dict, + proxy_config=MagicMock(), + general_settings={}, + version="test-version", + ) + + # The parsed metadata should be a dict and the proxy snapshot body + # should have been taken AFTER the strip (so no leaked user_api_key_* + # from a raw string snapshot). + assert isinstance(updated["metadata"], dict) + assert updated["metadata"].get("generation_name") == "test" + + +@pytest.mark.asyncio +async def test_add_litellm_data_to_request_proxy_server_request_body_is_post_strip(): + """Regression: proxy_server_request['body'] used to be snapshotted before + the admin-slot strip, so standard_logging_object and spend-tracking + readers saw attacker-injected payload. Snapshot must now be post-strip.""" + from litellm.proxy.litellm_pre_call_utils import add_litellm_data_to_request + + request_mock = MagicMock(spec=Request) + request_mock.url.path = "/v1/chat/completions" + request_mock.url = MagicMock() + request_mock.url.__str__.return_value = "http://localhost/v1/chat/completions" + request_mock.method = "POST" + request_mock.query_params = {} + request_mock.headers = {"Content-Type": "application/json"} + request_mock.client = MagicMock() + request_mock.client.host = "127.0.0.1" + + data = { + "model": "gpt-3.5-turbo", + "metadata": {"user_api_key_user_id": "victim"}, + } + + user_api_key_dict = UserAPIKeyAuth( + api_key="hashed-key", + user_id="real-user", + metadata={}, + team_metadata={}, + spend=0.0, + max_budget=100.0, + model_max_budget={}, + team_spend=0.0, + team_max_budget=200.0, + ) + + updated = await add_litellm_data_to_request( + data=data, + request=request_mock, + user_api_key_dict=user_api_key_dict, + proxy_config=MagicMock(), + general_settings={}, + version="test-version", + ) + + snapshot_body = updated["proxy_server_request"]["body"] + assert snapshot_body is not None + snapshot_metadata = snapshot_body.get("metadata") or {} + assert "user_api_key_user_id" not in snapshot_metadata or ( + snapshot_metadata["user_api_key_user_id"] != "victim" + ) + + @pytest.mark.asyncio async def test_add_litellm_data_to_request_strips_string_encoded_admin_injection(): """Regression: metadata arriving as a JSON string (multipart/form-data or