Updates to conform to section 3.1 of the OAuth 2.1 specification

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()

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)

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",

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"