scriptoriumadmin/litellm
1
0
Fork 0
mirror of https://github.com/BerriAI/litellm.git synced 2026-08-28 05:25:59 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 8

fix(mcp): enforce the OAuth endpoint trust rule at carry-forward too, elide default port

Browse source 
The corroboration check belongs to adopting a token_url from any non-manual
source, not to discovery alone. Carry-forward is the other such source: it
copied a prior registry entry's token_url/registration_url onto a rebuild
whose authorization_url had been re-pointed to a different server, reviving an
uncorroborated token endpoint the discovery gate would reject. Both sites now
share one predicate, _endpoints_corroborate_authorization_url: previous
endpoints carry forward only when the previous authorization_url corroborates
the authorize endpoint the build will use (absent -> the previous one is
adopted too, a consistent group; else it must match). Endpoint comparison now
elides the default port so :443 and formatting-only differences still match.
This commit is contained in:
Tin Chi Lo 2026-07-15 10:39:04 -07:00
parent a81c6ce350
commit 447d50fa40
2 changed files with 160 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

95
litellm/proxy/_experimental/mcp_server/mcp_server_manager.py
View file

@ -186,6 +186,46 @@ _UPSTREAM_OAUTH_DISCOVERY_AUTH_TYPES: tuple[MCPAuth, ...] = (
)
def _normalized_authorize_endpoint(url: str) -> str:
"""Compare authorize endpoints on scheme, host, and path only. The default port is elided and
the host is lowercased so ``https://IDP.example.com:443/authorize/`` and
``https://idp.example.com/authorize`` are the same identity; query and trailing slash are not."""
parsed = urlparse(url)
scheme = parsed.scheme.lower()
host = (parsed.hostname or "").lower()
default_port = {"https": 443, "http": 80}.get(scheme)
try:
port = parsed.port
except ValueError:
port = None
authority = host if port is None or port == default_port else f"{host}:{port}"
return f"{scheme}://{authority}{parsed.path.rstrip('/')}"
def _endpoints_corroborate_authorization_url(
source_authorization_url: str | None,
trusted_authorization_url: str | None,
) -> bool:
"""Whether a source's ``token_url``/``registration_url`` may be paired with a trusted authorize
endpoint. This is the single trust rule for adopting OAuth endpoints from any non-manual source.
Discovery is rooted at the MCP resource (RFC 9728), so a compromised upstream can advertise an
attacker-run authorization server. When ``authorization_url`` is admin-pinned, pairing it with a
``token_url`` from a different source is the RFC 9700 authorization-server mix-up: the user signs
in at the trusted authorize endpoint while the gateway redeems the code, with the stored client
secret and PKCE verifier, at the attacker's token endpoint. Endpoints are trustworthy together
only when they share an authorization server, so a source's endpoints are adopted only when the
same source advertised an ``authorization_endpoint`` matching the pinned value. With no pinned
value (``trusted_authorization_url is None``) there is nothing to protect: the authorize endpoint
comes from the same source as the token endpoint, so they corroborate each other by construction.
"""
if trusted_authorization_url is None:
return True
return bool(source_authorization_url) and _normalized_authorize_endpoint(
source_authorization_url
) == _normalized_authorize_endpoint(trusted_authorization_url)
def _carry_forward_resolved_oauth_endpoints(new_server: MCPServer, previous_server: MCPServer | None) -> None:
"""Keep the last known good OAuth endpoints when a rebuild's re-discovery comes back empty.
@ -193,57 +233,50 @@ def _carry_forward_resolved_oauth_endpoints(new_server: MCPServer, previous_serv
during re-discovery downgrades a working server (``authorization_url`` set) to a broken one
(``None``, /authorize 400s) with no configuration change. Mirrors the ``short_prefix``
carry-forward. Skipped when the server's ``url`` or ``auth_type`` changed, since the previous
endpoints may then belong to a different upstream. ``registration_url`` IS carried here even
though ``_persist_discovered_oauth_endpoints`` refuses to write it to the row: carrying only
restores the same in-memory value the previous build already ran with, while persisting it
would flip ``_dcr_bridge_relays_client_registration`` (which keys off the stored column) for
dcr_bridge servers that never had one configured.
endpoints may then belong to a different upstream. ``registration_url`` IS carried even though
``_persist_discovered_oauth_endpoints`` refuses to write it to the row: carrying only restores
the same in-memory value the previous build already ran with, while persisting it would flip
``_dcr_bridge_relays_client_registration`` (which keys off the stored column) for dcr_bridge
servers that never had one configured.
Carry-forward is a non-manual endpoint source, so the same trust rule as discovery applies: the
previous ``token_url``/``registration_url`` are carried only when the previous
``authorization_url`` corroborates the authorize endpoint this build will use, i.e. when the
incoming build has no pinned authorize endpoint (``None`` -> we adopt the previous one too, a
consistent group) or pins the same one. An admin re-pointing ``authorization_url`` to a different
server must not keep serving the old token endpoint.
"""
if previous_server is None:
return
if previous_server.url != new_server.url or previous_server.auth_type != new_server.auth_type:
return
may_carry_endpoints = _endpoints_corroborate_authorization_url(
previous_server.authorization_url, new_server.authorization_url
)
if new_server.authorization_url is None and previous_server.authorization_url:
new_server.authorization_url = previous_server.authorization_url
if new_server.token_url is None and previous_server.token_url:
if may_carry_endpoints and new_server.token_url is None and previous_server.token_url:
new_server.token_url = previous_server.token_url
if new_server.registration_url is None and previous_server.registration_url:
if may_carry_endpoints and new_server.registration_url is None and previous_server.registration_url:
new_server.registration_url = previous_server.registration_url
if not new_server.scopes and previous_server.scopes:
new_server.scopes = previous_server.scopes
def _normalized_authorize_endpoint(url: str) -> str:
parsed = urlparse(url)
return f"{parsed.scheme.lower()}://{parsed.netloc.lower()}{parsed.path.rstrip('/')}"
def _gate_discovered_endpoints_against_manual_authorization_url(
metadata: MCPOAuthMetadata | None,
manual_authorization_url: str | None,
server_identifier: str,
is_dcr_bridge: bool,
) -> MCPOAuthMetadata | None:
"""Refuse discovered token/registration endpoints the pinned authorize endpoint cannot vouch for.
Discovery is rooted at the MCP resource (RFC 9728), so a compromised upstream can advertise an
attacker-run authorization server. When the admin manually configured ``authorization_url``,
filling a blank ``token_url`` from that advertisement recreates the RFC 9700 mix-up attack at
configuration time: users sign in at the trusted authorize endpoint while the gateway redeems
the code, with the stored client secret and PKCE verifier, at the attacker's token endpoint.
Endpoints from one metadata document are only trustworthy together, so the discovered
``token_url`` and ``registration_url`` are accepted only when that same document's
``authorization_endpoint`` matches the pinned value (scheme+host+path; query and trailing slash
are not identity). Scope discovery stays ungated: scopes steer the redirect to the trusted
authorize endpoint and carry no credentials.
"""Apply :func:`_endpoints_corroborate_authorization_url` to freshly discovered metadata, the
other non-manual endpoint source. Drops the discovered ``token_url``/``registration_url`` (never
the scopes, which carry no credentials) when they cannot be vouched for by the pinned authorize
endpoint, logging why so an intentional mismatch can be resolved by setting Token URL by hand.
"""
if metadata is None or not manual_authorization_url:
if metadata is None or (not metadata.token_url and not metadata.registration_url):
return metadata
if not metadata.token_url and not metadata.registration_url:
return metadata
if metadata.authorization_url and _normalized_authorize_endpoint(
manual_authorization_url
) == _normalized_authorize_endpoint(metadata.authorization_url):
if _endpoints_corroborate_authorization_url(metadata.authorization_url, manual_authorization_url):
return metadata
bridge_note = (
" The discovered registration_url is rejected with it, so this dcr_bridge server stays on the"
@ -258,7 +291,7 @@ def _gate_discovered_endpoints_against_manual_authorization_url(
"authorization server. Configure Token URL manually if the mismatch is intentional.%s",
server_identifier,
_normalized_authorize_endpoint(metadata.authorization_url) if metadata.authorization_url else "<absent>",
_normalized_authorize_endpoint(manual_authorization_url),
_normalized_authorize_endpoint(manual_authorization_url) if manual_authorization_url else "<absent>",
bridge_note,
)
return metadata.model_copy(update={"token_url": None, "registration_url": None})

96
tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py
View file

@ -5287,6 +5287,102 @@ class TestMCPServerTimestamps:
_carry_forward_resolved_oauth_endpoints(new_server=explicit, previous_server=previous)
assert explicit.authorization_url == "https://configured.example.com/auth"
def test_carry_forward_does_not_revive_token_url_across_authorization_url_change(self):
"""Carry-forward is a non-manual endpoint source, so it obeys the same trust rule as
discovery: a previous token_url/registration_url belongs to the previous authorization
server, so it must not be pinned to a NEW authorization_url the admin re-pointed to. Without
this, re-pointing authorize to server B while the same MCP url keeps serving A's token
endpoint recreates the RFC 9700 mix-up, durably, and the discovery gate alone cannot catch
it because the stale endpoint comes from the registry, not from discovery."""
from litellm.proxy._experimental.mcp_server.mcp_server_manager import (
_carry_forward_resolved_oauth_endpoints,
)
previous = MCPServer(
server_id="s1",
name="s1",
url="https://up.example.com/mcp",
transport=MCPTransport.http,
auth_type=MCPAuth.oauth2,
authorization_url="https://idp-a.example.com/authorize",
token_url="https://idp-a.example.com/token",
registration_url="https://idp-a.example.com/register",
)
repointed = MCPServer(
server_id="s1",
name="s1",
url="https://up.example.com/mcp",
transport=MCPTransport.http,
auth_type=MCPAuth.oauth2,
authorization_url="https://idp-b.example.com/authorize",
)
_carry_forward_resolved_oauth_endpoints(new_server=repointed, previous_server=previous)
assert repointed.authorization_url == "https://idp-b.example.com/authorize"
assert repointed.token_url is None
assert repointed.registration_url is None
def test_carry_forward_restores_endpoints_when_authorization_url_unchanged(self):
"""The last-known-good path still works: a rebuild whose discovery blipped (no authorize
endpoint) adopts the previous authorize endpoint AND its token endpoint together as a
consistent group, and a rebuild that re-pins the same authorize endpoint (formatting aside)
keeps carrying the corroborated token endpoint."""
from litellm.proxy._experimental.mcp_server.mcp_server_manager import (
_carry_forward_resolved_oauth_endpoints,
)
def previous() -> MCPServer:
return MCPServer(
server_id="s1",
name="s1",
url="https://up.example.com/mcp",
transport=MCPTransport.http,
auth_type=MCPAuth.oauth2,
authorization_url="https://idp.example.com/authorize",
token_url="https://idp.example.com/token",
registration_url="https://idp.example.com/register",
)
blipped = MCPServer(
server_id="s1",
name="s1",
url="https://up.example.com/mcp",
transport=MCPTransport.http,
auth_type=MCPAuth.oauth2,
authorization_url=None,
)
_carry_forward_resolved_oauth_endpoints(new_server=blipped, previous_server=previous())
assert blipped.authorization_url == "https://idp.example.com/authorize"
assert blipped.token_url == "https://idp.example.com/token"
assert blipped.registration_url == "https://idp.example.com/register"
same_authorize = MCPServer(
server_id="s1",
name="s1",
url="https://up.example.com/mcp",
transport=MCPTransport.http,
auth_type=MCPAuth.oauth2,
authorization_url="https://IDP.example.com:443/authorize/",
)
_carry_forward_resolved_oauth_endpoints(new_server=same_authorize, previous_server=previous())
assert same_authorize.token_url == "https://idp.example.com/token"
assert same_authorize.registration_url == "https://idp.example.com/register"
def test_normalized_authorize_endpoint_treats_default_port_and_slash_as_identity(self):
"""The corroboration check must not fail on formatting-only differences an IdP legitimately
emits: default port, trailing slash, host case, and query string are not identity, but a
non-default port is."""
from litellm.proxy._experimental.mcp_server.mcp_server_manager import (
_normalized_authorize_endpoint,
)
canonical = _normalized_authorize_endpoint("https://idp.example.com/authorize")
assert _normalized_authorize_endpoint("https://idp.example.com:443/authorize") == canonical
assert _normalized_authorize_endpoint("https://IDP.example.com/authorize/") == canonical
assert _normalized_authorize_endpoint("https://idp.example.com/authorize?prompt=consent") == canonical
assert _normalized_authorize_endpoint("https://idp.example.com:8443/authorize") != canonical
def test_build_mcp_server_table_preserves_timestamps(self):
"""_build_mcp_server_table must use the MCPServer's stored timestamps, not datetime.now()."""
manager = MCPServerManager()