mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-06 02:48:13 +00:00
fix(proxy): stop a caller-supplied tag from shadowing a policy-backed identity tag
extract_identity/entry_applies (this module's own functions) resolve a tag_id via first-match-by-prefix over metadata.tags, but _merge_tags (litellm_pre_call_utils.py) keeps caller-supplied tags ahead of key/team/ project tags in that merged list. An authenticated caller could submit e.g. company_id:attacker-chosen ahead of the calling key's real company_id:real-company tag and have every rate-limit entry scoped to company_id resolve to the caller's own value instead of the key's. Adds order_tags_for_identity_resolution, which puts metadata.inherited_tags (the server-computed snapshot of only the tags the calling key/team/project's own config contributed) ahead of the full tags list before either lookup runs. veria-ai caught this while reviewing #38292 (whose branch currently carries this module's commits); porting the fix here since the vulnerable functions it defends are this PR's own. #38292 will wire the call sites in once it rebases onto this branch instead of carrying its own duplicate copy.
This commit is contained in:
parent
6862980ea3
commit
529104d65f
2 changed files with 59 additions and 0 deletions
|
|
@ -247,6 +247,28 @@ def extract_key_alias(request_kwargs: Mapping[str, object], metadata_variable_na
|
|||
return key_alias if isinstance(key_alias, str) else None
|
||||
|
||||
|
||||
def order_tags_for_identity_resolution(
|
||||
tags: Sequence[str], request_kwargs: Mapping[str, object], metadata_variable_name: str
|
||||
) -> tuple[str, ...]:
|
||||
"""`extract_identity`/`entry_applies` both resolve a `tag_id` via
|
||||
first-match-by-prefix. `_merge_tags` (litellm_pre_call_utils.py) appends
|
||||
key/team/project tags only if not already present, keeping caller-supplied
|
||||
tags first in the merged `tags` list -- so an authenticated caller could
|
||||
submit e.g. `company_id:attacker-chosen` ahead of the calling key's real
|
||||
`company_id:real-company` tag and have every entry scoped to `company_id`
|
||||
resolve to the caller's own value instead of the key's. `metadata.inherited_tags`
|
||||
is a separate, server-computed snapshot of only the tags the calling
|
||||
key/team/project's own config contributed (see that field's docstring in
|
||||
litellm_pre_call_utils.py), so putting it first makes a policy-backed tag
|
||||
win over a same-prefix caller-supplied one.
|
||||
"""
|
||||
active: Final = request_kwargs.get(metadata_variable_name) or EMPTY_MAPPING
|
||||
inherited_tags: Final = active.get("inherited_tags") if isinstance(active, Mapping) else None
|
||||
if not isinstance(inherited_tags, (list, tuple)) or not inherited_tags:
|
||||
return tuple(tags)
|
||||
return tuple(dict.fromkeys((*inherited_tags, *tags)))
|
||||
|
||||
|
||||
def fixed_length_identity(tag_value: str) -> str:
|
||||
"""
|
||||
`tag_value` is caller-controlled (whatever follows the tag_id prefix in
|
||||
|
|
|
|||
|
|
@ -14,6 +14,7 @@ from litellm.proxy.hooks.tag_rate_limits_shared import (
|
|||
extract_identity,
|
||||
extract_key_hash,
|
||||
fixed_length_identity,
|
||||
order_tags_for_identity_resolution,
|
||||
partition_key,
|
||||
)
|
||||
from litellm.types.router import TagRateLimitEntry, TagRateLimitScope
|
||||
|
|
@ -36,6 +37,42 @@ def test_extract_identity_skips_negation_tags():
|
|||
assert extract_identity(["!end_user_id:u1"], "end_user_id") is None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# order_tags_for_identity_resolution -- veria-ai finding on PR #38292: a
|
||||
# caller-supplied tag must not shadow a policy-backed (key/team/project)
|
||||
# tag sharing the same tag_id prefix
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_order_tags_for_identity_resolution_prefers_inherited_tag_over_caller_supplied():
|
||||
request_kwargs = {"metadata": {"inherited_tags": ["company_id:real-company"]}}
|
||||
tags = ["company_id:attacker-chosen", "end_user_id:u1"]
|
||||
ordered = order_tags_for_identity_resolution(tags, request_kwargs, "metadata")
|
||||
assert extract_identity(ordered, "company_id") == "real-company"
|
||||
|
||||
|
||||
def test_order_tags_for_identity_resolution_falls_back_to_caller_tags_when_nothing_inherited():
|
||||
request_kwargs = {"metadata": {}}
|
||||
tags = ["end_user_id:u1"]
|
||||
ordered = order_tags_for_identity_resolution(tags, request_kwargs, "metadata")
|
||||
assert extract_identity(ordered, "end_user_id") == "u1"
|
||||
|
||||
|
||||
def test_order_tags_for_identity_resolution_keeps_caller_only_tags_not_shadowed_by_a_different_tag_id():
|
||||
request_kwargs = {"metadata": {"inherited_tags": ["company_id:real-company"]}}
|
||||
tags = ["end_user_id:u1"]
|
||||
ordered = order_tags_for_identity_resolution(tags, request_kwargs, "metadata")
|
||||
assert extract_identity(ordered, "end_user_id") == "u1"
|
||||
assert extract_identity(ordered, "company_id") == "real-company"
|
||||
|
||||
|
||||
def test_order_tags_for_identity_resolution_deduplicates_identical_tag_present_in_both_sources():
|
||||
request_kwargs = {"metadata": {"inherited_tags": ["company_id:real-company"]}}
|
||||
tags = ["company_id:real-company"]
|
||||
ordered = order_tags_for_identity_resolution(tags, request_kwargs, "metadata")
|
||||
assert ordered.count("company_id:real-company") == 1
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# fixed_length_identity -- tag_value is caller-controlled with no length
|
||||
# bound; a hook's own contribution to a cache key must not grow with it
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue