From 86c2860bf91299a25529dc73f62dc646e2fd4c71 Mon Sep 17 00:00:00 2001 From: Padraig OhIceadha Date: Fri, 13 Nov 2026 15:27:02 +0000 Subject: [PATCH] Updates to conform to section 3.1 of the OAuth 2.1 specification --- src/mcp/client/auth/oauth2.py | 10 ++- src/mcp/shared/auth_utils.py | 62 +++++++++++++++++- tests/client/test_auth.py | 109 ++++++++++++++++++++++++++++++++ tests/shared/test_auth_utils.py | 44 ++++++++++++- 4 files changed, 221 insertions(+), 4 deletions(-) diff --git a/src/mcp/client/auth/oauth2.py b/src/mcp/client/auth/oauth2.py index c513a99b46..c381050de7 100644 --- a/src/mcp/client/auth/oauth2.py +++ b/src/mcp/client/auth/oauth2.py @@ -12,7 +12,7 @@ from collections.abc import AsyncGenerator, Awaitable, Callable from dataclasses import dataclass, field from typing import Any, Protocol, get_args -from urllib.parse import quote, urlencode, urljoin, urlparse +from urllib.parse import quote, urljoin, urlparse import anyio import httpx2 @@ -56,6 +56,7 @@ from mcp.shared.auth_utils import ( calculate_token_expiry, check_resource_allowed, + replace_query_params, resource_url_from_server_url, ) from mcp.shared.inbound import MCP_PROTOCOL_VERSION_HEADER @@ -397,6 +398,10 @@ async def _perform_authorization_code_grant(self) -> tuple[str, str]: auth_base_url = self.context.get_authorization_base_url(self.context.server_url) auth_endpoint = urljoin(auth_base_url, "/authorize") + # OAuth 2.1 section 3.1: the authorization endpoint URI MUST NOT include a fragment. + if urlparse(auth_endpoint).fragment: + raise OAuthFlowError(f"Authorization endpoint must not include a fragment: {auth_endpoint}") + if not self.context.client_info: raise OAuthFlowError("No client info available for authorization") # pragma: no cover @@ -425,7 +430,8 @@ async def _perform_authorization_code_grant(self) -> tuple[str, str]: if "offline_access" in self.context.client_metadata.scope.split(): auth_params["prompt"] = "consent" - authorization_url = f"{auth_endpoint}?{urlencode(auth_params)}" + # OAuth 2.1 section 3.1: retain the endpoint's query and never send a parameter twice. + authorization_url = replace_query_params(auth_endpoint, auth_params) with waiting_on_a_person(): await self.context.redirect_handler(authorization_url) result = await self.context.callback_handler() diff --git a/src/mcp/shared/auth_utils.py b/src/mcp/shared/auth_utils.py index 3ba880f40d..aedea1a801 100644 --- a/src/mcp/shared/auth_utils.py +++ b/src/mcp/shared/auth_utils.py @@ -1,7 +1,8 @@ """Utilities for OAuth 2.0 Resource Indicators (RFC 8707) and PKCE (RFC 7636).""" import time -from urllib.parse import urlparse, urlsplit, urlunsplit +from collections.abc import Iterable, Mapping +from urllib.parse import parse_qsl, urlencode, urlparse, urlsplit, urlunsplit from pydantic import AnyUrl, HttpUrl @@ -78,3 +79,62 @@ def calculate_token_expiry(expires_in: int | str | None) -> float | None: return None # pragma: no cover # Defensive: handle servers that return expires_in as string return time.time() + int(expires_in) + + +def add_query_params(url: str, params: Mapping[str, str]) -> str: + """Append query parameters to a URL, preserving any existing ones. + + New parameters are appended after the existing query rather than replacing + keys that are already present, so a key may end up appearing more than once. + The whole query string is re-encoded, which can change how existing + parameters are escaped (e.g. ``%20`` becomes ``+``). + + Args: + url: URL to add the parameters to + params: Mapping of parameter names to values to append + + Returns: + The URL with the parameters appended to its query string + """ + parts = urlsplit(url) + query = parse_qsl(parts.query, keep_blank_values=True) + query.extend(params.items()) + return urlunsplit(parts._replace(query=urlencode(query))) + + +def remove_query_params(url: str, keys: Iterable[str]) -> str: + """Remove query parameters from a URL by name. + + Every occurrence of each named key is removed; other parameters keep their + original order. The remaining query string is re-encoded, which can change + how those parameters are escaped (e.g. ``%20`` becomes ``+``). + + Args: + url: URL to remove the parameters from + keys: Iterable of parameter names to remove + + Returns: + The URL without the named query parameters + """ + keys = set(keys) + parts = urlsplit(url) + query = [(k, v) for k, v in parse_qsl(parts.query, keep_blank_values=True) if k not in keys] + return urlunsplit(parts._replace(query=urlencode(query))) + + +def replace_query_params(url: str, params: Mapping[str, str]) -> str: + """Set query parameters on a URL, replacing any existing values for those keys. + + Other existing parameters are retained in order, and each given key appears + exactly once, after the retained parameters. The whole query string is + re-encoded, which can change how existing parameters are escaped (e.g. + ``%20`` becomes ``+``). + + Args: + url: URL to set the parameters on + params: Mapping of parameter names to values to set + + Returns: + The URL with the given parameters replacing any same-named existing ones + """ + return add_query_params(remove_query_params(url, params), params) diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index 18a1566705..222224d99a 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -1785,6 +1785,115 @@ async def test_403_without_a_scope_challenge_is_returned_to_the_caller( await auth_flow.asend(response_403) +@pytest.mark.anyio +async def test_authorization_url_retains_the_endpoint_query_component(oauth_provider: OAuthClientProvider): + """Spec-mandated (OAuth 2.1 section 3.1): the authorization endpoint's existing + application/x-www-form-urlencoded query is retained when the request parameters are added.""" + oauth_provider.context.oauth_metadata = OAuthMetadata( + issuer=AnyHttpUrl("https://auth.example.com"), + authorization_endpoint=AnyHttpUrl("https://auth.example.com/authorize?tenant=acme"), + token_endpoint=AnyHttpUrl("https://auth.example.com/token"), + ) + oauth_provider.context.client_info = OAuthClientInformationFull( + client_id="test_client_id", + redirect_uris=[AnyUrl("http://localhost:3030/callback")], + ) + + captured_url: str | None = None + captured_state: str | None = None + + async def capture_redirect(url: str) -> None: + nonlocal captured_url, captured_state + captured_url = url + captured_state = parse_qs(urlparse(url).query)["state"][0] + + async def echo_callback() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="auth_code", state=captured_state) + + oauth_provider.context.redirect_handler = capture_redirect + oauth_provider.context.callback_handler = echo_callback + + await oauth_provider._perform_authorization_code_grant() + + assert captured_url is not None + assert captured_url.count("?") == 1 + parsed = urlparse(captured_url) + assert parsed.path == "/authorize" + params = parse_qs(parsed.query) + assert params["tenant"] == ["acme"] + assert params["response_type"] == ["code"] + assert params["state"] == [captured_state] + + +@pytest.mark.anyio +async def test_authorization_url_sends_each_client_set_param_once_overriding_the_endpoint_query( + oauth_provider: OAuthClientProvider, +): + """Spec-mandated (OAuth 2.1 section 3.1): a request parameter MUST NOT be included more + than once, so client-set values override same-named endpoint query parameters.""" + client_info = OAuthClientInformationFull( + client_id="test_client_id", + redirect_uris=[AnyUrl("http://localhost:3030/callback")], + ) + oauth_provider.context.oauth_metadata = OAuthMetadata( + issuer=AnyHttpUrl("https://auth.example.com"), + authorization_endpoint=AnyHttpUrl( + "https://auth.example.com/authorize?response_type=token&client_id=other&tenant=acme" + ), + token_endpoint=AnyHttpUrl("https://auth.example.com/token"), + ) + oauth_provider.context.client_info = client_info + + captured_url: str | None = None + captured_state: str | None = None + + async def capture_redirect(url: str) -> None: + nonlocal captured_url, captured_state + captured_url = url + captured_state = parse_qs(urlparse(url).query)["state"][0] + + async def echo_callback() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="auth_code", state=captured_state) + + oauth_provider.context.redirect_handler = capture_redirect + oauth_provider.context.callback_handler = echo_callback + + await oauth_provider._perform_authorization_code_grant() + + assert captured_url is not None + params = parse_qs(urlparse(captured_url).query) + assert params["response_type"] == ["code"] + assert params["client_id"] == [client_info.client_id] + assert params["tenant"] == ["acme"] + + +@pytest.mark.anyio +async def test_authorization_endpoint_with_fragment_raises_before_redirecting(oauth_provider: OAuthClientProvider): + """Spec-mandated (OAuth 2.1 section 3.1): the authorization endpoint URI MUST NOT include + a fragment, so a fragment-bearing endpoint fails before any redirect happens.""" + oauth_provider.context.oauth_metadata = OAuthMetadata( + issuer=AnyHttpUrl("https://auth.example.com"), + authorization_endpoint=AnyHttpUrl("https://auth.example.com/authorize#frag"), + token_endpoint=AnyHttpUrl("https://auth.example.com/token"), + ) + oauth_provider.context.client_info = OAuthClientInformationFull( + client_id="test_client_id", + redirect_uris=[AnyUrl("http://localhost:3030/callback")], + ) + + async def redirect_handler(url: str) -> None: + raise NotImplementedError + + async def callback_handler() -> AuthorizationCodeResult: + raise NotImplementedError + + oauth_provider.context.redirect_handler = redirect_handler + oauth_provider.context.callback_handler = callback_handler + + with pytest.raises(OAuthFlowError): + await oauth_provider._perform_authorization_code_grant() + + @pytest.mark.parametrize( ( "issuer_url", diff --git a/tests/shared/test_auth_utils.py b/tests/shared/test_auth_utils.py index 5ae0e22b0c..421e572fe3 100644 --- a/tests/shared/test_auth_utils.py +++ b/tests/shared/test_auth_utils.py @@ -2,7 +2,13 @@ from pydantic import HttpUrl -from mcp.shared.auth_utils import check_resource_allowed, resource_url_from_server_url +from mcp.shared.auth_utils import ( + add_query_params, + check_resource_allowed, + remove_query_params, + replace_query_params, + resource_url_from_server_url, +) # Tests for resource_url_from_server_url function @@ -121,3 +127,39 @@ def test_check_resource_allowed_empty_paths(): assert check_resource_allowed("https://example.com", "https://example.com") is True assert check_resource_allowed("https://example.com/", "https://example.com") is True assert check_resource_allowed("https://example.com/api", "https://example.com") is True + + +def test_add_query_params_appends_after_existing_params_and_keeps_fragment(): + """SDK-defined: new params follow the existing query; path and fragment are untouched.""" + url = add_query_params("https://example.com/cb?state=abc#frag", {"code": "xyz"}) + assert url == "https://example.com/cb?state=abc&code=xyz#frag" + + +def test_add_query_params_duplicates_existing_key_instead_of_replacing(): + """SDK-defined: an existing key is kept and the new value is appended alongside it.""" + assert add_query_params("https://example.com/?a=1", {"a": "2"}) == "https://example.com/?a=1&a=2" + + +def test_add_query_params_reencodes_existing_query(): + """SDK-defined: the whole query is re-encoded, so `%20` in existing params becomes `+`.""" + assert add_query_params("https://example.com/?q=a%20b", {"x": "y"}) == "https://example.com/?q=a+b&x=y" + + +def test_remove_query_params_drops_every_occurrence_and_keeps_order_of_the_rest(): + """SDK-defined: all values of a named key are removed; other params keep their order.""" + url = remove_query_params("https://example.com/?a=1&b=2&a=3&c=&d=4", ["a", "d"]) + assert url == "https://example.com/?b=2&c=" + + +def test_remove_query_params_omits_question_mark_when_query_becomes_empty(): + """SDK-defined: removing the only params yields a URL with no query component.""" + assert remove_query_params("https://example.com/p?a=1#frag", ["a"]) == "https://example.com/p#frag" + + +def test_replace_query_params_overrides_given_keys_and_keeps_the_rest(): + """SDK-defined: given keys replace all existing occurrences and are appended once. + + Other params and the fragment are kept. + """ + url = replace_query_params("https://example.com/p?a=1&b=2&a=3#f", {"a": "x", "c": "y"}) + assert url == "https://example.com/p?b=2&a=x&c=y#f"