diff --git a/litellm/proxy/_types.py b/litellm/proxy/_types.py index b960ca2c1b3..ca6020d1c31 100644 --- a/litellm/proxy/_types.py +++ b/litellm/proxy/_types.py @@ -834,14 +834,10 @@ class LiteLLMRoutes(enum.Enum): # handler calls _verify_team_access, which admits only a proxy admin, an # org admin for the team, or an admin of this team. # - # Two spellings per route because neither placeholder alone covers every - # team id the router accepts: the gate expands {x:path} to "[^:]+", which - # takes a slash but not a colon, and {x} to "[^/]+", which takes a colon - # but not a slash. team_id is a free-form string, so both are reachable. + # team_id is a free-form string, so it spells these with the same path + # converter the router uses; the gate matches that converter. "/team/{team_id:path}/callback", "/team/{team_id:path}/callback/{callback_name}", - "/team/{team_id}/callback", - "/team/{team_id}/callback/{callback_name}", "/model/new", "/model/update", "/model/delete", diff --git a/litellm/proxy/auth/route_checks.py b/litellm/proxy/auth/route_checks.py index 4dba2497bb9..b18a5177eec 100644 --- a/litellm/proxy/auth/route_checks.py +++ b/litellm/proxy/auth/route_checks.py @@ -497,10 +497,17 @@ class RouteChecks: def _placeholder_to_regex(match: re.Match) -> str: placeholder: Final = match.group(0).strip("{}") - if placeholder.endswith(":path"): - # allow "/" in the placeholder value, but don't eat the route suffix after ":" - return r"[^:]+" - return r"[^/]+" + if not placeholder.endswith(":path"): + return r"[^/]+" + # A ":path" placeholder takes whatever the router's own path + # converter takes, slashes and colons alike, so an id spelled with + # either (or both) still matches the template it was mounted under. + # + # Unless the template puts a ":" literal of its own after the + # placeholder: the Google routes end in ":generateContent" and + # friends, and there the value has to stop before that suffix + # rather than swallow it and match a different verb. + return r"[^:]+" if ":" in match.string[match.end() :] else r".+" pattern = re.sub(r"\{[^}]+\}", _placeholder_to_regex, pattern) # Anchor the pattern to match the entire string diff --git a/tests/test_litellm/proxy/auth/test_route_checks.py b/tests/test_litellm/proxy/auth/test_route_checks.py index 7f53a75b072..d22b7c92b3c 100644 --- a/tests/test_litellm/proxy/auth/test_route_checks.py +++ b/tests/test_litellm/proxy/auth/test_route_checks.py @@ -3551,10 +3551,13 @@ TEAM_CALLBACK_ROUTES = ( # contain a slash "/team/tenant/06bda574/callback", "/team/tenant/06bda574/callback/langfuse", - # team_id is a free-form string, so it may also contain a colon, which the - # gate's :path expansion excludes + # team_id is a free-form string, so it may also contain a colon "/team/tenant:06bda574/callback", "/team/tenant:06bda574/callback/langfuse", + # or both, which is the shape neither a "[^:]+" nor a "[^/]+" expansion + # of the placeholder reaches on its own + "/team/tenant:acme/prod/callback", + "/team/tenant:acme/prod/callback/langfuse", ) @@ -3597,8 +3600,6 @@ def test_team_callback_routes_are_self_managed(): for template in ( "/team/{team_id:path}/callback", "/team/{team_id:path}/callback/{callback_name}", - "/team/{team_id}/callback", - "/team/{team_id}/callback/{callback_name}", ): assert template in LiteLLMRoutes.self_managed_routes.value @@ -3625,6 +3626,50 @@ def test_team_callback_routes_reach_their_handler_for_non_admins(route, role): assert _gate(route, role) == "allowed" +@pytest.mark.parametrize( + "pattern, route, matches", + [ + # a :path placeholder takes what the router's path converter takes + ("/team/{team_id:path}/callback", "/team/plain/callback", True), + ("/team/{team_id:path}/callback", "/team/tenant/acme/callback", True), + ("/team/{team_id:path}/callback", "/team/tenant:acme/callback", True), + ("/team/{team_id:path}/callback", "/team/tenant:acme/prod/callback", True), + # and still has to reach the template's own suffix + ("/team/{team_id:path}/callback", "/team/tenant:acme/disable_logging", False), + # a template with a ":" literal after the placeholder keeps the suffix + ( + "/v1beta/models/{model_name:path}:generateContent", + "/v1beta/models/gemini-2.5-flash:generateContent", + True, + ), + ( + "/v1beta/models/{model_name:path}:generateContent", + "/v1beta/models/publishers/google/gemini-2.5-flash:generateContent", + True, + ), + # the value must not swallow that suffix and match a different verb + ( + "/v1beta/models/{model_name:path}:generateContent", + "/v1beta/models/gemini-2.5-flash:countTokens", + False, + ), + # an ordinary placeholder stays one segment + ("/team/{team_id}/members/me", "/team/abc/members/me", True), + ("/team/{team_id}/members/me", "/team/tenant/abc/members/me", False), + ], +) +def test_path_placeholder_matches_what_the_router_accepts(pattern, route, matches): + """The gate's placeholder expansion has to agree with the router's. + + A team id may carry a slash, a colon, or both, and the router mounted these + paths with the same :path converter, so an id the router routes must not be + an id the gate fails to recognize. The one narrowing that stays is a template + whose own suffix begins with a colon: there the value stops before it, or + ":generateContent" would also match a ":countTokens" request. + """ + assert RouteChecks._route_matches_pattern(route=route, pattern=pattern) is matches + + def test_team_disable_logging_stays_proxy_admin_only(): """disable_logging was left out of the grant, so it must still be rejected at the gate. It is the one team callback route a team admin cannot reach."""