From 529104d65fd2ef2ef8e5a01b784b1eecb15b661f Mon Sep 17 00:00:00 2001 From: Deepanshu Date: Wed, 26 Aug 2026 11:36:53 -0400 Subject: [PATCH] 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. --- litellm/proxy/hooks/tag_rate_limits_shared.py | 22 +++++++++++ .../hooks/test_tag_rate_limits_shared.py | 37 +++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/litellm/proxy/hooks/tag_rate_limits_shared.py b/litellm/proxy/hooks/tag_rate_limits_shared.py index 5f48f3cf1c1..34989e5bf5a 100644 --- a/litellm/proxy/hooks/tag_rate_limits_shared.py +++ b/litellm/proxy/hooks/tag_rate_limits_shared.py @@ -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 diff --git a/tests/test_litellm/proxy/hooks/test_tag_rate_limits_shared.py b/tests/test_litellm/proxy/hooks/test_tag_rate_limits_shared.py index 153737ecda1..d645b09075c 100644 --- a/tests/test_litellm/proxy/hooks/test_tag_rate_limits_shared.py +++ b/tests/test_litellm/proxy/hooks/test_tag_rate_limits_shared.py @@ -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