mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-03 02:22:24 +00:00
fix(security): only reorder pass-through routes that actually overlap a generic provider route
Addresses the veria-ai automated security review on PR #38017 (https://github.com/BerriAI/litellm/pull/38017#discussion_r4003591579). _move_before_generic_provider_routes previously moved ANY newly-appended route ahead of the first route containing "{provider}" in its path, unconditionally. Since insertion is positional, this also promoted the new route ahead of every other route registered afterward -- including unrelated, authenticated built-in routes. A wildcard pass-through configured with an overlapping prefix (e.g. "/key/{subpath:path}") would shadow "/key/generate" and receive request bodies meant for the management API. Now the candidate generic route's template must actually match the new route's own path (via the same route.matches() check the test file's _resolve_route_name helper already uses) before any reordering happens. A route whose own path never structurally resembles a "/{provider}/..." template -- any wildcard/parameterized path -- is left exactly where it was appended, same as when no generic route exists at all. Adds test_wildcard_pass_through_does_not_shadow_unrelated_built_in_routes covering the "/key/{subpath:path}" vs "/key/generate" scenario from the review.
This commit is contained in:
parent
d38ec59f5e
commit
76f78727ee
2 changed files with 69 additions and 10 deletions
|
|
@ -26,6 +26,7 @@ from fastapi import (
|
|||
)
|
||||
from fastapi.responses import StreamingResponse
|
||||
from starlette.datastructures import UploadFile as StarletteUploadFile
|
||||
from starlette.routing import Match
|
||||
from starlette.websockets import WebSocketState
|
||||
from websockets.asyncio.client import connect
|
||||
from websockets.exceptions import (
|
||||
|
|
@ -2772,24 +2773,44 @@ class SafeRouteAdder:
|
|||
https://github.com/BerriAI/litellm/issues/37925).
|
||||
|
||||
Move the just-appended route (the last item in app.routes) to sit immediately
|
||||
before the first such generic route, so it is matched first instead. If no
|
||||
generic provider route is registered (e.g. a minimal deployment), leave the
|
||||
route appended -- current behavior is preserved as a safe fallback.
|
||||
before the first generic route whose template would actually capture this
|
||||
route's own path, so it is matched first instead. A route is moved only when
|
||||
that genuine overlap exists: reordering unconditionally on any "{provider}"
|
||||
sighting would also promote unrelated wildcard pass-throughs (e.g. a
|
||||
"/key/{subpath:path}" entry) ahead of every route registered afterward,
|
||||
including unrelated, authenticated built-in routes like "/key/generate" --
|
||||
a real route.matches() check on this route's own path rules that out, since
|
||||
a route path containing its own "{...}" placeholder never structurally
|
||||
matches a "/{provider}/..." template. If no generic provider route captures
|
||||
this path (including when none is registered at all, e.g. a minimal
|
||||
deployment), leave the route appended -- current behavior is preserved as a
|
||||
safe fallback.
|
||||
|
||||
Builds the reordered list in one expression and reassigns app.router.routes
|
||||
wholesale, rather than mutating the existing list in place with pop()/insert().
|
||||
"""
|
||||
routes: Final = app.routes
|
||||
new_route: Final = routes[-1]
|
||||
scope: Final = {
|
||||
"type": "http",
|
||||
"method": "GET",
|
||||
"path": new_route.path,
|
||||
"headers": [],
|
||||
"query_string": b"",
|
||||
"root_path": "",
|
||||
}
|
||||
for index, route in enumerate(routes[:-1]):
|
||||
route_path = getattr(route, "path", None)
|
||||
if route_path and SafeRouteAdder._GENERIC_PROVIDER_PATH_MARKER in route_path:
|
||||
app.router.routes = [ # mutable-ok: framework's list # rebind-ok: reordering is the fix
|
||||
*routes[:index],
|
||||
new_route,
|
||||
*routes[index:-1],
|
||||
]
|
||||
return
|
||||
if not (route_path and SafeRouteAdder._GENERIC_PROVIDER_PATH_MARKER in route_path):
|
||||
continue
|
||||
if route.matches(scope)[0] == Match.NONE:
|
||||
continue
|
||||
app.router.routes = [ # mutable-ok: framework's list # rebind-ok: reordering is the fix
|
||||
*routes[:index],
|
||||
new_route,
|
||||
*routes[index:-1],
|
||||
]
|
||||
return
|
||||
|
||||
@staticmethod
|
||||
def add_api_route_if_not_exists(
|
||||
|
|
|
|||
|
|
@ -3451,6 +3451,44 @@ def test_custom_pass_through_endpoint_prefix_wins_over_native_provider_routes():
|
|||
]
|
||||
|
||||
|
||||
def test_wildcard_pass_through_does_not_shadow_unrelated_built_in_routes():
|
||||
"""
|
||||
A pass_through_endpoints entry with a wildcard subpath (e.g. "/key/{subpath:path}")
|
||||
must not be promoted ahead of unrelated, authenticated built-in routes like
|
||||
"/key/generate" just because it lands after the first "/{provider}/..." route in
|
||||
app.routes once appended. Its own path never structurally matches a
|
||||
"/{provider}/..." template, so it must be left exactly where it was appended.
|
||||
"""
|
||||
from litellm.proxy.pass_through_endpoints.pass_through_endpoints import (
|
||||
InitPassThroughEndpointHelpers,
|
||||
)
|
||||
from litellm.proxy.proxy_server import app
|
||||
|
||||
key_generate_route_name_before = _resolve_route_name("POST", "/key/generate")
|
||||
assert key_generate_route_name_before is not None
|
||||
|
||||
added_paths = {"/key/{subpath:path}"}
|
||||
try:
|
||||
InitPassThroughEndpointHelpers.add_exact_path_route(
|
||||
app=app,
|
||||
path="/key/{subpath:path}",
|
||||
target="https://example.com/",
|
||||
custom_headers=None,
|
||||
forward_headers=False,
|
||||
merge_query_params=False,
|
||||
dependencies=None,
|
||||
cost_per_request=None,
|
||||
endpoint_id="test-key-wildcard",
|
||||
)
|
||||
|
||||
assert _resolve_route_name("POST", "/key/generate") == key_generate_route_name_before
|
||||
finally:
|
||||
app.router.routes = [
|
||||
route for route in app.router.routes
|
||||
if getattr(route, "path", None) not in added_paths
|
||||
]
|
||||
|
||||
|
||||
def test_move_before_generic_provider_routes_is_a_no_op_without_a_generic_route():
|
||||
"""
|
||||
If no generic "/{provider}/..." route is registered on the app (e.g. a minimal
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue