fix(mcp): strip root_path before matching the per-server MCP route spelling - #35576
Conversation
Greptile SummaryThe PR normalizes raw MCP request paths against the deployment root path before selecting the per-server OAuth discovery spelling
Confidence Score: 5/5The PR appears safe to merge; the normalization matches the production scope rewrite and preserves both supported MCP route spellings The helper strips only an exact root-path prefix or a prefix followed by a segment boundary, and the regression test exercises the real challenge-building caller for both affected route forms
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/oauth_utils.py | Adds narrowly scoped root-path normalization before the existing route-spelling comparison, preserving root-mounted behavior and segment boundaries |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Adds focused regression coverage through process_mcp_request for both supported per-server spellings under a deployment root path |
Reviews (1): Last reviewed commit: "fix(mcp): strip root_path before matchin..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| return "" if root == "/" else root | ||
|
|
||
|
|
||
| def get_route_relative_request_path(scope: Scope) -> str: |
There was a problem hiding this comment.
the authorization_uri branch in server.py picks its /mcp/{server_name} vs /{server_name} well-known shape off the same raw scope["_original_path"] (server.py:3767), so under SERVER_ROOT_PATH it takes the else-branch for both spellings and hands back /.well-known/oauth-authorization-server/{server}. Same mismatch this fixes, just the gateway-managed authorization_code path instead of the passthrough one — worth routing that one through get_route_relative_request_path too.
There was a problem hiding this comment.
Fixed in 91ba899: the gateway-managed authorization_code branch now builds _path via get_route_relative_request_path(scope) instead of reading scope["_original_path"]/scope["path"] raw, and the AS-metadata root now goes through well_known_root_suffix() so the URL carries the same root-path segment the discovery routes are registered under. Added TestPreemptive401ModeAware::test_gateway_as_metadata_challenge_under_server_root_path, parametrized over both spellings, pinning the fix through the real process_mcp_request path (mutation-tested: reverting either half fails the new test).
c033199 to
91ba899
Compare
|
@greptileai can you reply here with score and why |
|
bugbot run |
|
Score: 4.5 / 5 What this PR does well:
Minor concerns:
These are all minor. The core logic is sound, the proof-of-fix output is compelling, and the regression tests are structured correctly. |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 91ba899. Configure here.
…elling
The 401 challenge for a gateway-managed oauth2 MCP server advertises the
protected-resource metadata URL in the spelling the client connected on, so a
strict RFC 9728 section 3 client lands on a document whose `resource` equals the
URL it actually called. That spelling test compared `_original_path` against the
root-relative `/{server}/mcp` shape, but `_original_path` and `scope["path"]`
are raw request-line paths that still carry the deployment's `root_path`
On a SERVER_ROOT_PATH deployment the prefix therefore made the legacy test fail
and every request fell through to the standard `/mcp/{server}` branch. A client
connecting on `/litellm/github/mcp` was pointed at the standard-pattern
document, which serves `resource = {base}/litellm/mcp/github`; that is not the
URL the client called, so a strict client aborts discovery before the MCP
request fires
Route the path through `get_route_relative_request_path` first, which removes
`root_path` on a segment boundary the same way
`litellm.proxy.auth.auth_utils.get_request_route` already does for the rest of
the MCP auth path, so `/litellmfoo` is not truncated under `root_path=/litellm`
The gateway-managed authorization_code challenge in process_mcp_request
built its AS-metadata URL from two root-path-unaware pieces:
- it matched the caller's spelling against `scope["_original_path"]`, a
raw request-line path that still carries the deployment prefix, so on a
SERVER_ROOT_PATH deployment the `/mcp/{server}` branch never matched and
every request fell through to the legacy one-segment form
- it hardcoded `/.well-known/oauth-authorization-server` without the
root-path segment the discovery route decorators bake in, so the URL
404'd under a sub-path deployment regardless of which branch was taken
Route the spelling match through get_route_relative_request_path and the
well-known root through well_known_root_suffix, the same two helpers the
discovery route registrations derive their paths from, so the advertised
URL cannot drift from the route that serves it.
Root-mounted deployments are unaffected: both helpers are no-ops when
SERVER_ROOT_PATH is unset.
Co-Authored-By: Claude <noreply@anthropic.com>
91ba899 to
50b34ad
Compare
TLDR
Problem this solves:
_original_pathagainst the root-relative/{server}/mcpshape_original_path(andscope["path"]) are raw request-line paths, so on aSERVER_ROOT_PATHdeployment they still carry the prefix and that match always fails/litellm/{server}/mcpis therefore pointed at the standard-pattern discovery document, whoseresourceis{base}/litellm/mcp/{server}rather than the URL it called, and a strict RFC 9728 section 3 client aborts before the MCP request firesThe same raw-path assumption appears twice, in two different challenge shapes:
resource_metadatachallenge inoauth_utils.pyauthorization_codeauthorization_urichallenge inserver.py, which additionally hardcoded/.well-known/oauth-authorization-serverwith no root-path segment, so the URL it advertised 404'd under a sub-path deployment regardless of which spelling branch was takenHow it solves it:
root_pathfrom the path before the spelling match, on a segment boundary, the same waylitellm.proxy.auth.auth_utils.get_request_routealready does for the rest of the MCP auth pathwell_known_root_suffix(), the same helper the discovery route registrations derive their paths from, so the advertised URL cannot drift from the route that serves itroot_pathRelevant issues
SERVER_ROOT_PATH(sub-path) deploymentresourcea client discovers now equals the MCP URL it connected to, in both the/{server}/mcpand/mcp/{server}spellingsSERVER_ROOT_PATHthrough the realprocess_mcp_requestcallerauthorization_codechallenge (raised in review by @Sanjays2402), covered byTestPreemptive401ModeAware::test_gateway_as_metadata_challenge_under_server_root_pathSurfaced by the discussion on #35226, which reported the same class of
resourcemismatch. That PR proposes a new opt-in env var to derive the discovery path from the request; this change instead fixes the root-path normalization the existing code already relies on, which covers the sub-path deployment without new configurationLinear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Pass-through
resource_metadatachallengeLive proxy on a sub-path deployment, config below, started with
Before (this branch's base,
ba480a619f) both spellings collapse onto the standard-pattern document:Following the challenge a
/litellm/github/mcpclient was sent to shows the mismatch it aborts on:After (this branch) each spelling keeps its own document:
and each document's
resourceis exactly the URL the client connected to:Gateway-managed
authorization_codechallenge (the second fix site)This challenge only fires for a caller that authenticated to the proxy but has no interactive gateway session yet, so the repro needs a virtual key rather than an anonymous request. Same sub-path deployment, server named
interactive, sameoauth2/authorization_codeblock as the config aboveBefore (parent commit
e3869926f7) the advertised authorization-server document has no root-path segment, so following it 404s:After (this branch) the challenge points at a route the proxy actually registered:
The
/litellm/mcp/{server}spelling cannot be shown end to end here because it never reaches this branch under a sub-path deployment:_get_mcp_servers_in_pathsplits the raw request path, reads the root segment as the server name, and the request resolves to no server and returns an empty tool list. That is a separate pre-existing bug and is left alone in this PR; the spelling selection itself is pinned by the parametrized regression testType
🐛 Bug Fix
Changes
oauth_utils.pygainsget_route_relative_request_path, which reads_original_path(falling back toscope["path"]) and removes the deployment'sroot_pathwhen the raw path is that prefix or continues past it on a/boundary, so/litellmfoois not truncated underroot_path=/litellm.get_passthrough_resource_metadata_urlnow compares that normalized path instead of the raw oneNothing else changes. On a root-mounted proxy
root_pathis empty and the helper returns the raw path unchanged, so the emitted metadata URL is byte-identical to today'sThe regression test lives in the existing
TestAggregateGatewayDcrChallengeclass next to the non-root-path spelling test it mirrors, and drives the realprocess_mcp_requestentry point rather than the helper, so it fails if either the challenge or the spelling selection regresses. It reverts to the pre-fix assertion failure when the normalization is removedserver.py's gateway-managedauthorization_codebranch gets the same treatment:_pathnow comes fromget_route_relative_request_path(scope)instead of a rawscope["_original_path"]read, and the well-known root is built withwell_known_root_suffix()so the advertised AS-metadata URL resolves to a registered route. Its regression test lives inTestPreemptive401ModeAware, is parametrized over both spellings, and drivesprocess_mcp_requestthe same wayBoth halves of the
server.pyfix are mutation-tested: reverting either the path normalization or the root suffix on its own fails the new test. The root suffix half is also reproduced against a live proxy in the proof section aboveQA runbook
uv run --no-sync pytest tests/test_litellm/proxy/_experimental/mcp_server/auth/ tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py -qmcp_serversconfig above tomcp_rootpath_config.yamlSERVER_ROOT_PATH=/litellm LITELLM_MASTER_KEY=sk-1234 python litellm/proxy/proxy_cli.py --config mcp_rootpath_config.yaml --port 4111curlloops from the proof section and confirm the two spellings now advertise differentresource_metadataURLsGETeach advertised URL and confirmresourceequals the MCP URL from step 4SERVER_ROOT_PATHunset and confirm the challenges are unchanged from before this PR (/.well-known/oauth-protected-resource/github/mcpand/.well-known/oauth-protected-resource/mcp/github)interactiveserver with the sameoauth2/authorization_codeblock, restart underSERVER_ROOT_PATH=/litellm, and mint a virtual key withPOST /litellm/key/generatePOST /litellm/interactive/mcpwith that key and confirm theauthorization_uriin the 401 carries the/litellmroot segmentGETthat URL and confirm it returns the authorization-server metadata instead of a 404Final Attestation
Note
Medium Risk
Touches MCP OAuth 401 WWW-Authenticate URLs used by strict RFC 9728 clients; wrong URLs would break login, but the change is a path-normalization fix with regression tests and no-op on root-mounted proxies.
Overview
Fixes MCP OAuth 401 challenges on sub-path deployments (
SERVER_ROOT_PATH). Previously, spelling detection used the raw request path, so/litellm/{server}/mcpnever matched the root-relative/{server}/mcpshape and clients were sent to the wrong RFC 9728 metadata document (or a 404 AS-metadata URL).Adds
get_route_relative_request_path(segment-boundary strip, same idea asget_request_route) and uses it for both the pass-throughresource_metadatachallenge and the gateway-managedauthorization_urichallenge. The latter also builds the well-known root viawell_known_root_suffix()so advertised URLs match registered routes. Root-mounted proxies are unchanged.Also catalogs Moonshot
moonshot/kimi-k3pricing.Reviewed by Cursor Bugbot for commit 91ba899. Bugbot is set up for automated code reviews on this repo. Configure here.