mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-12 23:01:41 +00:00
fix(proxy): move metadata strip after JSON-string parse
Veria AI caught a bypass: metadata can arrive as a JSON string via multipart/form-data or extra_body, and the existing strip block ran before the string-to-dict parse. The isinstance(_user_meta, dict) guard returned False on the string, the strip was skipped, and then the parse turned the string into a dict — leaving user_api_key_metadata / user_api_key_team_metadata / _pipeline_managed_guardrails / tags intact in the parsed dict. Move the strip to run AFTER the parse and BEFORE the merge of litellm_metadata into data[_metadata_variable_name], closing the bypass for both raw-dict and string-encoded payloads. Regression test: test_add_litellm_data_to_request_strips_string_encoded_admin_injection.
This commit is contained in:
user
parent
9622864ef2
commit
db19f24d69
2 changed files with 122 additions and 49 deletions
|
|
@ -977,49 +977,6 @@ async def add_litellm_data_to_request( # noqa: PLR0915
|
|||
"Setting client-provided x-api-key as api_key parameter (will override deployment key)"
|
||||
)
|
||||
|
||||
# Strip internal pipeline state and admin-injection slots from user input.
|
||||
# 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.
|
||||
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)
|
||||
|
||||
# Strip caller-supplied routing/budget tags unless the admin has opted
|
||||
# this key or team in via metadata.allow_client_tags=True. Tags drive
|
||||
# tag-based routing and tag budget attribution — accepting them from
|
||||
# untrusted callers lets an attacker reach restricted deployments or
|
||||
# misattribute spend to a victim team's tag.
|
||||
_admin_allow_client_tags = False
|
||||
for _admin_meta in (
|
||||
user_api_key_dict.metadata,
|
||||
user_api_key_dict.team_metadata,
|
||||
):
|
||||
if (
|
||||
isinstance(_admin_meta, dict)
|
||||
and _admin_meta.get("allow_client_tags") is True
|
||||
):
|
||||
_admin_allow_client_tags = True
|
||||
break
|
||||
if not _admin_allow_client_tags:
|
||||
_stripped_from: List[str] = []
|
||||
for _meta_key in ("metadata", "litellm_metadata"):
|
||||
_user_meta = data.get(_meta_key)
|
||||
if isinstance(_user_meta, dict) and "tags" in _user_meta:
|
||||
_user_meta.pop("tags", None)
|
||||
_stripped_from.append(_meta_key)
|
||||
if _stripped_from:
|
||||
verbose_proxy_logger.warning(
|
||||
"Stripped caller-supplied tags from %s: this key/team does "
|
||||
"not have `allow_client_tags: true` in its metadata. Set it "
|
||||
"to opt into client-supplied routing/budget tags.",
|
||||
", ".join(_stripped_from),
|
||||
)
|
||||
|
||||
##########################################################
|
||||
# Init - Proxy Server Request
|
||||
# we do this as soon as entering so we track the original request
|
||||
|
|
@ -1126,11 +1083,61 @@ async def add_litellm_data_to_request( # noqa: PLR0915
|
|||
)
|
||||
else:
|
||||
data["litellm_metadata"] = parsed_litellm_metadata
|
||||
# Merge litellm_metadata into the metadata variable (preserving existing values)
|
||||
if isinstance(data["litellm_metadata"], dict):
|
||||
for key, value in data["litellm_metadata"].items():
|
||||
if key not in data[_metadata_variable_name]:
|
||||
data[_metadata_variable_name][key] = value
|
||||
|
||||
# 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.
|
||||
#
|
||||
# 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.
|
||||
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)
|
||||
|
||||
# Strip caller-supplied routing/budget tags unless the admin has opted
|
||||
# this key or team in via metadata.allow_client_tags=True. Tags drive
|
||||
# tag-based routing and tag budget attribution — accepting them from
|
||||
# untrusted callers lets an attacker reach restricted deployments or
|
||||
# misattribute spend to a victim team's tag.
|
||||
_admin_allow_client_tags = False
|
||||
for _admin_meta in (
|
||||
user_api_key_dict.metadata,
|
||||
user_api_key_dict.team_metadata,
|
||||
):
|
||||
if (
|
||||
isinstance(_admin_meta, dict)
|
||||
and _admin_meta.get("allow_client_tags") is True
|
||||
):
|
||||
_admin_allow_client_tags = True
|
||||
break
|
||||
if not _admin_allow_client_tags:
|
||||
_stripped_from: List[str] = []
|
||||
for _meta_key in ("metadata", "litellm_metadata"):
|
||||
_user_meta = data.get(_meta_key)
|
||||
if isinstance(_user_meta, dict) and "tags" in _user_meta:
|
||||
_user_meta.pop("tags", None)
|
||||
_stripped_from.append(_meta_key)
|
||||
if _stripped_from:
|
||||
verbose_proxy_logger.warning(
|
||||
"Stripped caller-supplied tags from %s: this key/team does "
|
||||
"not have `allow_client_tags: true` in its metadata. Set it "
|
||||
"to opt into client-supplied routing/budget tags.",
|
||||
", ".join(_stripped_from),
|
||||
)
|
||||
|
||||
# Now merge litellm_metadata into the metadata variable (preserving existing
|
||||
# values) — runs AFTER the strip so attacker injections in litellm_metadata
|
||||
# cannot cross-contaminate the admin-authoritative metadata dict.
|
||||
if "litellm_metadata" in data and isinstance(data["litellm_metadata"], dict):
|
||||
for key, value in data["litellm_metadata"].items():
|
||||
if key not in data[_metadata_variable_name]:
|
||||
data[_metadata_variable_name][key] = value
|
||||
|
||||
data = LiteLLMProxyRequestSetup.add_user_api_key_auth_to_request_metadata(
|
||||
data=data,
|
||||
|
|
|
|||
|
|
@ -280,6 +280,70 @@ 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_string_encoded_admin_injection():
|
||||
"""Regression: metadata arriving as a JSON string (multipart/form-data or
|
||||
extra_body) must not bypass the admin-injection strip. The parse happens
|
||||
AFTER receipt, so the strip has to run after the parse, not before.
|
||||
"""
|
||||
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"
|
||||
|
||||
# Attacker encodes an admin-injection payload inside a JSON string.
|
||||
attacker_payload = {
|
||||
"user_api_key_metadata": {"disable_global_guardrails": True},
|
||||
"user_api_key_team_metadata": {"disable_global_guardrails": True},
|
||||
"_pipeline_managed_guardrails": ["evaded"],
|
||||
}
|
||||
data = {
|
||||
"model": "gpt-3.5-turbo",
|
||||
"metadata": json.dumps(attacker_payload),
|
||||
"litellm_metadata": json.dumps(attacker_payload),
|
||||
}
|
||||
|
||||
real_admin_metadata = {"admin_flag": "from_proxy"}
|
||||
user_api_key_dict = UserAPIKeyAuth(
|
||||
api_key="hashed-key",
|
||||
metadata=real_admin_metadata,
|
||||
team_metadata=real_admin_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",
|
||||
)
|
||||
|
||||
populated = updated["metadata"]
|
||||
# The real admin payload from user_api_key_dict wins.
|
||||
assert populated["user_api_key_metadata"] == real_admin_metadata
|
||||
assert populated["user_api_key_team_metadata"] == real_admin_metadata
|
||||
assert populated.get("_pipeline_managed_guardrails") != ["evaded"]
|
||||
|
||||
other = updated.get("litellm_metadata") or {}
|
||||
# After the strip, litellm_metadata has no admin-injection slots.
|
||||
assert "user_api_key_metadata" not in other
|
||||
assert "user_api_key_team_metadata" not in other
|
||||
assert "_pipeline_managed_guardrails" not in other
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_add_litellm_data_to_request_strips_user_tags_without_permission():
|
||||
"""Caller-supplied metadata.tags must be stripped when the key/team
|
||||
|
|
@ -486,9 +550,11 @@ async def test_add_litellm_data_to_request_audio_transcription_multipart():
|
|||
"file": b"Fake audio bytes",
|
||||
}
|
||||
|
||||
# Opt the key in to client-supplied tags so the parsed tags from the
|
||||
# JSON-string multipart body aren't stripped by the admin-injection strip.
|
||||
user_api_key_dict = UserAPIKeyAuth(
|
||||
api_key="hashed-key",
|
||||
metadata={},
|
||||
metadata={"allow_client_tags": True},
|
||||
team_metadata={},
|
||||
spend=0.0,
|
||||
max_budget=100.0,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue