mirror of
https://github.com/BerriAI/litellm.git
synced 2026-08-28 05:25:59 +00:00
fix(mcp): strip root_path before matching the per-server MCP route spelling (#35576)
* fix(mcp): strip root_path before matching the per-server MCP route spelling
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`
* fix(mcp): make the gateway-managed 401 challenge root-path aware
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>
---------
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
tin-berri
GitHub
parent
dd64331967
commit
d193c7aefe
4 changed files with 115 additions and 4 deletions
|
|
@ -180,6 +180,22 @@ def well_known_root_suffix() -> str:
|
|||
return "" if root == "/" else root
|
||||
|
||||
|
||||
def get_route_relative_request_path(scope: Scope) -> str:
|
||||
"""The request path the MCP route shapes are written against: the raw ASGI path with the
|
||||
deployment's ``root_path`` removed.
|
||||
|
||||
``scope["path"]`` and ``_original_path`` are both raw request-line paths, so on a sub-path
|
||||
deployment they still carry the ``SERVER_ROOT_PATH`` prefix (``/litellm/{server}/mcp``) while
|
||||
every route shape compared against them is root-relative. Mirrors the segment-boundary strip in
|
||||
:func:`litellm.proxy.auth.auth_utils.get_request_route`, which the rest of the MCP auth path
|
||||
already routes through, so ``/litellmfoo`` is not truncated under ``root_path=/litellm``."""
|
||||
raw_path = str(scope.get("_original_path") or scope.get("path", "") or "")
|
||||
root_path = str(scope.get("app_root_path") or scope.get("root_path") or "").rstrip("/")
|
||||
if root_path and (raw_path == root_path or raw_path.startswith(f"{root_path}/")):
|
||||
return raw_path[len(root_path) :]
|
||||
return raw_path
|
||||
|
||||
|
||||
def get_passthrough_resource_metadata_url(scope: Scope, server_name: str) -> str:
|
||||
"""The per-server protected-resource metadata URL matching the spelling the request
|
||||
arrived on, so a strict RFC 9728 client resolves the same route the proxy registered.
|
||||
|
|
@ -188,7 +204,7 @@ def get_passthrough_resource_metadata_url(scope: Scope, server_name: str) -> str
|
|||
the route decorators insert it (see :func:`well_known_root_suffix`)."""
|
||||
request: Final = Request(scope)
|
||||
base_url: Final = get_request_base_url(request)
|
||||
_path: Final = scope.get("_original_path") or scope.get("path", "") or ""
|
||||
_path: Final = get_route_relative_request_path(scope)
|
||||
|
||||
if _path.startswith(f"/{server_name}/mcp"):
|
||||
return f"{base_url}/.well-known/oauth-protected-resource{well_known_root_suffix()}/{server_name}/mcp"
|
||||
|
|
|
|||
|
|
@ -51,6 +51,8 @@ from litellm.proxy._experimental.mcp_server.mcp_debug import MCPDebug
|
|||
from litellm.proxy._experimental.mcp_server.oauth_utils import (
|
||||
_redact_mcp_resource_url,
|
||||
get_passthrough_www_authenticate,
|
||||
get_route_relative_request_path,
|
||||
well_known_root_suffix,
|
||||
)
|
||||
from litellm.proxy._experimental.mcp_server.utils import (
|
||||
LITELLM_MCP_SERVER_DESCRIPTION,
|
||||
|
|
@ -3782,14 +3784,15 @@ if MCP_AVAILABLE:
|
|||
|
||||
request = StarletteRequest(scope)
|
||||
base_url = get_request_base_url(request)
|
||||
_path = scope.get("_original_path") or scope.get("path", "") or ""
|
||||
_path = get_route_relative_request_path(scope)
|
||||
|
||||
# Pick the well-known AS-metadata form that matches the inbound route
|
||||
# so strict RFC 9728 §3.2 clients can resolve it correctly.
|
||||
as_metadata_root = f"{base_url}/.well-known/oauth-authorization-server{well_known_root_suffix()}"
|
||||
if _path.startswith(f"/mcp/{server_name}"):
|
||||
_as_url = f"{base_url}/.well-known/oauth-authorization-server/mcp/{server_name}"
|
||||
_as_url = f"{as_metadata_root}/mcp/{server_name}"
|
||||
else:
|
||||
_as_url = f"{base_url}/.well-known/oauth-authorization-server/{server_name}"
|
||||
_as_url = f"{as_metadata_root}/{server_name}"
|
||||
authorization_uri = f'Bearer authorization_uri="{_as_url}"'
|
||||
|
||||
raise HTTPException(
|
||||
|
|
|
|||
|
|
@ -6314,6 +6314,49 @@ class TestAggregateGatewayDcrChallenge:
|
|||
www_authenticate = (exc_info.value.headers or {})["WWW-Authenticate"]
|
||||
assert www_authenticate == f'Bearer resource_metadata="http://testserver{expected_metadata_path}"'
|
||||
|
||||
async def test_per_server_challenge_keeps_spelling_under_server_root_path(self):
|
||||
"""On a sub-path deployment the challenge must still advertise the spelling the client
|
||||
used. ``_original_path`` is a raw request-line path, so under SERVER_ROOT_PATH it reads
|
||||
``/litellm/{server}/mcp``; matching that against the root-relative ``/{server}/mcp`` shape
|
||||
used to fail, silently pointing a legacy-spelling client at the standard-pattern document
|
||||
whose ``resource`` is ``{base}/mcp/{server}`` rather than the ``{base}/{server}/mcp`` URL it
|
||||
called, which a strict RFC 9728 section 3 client rejects."""
|
||||
import os
|
||||
|
||||
from litellm.types.mcp import MCPAuth
|
||||
from litellm.types.mcp_server.mcp_server_manager import MCPServer
|
||||
|
||||
server = MCPServer(
|
||||
server_id="gh-id",
|
||||
name="github",
|
||||
server_name="github",
|
||||
url="https://upstream.example/mcp",
|
||||
transport="http",
|
||||
auth_type=MCPAuth.oauth2,
|
||||
)
|
||||
for original_path, expected_metadata_path in (
|
||||
("/litellm/mcp/github", "/litellm/.well-known/oauth-protected-resource/litellm/mcp/github"),
|
||||
("/litellm/github/mcp", "/litellm/.well-known/oauth-protected-resource/litellm/github/mcp"),
|
||||
):
|
||||
scope = {
|
||||
**self._scope(path="/mcp/github"),
|
||||
"root_path": "/litellm",
|
||||
"_original_path": original_path,
|
||||
}
|
||||
with (
|
||||
patch.dict(os.environ, {"SERVER_ROOT_PATH": "/litellm"}),
|
||||
patch(self._AUTH_PATCH_TARGET, side_effect=self._auth_401()),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager"
|
||||
) as mock_mgr,
|
||||
):
|
||||
mock_mgr.get_mcp_server_by_name.return_value = server
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await MCPRequestHandler.process_mcp_request(scope)
|
||||
assert exc_info.value.status_code == 401
|
||||
www_authenticate = (exc_info.value.headers or {})["WWW-Authenticate"]
|
||||
assert www_authenticate == f'Bearer resource_metadata="http://testserver{expected_metadata_path}"'
|
||||
|
||||
async def test_no_per_server_challenge_for_non_gateway_managed_targets(self):
|
||||
"""The per-server challenge fires only for the server set the gateway's keyless flow
|
||||
serves: an OBO server and a multi-server CSV path keep the original admission error
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import asyncio
|
||||
import contextvars
|
||||
import os
|
||||
from datetime import datetime, timedelta
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
|
|
@ -8094,6 +8095,54 @@ class TestPreemptive401ModeAware:
|
|||
assert exc.value.status_code == 401
|
||||
assert "www-authenticate" in {k.lower() for k in exc.value.headers}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"original_path, expected_as_path",
|
||||
(
|
||||
("/litellm/mcp/interactive", "/litellm/.well-known/oauth-authorization-server/litellm/mcp/interactive"),
|
||||
("/litellm/interactive/mcp", "/litellm/.well-known/oauth-authorization-server/litellm/interactive"),
|
||||
),
|
||||
)
|
||||
async def test_gateway_as_metadata_challenge_under_server_root_path(self, original_path, expected_as_path):
|
||||
"""Under SERVER_ROOT_PATH the challenge must keep the spelling the client called and point at
|
||||
a route the proxy registered, so it has to compare a route-relative path and carry the root suffix."""
|
||||
from litellm.proxy._experimental.mcp_server import server as server_module
|
||||
|
||||
server = _make_oauth2_server("interactive", oauth2_flow="authorization_code")
|
||||
scope = {
|
||||
**self._scope(server.alias),
|
||||
"root_path": "/litellm",
|
||||
"_original_path": original_path,
|
||||
"headers": [(b"host", b"testserver")],
|
||||
}
|
||||
with (
|
||||
patch.dict(os.environ, {"SERVER_ROOT_PATH": "/litellm"}),
|
||||
patch.object(
|
||||
server_module.global_mcp_server_manager,
|
||||
"get_mcp_server_by_name",
|
||||
return_value=server,
|
||||
),
|
||||
patch.object(
|
||||
server_module.global_mcp_server_manager,
|
||||
"has_user_oauth_token",
|
||||
new_callable=AsyncMock,
|
||||
return_value=False,
|
||||
),
|
||||
pytest.raises(HTTPException) as exc,
|
||||
):
|
||||
await server_module._raise_preemptive_401_for_unauthenticated_servers(
|
||||
scope=scope,
|
||||
mcp_servers=[server.alias],
|
||||
oauth2_headers=None,
|
||||
mcp_server_auth_headers=None,
|
||||
user_api_key_auth=UserAPIKeyAuth(api_key="sk-litellm-virtual-key"),
|
||||
client_ip=None,
|
||||
)
|
||||
|
||||
assert exc.value.status_code == 401
|
||||
headers = {k.lower(): v for k, v in (exc.value.headers or {}).items()}
|
||||
assert headers["www-authenticate"] == f'Bearer authorization_uri="http://testserver{expected_as_path}"'
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_gateway_managed_interactive_no_token_challenges_with_authorization_bearer(self):
|
||||
"""The bug fix: no stored token, key in Authorization (oauth2_headers
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue