chore(proxy): resolve request route from ASGI scope - #27879
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR closes an auth-bypass vulnerability where a malformed
Confidence Score: 5/5The change is safe to merge — it removes a concrete auth-bypass vector without altering any public API or breaking any existing call contracts. All modified call sites consistently delegate to the centralised helper; the WebSocket scope propagation is correct; the test coverage directly exercises the malicious Host header variants. No logic regressions were identified in the auth paths. No files require special attention; the two minor nits are in the test file and the fallback branch of the helper, neither of which affects production behaviour.
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/auth_utils.py | Core of the fix: get_request_route now reads scope["path"] (the authoritative ASGI value) instead of request.url.path (Host-header-influenced). Logic is correct; the exception fallback loses its prior debug log. |
| litellm/proxy/auth/user_api_key_auth.py | WebSocket synthetic HTTP request now carries path, raw_path, root_path, and query_string from the WebSocket scope so get_request_route sees the real route instead of falling back to "/". Change is correct and well-documented. |
| litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py | Replaces all three direct request.url.path reads with get_request_route(request) for the /.well-known/ bypass and the OAuth2 delegate/upstream path lookups. Straightforward and correct. |
| litellm/proxy/auth/route_checks.py | One-line change: request.url.path replaced by get_request_route(request) in the thread/assistant route classifier. Safe. |
| litellm/proxy/vector_store_endpoints/utils.py | Four request.url.path reads in the read/write ACL matchers replaced with a single get_request_route call per function. Correct; avoids redundant calls. |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | PKCE /authorize–/token anonymous-bypass branch now uses get_request_route. The removed or "" guard is safely covered by get_request_route always returning at least "/". Correct. |
| tests/test_litellm/proxy/auth/test_request_route_resolution.py | New regression test suite covering Host-header bypass variants, root_path stripping, and the WebSocket scope propagation. Uses deprecated asyncio.get_event_loop() in the WebSocket test which can cause warnings or failures on Python 3.10+. |
Reviews (2): Last reviewed commit: "fix(proxy): carry path / root_path throu..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
4d396ef to
25cc0b0
Compare
``get_request_route`` previously read ``request.url.path``, which Starlette builds by interpolating the ``Host`` header into a URL string and re-parsing with ``urlsplit``. A ``Host`` value containing ``/?`` or ``/#`` (e.g. ``localhost/?x=1``) collapses ``url.path`` to ``"/"`` — the real path falls into the query/fragment. ``"/"`` is in ``LiteLLMRoutes.public_routes``, so route-based auth gates would treat protected routes as public; FastAPI's router uses ``scope["path"]`` for dispatch, so the protected handler still executes. Read ``scope["path"]`` directly (the authoritative ASGI path that FastAPI uses for routing) and strip ``scope["root_path"]`` rather than relying on ``request.base_url.path`` (also Host-derived). The other auth-time path reads in the proxy use the same helper: - ``_experimental/mcp_server/auth/user_api_key_auth_mcp.py`` — ``/.well-known/`` public-route bypass and upstream-OAuth2 delegate lookup - ``auth/route_checks.py`` — ``thread`` / ``assistant`` route classification - ``vector_store_endpoints/utils.py`` — vector store ACL endpoint matching - ``management_endpoints/mcp_management_endpoints.py`` — PKCE ``/authorize`` / ``/token`` anonymous-bypass branch A fallback to ``request.url.path`` triggers only when ``scope`` is not a dict — the case produced by ``MagicMock(spec=Request)`` test stubs. Production ASGI scope is always a dict, so the secure read above is the only path that runs in real requests. Regression test covers the four malformed-Host shapes plus three additional variants (``user@``, ``[::1]``, backslash). The end-to-end case dispatches through the real FastAPI app via ``TestClient`` and asserts protected admin routes still return ``401``.
25cc0b0 to
3ab068b
Compare
stuxf
changed the title
``os.environ.setdefault`` only writes when the key is absent, but if it was absent the value persists for the worker lifetime — a later test asserting ``LITELLM_MASTER_KEY`` is unset would see a stale value. Snapshot and restore the affected keys in a ``finally`` block.
…c request ``user_api_key_auth_websocket`` previously built the synthetic HTTP ``Request`` from only the WebSocket scope headers. After the helper switched to reading ``scope["path"]`` rather than ``request.url.path``, the route resolved to an empty string and the auth-side classification treated the connection as the public ``/`` route. Copy ``path``, ``raw_path``, ``root_path``, and ``query_string`` from the WebSocket scope so the synthetic request carries the same routing fields a real HTTP request would. Regression test asserts the downstream auth helper sees the actual route.
|
@greptileai please re-review — the os.environ.setdefault leak was addressed in 952c080 (env snapshot + restore in fixture finalizer), and the websocket synthetic-request regression was closed in 4084911. |
|
🤖 litellm-agent: This PR is currently BLOCKED from merge. Score: 3/5 ❌ Why blocked:
Details: Score docked for: 1 PR-related CI failure (Size gate: tests (+180) exceed code (+54) by more than 3× — over-specified or feature too thin. Add the Fix the issues above and push an update — the bot will re-review automatically.
|
|
Superseded by #28547, which landed the call-site sweep through |
1 participant
Summary
scope[\"path\"]instead ofrequest.url.path. Starlette constructsrequest.urlby interpolating theHostheader into a URL string and re-parsing withurlsplit, so a malformedHost(e.g. one containing/?or/#) collapsesurl.pathto\"/\"and corrupts the value the auth gate compares against.scope[\"root_path\"]directly rather than viarequest.base_url.path(also Host-derived)._experimental/mcp_server/auth/user_api_key_auth_mcp.py(/.well-known/public-route handling and upstream-OAuth2 delegate lookup),auth/route_checks.py(thread/assistantroute classification),vector_store_endpoints/utils.py(vector store ACL endpoint matching),management_endpoints/mcp_management_endpoints.py(PKCE/authorize//tokenanonymous-bypass branch).A fallback to
request.url.pathtriggers only whenscopeisn't a dict — the case produced byMagicMock(spec=Request)test stubs. Production ASGI scope is always a dict.