mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-07 08:26:10 +00:00
fix(mcp): reserve mcp for the aggregate AS and root-path the discovery challenges
Two RFC 9728 / 8414 discovery fixes on the aggregate front door, both raised by Bugbot on this PR
The aggregate authorization-server document at /.well-known/oauth-authorization-server/mcp used to
defer to a per-server row literally named "mcp", serving issuer {base} while the aggregate
protected-resource document advertises {base}/mcp as its authorization server. A spec client
following that chain fails the RFC 8414 issuer check and cannot sign in. The single segment /mcp is
now reserved for the aggregate so the issuer stays {base}/mcp and matches the protected-resource
document; a server named "mcp" keeps its standard two-segment discovery at
/.well-known/oauth-authorization-server/mcp/mcp
The 401 challenges built the resource_metadata URL as {base}/.well-known/oauth-protected-resource/mcp
with no SERVER_ROOT_PATH segment, but the routes are registered with the path-inserted root segment,
so a proxy mounted under a sub-path pointed DCR clients at a URL that 404s. Both the aggregate
challenge and the pre-existing per-server pass-through challenge now derive the path from one
well_known_root_suffix helper that the route registrations also use, so the advertised URL cannot
drift from the served route
This commit is contained in:
parent
14b1647cd6
commit
5e1050709d
6 changed files with 87 additions and 52 deletions
|
|
@ -12,6 +12,7 @@ import litellm
|
|||
from litellm._logging import verbose_logger
|
||||
from litellm.proxy._experimental.mcp_server.oauth_utils import (
|
||||
get_request_base_url,
|
||||
well_known_root_suffix,
|
||||
)
|
||||
from litellm.proxy._experimental.mcp_server.outbound_credentials.bridge_credentials import (
|
||||
BridgeEnvelopeAdmitted,
|
||||
|
|
@ -157,7 +158,9 @@ def _aggregate_gateway_dcr_challenge(request: Request, invalid_token: bool) -> H
|
|||
spec-compliant clients to re-authorize rather than retry; a request with
|
||||
no credentials at all gets the bare challenge per RFC 6750 section 3.1."""
|
||||
error_attr = 'error="invalid_token", ' if invalid_token else ""
|
||||
resource_metadata_url = f"{get_request_base_url(request)}/.well-known/oauth-protected-resource/mcp"
|
||||
resource_metadata_url = (
|
||||
f"{get_request_base_url(request)}/.well-known/oauth-protected-resource{well_known_root_suffix()}/mcp"
|
||||
)
|
||||
return HTTPException(
|
||||
status_code=401,
|
||||
detail={
|
||||
|
|
|
|||
|
|
@ -43,6 +43,7 @@ from litellm.proxy._experimental.mcp_server.oauth_utils import (
|
|||
TOKEN_NO_CACHE_HEADERS,
|
||||
get_request_base_url,
|
||||
validate_trusted_redirect_uri,
|
||||
well_known_root_suffix,
|
||||
)
|
||||
from litellm.proxy.auth.ip_address_utils import IPAddressUtils
|
||||
from litellm.proxy.common_utils.encrypt_decrypt_utils import (
|
||||
|
|
@ -50,7 +51,6 @@ from litellm.proxy.common_utils.encrypt_decrypt_utils import (
|
|||
encrypt_value_helper,
|
||||
)
|
||||
from litellm.proxy.common_utils.http_parsing_utils import _read_request_body
|
||||
from litellm.proxy.utils import get_server_root_path
|
||||
from litellm.types.mcp import MCPAuth, MCPCredentials
|
||||
from litellm.types.mcp_server.mcp_server_manager import MCPServer
|
||||
|
||||
|
|
@ -1882,31 +1882,13 @@ def _build_aggregate_authorization_server_response(request: Request) -> dict:
|
|||
}
|
||||
|
||||
|
||||
def _mcp_named_server_exists(request: Request) -> bool:
|
||||
"""True when a server literally named ``mcp`` is configured and visible to this caller.
|
||||
|
||||
Its per-server authorization-server document is served at
|
||||
``/.well-known/oauth-authorization-server/mcp``, a single segment that collides with the
|
||||
aggregate path. When such a server exists the real server wins the route, so that
|
||||
deployment keeps its per-server discovery regardless of whether the aggregate front door
|
||||
is on."""
|
||||
from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( # noqa: PLC0415 # circular import with mcp_server_manager at module load
|
||||
global_mcp_server_manager,
|
||||
)
|
||||
|
||||
client_ip = IPAddressUtils.get_mcp_client_ip(request)
|
||||
return global_mcp_server_manager.get_mcp_server_by_name("mcp", client_ip=client_ip) is not None
|
||||
|
||||
|
||||
# RFC 9728 path-appended discovery for the aggregate /mcp endpoint. A client
|
||||
# pointed at {base}/mcp inserts the well-known segment before the resource
|
||||
# path, so this exact route must exist for aggregate discovery to work at all.
|
||||
# Declared before the parameterized well-known routes below: Starlette matches
|
||||
# in registration order, and /.well-known/oauth-authorization-server/{name}
|
||||
# would otherwise capture the "/mcp" suffix as a server name.
|
||||
@router.get(
|
||||
f"/.well-known/oauth-protected-resource{'' if get_server_root_path() == '/' else get_server_root_path()}/mcp"
|
||||
)
|
||||
@router.get(f"/.well-known/oauth-protected-resource{well_known_root_suffix()}/mcp")
|
||||
async def oauth_protected_resource_aggregate(request: Request):
|
||||
"""
|
||||
OAuth protected resource discovery for the aggregate /mcp endpoint.
|
||||
|
|
@ -1918,28 +1900,26 @@ async def oauth_protected_resource_aggregate(request: Request):
|
|||
return _build_aggregate_protected_resource_response(request)
|
||||
|
||||
|
||||
@router.get(
|
||||
f"/.well-known/oauth-authorization-server{'' if get_server_root_path() == '/' else get_server_root_path()}/mcp"
|
||||
)
|
||||
@router.get(f"/.well-known/oauth-authorization-server{well_known_root_suffix()}/mcp")
|
||||
async def oauth_authorization_server_aggregate(request: Request):
|
||||
"""
|
||||
OAuth authorization server discovery for the aggregate /mcp endpoint, the RFC 8414
|
||||
path-inserted form for a client that treats {base}/mcp as its authorization base URL.
|
||||
|
||||
This single-segment path collides with the parameterized ``/{mcp_server_name}`` route
|
||||
below, so a server literally named ``mcp`` wins it and keeps its per-server discovery;
|
||||
only when no such server exists is the aggregate document served.
|
||||
The single-segment /mcp is reserved for the aggregate so the discovery chain stays
|
||||
consistent: the aggregate protected-resource document advertises {base}/mcp as its
|
||||
authorization server, so the document served here must have issuer {base}/mcp. A server
|
||||
literally named ``mcp`` therefore does not take this route; it keeps its standard
|
||||
two-segment discovery at /.well-known/oauth-authorization-server/mcp/mcp. Letting the
|
||||
per-server row win here instead would serve an issuer of {base} against a resource that
|
||||
advertised {base}/mcp, which fails the RFC 8414 issuer check and breaks the front door.
|
||||
"""
|
||||
if _mcp_named_server_exists(request):
|
||||
return _build_oauth_authorization_server_response(request=request, mcp_server_name="mcp")
|
||||
return _build_aggregate_authorization_server_response(request)
|
||||
|
||||
|
||||
# Standard MCP pattern: /.well-known/oauth-protected-resource/mcp/{server_name}
|
||||
# This is the pattern expected by standard MCP clients (mcp-inspector, VSCode Copilot)
|
||||
@router.get(
|
||||
f"/.well-known/oauth-protected-resource{'' if get_server_root_path() == '/' else get_server_root_path()}/mcp/{{mcp_server_name}}"
|
||||
)
|
||||
@router.get(f"/.well-known/oauth-protected-resource{well_known_root_suffix()}/mcp/{{mcp_server_name}}")
|
||||
async def oauth_protected_resource_mcp_standard(request: Request, mcp_server_name: str):
|
||||
"""
|
||||
OAuth protected resource discovery endpoint using standard MCP URL pattern.
|
||||
|
|
@ -1959,9 +1939,7 @@ async def oauth_protected_resource_mcp_standard(request: Request, mcp_server_nam
|
|||
|
||||
# LiteLLM legacy pattern: /.well-known/oauth-protected-resource/{server_name}/mcp
|
||||
# Kept for backward compatibility with existing deployments
|
||||
@router.get(
|
||||
f"/.well-known/oauth-protected-resource{'' if get_server_root_path() == '/' else get_server_root_path()}/{{mcp_server_name}}/mcp"
|
||||
)
|
||||
@router.get(f"/.well-known/oauth-protected-resource{well_known_root_suffix()}/{{mcp_server_name}}/mcp")
|
||||
@router.get("/.well-known/oauth-protected-resource")
|
||||
async def oauth_protected_resource_mcp(request: Request, mcp_server_name: Optional[str] = None):
|
||||
"""
|
||||
|
|
@ -2031,9 +2009,7 @@ def _build_oauth_authorization_server_response(
|
|||
|
||||
|
||||
# Standard MCP pattern: /.well-known/oauth-authorization-server/mcp/{server_name}
|
||||
@router.get(
|
||||
f"/.well-known/oauth-authorization-server{'' if get_server_root_path() == '/' else get_server_root_path()}/mcp/{{mcp_server_name}}"
|
||||
)
|
||||
@router.get(f"/.well-known/oauth-authorization-server{well_known_root_suffix()}/mcp/{{mcp_server_name}}")
|
||||
async def oauth_authorization_server_mcp_standard(request: Request, mcp_server_name: str):
|
||||
"""
|
||||
OAuth authorization server discovery endpoint using standard MCP URL pattern.
|
||||
|
|
@ -2048,9 +2024,7 @@ async def oauth_authorization_server_mcp_standard(request: Request, mcp_server_n
|
|||
|
||||
|
||||
# LiteLLM legacy pattern and root endpoint
|
||||
@router.get(
|
||||
f"/.well-known/oauth-authorization-server{'' if get_server_root_path() == '/' else get_server_root_path()}/{{mcp_server_name}}"
|
||||
)
|
||||
@router.get(f"/.well-known/oauth-authorization-server{well_known_root_suffix()}/{{mcp_server_name}}")
|
||||
@router.get("/.well-known/oauth-authorization-server")
|
||||
async def oauth_authorization_server_mcp(request: Request, mcp_server_name: Optional[str] = None):
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -132,6 +132,18 @@ def get_request_base_url(request: Request) -> str:
|
|||
return urlunparse((scheme, _strip_default_port(scheme, netloc), parsed.path, "", "", ""))
|
||||
|
||||
|
||||
def well_known_root_suffix() -> str:
|
||||
"""The ``SERVER_ROOT_PATH`` segment inserted into a ``.well-known`` path (RFC 8414 / 9728
|
||||
path insertion), empty for a root-mounted proxy or an explicit ``/``.
|
||||
|
||||
The discovery route registrations and the 401 challenges that advertise those routes both
|
||||
derive their path from this one function, so the ``resource_metadata`` URL a client is told
|
||||
to fetch cannot drift from the route that actually serves it.
|
||||
"""
|
||||
root = os.getenv("SERVER_ROOT_PATH", "")
|
||||
return "" if root == "/" else root
|
||||
|
||||
|
||||
def validate_loopback_redirect_uri(redirect_uri: str) -> None:
|
||||
"""Require a loopback ``redirect_uri`` (OAuth 2.1 §4.1.2.1 + RFC 8252
|
||||
§7.3 native-app pattern). MCP clients are native apps that listen on
|
||||
|
|
|
|||
|
|
@ -48,6 +48,7 @@ from litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp import (
|
|||
)
|
||||
from litellm.proxy._experimental.mcp_server.discoverable_endpoints import (
|
||||
get_request_base_url,
|
||||
well_known_root_suffix,
|
||||
)
|
||||
from litellm.proxy._experimental.mcp_server.exceptions import (
|
||||
MCPToolResultError,
|
||||
|
|
@ -3525,9 +3526,10 @@ if MCP_AVAILABLE:
|
|||
base_url = get_request_base_url(request)
|
||||
_path = scope.get("_original_path") or scope.get("path", "") or ""
|
||||
|
||||
suffix = well_known_root_suffix()
|
||||
if _path.startswith(f"/{server_name}/mcp"):
|
||||
return f"{base_url}/.well-known/oauth-protected-resource/{server_name}/mcp"
|
||||
return f"{base_url}/.well-known/oauth-protected-resource/mcp/{server_name}"
|
||||
return f"{base_url}/.well-known/oauth-protected-resource{suffix}/{server_name}/mcp"
|
||||
return f"{base_url}/.well-known/oauth-protected-resource{suffix}/mcp/{server_name}"
|
||||
|
||||
def _get_passthrough_www_authenticate(
|
||||
scope: Scope,
|
||||
|
|
|
|||
|
|
@ -6189,6 +6189,23 @@ class TestAggregateGatewayDcrChallenge:
|
|||
www_authenticate = (exc_info.value.headers or {})["WWW-Authenticate"]
|
||||
assert www_authenticate == f'Bearer error="invalid_token", {self._EXPECTED_RESOURCE_METADATA}'
|
||||
|
||||
async def test_challenge_inserts_server_root_path(self):
|
||||
"""With SERVER_ROOT_PATH set the resource_metadata URL must carry the same path-inserted
|
||||
root segment the aggregate PRM route is registered with (both derive it from
|
||||
well_known_root_suffix), so a DCR client behind a sub-path is pointed at a route that
|
||||
exists instead of a 404. Regression: the challenge used to hard-code /mcp and omit the
|
||||
root path the route inserts."""
|
||||
import os
|
||||
|
||||
with (
|
||||
patch.dict(os.environ, {"SERVER_ROOT_PATH": "/litellm"}),
|
||||
patch(self._AUTH_PATCH_TARGET, side_effect=self._auth_401()),
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await MCPRequestHandler.process_mcp_request(self._scope())
|
||||
www_authenticate = (exc_info.value.headers or {})["WWW-Authenticate"]
|
||||
assert 'resource_metadata="http://testserver/.well-known/oauth-protected-resource/litellm/mcp"' in www_authenticate
|
||||
|
||||
async def test_no_challenge_for_explicit_litellm_key(self):
|
||||
"""An explicit x-litellm-api-key declares a litellm-key client; a typo
|
||||
there must surface the real auth error, never a DCR challenge that
|
||||
|
|
|
|||
|
|
@ -7163,11 +7163,13 @@ def test_aggregate_wellknown_routes_serve_gateway_metadata():
|
|||
assert "none" in asm.json()["token_endpoint_auth_methods_supported"]
|
||||
|
||||
|
||||
def test_as_aggregate_route_prefers_a_real_server_named_mcp():
|
||||
"""A server literally named ``mcp`` wins the single-segment
|
||||
/.well-known/oauth-authorization-server/mcp route (it collides with the parameterized
|
||||
/{server_name} route) and keeps its per-server discovery; the aggregate document is
|
||||
served only when no such server exists."""
|
||||
def test_as_aggregate_route_reserves_mcp_for_the_aggregate():
|
||||
"""The single-segment /.well-known/oauth-authorization-server/mcp is reserved for the
|
||||
aggregate even when a server is literally named ``mcp``. The aggregate protected-resource
|
||||
document advertises {base}/mcp as its authorization server, so the document served here
|
||||
must carry issuer {base}/mcp for the RFC 8414 issuer check to pass. Letting the per-server
|
||||
row win (issuer {base}) breaks that chain, so the aggregate wins and the mcp-named server
|
||||
keeps its standard two-segment discovery at /.well-known/oauth-authorization-server/mcp/mcp."""
|
||||
from fastapi import FastAPI
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
|
|
@ -7186,14 +7188,39 @@ def test_as_aggregate_route_prefers_a_real_server_named_mcp():
|
|||
try:
|
||||
asm = client.get("/.well-known/oauth-authorization-server/mcp")
|
||||
assert asm.status_code == 200
|
||||
# the real server's own document (issuer is the bare origin, endpoint is /mcp/authorize),
|
||||
# not the aggregate one (whose issuer would be {base}/mcp)
|
||||
assert asm.json()["issuer"] == "http://testserver"
|
||||
assert "/mcp/authorize" in asm.json()["authorization_endpoint"]
|
||||
# the aggregate document, whose issuer matches what the aggregate PRM advertises
|
||||
assert asm.json()["issuer"] == "http://testserver/mcp"
|
||||
|
||||
prm = client.get("/.well-known/oauth-protected-resource/mcp")
|
||||
assert prm.status_code == 200
|
||||
assert prm.json()["authorization_servers"] == [asm.json()["issuer"]]
|
||||
|
||||
# the mcp-named server keeps its own document on the standard two-segment route
|
||||
per_server = client.get("/.well-known/oauth-authorization-server/mcp/mcp")
|
||||
assert per_server.status_code == 200
|
||||
assert "/mcp/authorize" in per_server.json()["authorization_endpoint"]
|
||||
finally:
|
||||
global_mcp_server_manager.registry.clear()
|
||||
|
||||
|
||||
def test_well_known_root_suffix_reflects_server_root_path():
|
||||
"""The single path segment both the discovery routes and the 401 challenges insert for RFC
|
||||
8414/9728 path insertion: empty for a root-mounted proxy or an explicit ``/``, the configured
|
||||
path otherwise. Sharing this one function is what keeps the advertised resource_metadata URL
|
||||
equal to the route that serves it."""
|
||||
import os
|
||||
from unittest.mock import patch
|
||||
|
||||
from litellm.proxy._experimental.mcp_server.oauth_utils import well_known_root_suffix
|
||||
|
||||
with patch.dict(os.environ, {"SERVER_ROOT_PATH": ""}):
|
||||
assert well_known_root_suffix() == ""
|
||||
with patch.dict(os.environ, {"SERVER_ROOT_PATH": "/"}):
|
||||
assert well_known_root_suffix() == ""
|
||||
with patch.dict(os.environ, {"SERVER_ROOT_PATH": "/litellm"}):
|
||||
assert well_known_root_suffix() == "/litellm"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_bare_origin_discovery_resolves_single_server_not_aggregate():
|
||||
"""The always-on aggregate front door must not change bare-origin discovery: with one
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue