Skip to content

chore(proxy): cherry-pick #28547 onto patch/v1.84.1#28967

Merged
yuneng-berri merged 3 commits into
patch/v1.84.1from
litellm_yj/cherry-pick-28547-v1.84.1
May 27, 2026
Merged

chore(proxy): cherry-pick #28547 onto patch/v1.84.1#28967
yuneng-berri merged 3 commits into
patch/v1.84.1from
litellm_yj/cherry-pick-28547-v1.84.1

Conversation

@yuneng-berri

Copy link
Copy Markdown
Collaborator

Backport of #28547 (d480ffda3c) onto patch/v1.84.1.

Routes the remaining path-dependent call sites in auth, ACL, routing, and audit-log decisions through get_request_route(request) so they read from the ASGI scope["path"] instead of request.url.path. The helper itself already exists on v1.84.1 (added by #27904 / #27878); this PR extends the helper's usage to the additional sites listed below.

Sites routed through get_request_route

  • _experimental/mcp_server/auth/user_api_key_auth_mcp.py
  • management_endpoints/mcp_management_endpoints.py
  • vector_store_endpoints/utils.py
  • pass_through_endpoints/pass_through_endpoints.py
  • auth/route_checks.py
  • litellm_pre_call_utils.py
  • spend_tracking/spend_management_endpoints.py
  • common_utils/http_parsing_utils.py
  • management_helpers/utils.py
  • health_endpoints/_health_endpoints.py

Conflict resolution

Two files conflicted because v1.84.1's base predates the delegate_auth_to_upstream feature (#27834 — not on v1.84.1):

  1. _experimental/mcp_server/auth/user_api_key_auth_mcp.py — the cherry-pick brought in a _target_servers_delegate_auth_to_upstream elif branch in process_mcp_request. That branch is feature drift from feat(mcp): add delegate_auth_to_upstream flag for PKCE passthrough #27834 and is irrelevant to the path-resolution change. Dropped the elif block; kept the get_request_route swap on the existing well-known / _target_servers_use_oauth2 call sites.
  2. management_endpoints/mcp_management_endpoints.py — the cherry-pick brought in the entire _mcp_oauth_user_api_key_auth function. That function does not exist on v1.84.1 (added by feat(mcp): add delegate_auth_to_upstream flag for PKCE passthrough #27834); the chore(proxy): route path-dependent call sites through get_request_route #28547 change inside it is just a request.url.pathget_request_route(request) swap. Dropped the function entirely.

The other 8 production files and the test file auto-merged cleanly and contain only request.url.pathget_request_route(request) swaps plus the lazy auth_utils import (no feature drift).

Test plan

  • uv run pytest tests/proxy_unit_tests/test_proxy_routes.py -v
  • make test-unit

Backport of #28547 (`d480ffda3c`) onto the `patch/v1.84.1` branch.

Routes the remaining path-dependent call sites in auth, ACL, routing,
and audit-log decisions through `get_request_route(request)` so they
read from the ASGI `scope["path"]` instead of `request.url.path`. The
helper itself already exists on v1.84.1 (added by #27904 / #27878);
this PR extends the helper's usage to the additional sites listed
below.

Sites routed through get_request_route:
- _experimental/mcp_server/auth/user_api_key_auth_mcp.py
- management_endpoints/mcp_management_endpoints.py
- vector_store_endpoints/utils.py
- pass_through_endpoints/pass_through_endpoints.py
- auth/route_checks.py
- litellm_pre_call_utils.py
- spend_tracking/spend_management_endpoints.py
- common_utils/http_parsing_utils.py
- management_helpers/utils.py
- health_endpoints/_health_endpoints.py

Regression tests in tests/proxy_unit_tests/test_proxy_routes.py
construct a Request with scope["path"] set to a benign route and the
Host header crafted so url.path would resolve differently; each
site's decision is asserted against scope["path"].

Conflict resolution
-------------------

Two files conflicted because v1.84.1's base predates the
delegate_auth_to_upstream feature (#27834 — not on v1.84.1):

1. _experimental/mcp_server/auth/user_api_key_auth_mcp.py
   The cherry-pick brought in a `_target_servers_delegate_auth_to_upstream`
   elif branch in `process_mcp_request`. That branch is feature drift
   from #27834 and is irrelevant to the path-resolution change. Dropped
   the elif block; kept the get_request_route swap on the existing
   well-known/_target_servers_use_oauth2 call sites.

2. management_endpoints/mcp_management_endpoints.py
   The cherry-pick brought in the entire `_mcp_oauth_user_api_key_auth`
   function. That function does not exist on v1.84.1 (added by #27834);
   the #28547 change inside it is just a `request.url.path` →
   `get_request_route` swap. Dropped the function entirely.

The other 8 production files and the test file auto-merged cleanly
and contain only `request.url.path` → `get_request_route(request)`
swaps plus the lazy auth_utils import (no feature drift).
@greptile-apps

greptile-apps Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This backport routes the remaining request.url.path call sites in auth, ACL, routing, and audit-log decisions through get_request_route(request), which reads from the ASGI scope[\"path\"] instead of Starlette's host-derived url.path. The conflict resolution correctly drops the delegate_auth_to_upstream elif block and the _mcp_oauth_user_api_key_auth function that are absent from v1.84.1.

  • All ten production files consistently adopt inline imports of get_request_route to break the existing auth-utils import cycle, and every request.url.path reference in auth/ACL/routing paths is replaced.
  • The new test_call_site_uses_scope_path parametrized test covers the changed call sites, though several entries (assistants, metadata, vector-store, spend-logs) pass vacuously because the crafted bypass hosts collapse url.path to \"/\" \u2014 a value that also doesn't match those patterns under the old code.

Confidence Score: 4/5

All production changes are mechanical and correct; the only concern is in test coverage quality.

The production code changes are straightforward one-for-one replacements of request.url.path with get_request_route(request), the helper implementation itself is unchanged, and the backport conflict resolution is handled properly. The sole observation is that most new parametrized test entries happen to pass for the wrong reason — the crafted bypass hosts collapse url.path to "/" before it can match the target substrings, so those tests don't actually demonstrate that the old code was vulnerable to bypass with those specific host values.

tests/proxy_unit_tests/test_proxy_routes.py — the new parametrized test entries for assistants, metadata, vector-store, and spend-logs call sites deserve closer inspection to verify they are genuinely testing bypass prevention.

Important Files Changed

Filename Overview
litellm/proxy/auth/auth_utils.py Docstring updated to explain why scope["path"] is authoritative over url.path — no logic change.
litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py Replaces request.url.path with get_request_route on two call sites; drops the delegate_auth_to_upstream elif block absent from v1.84.1 base.
litellm/proxy/auth/route_checks.py _is_assistants_api_request now reads route via get_request_route instead of request.url.path.
litellm/proxy/common_utils/http_parsing_utils.py _add_vector_store_id_from_path uses get_request_route for the regex path extraction.
litellm/proxy/health_endpoints/_health_endpoints.py test_endpoint now echoes the ASGI scope path rather than the host-derived url.path.
litellm/proxy/litellm_pre_call_utils.py _get_metadata_variable_name uses get_request_route instead of request.url.path.
litellm/proxy/management_helpers/utils.py Both success and error branches of management_endpoint_wrapper now use get_request_route for OTel logging.
litellm/proxy/pass_through_endpoints/pass_through_endpoints.py Pass-through route handler reads scope path via get_request_route for is_registered_pass_through_route check.
litellm/proxy/spend_tracking/spend_management_endpoints.py ui_view_spend_logs uses get_request_route to detect v2 path variant.
litellm/proxy/vector_store_endpoints/utils.py Both is_allowed_to_call_vector_store_endpoint and is_allowed_to_call_vector_store_files_endpoint use get_request_route for permission type resolution.
tests/proxy_unit_tests/test_proxy_routes.py Adds regression tests for the new call sites; some entries pass vacuously since the crafted bypass hosts collapse url.path to "/" which already doesn't match the target patterns.

Reviews (1): Last reviewed commit: "chore(proxy): cherry-pick #28547 onto pa..." | Re-trigger Greptile

Comment on lines +281 to +317
("assistants_classification", "/key/generate", "%s/thread", _is_assistants, False),
(
"metadata_variable_name",
"/chat/completions",
"%s/thread",
_metadata_var_name,
"metadata",
),
(
"vector_store_id_extraction",
"/key/generate",
"%s/vector_stores/x/files",
_vector_store_id_in_path,
False,
),
(
"well_known_mcp_bypass",
"/mcp/tools/call",
"/.well-known/%s",
lambda r: get_request_route(r).startswith("/.well-known/"),
False,
),
(
"pkce_token_suffix",
"/mcp/server-id/token",
"%s",
lambda r: get_request_route(r).rstrip("/").lower().endswith("/token"),
True,
),
(
"spend_logs_v2_classification",
"/spend/logs",
"%s/spend/logs/v2",
lambda r: "/spend/logs/v2" in get_request_route(r),
False,
),
("health_route_echo", "/test", "%s", lambda r: get_request_route(r), "/test"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Several _CALL_SITES entries pass vacuously against all four bypass hosts

For assistants_classification, metadata_variable_name, vector_store_id_extraction, and spend_logs_v2_classification, the bypass hosts in _BYPASS_HOSTS (e.g. localhost/?x=1) cause Starlette to collapse url.path to "/" — which already does not contain "thread", "vector_stores", or "spend/logs/v2". So both the old and new code return False/"metadata", and the test cannot distinguish between them. The expected values are correct, but the tests don't prove the fix was necessary for these particular hosts.

By contrast, well_known_mcp_bypass (with host prefix /.well-known/…) and pkce_token_suffix (collapses url.path to "/", causing a false-negative denial) each represent genuine old-vs-new divergence. Consider adding at least one host variant per remaining call site where url.path would have resolved to the dangerous substring (e.g. host localhost:4000/thread? for the assistants and metadata cases) to make the parametrized set a real regression gate rather than a correctness check.

@yuneng-berri yuneng-berri merged commit 5560f35 into patch/v1.84.1 May 27, 2026
46 of 63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant