From 84e2d4e23cdc2cf5dd0c5b176827476553e08072 Mon Sep 17 00:00:00 2001 From: abhi Date: Sat, 15 Aug 2026 11:18:57 -0700 Subject: [PATCH] fix: address review findings (team-scoping leak, sentinel collision, log dedup) Three issues flagged by automated PR review (greptile-apps, veria-ai), all confirmed real and reproduced before fixing: 1. [High/Security] Team-owned deployments entered the global canonical index unconditionally. A no-team (unrestricted) key could request an unclaimed spelling of a model (e.g. the dated Anthropic ID) whose only server was a team's private deployment, resolve onto it, and use that team's credentials/quota -- target-authorization passes for unrestricted keys and doesn't itself re-derive team ownership. Fixed by excluding any deployment with model_info.team_id set from the index entirely: a team boundary is an access/billing boundary exactly like the cross-provider boundary this module already respects, and team-scoped models remain reachable exactly as before, via team_public_model_name through the existing team-route machinery this module never touches. 2. [P1] Sentinel collision defeated the ambiguity guard: index.get(key, '__absent__') treated a model group literally named '__absent__' as a missing entry, so a second group with the same identity would silently overwrite it instead of triggering the ambiguity decline. Fixed with a proper 'in' check. 3. [P2] Log deduplication was keyed on target alone, so a second distinct requested spelling resolving to an already-logged target never got its own log line -- undercounting the (requested, target) cardinality that's the whole point of the observability story (sizing follow-up-resolution demand). Now keyed on the (requested, target) pair. Added 5 regression tests reproducing each bug pre-fix and asserting the fixed behavior. Full affected suite: 522 passed. router_code_coverage: 0.0% untested. ruff-strict BLE001/PERF401: unchanged at base parity. basedpyright: new module 0 errors. Co-Authored-By: Claude --- litellm/router.py | 16 ++- .../canonical_model_resolution.py | 23 +++- .../test_router_canonical_model_resolution.py | 102 ++++++++++++++++++ 3 files changed, 133 insertions(+), 8 deletions(-) diff --git a/litellm/router.py b/litellm/router.py index 8f02a5255e1..94ca7854233 100644 --- a/litellm/router.py +++ b/litellm/router.py @@ -638,9 +638,14 @@ class Router: # group info cache and on cost-map mutation (generation counter). self._canonical_model_index: dict[tuple[str, str], str | None] | None = None self._canonical_model_index_cost_generation: int = -1 - # Targets already announced at INFO, so a hot path logs once per target - # rather than once per request. - self._canonical_resolution_logged: set[str] = set() + # (requested, target) pairs already announced at INFO, so a hot path + # logs once per distinct pair rather than once per request. Keyed on + # the pair, not just the target, so each new requested spelling that + # resolves to an already-seen target is still observable -- this is + # the signal used to size demand for possible follow-up resolution + # rules, so a second spelling silently sharing the first's log line + # would undercount it. + self._canonical_resolution_logged: set[tuple[str, str]] = set() self._init_routing_groups(None) self.deployment_affinity_ttl_seconds = deployment_affinity_ttl_seconds @@ -10758,8 +10763,9 @@ class Router: # A target whose deployments have all been removed is not a live route. if not self.model_name_to_deployment_indices.get(target): return None - if target not in self._canonical_resolution_logged: - self._canonical_resolution_logged.add(target) + log_key: Final = (model, target) + if log_key not in self._canonical_resolution_logged: + self._canonical_resolution_logged.add(log_key) verbose_router_logger.info("canonical-resolution: '%s' -> '%s'", model, target) return target diff --git a/litellm/router_utils/canonical_model_resolution.py b/litellm/router_utils/canonical_model_resolution.py index ebb4688aeb1..82344910bc9 100644 --- a/litellm/router_utils/canonical_model_resolution.py +++ b/litellm/router_utils/canonical_model_resolution.py @@ -146,6 +146,17 @@ def build_canonical_index( the same canonical identity; mixed groups are skipped. When two groups claim one identity the entry is set to ``_AMBIGUOUS`` (None) so lookups decline. + Team-owned deployments (``model_info.team_id`` set) are never indexed. A + team boundary is an operator-drawn access/billing boundary exactly like a + provider boundary (see the module docstring's rule 2): auto-resolution must + not cross it. Concretely, without this exclusion a global (no-team) key + could request a team's deployment under an unclaimed spelling -- e.g. the + dated Anthropic ID -- and land on that team's credentials and quota, since + ``is_recognized_model``/target-authorization checks pass for unrestricted + keys and don't themselves re-derive team ownership. A team-scoped model + remains reachable exactly as it is today: by its team_public_model_name, + through the existing team-route machinery, which this module never touches. + Never raises: a malformed deployment or cost-map entry degrades to a smaller index, never to a router that fails to boot. """ @@ -154,6 +165,11 @@ def build_canonical_index( for deployment in deployments: try: + model_info = deployment.get("model_info") or {} + if isinstance( # pyright: ignore[reportUnnecessaryIsInstance] - config/DB rows can violate the TypedDict + model_info, Mapping + ) and model_info.get("team_id"): + continue # ``model_name``/``model`` are typed Required[str], but this index is # built from operator config and DB rows that can violate the type, # so both are validated at runtime rather than trusted. @@ -196,10 +212,11 @@ def build_canonical_index( for spelling in spellings: key = (provider, spelling) - existing = index.get(key, "__absent__") - if existing == "__absent__": + if key not in index: index[key] = model_group - elif existing != model_group: + continue + existing = index[key] + if existing != model_group: # Two groups, same identity: decline rather than choose. index[key] = _AMBIGUOUS verbose_router_logger.info( diff --git a/tests/test_litellm/router_utils/test_router_canonical_model_resolution.py b/tests/test_litellm/router_utils/test_router_canonical_model_resolution.py index c9b06888312..93109f56d04 100644 --- a/tests/test_litellm/router_utils/test_router_canonical_model_resolution.py +++ b/tests/test_litellm/router_utils/test_router_canonical_model_resolution.py @@ -136,6 +136,70 @@ class TestBuildIndexAndLookup: assert lookup(index, DATED) is None assert lookup(index, UNDATED) is None + def test_ambiguity_guard_not_defeated_by_group_named_like_sentinel(self): + """Regression: the ambiguity check used to compare against the string + '__absent__' as an "is this key missing" sentinel. A model group + literally named '__absent__' collided with that sentinel and could + silently overwrite a same-identity entry instead of triggering the + ambiguity decline.""" + index = build_canonical_index( + [ + { + "model_name": "__absent__", + "litellm_params": {"model": "anthropic/claude-haiku-4-5"}, + }, + { + "model_name": "haiku-2", + "litellm_params": {"model": "anthropic/claude-haiku-4-5"}, + }, + ] + ) + assert lookup(index, DATED) is None + assert lookup(index, UNDATED) is None + + def test_team_owned_deployment_never_indexed(self): + """Regression: a team-owned deployment (model_info.team_id set) must + never enter the global canonical index. Without this, a no-team key + could request an unclaimed spelling of a model whose only server is a + team's private deployment and land on that team's credentials/quota -- + a team boundary is an access/billing boundary exactly like the + cross-provider boundary and must not be crossed by inference.""" + index = build_canonical_index( + [ + { + "model_name": "internal-team-model-xyz", + "litellm_params": {"model": "anthropic/claude-haiku-4-5"}, + "model_info": { + "team_id": "team-A", + "team_public_model_name": "claude-haiku-4-5", + }, + }, + ] + ) + assert lookup(index, DATED) is None + assert lookup(index, UNDATED) is None + + def test_team_owned_deployment_does_not_block_global_sibling(self): + """A team-owned deployment coexisting with a global deployment of the + same identity must not suppress resolution to the global one.""" + index = build_canonical_index( + [ + { + "model_name": "internal-team-model-xyz", + "litellm_params": {"model": "anthropic/claude-haiku-4-5"}, + "model_info": { + "team_id": "team-A", + "team_public_model_name": "claude-haiku-4-5", + }, + }, + { + "model_name": ANTHROPIC_GROUP, + "litellm_params": {"model": "anthropic/claude-haiku-4-5"}, + }, + ] + ) + assert lookup(index, DATED) == ANTHROPIC_GROUP + def test_request_for_own_group_name_is_not_a_rewrite(self): index = build_canonical_index( [ @@ -169,6 +233,44 @@ class TestRouterResolveCanonicalModelName: """I1: a name the router already serves is never rewritten.""" assert anthropic_router.resolve_canonical_model_name(ANTHROPIC_GROUP) is None + def test_no_team_caller_never_resolves_onto_team_owned_deployment(self): + """Regression: end-to-end version of the team-leak fix. A no-team + caller asking for an unclaimed spelling must not resolve onto a + deployment that is the sole server of that identity but is owned by a + team -- that would leak the team's credentials/quota to a global key.""" + router = Router( + model_list=[ + { + "model_name": "internal-team-model-xyz", + "litellm_params": {"model": "anthropic/claude-haiku-4-5", "api_key": "team-secret"}, + "model_info": {"team_id": "team-A", "team_public_model_name": "claude-haiku-4-5"}, + }, + ] + ) + assert router.resolve_canonical_model_name(DATED, request_team_id=None) is None + # Even the requesting team's own id must not resolve through this path -- + # team-scoped models are reached via the existing team-route machinery, + # not via canonical inference. + assert router.resolve_canonical_model_name(DATED, request_team_id="team-A") is None + + def test_log_dedup_keyed_on_pair_not_target_alone(self, anthropic_router: Router, caplog: pytest.LogCaptureFixture): + """Regression: a second distinct requested spelling resolving to an + already-logged target must still get its own log line -- the + (requested, target) cardinality is the signal used to size demand for + follow-up resolution rules, so deduping on target alone would + undercount it.""" + import logging + + with caplog.at_level(logging.INFO, logger="LiteLLM Router"): + assert anthropic_router.resolve_canonical_model_name(DATED) == ANTHROPIC_GROUP + assert anthropic_router.resolve_canonical_model_name(UNDATED) == ANTHROPIC_GROUP + # Re-requesting the same spelling must not double-log. + assert anthropic_router.resolve_canonical_model_name(DATED) == ANTHROPIC_GROUP + messages = [r.message for r in caplog.records if "canonical-resolution" in r.message] + assert any(DATED in m for m in messages) + assert any(UNDATED in m for m in messages) + assert sum(1 for m in messages if DATED in m) == 1 + def test_strict_mode_disables_resolution(self): from litellm.types.router import RouterGeneralSettings