mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-17 23:51:30 +00:00
fix: address three bugbot findings (team re-auth, provider override, proxy settings)
All three confirmed real and reproduced before fixing.
1. [High] Team allowlist bypassed AND-on-target. The rewrite re-auth called
can_key_call_model, which checks only the key's own allowlist, so team,
team-member, and project allowlists were never re-checked against the
resolved target. An unrestricted key on a team whose allowlist held only a
stale unserved name could ride the rewrite onto a deployment that team was
never granted. Now calls can_key_call_resolved_model -- the helper every
other post-resolution auth site already uses (model_group_alias rewrites,
realtime endpoints, auto-router), which runs the full key/team/member/project
chain.
2. [Medium] Index ignored the deployment's custom_llm_provider, canonicalizing
only litellm_params.model. Reproduced: a deployment with
model='claude-haiku-4-5' + custom_llm_provider='openrouter' was indexed under
('anthropic', ...), so an Anthropic-form request resolved onto OpenRouter
credentials, quota, and billing -- precisely the cross-provider hop rule 2
of the module docstring forbids. canonicalize() now takes the override and
prefers it over the provider inferred from the model string.
3. [Medium] Operators could not select model_name_resolution: strict on the
proxy. Both proxy Router construction sites passed a hardcoded
RouterGeneralSettings(async_only_mode=True) as an explicit keyword alongside
**router_params. Since router_general_settings IS an accepted config key,
setting it raised TypeError: got multiple values for keyword argument at
startup -- so the documented opt-out was unreachable either way. Added
_proxy_router_general_settings(), which forces async_only_mode (a proxy
runtime requirement) while preserving every other operator-set field, and
copies rather than mutating the caller's object.
Added 10 regression tests. Verified the two re-auth tests actually fail against
the pre-fix code (2 failed / 1 passed) rather than passing vacuously.
Gates: pytest 531 passed - ruff format clean - ruff check clean - strict
BLE001/PERF401 at base parity (2957/22) - type-discipline all LIT rules at base
parity - basedpyright new module 0 errors - router_code_coverage 0.0% untested
- proxy_server imports cleanly.
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
parent
5a677ca271
commit
9b1fdc1d8d
4 changed files with 219 additions and 12 deletions
|
|
@ -4057,6 +4057,30 @@ def _swap_in_model_cost_map(new_model_cost_map: dict) -> int:
|
|||
return fetched_model_count
|
||||
|
||||
|
||||
def _proxy_router_general_settings(
|
||||
configured: RouterGeneralSettings | Mapping[str, Any] | None,
|
||||
) -> RouterGeneralSettings:
|
||||
"""The proxy's RouterGeneralSettings, preserving operator config.
|
||||
|
||||
``async_only_mode`` is a proxy-runtime requirement -- only async clients are
|
||||
initialised on this path -- so it is always forced on. Every other field the
|
||||
operator set under ``router_settings.router_general_settings`` is kept.
|
||||
|
||||
Before this existed the proxy passed a hardcoded
|
||||
``RouterGeneralSettings(async_only_mode=True)`` as an explicit keyword
|
||||
alongside ``**router_params``, so an operator who set the key in config hit
|
||||
"got multiple values for keyword argument" at startup and had no way to set
|
||||
proxy-side router general settings at all (e.g. ``model_name_resolution``).
|
||||
"""
|
||||
if configured is None:
|
||||
return RouterGeneralSettings(async_only_mode=True)
|
||||
settings: Final = (
|
||||
RouterGeneralSettings(**configured) if isinstance(configured, Mapping) else configured.model_copy()
|
||||
)
|
||||
settings.async_only_mode = True
|
||||
return settings
|
||||
|
||||
|
||||
class ProxyConfig:
|
||||
"""
|
||||
Abstraction class on top of config loading/updating logic. Gives us one place to control all config updating logic.
|
||||
|
|
@ -5332,13 +5356,18 @@ class ProxyConfig:
|
|||
verbose_proxy_logger.warning(
|
||||
"Key '%s' is not a valid argument for Router.__init__(). Ignoring this key.", k
|
||||
)
|
||||
# `async_only_mode` is a proxy-runtime requirement (only async clients are
|
||||
# initialised here), so it is forced on. Everything else the operator set
|
||||
# under `router_settings.router_general_settings` -- e.g.
|
||||
# `model_name_resolution: strict` -- is preserved; passing the key in
|
||||
# config used to collide with this keyword and raise TypeError at startup.
|
||||
router_params["router_general_settings"] = _proxy_router_general_settings(
|
||||
router_params.get("router_general_settings")
|
||||
)
|
||||
router = litellm.Router(
|
||||
**router_params,
|
||||
assistants_config=assistants_config,
|
||||
search_tools=search_tools,
|
||||
router_general_settings=RouterGeneralSettings(
|
||||
async_only_mode=True # only init async clients
|
||||
),
|
||||
ignore_invalid_deployments=True, # don't raise an error if a deployment is invalid
|
||||
)
|
||||
|
||||
|
|
@ -5792,9 +5821,10 @@ class ProxyConfig:
|
|||
verbose_proxy_logger.debug("_model_list: %s", _model_list)
|
||||
llm_router = litellm.Router(
|
||||
model_list=_model_list,
|
||||
router_general_settings=RouterGeneralSettings(
|
||||
async_only_mode=True # only init async clients
|
||||
),
|
||||
# DB-sourced model list: no config router_settings in scope
|
||||
# here, so this is the proxy default (async_only_mode on,
|
||||
# everything else at its RouterGeneralSettings default).
|
||||
router_general_settings=_proxy_router_general_settings(None),
|
||||
search_tools=search_tools,
|
||||
ignore_invalid_deployments=True,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -250,6 +250,15 @@ async def _canonical_target_is_allowed(
|
|||
ride the rewrite onto a deployment it was never granted. (Auth ran on the
|
||||
requested string before routing, when the target group was not yet known.)
|
||||
|
||||
Uses ``can_key_call_resolved_model`` -- the same helper every other
|
||||
post-resolution auth site uses (model_group_alias rewrites, realtime
|
||||
endpoints, auto-router) -- so the key, team, team-member, and project
|
||||
allowlists are all re-checked against the target. Checking only the key
|
||||
(``can_key_call_model``) would leave a team whose allowlist holds a stale
|
||||
unserved name able to ride the rewrite onto a deployment it was never
|
||||
granted, since an unrestricted *key* on that team passes the key-level
|
||||
check on its own.
|
||||
|
||||
A denial returns False rather than raising, so the request falls through to
|
||||
the same 400 an unresolvable model gets today and the response reveals
|
||||
nothing about the target's existence.
|
||||
|
|
@ -257,13 +266,13 @@ async def _canonical_target_is_allowed(
|
|||
if user_api_key_dict is None:
|
||||
return True
|
||||
from litellm.proxy.auth.auth_checks import (
|
||||
can_key_call_model, # pyright: ignore[reportUnknownVariableType] - auth_checks is partially typed
|
||||
can_key_call_resolved_model, # pyright: ignore[reportUnknownVariableType] - auth_checks is partially typed
|
||||
)
|
||||
|
||||
try:
|
||||
await can_key_call_model(
|
||||
await can_key_call_resolved_model(
|
||||
model=canonical_target,
|
||||
llm_model_list=llm_router.get_model_list(),
|
||||
llm_model_list=llm_router.model_list,
|
||||
valid_token=user_api_key_dict,
|
||||
llm_router=llm_router,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -96,7 +96,7 @@ def _infer_provider(model: str) -> str | None:
|
|||
return custom_llm_provider or None
|
||||
|
||||
|
||||
def canonicalize(model: str) -> tuple[str, str] | None:
|
||||
def canonicalize(model: str, custom_llm_provider: str | None = None) -> tuple[str, str] | None:
|
||||
"""Reduce ``model`` to a ``(provider, canonical_name)`` identity.
|
||||
|
||||
The canonical name is the model string with any LiteLLM provider-route
|
||||
|
|
@ -104,11 +104,19 @@ def canonicalize(model: str) -> tuple[str, str] | None:
|
|||
LiteLLM's own routing syntax rather than part of the model's identity. The
|
||||
provider is carried alongside so equality checks are always provider-scoped.
|
||||
|
||||
``custom_llm_provider`` -- the deployment's explicit provider override --
|
||||
wins over whatever the model string implies. A deployment can carry a
|
||||
first-party-looking id (``claude-haiku-4-5``) while actually being served
|
||||
through Bedrock, Vertex, or OpenRouter; inferring the provider from the
|
||||
string alone would index it as Anthropic and let an Anthropic-form request
|
||||
be rewritten onto that other provider's credentials, quota, and bill --
|
||||
exactly the cross-provider hop rule 2 in the module docstring forbids.
|
||||
|
||||
Returns None when the provider cannot be inferred.
|
||||
"""
|
||||
if not model:
|
||||
return None
|
||||
provider: Final = _infer_provider(model)
|
||||
provider: Final = custom_llm_provider or _infer_provider(model)
|
||||
if provider is None:
|
||||
return None
|
||||
# get_llm_provider returns the model with its routing prefix stripped, which
|
||||
|
|
@ -183,7 +191,14 @@ def build_canonical_index(
|
|||
if not isinstance(model_group, str) or not isinstance(underlying, str):
|
||||
continue
|
||||
|
||||
identity = canonicalize(underlying)
|
||||
# An explicit provider override decides the provider; see canonicalize().
|
||||
provider_override = (
|
||||
litellm_params.get("custom_llm_provider") if isinstance(litellm_params, Mapping) else None
|
||||
) # rebind-ok: per-deployment loop variable
|
||||
identity = canonicalize(
|
||||
underlying,
|
||||
custom_llm_provider=provider_override if isinstance(provider_override, str) else None,
|
||||
)
|
||||
if model_group in group_identity and group_identity[model_group] != identity:
|
||||
# Deployments in this group disagree about what they serve; the
|
||||
# group cannot stand for a single canonical identity.
|
||||
|
|
|
|||
|
|
@ -157,6 +157,47 @@ class TestBuildIndexAndLookup:
|
|||
assert lookup(index, DATED) is None
|
||||
assert lookup(index, UNDATED) is None
|
||||
|
||||
def test_custom_llm_provider_override_decides_provider(self):
|
||||
"""Regression (I2): a deployment's explicit custom_llm_provider wins over
|
||||
whatever the model string implies. A first-party-looking id served via
|
||||
OpenRouter/Bedrock/Vertex must not be indexed as Anthropic -- otherwise
|
||||
an Anthropic-form request rides the rewrite onto that other provider's
|
||||
credentials, quota, and bill, which is exactly the cross-provider hop
|
||||
rule 2 forbids."""
|
||||
index = build_canonical_index(
|
||||
[
|
||||
{
|
||||
"model_name": "haiku-via-openrouter",
|
||||
"litellm_params": {
|
||||
"model": "claude-haiku-4-5",
|
||||
"custom_llm_provider": "openrouter",
|
||||
},
|
||||
},
|
||||
]
|
||||
)
|
||||
# Indexed under the real provider, not the one the string implies.
|
||||
assert ("openrouter", UNDATED) in index
|
||||
assert ("anthropic", UNDATED) not in index
|
||||
# An Anthropic-form request must not reach the OpenRouter deployment.
|
||||
assert lookup(index, DATED) is None
|
||||
assert lookup(index, UNDATED) is None
|
||||
|
||||
def test_custom_llm_provider_override_still_resolves_within_provider(self):
|
||||
"""The override narrows the provider, it does not disable resolution:
|
||||
a request that infers to the same overridden provider still resolves."""
|
||||
index = build_canonical_index(
|
||||
[
|
||||
{
|
||||
"model_name": "haiku-via-bedrock",
|
||||
"litellm_params": {
|
||||
"model": "claude-haiku-4-5",
|
||||
"custom_llm_provider": "bedrock",
|
||||
},
|
||||
},
|
||||
]
|
||||
)
|
||||
assert index[("bedrock", UNDATED)] == "haiku-via-bedrock"
|
||||
|
||||
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
|
||||
|
|
@ -410,3 +451,115 @@ class TestAuthAndOnTarget:
|
|||
)
|
||||
is True
|
||||
)
|
||||
|
||||
|
||||
class TestCanonicalTargetReAuth:
|
||||
"""The rewrite's re-auth must use the *resolved-model* helper.
|
||||
|
||||
Regression: the hook originally called ``can_key_call_model``, which checks
|
||||
only the key's own allowlist. Team, team-member, and project allowlists were
|
||||
therefore never re-checked against the resolved target, so an unrestricted
|
||||
key on a team whose allowlist held only a stale unserved name could ride the
|
||||
rewrite onto a deployment that team was never granted.
|
||||
``can_key_call_resolved_model`` is the helper every other post-resolution
|
||||
auth site uses (model_group_alias rewrites, realtime endpoints, auto-router).
|
||||
"""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_uses_resolved_model_helper_so_team_scope_is_rechecked(self, anthropic_router: Router):
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
from litellm.proxy.route_llm_request import _canonical_target_is_allowed
|
||||
|
||||
with patch(
|
||||
"litellm.proxy.auth.auth_checks.can_key_call_resolved_model",
|
||||
new=AsyncMock(return_value=None),
|
||||
) as resolved_check:
|
||||
allowed = await _canonical_target_is_allowed(
|
||||
canonical_target=ANTHROPIC_GROUP,
|
||||
llm_router=anthropic_router,
|
||||
user_api_key_dict=UserAPIKeyAuth(token="t", team_id="team-A"),
|
||||
)
|
||||
|
||||
assert allowed is True
|
||||
resolved_check.assert_awaited_once()
|
||||
assert resolved_check.await_args.kwargs["model"] == ANTHROPIC_GROUP
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_denial_declines_rewrite_rather_than_raising(self, anthropic_router: Router):
|
||||
"""A denial must return False (request falls through to the usual 400),
|
||||
never propagate an exception that would leak the target's existence."""
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
|
||||
from litellm.proxy.route_llm_request import _canonical_target_is_allowed
|
||||
|
||||
with patch(
|
||||
"litellm.proxy.auth.auth_checks.can_key_call_resolved_model",
|
||||
new=AsyncMock(side_effect=ProxyException(message="denied", type="auth_error", param=None, code=401)),
|
||||
):
|
||||
allowed = await _canonical_target_is_allowed(
|
||||
canonical_target=ANTHROPIC_GROUP,
|
||||
llm_router=anthropic_router,
|
||||
user_api_key_dict=UserAPIKeyAuth(token="t", team_id="team-A"),
|
||||
)
|
||||
|
||||
assert allowed is False
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_auth_context_is_allowed(self, anthropic_router: Router):
|
||||
"""No key context (non-proxy Router use) leaves the rewrite unguarded by
|
||||
key auth, matching the surrounding call path."""
|
||||
from litellm.proxy.route_llm_request import _canonical_target_is_allowed
|
||||
|
||||
assert (
|
||||
await _canonical_target_is_allowed(
|
||||
canonical_target=ANTHROPIC_GROUP,
|
||||
llm_router=anthropic_router,
|
||||
user_api_key_dict=None,
|
||||
)
|
||||
is True
|
||||
)
|
||||
|
||||
|
||||
class TestProxyRouterGeneralSettings:
|
||||
"""Operators must be able to set model_name_resolution on the proxy.
|
||||
|
||||
Regression: the proxy passed a hardcoded RouterGeneralSettings(async_only_mode=True)
|
||||
as an explicit keyword alongside **router_params, so setting
|
||||
router_general_settings in config raised "got multiple values for keyword
|
||||
argument" at startup -- leaving no way to select 'strict'.
|
||||
"""
|
||||
|
||||
def test_config_settings_preserved_and_async_only_forced(self):
|
||||
from litellm.proxy.proxy_server import _proxy_router_general_settings
|
||||
|
||||
settings = _proxy_router_general_settings({"model_name_resolution": "strict"})
|
||||
assert settings.model_name_resolution == "strict"
|
||||
assert settings.async_only_mode is True
|
||||
|
||||
def test_async_only_mode_cannot_be_disabled_by_config(self):
|
||||
from litellm.proxy.proxy_server import _proxy_router_general_settings
|
||||
|
||||
settings = _proxy_router_general_settings({"async_only_mode": False, "model_name_resolution": "strict"})
|
||||
assert settings.async_only_mode is True
|
||||
assert settings.model_name_resolution == "strict"
|
||||
|
||||
def test_none_yields_proxy_default(self):
|
||||
from litellm.proxy.proxy_server import _proxy_router_general_settings
|
||||
|
||||
settings = _proxy_router_general_settings(None)
|
||||
assert settings.async_only_mode is True
|
||||
assert settings.model_name_resolution == "canonical"
|
||||
|
||||
def test_model_instance_is_not_mutated(self):
|
||||
from litellm.proxy.proxy_server import _proxy_router_general_settings
|
||||
from litellm.types.router import RouterGeneralSettings
|
||||
|
||||
original = RouterGeneralSettings(async_only_mode=False, model_name_resolution="strict")
|
||||
settings = _proxy_router_general_settings(original)
|
||||
assert settings.async_only_mode is True
|
||||
# The caller's object must not be rewritten at a distance.
|
||||
assert original.async_only_mode is False
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue