From 64fc3f93e9290b0a1c1bbdb1b4585793a5e7ddb9 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Sun, 26 Jul 2026 11:03:23 +0000 Subject: [PATCH 1/4] Split the registration request model from the registered-client record OAuthClientInformationFull, the client's parse of the authorization server's Dynamic Client Registration response, inherited from OAuthClientMetadata, the request the client sends. That typed the response as though it had to be a request this SDK would send. RFC 7591 3.2.1 says otherwise: the server may reject or replace any requested metadata value, and real servers echo an application_type outside OIDC Registration's web/native, an explicit null, an auth method the SDK does not implement, or an empty redirect_uris. Each raised ValidationError on a 2xx response, after the server had already provisioned the client, so the registration was discarded and orphaned. Make the two models siblings over a shared OAuthClientMetadataBase. The request keeps its strict types, so the SDK still refuses to send an unregistered application_type. The record accepts what a server may echo: application_type and token_endpoint_auth_method are str | None (with an echoed "" read as absent, as the optional URL fields already were), grant_types is list[str], and redirect_uris may be absent or empty. client_id is now required, as RFC 7591 3.2.1 makes it in the response. Whether a substituted value is usable is judged where it matters, not at parse: an auth method the client cannot apply is reported as an OAuthRegistrationError when the registration completes, before the record is stored or any interactive authorization begins, and prepare_token_auth reports the same for a stored record. The recognized set is derived from the one TokenEndpointAuthMethod type so the two cannot drift. The bundled registration endpoint now returns all registered metadata in its 201 response, building the record from the validated request's dump so a field can no longer be silently dropped from the echo; it previously omitted application_type, reporting the default in place of a client's "web". --- docs/migration.md | 21 +++ src/mcp/client/auth/oauth2.py | 44 +++++- src/mcp/client/auth/utils.py | 8 +- src/mcp/server/auth/handlers/register.py | 32 ++--- src/mcp/shared/auth.py | 132 ++++++++++++------ .../extensions/test_client_credentials.py | 8 +- tests/client/test_auth.py | 64 ++++++++- tests/interaction/_requirements.py | 21 +++ tests/interaction/auth/test_as_handlers.py | 17 +++ tests/interaction/auth/test_discovery.py | 79 ++++++++++- tests/shared/test_auth.py | 89 +++++++++++- 11 files changed, 435 insertions(+), 80 deletions(-) diff --git a/docs/migration.md b/docs/migration.md index 5eb7659c23..11397c0f75 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -2179,6 +2179,27 @@ client_metadata = OAuthClientMetadata( Under OIDC, omitting `application_type` defaults to `"web"`, which an authorization server may reject for the `localhost` redirect URIs native clients use; sending `"native"` avoids that. Non-OIDC servers ignore the parameter. +### `OAuthClientInformationFull` no longer subclasses `OAuthClientMetadata`, and parses server-substituted metadata + +`OAuthClientMetadata` is the registration request a client sends; `OAuthClientInformationFull` is the authorization server's record of a registered client, parsed from its Dynamic Client Registration response. In v1 the second inherited from the first, which typed the response as though it had to be a request this SDK would send. It does not: [RFC 7591 §3.2.1](https://datatracker.ietf.org/doc/html/rfc7591#section-3.2.1) lets the server "reject or replace any of the client's requested metadata values submitted during the registration and substitute them with suitable values", and real servers return an `application_type` outside OIDC Registration's `web`/`native`, an explicit `null`, a `token_endpoint_auth_method` the SDK does not implement, or an empty `redirect_uris`. The inherited strict types turned each of those into a `ValidationError` on a 2xx response - after the server had already provisioned the client, so the registration was discarded and orphaned. + +The two are now siblings over a shared `OAuthClientMetadataBase`. `OAuthClientMetadata` keeps its strict types (the SDK still refuses to *send* an unregistered `application_type`), while `OAuthClientInformationFull` accepts what a server may echo: + +```python +# v1 +class OAuthClientInformationFull(OAuthClientMetadata): ... + +# v2 +class OAuthClientMetadata(OAuthClientMetadataBase): ... # request: strict +class OAuthClientInformationFull(OAuthClientMetadataBase): ... # server record: tolerant +``` + +On `OAuthClientInformationFull`, `application_type` and `token_endpoint_auth_method` are now `str | None`, `grant_types` is `list[str]`, and `redirect_uris` is optional (`list[AnyUrl] | None`, no minimum length). `client_id` is now required (`str`): [RFC 7591 §3.2.1](https://datatracker.ietf.org/doc/html/rfc7591#section-3.2.1) makes it mandatory in the response, and a record of a registered client without one was never meaningful. Code that only reads these fields is unaffected. Code that relied on `isinstance(client_info, OAuthClientMetadata)`, or passed an `OAuthClientInformationFull` where an `OAuthClientMetadata` is expected, must reference the record type directly. `validate_scope()` and `validate_redirect_uri()` moved with the record: they are methods of `OAuthClientInformationFull` (the type authorization-server code holds) and are no longer available on `OAuthClientMetadata`. + +A registration response the server sends is no longer rejected on these fields (an echoed `""` for `token_endpoint_auth_method` or `application_type` is treated as absent, as it already was for the optional URL fields). Whether a substituted value is usable is decided where the value is used: a registered `token_endpoint_auth_method` this SDK does not recognize (anything other than `none`, `client_secret_post`, `client_secret_basic`, or `private_key_jwt`) now raises `OAuthTokenError` naming the method at the token exchange, rather than the token request being sent unauthenticated for the server to reject. + +The SDK's own registration endpoint also now returns all registered metadata in its 201 response (RFC 7591 §3.2.1), including the client's `application_type`, which v1 dropped from the echo (silently reporting the default in place of a client's `"web"`). + ### Stricter client authentication at `/token` and `/revoke` v2 hardens client authentication on SDK-hosted authorization servers (`create_auth_routes`) in two ways. Both apply automatically; server code only needs changing if you hand-provision client records. diff --git a/src/mcp/client/auth/oauth2.py b/src/mcp/client/auth/oauth2.py index 07e224148a..a67fe4bdf1 100644 --- a/src/mcp/client/auth/oauth2.py +++ b/src/mcp/client/auth/oauth2.py @@ -11,7 +11,7 @@ import time from collections.abc import AsyncGenerator, Awaitable, Callable from dataclasses import dataclass, field -from typing import Any, Protocol +from typing import Any, Protocol, get_args from urllib.parse import quote, urlencode, urljoin, urlparse import anyio @@ -19,7 +19,7 @@ from mcp_types.version import is_version_at_least from pydantic import BaseModel, Field, ValidationError -from mcp.client.auth.exceptions import OAuthFlowError, OAuthTokenError +from mcp.client.auth.exceptions import OAuthFlowError, OAuthRegistrationError, OAuthTokenError from mcp.client.auth.utils import ( build_oauth_authorization_server_metadata_discovery_urls, build_protected_resource_metadata_discovery_urls, @@ -48,6 +48,7 @@ OAuthMetadata, OAuthToken, ProtectedResourceMetadata, + TokenEndpointAuthMethod, ) from mcp.shared.auth_utils import ( calculate_token_expiry, @@ -58,6 +59,33 @@ logger = logging.getLogger(__name__) +# Methods `OAuthContext.prepare_token_auth` recognizes on a registered client, derived from the +# same set the SDK is willing to request so the two cannot drift. `None`/"none" send no client +# secret; `private_key_jwt` adds no secret here (its assertion is supplied by +# `PrivateKeyJWTOAuthProvider`). Anything else is a method this client cannot apply. +_RECOGNIZED_TOKEN_ENDPOINT_AUTH_METHODS: tuple[str | None, ...] = (None, *get_args(TokenEndpointAuthMethod)) + + +def check_registration_usable(client_info: OAuthClientInformationFull) -> None: + """Confirm a completed registration is one this client can act on. + + RFC 7591 §3.2.1 lets the authorization server replace requested metadata and leaves it to + the client to "check the values in the response to determine if the registration is + sufficient for use". The one substitution that makes the minted credentials unusable is a + token-endpoint auth method this client cannot apply, so it is judged here - before the + record is persisted or any interactive authorization begins - rather than surfacing later + as an opaque failure at the token endpoint. + + Raises: + OAuthRegistrationError: The server registered the client with a + `token_endpoint_auth_method` this client does not implement. + """ + if client_info.token_endpoint_auth_method not in _RECOGNIZED_TOKEN_ENDPOINT_AUTH_METHODS: + raise OAuthRegistrationError( + "Authorization server registered the client with unsupported token_endpoint_auth_method " + f"{client_info.token_endpoint_auth_method!r}" + ) + class PKCEParameters(BaseModel): """PKCE (Proof Key for Code Exchange) parameters.""" @@ -190,6 +218,12 @@ def prepare_token_auth( Returns: Tuple of (updated_data, updated_headers) + + Raises: + OAuthTokenError: The registered client's `token_endpoint_auth_method` is one this + client does not implement. The authorization server may assign a method other + than the one requested (RFC 7591 §3.2.1); the registration record accepts it, + and the mismatch is reported here, where the method is applied. """ if headers is None: headers = {} # pragma: no cover @@ -212,7 +246,10 @@ def prepare_token_auth( # Include client_id and client_secret in request body (RFC 6749 §2.3.1) data["client_id"] = self.client_info.client_id data["client_secret"] = self.client_info.client_secret - # For auth_method == "none", don't add any client_secret + elif auth_method not in _RECOGNIZED_TOKEN_ENDPOINT_AUTH_METHODS: + raise OAuthTokenError(f"Registered client uses unsupported token_endpoint_auth_method {auth_method!r}") + # For "none" (or absent), don't add any client_secret; "private_key_jwt" adds its + # assertion in the provider that implements it, not here. return data, headers @@ -664,6 +701,7 @@ async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx ) registration_response = yield registration_request client_information = await handle_registration_response(registration_response) + check_registration_usable(client_information) # Only record the issuer when the registration above actually targeted # the discovered AS — either via its published registration_endpoint, # or because the resource-origin /register fallback is on the issuer's diff --git a/src/mcp/client/auth/utils.py b/src/mcp/client/auth/utils.py index fc87c9e469..282f23b879 100644 --- a/src/mcp/client/auth/utils.py +++ b/src/mcp/client/auth/utils.py @@ -301,8 +301,8 @@ async def handle_registration_response(response: Response) -> OAuthClientInforma content = await response.aread() client_info = OAuthClientInformationFull.model_validate_json(content) return client_info - except ValidationError as e: # pragma: no cover - raise OAuthRegistrationError(f"Invalid registration response: {e}") + except ValidationError as e: + raise OAuthRegistrationError(f"Invalid registration response: {e}") from e def is_valid_client_metadata_url(url: str | None) -> bool: @@ -381,8 +381,8 @@ def create_client_info_from_metadata_url( Args: client_metadata_url: The URL to use as the client_id - redirect_uris: The redirect URIs from the client metadata (passed through for - compatibility with OAuthClientInformationFull which inherits from OAuthClientMetadata) + redirect_uris: The redirect URIs from the client metadata, recorded on the client + information alongside the client_id Returns: OAuthClientInformationFull with the URL as client_id diff --git a/src/mcp/server/auth/handlers/register.py b/src/mcp/server/auth/handlers/register.py index e565b27383..ed857ecfe7 100644 --- a/src/mcp/server/auth/handlers/register.py +++ b/src/mcp/server/auth/handlers/register.py @@ -112,27 +112,17 @@ async def handle(self, request: Request) -> Response: else None ) - client_info = OAuthClientInformationFull( - client_id=client_id, - client_id_issued_at=client_id_issued_at, - client_secret=client_secret, - client_secret_expires_at=client_secret_expires_at, - # passthrough information from the client request - redirect_uris=client_metadata.redirect_uris, - token_endpoint_auth_method=client_metadata.token_endpoint_auth_method, - grant_types=client_metadata.grant_types, - response_types=client_metadata.response_types, - client_name=client_metadata.client_name, - client_uri=client_metadata.client_uri, - logo_uri=client_metadata.logo_uri, - scope=client_metadata.scope, - contacts=client_metadata.contacts, - tos_uri=client_metadata.tos_uri, - policy_uri=client_metadata.policy_uri, - jwks_uri=client_metadata.jwks_uri, - jwks=client_metadata.jwks, - software_id=client_metadata.software_id, - software_version=client_metadata.software_version, + # RFC 7591 §3.2.1: the response returns all registered metadata about the client, so + # the record is the whole validated request plus the credentials minted here - built + # from the request's dump so no metadata field can be silently omitted from the echo. + client_info = OAuthClientInformationFull.model_validate( + { + **client_metadata.model_dump(), + "client_id": client_id, + "client_id_issued_at": client_id_issued_at, + "client_secret": client_secret, + "client_secret_expires_at": client_secret_expires_at, + } ) try: # Register client diff --git a/src/mcp/shared/auth.py b/src/mcp/shared/auth.py index 2bbf7a715a..b338ca67df 100644 --- a/src/mcp/shared/auth.py +++ b/src/mcp/shared/auth.py @@ -5,6 +5,23 @@ # RFC 7523 JWT bearer grant; SEP-990 leg 2 uses this to present the ID-JAG. JWT_BEARER_GRANT_TYPE = "urn:ietf:params:oauth:grant-type:jwt-bearer" +# Token-endpoint client authentication methods this SDK's clients request, and the set +# `OAuthContext.prepare_token_auth` recognizes on a registered client (`private_key_jwt` is +# applied by `PrivateKeyJWTOAuthProvider`; the rest send a client secret or nothing). +TokenEndpointAuthMethod = Literal["none", "client_secret_post", "client_secret_basic", "private_key_jwt"] + +# grant_types a client requests when it does not specify its own (RFC 7591 §2). +DEFAULT_GRANT_TYPES = ["authorization_code", "refresh_token"] + + +def _empty_str_to_none(v: object) -> object: + # Some authorization servers echo omitted metadata back as "" instead of dropping the + # key. RFC 7591 §2 marks these fields OPTIONAL; treat "" as absent rather than rejecting + # an otherwise valid registration response over a placeholder. + if v == "": + return None + return v + class OAuthToken(BaseModel): """See https://datatracker.ietf.org/doc/html/rfc6749#section-5.1""" @@ -47,32 +64,21 @@ def __init__(self, message: str): self.message = message -class OAuthClientMetadata(BaseModel): - """RFC 7591 OAuth 2.0 Dynamic Client Registration Metadata. +class OAuthClientMetadataBase(BaseModel): + """RFC 7591 OAuth 2.0 Dynamic Client Registration metadata shared verbatim by the + registration request (`OAuthClientMetadata`) and the authorization server's record of a + registered client (`OAuthClientInformationFull`). Fields whose acceptable values differ + between the two - what this SDK sends versus what a third-party server may echo - are + declared on each model rather than here. See https://datatracker.ietf.org/doc/html/rfc7591#section-2 """ model_config = ConfigDict(url_preserve_empty_path=True) - redirect_uris: list[AnyUrl] | None = Field(..., min_length=1) - # supported auth methods for the token endpoint - token_endpoint_auth_method: ( - Literal["none", "client_secret_post", "client_secret_basic", "private_key_jwt"] | None - ) = None - # supported grant_types of this implementation - grant_types: list[ - Literal["authorization_code", "refresh_token", "urn:ietf:params:oauth:grant-type:jwt-bearer"] | str - ] = [ - "authorization_code", - "refresh_token", - ] # The MCP spec requires the "code" response type, but OAuth # servers may also return additional types they support response_types: list[str] = ["code"] scope: str | None = None - # SEP-837: OIDC application_type. Defaults to "native" since MCP clients typically use - # loopback redirect URIs; set "web" for remote browser-based clients on a non-local host. - application_type: Literal["web", "native"] = "native" # these fields are currently unused, but we support & store them for potential # future use @@ -97,13 +103,69 @@ class OAuthClientMetadata(BaseModel): ) @classmethod def _empty_string_optional_url_to_none(cls, v: object) -> object: - # RFC 7591 §2 marks these URL fields OPTIONAL. Some authorization servers - # echo omitted metadata back as "" instead of dropping the keys, which - # AnyHttpUrl would otherwise reject — throwing away an otherwise valid - # registration response. Treat "" as absent. - if v == "": - return None - return v + # These URL fields are OPTIONAL; an echoed "" would otherwise fail AnyHttpUrl + # and throw away an otherwise valid registration response. + return _empty_str_to_none(v) + + +class OAuthClientMetadata(OAuthClientMetadataBase): + """RFC 7591 OAuth 2.0 Dynamic Client Registration request metadata: what an MCP + client sends when it registers. Field values are narrowed to what this SDK will put + on the wire; parsing the authorization server's response is `OAuthClientInformationFull`'s + job. See https://datatracker.ietf.org/doc/html/rfc7591#section-2 + """ + + redirect_uris: list[AnyUrl] | None = Field(..., min_length=1) + # supported auth methods for the token endpoint + token_endpoint_auth_method: TokenEndpointAuthMethod | None = None + # supported grant_types of this implementation + grant_types: list[ + Literal["authorization_code", "refresh_token", "urn:ietf:params:oauth:grant-type:jwt-bearer"] | str + ] = list(DEFAULT_GRANT_TYPES) + # SEP-837: OIDC application_type. Defaults to "native" since MCP clients typically use + # loopback redirect URIs; set "web" for remote browser-based clients on a non-local host. + application_type: Literal["web", "native"] = "native" + + +class OAuthClientInformationFull(OAuthClientMetadataBase): + """RFC 7591 OAuth 2.0 Dynamic Client Registration client information response + (client information plus metadata) - the authorization server's record of a + registered client. See https://datatracker.ietf.org/doc/html/rfc7591#section-3.2.1 + + A third-party authorization server "MAY reject or replace any of the client's + requested metadata values submitted during the registration and substitute them with + suitable values", so `application_type`, `token_endpoint_auth_method`, and `grant_types` + are typed to accept any string the server echoes, and `redirect_uris` may be absent or + empty. Whether a substituted value is usable is decided where the value is used, not at + parse. `redirect_uris` elements are still parsed as URLs, as the authorization server + compares them against a client's requested `redirect_uri`. + """ + + redirect_uris: list[AnyUrl] | None = None + # RFC 7591 §3.2.1: the server may assign an auth method other than the one requested, + # including methods this SDK does not implement, or omit it. + token_endpoint_auth_method: str | None = None + grant_types: list[str] = list(DEFAULT_GRANT_TYPES) + # SEP-837: OIDC application_type. OIDC Registration §2 defines "web" and "native", but + # servers echo other strings or an explicit null; the value is informational here. + application_type: str | None = None + + # RFC 7591 §3.2.1: client_id is REQUIRED in a client information response - a body + # without one is not a registration, whatever else it echoes. + client_id: str + client_secret: str | None = None + client_id_issued_at: int | None = None + client_secret_expires_at: int | None = None + # SEP-2352: the issuer these credentials were registered with, recorded by the SDK (not an + # RFC 7591 field) to detect authorization-server migration and avoid cross-AS credential reuse. + issuer: str | None = None + + @field_validator("token_endpoint_auth_method", "application_type", mode="before") + @classmethod + def _empty_string_optional_metadata_to_none(cls, v: object) -> object: + # An echoed "" here would otherwise read as an unrecognized method/type; treat it as + # absent, matching the URL-field coercion. + return _empty_str_to_none(v) def validate_scope(self, requested_scope: str | None) -> list[str] | None: if requested_scope is None: @@ -118,27 +180,15 @@ def validate_scope(self, requested_scope: str | None) -> list[str] | None: def validate_redirect_uri(self, redirect_uri: AnyUrl | None) -> AnyUrl: if redirect_uri is not None: # Validate redirect_uri against client's registered redirect URIs - if self.redirect_uris is None or redirect_uri not in self.redirect_uris: + if not self.redirect_uris or redirect_uri not in self.redirect_uris: raise InvalidRedirectUriError(f"Redirect URI '{redirect_uri}' not registered for client") return redirect_uri - elif self.redirect_uris is not None and len(self.redirect_uris) == 1: + elif self.redirect_uris and len(self.redirect_uris) == 1: return self.redirect_uris[0] else: - raise InvalidRedirectUriError("redirect_uri must be specified when client has multiple registered URIs") - - -class OAuthClientInformationFull(OAuthClientMetadata): - """RFC 7591 OAuth 2.0 Dynamic Client Registration full response - (client information plus metadata). - """ - - client_id: str | None = None - client_secret: str | None = None - client_id_issued_at: int | None = None - client_secret_expires_at: int | None = None - # SEP-2352: the issuer these credentials were registered with, recorded by the SDK (not an - # RFC 7591 field) to detect authorization-server migration and avoid cross-AS credential reuse. - issuer: str | None = None + raise InvalidRedirectUriError( + "redirect_uri must be specified unless the client has exactly one registered URI" + ) class OAuthMetadata(BaseModel): diff --git a/tests/client/auth/extensions/test_client_credentials.py b/tests/client/auth/extensions/test_client_credentials.py index cb8fd5e2b6..84296eb6de 100644 --- a/tests/client/auth/extensions/test_client_credentials.py +++ b/tests/client/auth/extensions/test_client_credentials.py @@ -151,7 +151,7 @@ async def test_exchange_token_client_secret_post_includes_client_id(self, mock_s @pytest.mark.anyio async def test_exchange_token_client_secret_post_without_client_id(self, mock_storage: MockTokenStorage): - """Test client_secret_post skips body credentials when client_id is None.""" + """Test client_secret_post skips body credentials when client_id is empty.""" provider = ClientCredentialsOAuthProvider( server_url="https://api.example.com/v1/mcp", storage=mock_storage, @@ -167,10 +167,10 @@ async def test_exchange_token_client_secret_post_without_client_id(self, mock_st token_endpoint=AnyHttpUrl("https://api.example.com/token"), ) provider.context.protocol_version = "2025-06-18" - # Override client_info to have client_id=None (edge case) + # Override client_info to have an empty client_id (edge case) provider.context.client_info = OAuthClientInformationFull( redirect_uris=None, - client_id=None, + client_id="", client_secret="test-client-secret", grant_types=["client_credentials"], token_endpoint_auth_method="client_secret_post", @@ -181,7 +181,7 @@ async def test_exchange_token_client_secret_post_without_client_id(self, mock_st content = urllib.parse.unquote_plus(request.content.decode()) assert "grant_type=client_credentials" in content - # Neither client_id nor client_secret should be in body since client_id is None + # Neither client_id nor client_secret should be in body since client_id is empty # (RFC 6749 §2.3.1 requires both for client_secret_post) assert "client_id=" not in content assert "client_secret=" not in content diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index 6bea7bcf70..48ca608b8a 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -12,7 +12,7 @@ from pydantic import AnyHttpUrl, AnyUrl from mcp.client.auth import OAuthClientProvider, PKCEParameters -from mcp.client.auth.exceptions import OAuthFlowError, OAuthTokenError +from mcp.client.auth.exceptions import OAuthFlowError, OAuthRegistrationError, OAuthTokenError from mcp.client.auth.utils import ( build_oauth_authorization_server_metadata_discovery_urls, build_protected_resource_metadata_discovery_urls, @@ -1008,6 +1008,68 @@ def text(self): assert "Registration failed: 400" in str(exc_info.value) +@pytest.mark.anyio +async def test_registration_response_with_substituted_metadata_yields_the_credentials(): + """A 201 whose echoed metadata differs from the request still registers the client. + + The authorization server returned an application_type outside OIDC Registration's set, + a null redirect_uris, and an auth method the SDK does not implement. RFC 7591 §3.2.1 + permits the server to substitute values; the client keeps the credentials it minted. + """ + body = ( + b'{"client_id": "issued-id", "client_secret": "issued-secret", ' + b'"application_type": "confidential", "redirect_uris": null, ' + b'"token_endpoint_auth_method": "client_secret_jwt"}' + ) + response = httpx2.Response(201, content=body) + + client_info = await handle_registration_response(response) + + assert client_info.client_id == "issued-id" + assert client_info.client_secret == "issued-secret" + assert client_info.application_type == "confidential" + + +@pytest.mark.anyio +async def test_a_2xx_body_that_is_not_client_information_is_an_oauth_registration_error(): + """A success status whose body is not client information surfaces as OAuthRegistrationError.""" + response = httpx2.Response(201, content=b"not json") + + with pytest.raises(OAuthRegistrationError): + await handle_registration_response(response) + + +@pytest.mark.anyio +async def test_token_exchange_reports_an_unimplemented_registered_auth_method(oauth_provider: OAuthClientProvider): + """A server-assigned auth method the SDK cannot apply (RFC 7591 §3.2.1 lets the server + substitute one) is reported at the token exchange rather than sending the request + unauthenticated for the server to reject as invalid_client.""" + oauth_provider.context.client_info = OAuthClientInformationFull( + client_id="registered-id", + client_secret="registered-secret", + token_endpoint_auth_method="client_secret_jwt", + ) + + with pytest.raises(OAuthTokenError): + await oauth_provider._exchange_token_authorization_code("test_auth_code", "test_verifier") + + +def test_prepare_token_auth_leaves_a_private_key_jwt_client_to_its_provider(oauth_provider: OAuthClientProvider): + """private_key_jwt is recognized (PrivateKeyJWTOAuthProvider supplies the assertion), so + the base leaves the request untouched rather than reporting an unsupported method - the + refresh path a private-key-JWT client inherits keeps working.""" + oauth_provider.context.client_info = OAuthClientInformationFull( + client_id="registered-id", + client_secret="registered-secret", + token_endpoint_auth_method="private_key_jwt", + ) + + data, headers = oauth_provider.context.prepare_token_auth({"grant_type": "refresh_token"}, {}) + + assert data == {"grant_type": "refresh_token"} + assert headers == {} + + class TestCreateClientRegistrationRequest: """Test client registration request creation.""" diff --git a/tests/interaction/_requirements.py b/tests/interaction/_requirements.py index 17bc3a4b5a..6c77af5a45 100644 --- a/tests/interaction/_requirements.py +++ b/tests/interaction/_requirements.py @@ -2940,6 +2940,16 @@ def __post_init__(self) -> None: transports=("streamable-http",), note="Auth is enforced at the HTTP layer; Cache-Control is an HTTP header.", ), + "hosting:auth:as:register-echo": Requirement( + source="sdk", + behavior=( + "The bundled registration endpoint returns all registered metadata about the client " + "in its 201 response (RFC 7591 §3.2.1), including the client's `application_type` " + "rather than a substituted default." + ), + transports=("streamable-http",), + note="Auth is enforced at the HTTP layer; the bundled AS is an ASGI app.", + ), "hosting:auth:as:register-error-response": Requirement( source="sdk", behavior=( @@ -3660,6 +3670,17 @@ def __post_init__(self) -> None: transports=("streamable-http",), note="OAuth is HTTP-only.", ), + "client-auth:dcr:substituted-metadata": Requirement( + source="sdk", + behavior=( + "A 201 registration response whose echoed metadata the server substituted (RFC 7591 §3.2.1) - " + "an unregistered application_type, null redirect_uris, extra grant types - completes the flow; " + "a substituted token_endpoint_auth_method the client cannot apply is instead reported as an " + "OAuthRegistrationError before the record is persisted or authorization begins." + ), + transports=("streamable-http",), + note="OAuth is HTTP-only.", + ), "client-auth:dcr": Requirement( source=f"{SPEC_BASE_URL}/basic/authorization#dynamic-client-registration", behavior=( diff --git a/tests/interaction/auth/test_as_handlers.py b/tests/interaction/auth/test_as_handlers.py index f59478b49a..7182a62e66 100644 --- a/tests/interaction/auth/test_as_handlers.py +++ b/tests/interaction/auth/test_as_handlers.py @@ -245,6 +245,23 @@ async def test_registration_with_invalid_metadata_is_rejected_with_400( assert body["error_description"].startswith("Requested scopes are not valid: ") +@requirement("hosting:auth:as:register-echo") +@pytest.mark.parametrize("application_type", ["web", "native"]) +async def test_registration_response_echoes_the_registered_application_type( + as_app: tuple[httpx2.AsyncClient, InMemoryAuthorizationServerProvider], + application_type: str, +) -> None: + """The 201 body reflects the application_type the client registered (RFC 7591 §3.2.1).""" + http, _ = as_app + body = oauth_client_metadata().model_dump(mode="json", exclude_none=True) + + response = await http.post("/register", json=body | {"application_type": application_type}) + + assert response.status_code == 201 + info = OAuthClientInformationFull.model_validate_json(response.content) + assert info.application_type == application_type + + @requirement("hosting:auth:as:redirect-uri-binding") async def test_authorize_with_an_unregistered_redirect_uri_is_rejected_directly( as_app: tuple[httpx2.AsyncClient, InMemoryAuthorizationServerProvider], diff --git a/tests/interaction/auth/test_discovery.py b/tests/interaction/auth/test_discovery.py index dc9f3794af..5a61a81032 100644 --- a/tests/interaction/auth/test_discovery.py +++ b/tests/interaction/auth/test_discovery.py @@ -17,14 +17,16 @@ import pytest from inline_snapshot import snapshot from mcp_types import ListToolsResult, Tool -from pydantic import AnyHttpUrl +from pydantic import AnyHttpUrl, AnyUrl from mcp.client.auth import OAuthFlowError, OAuthRegistrationError from mcp.server import Server, ServerRequestContext -from mcp.shared.auth import OAuthMetadata, ProtectedResourceMetadata +from mcp.shared.auth import OAuthClientInformationFull, OAuthMetadata, ProtectedResourceMetadata from tests.interaction._connect import BASE_URL, mounted_app from tests.interaction._requirements import requirement from tests.interaction.auth._harness import ( + REDIRECT_URI, + InMemoryTokenStorage, RecordedRequest, auth_settings, connect_with_oauth, @@ -174,6 +176,79 @@ async def test_a_400_from_the_registration_endpoint_surfaces_as_a_registration_e assert [r.path for r in recorded if r.path in ("/authorize", "/token")] == [] +@requirement("client-auth:dcr:substituted-metadata") +async def test_a_registration_response_with_substituted_metadata_completes_the_flow() -> None: + """A 201 whose echoed metadata differs from the request still yields a working client. + + The shim replaces the real `/register` with a body that echoes an `application_type` + outside OIDC Registration's set, a null `redirect_uris`, and an extra grant type - a + substitution RFC 7591 §3.2.1 permits. The registration proceeds and, after `/register` + stopped answering, the client authorizes and exchanges a token as normal. + """ + recorded, on_request = record_requests() + provider = InMemoryAuthorizationServerProvider() + server = Server("guarded", on_list_tools=list_tools) + client_id = "substituted-client" + provider.clients[client_id] = OAuthClientInformationFull( + client_id=client_id, + client_secret="s3cr3t", + redirect_uris=[AnyUrl(REDIRECT_URI)], + token_endpoint_auth_method="client_secret_post", + scope="mcp", + ) + body = json.dumps( + { + "client_id": client_id, + "client_secret": "s3cr3t", + "token_endpoint_auth_method": "client_secret_post", + "application_type": "confidential", + "redirect_uris": None, + "grant_types": ["authorization_code", "refresh_token", "client_credentials"], + } + ).encode() + app_shim = shim(serve={"/register": (201, body)}) + storage = InMemoryTokenStorage() + + with anyio.fail_after(5): + async with connect_with_oauth( + server, provider=provider, storage=storage, app_shim=app_shim, on_request=on_request + ) as (client, _): + result = await client.list_tools() + + assert result.tools[0].name == snapshot("probe") + assert storage.client_info is not None + assert storage.client_info.client_id == client_id + assert storage.client_info.application_type == "confidential" + assert [r.path for r in recorded].index("/register") < [r.path for r in recorded].index("/token") + + +@requirement("client-auth:dcr:substituted-metadata") +async def test_a_registration_assigning_an_unusable_auth_method_surfaces_as_a_registration_error() -> None: + """A 201 assigning an auth method the client cannot apply is a registration error. + + RFC 7591 §3.2.1 leaves it to the client to judge whether a substituted value makes the + registration usable; an unimplemented token-endpoint auth method does not, and is + reported before the record is stored or any authorize/token request is made. + """ + recorded, on_request = record_requests() + provider = InMemoryAuthorizationServerProvider() + server = Server("guarded", on_list_tools=list_tools) + body = json.dumps( + {"client_id": "jwt-only", "client_secret": "s3cr3t", "token_endpoint_auth_method": "client_secret_jwt"} + ).encode() + app_shim = shim(serve={"/register": (201, body)}) + storage = InMemoryTokenStorage() + + with anyio.fail_after(5): + with pytest.RaisesGroup(pytest.RaisesExc(OAuthRegistrationError), flatten_subgroups=True): + await connect_with_oauth( + server, provider=provider, storage=storage, app_shim=app_shim, on_request=on_request + ).__aenter__() + + assert storage.client_info is None + assert [r.path for r in recorded if r.path in ("/authorize", "/token")] == [] + + @requirement("client-auth:prm-resource-mismatch") async def test_prm_with_a_mismatched_resource_aborts_the_flow_before_authorize() -> None: """A PRM document whose `resource` does not cover the server URL aborts the flow. diff --git a/tests/shared/test_auth.py b/tests/shared/test_auth.py index 7463bc5a8a..e3403e914e 100644 --- a/tests/shared/test_auth.py +++ b/tests/shared/test_auth.py @@ -1,9 +1,9 @@ """Tests for OAuth 2.0 shared code.""" import pytest -from pydantic import ValidationError +from pydantic import AnyUrl, ValidationError -from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata, OAuthMetadata +from mcp.shared.auth import InvalidRedirectUriError, OAuthClientInformationFull, OAuthClientMetadata, OAuthMetadata def test_oauth(): @@ -110,8 +110,8 @@ def test_valid_url_passes_through_unchanged(): def test_information_full_inherits_coercion(): - """OAuthClientInformationFull subclasses OAuthClientMetadata, so the - same coercion applies to DCR responses parsed via the full model.""" + """OAuthClientInformationFull shares the metadata base, so the same + coercion applies to DCR responses parsed via the full model.""" data = { "client_id": "abc123", "redirect_uris": ["https://example.com/callback"], @@ -130,6 +130,87 @@ def test_information_full_inherits_coercion(): assert info.jwks_uri is None +# RFC 7591 §3.2.1 lets the authorization server reject or replace any requested metadata +# value in its registration response. Real servers echo values outside the sets the client +# would send (an unregistered application_type, an explicit null, an auth method the SDK +# does not implement, an empty redirect_uris array); a parse failure there discards a +# registration whose client_id the server has already provisioned. + + +@pytest.mark.parametrize( + "substituted", + [ + pytest.param({"application_type": "confidential"}, id="unregistered-application-type"), + pytest.param({"application_type": ""}, id="empty-application-type"), + pytest.param({"application_type": None}, id="null-application-type"), + pytest.param({"token_endpoint_auth_method": "client_secret_jwt"}, id="unimplemented-auth-method"), + pytest.param({"grant_types": ["authorization_code", "client_credentials"]}, id="extra-grant-type"), + pytest.param({"redirect_uris": []}, id="empty-redirect-uris"), + ], +) +def test_client_information_accepts_server_substituted_metadata(substituted: dict[str, object]): + data = {"client_id": "abc123", "client_secret": "s3cr3t", **substituted} + info = OAuthClientInformationFull.model_validate(data) + assert info.client_id == "abc123" + assert info.client_secret == "s3cr3t" + + +def test_client_information_without_echoed_metadata_still_parses(): + """A response holding only the credentials the server minted is a usable registration.""" + info = OAuthClientInformationFull.model_validate({"client_id": "abc123"}) + assert info.client_id == "abc123" + assert info.redirect_uris is None + assert info.application_type is None + + +def test_every_request_metadata_field_exists_on_the_client_record(): + """The registration handler builds its 201 echo from the request's dump; every request + field must exist on the record so none can be silently dropped from the response.""" + assert set(OAuthClientMetadata.model_fields) <= set(OAuthClientInformationFull.model_fields) + + +def test_a_registration_response_without_a_client_id_is_rejected(): + """RFC 7591 §3.2.1 makes client_id REQUIRED; a body without one is not a registration, + however permissive the parse is about the metadata around it.""" + with pytest.raises(ValidationError): + OAuthClientInformationFull.model_validate({"application_type": "web"}) + + +@pytest.mark.parametrize("empty_field", ["token_endpoint_auth_method", "application_type"]) +def test_client_information_empty_string_metadata_coerced_to_absent(empty_field: str): + """An echoed "" reads as absent, matching the URL-field coercion, so it is neither + stored as a value nor later mistaken for an unrecognized method or type.""" + info = OAuthClientInformationFull.model_validate({"client_id": "abc123", empty_field: ""}) + assert getattr(info, empty_field) is None + + +@pytest.mark.parametrize("redirect_uris", [None, []], ids=["absent", "empty"]) +@pytest.mark.parametrize( + "redirect_uri", [None, AnyUrl("https://example.com/callback")], ids=["unspecified", "specified"] +) +def test_client_with_no_registered_redirect_uris_cannot_resolve_a_redirect( + redirect_uris: list[str] | None, redirect_uri: AnyUrl | None +): + """With no registered redirect URIs (absent or empty), no redirect resolves - neither a + supplied one (nothing to match against) nor an unspecified one (no single default).""" + info = OAuthClientInformationFull.model_validate({"client_id": "abc123", "redirect_uris": redirect_uris}) + with pytest.raises(InvalidRedirectUriError): + info.validate_redirect_uri(redirect_uri) + + +def test_request_metadata_restricts_application_type_to_the_values_the_sdk_sends(): + """What the SDK sends stays narrow even though what it accepts back is wide.""" + with pytest.raises(ValidationError): + OAuthClientMetadata.model_validate( + {"redirect_uris": ["https://example.com/callback"], "application_type": "confidential"} + ) + + +def test_request_metadata_requires_at_least_one_redirect_uri(): + with pytest.raises(ValidationError): + OAuthClientMetadata.model_validate({"redirect_uris": []}) + + def test_invalid_non_empty_url_still_rejected(): """Coercion must only touch empty strings — garbage URLs still raise.""" data = { From 470bff3dfca3b4fc797e3a838ebf9432c9a474ef Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Mon, 27 Jul 2026 15:47:28 +0000 Subject: [PATCH 2/4] Address review: split the auth-method usability sets, seal the parse boundary Two conflated questions shared one recognized-method set: whether the authorization-code registration flow can act on an assigned method, and whether a stored record may carry a method without the token step erroring. The flow authenticates with the minted secret and holds no key to sign a private_key_jwt assertion, so it now rejects that method at registration too, while prepare_token_auth still tolerates it for PrivateKeyJWTOAuthProvider's inherited refresh path. An explicit JSON null in the registration response is now read as an omitted key across the whole record, so grant_types and response_types (which pydantic rejects null for) parse like the other fields. The SDK-internal issuer binding is cleared after parsing, so a body echoing an "issuer" member cannot seed it. The bundled registration endpoint sets client_secret_expires_at to 0 rather than omitting it whenever a client_secret is issued with no expiry configured, as RFC 7591 3.2.1 requires. The migration entry now names both exception surfaces. --- docs/migration.md | 2 +- src/mcp/client/auth/oauth2.py | 34 ++++++++++++++-------- src/mcp/client/auth/utils.py | 6 +++- src/mcp/server/auth/handlers/register.py | 14 +++++---- src/mcp/shared/auth.py | 23 +++++++++++---- tests/client/test_auth.py | 13 +++++++++ tests/interaction/_requirements.py | 10 ++++--- tests/interaction/auth/test_as_handlers.py | 8 +++-- tests/interaction/auth/test_discovery.py | 15 ++++++---- tests/shared/test_auth.py | 19 ++++++++++++ 10 files changed, 109 insertions(+), 35 deletions(-) diff --git a/docs/migration.md b/docs/migration.md index 11397c0f75..82bb37f52d 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -2196,7 +2196,7 @@ class OAuthClientInformationFull(OAuthClientMetadataBase): ... # server record On `OAuthClientInformationFull`, `application_type` and `token_endpoint_auth_method` are now `str | None`, `grant_types` is `list[str]`, and `redirect_uris` is optional (`list[AnyUrl] | None`, no minimum length). `client_id` is now required (`str`): [RFC 7591 §3.2.1](https://datatracker.ietf.org/doc/html/rfc7591#section-3.2.1) makes it mandatory in the response, and a record of a registered client without one was never meaningful. Code that only reads these fields is unaffected. Code that relied on `isinstance(client_info, OAuthClientMetadata)`, or passed an `OAuthClientInformationFull` where an `OAuthClientMetadata` is expected, must reference the record type directly. `validate_scope()` and `validate_redirect_uri()` moved with the record: they are methods of `OAuthClientInformationFull` (the type authorization-server code holds) and are no longer available on `OAuthClientMetadata`. -A registration response the server sends is no longer rejected on these fields (an echoed `""` for `token_endpoint_auth_method` or `application_type` is treated as absent, as it already was for the optional URL fields). Whether a substituted value is usable is decided where the value is used: a registered `token_endpoint_auth_method` this SDK does not recognize (anything other than `none`, `client_secret_post`, `client_secret_basic`, or `private_key_jwt`) now raises `OAuthTokenError` naming the method at the token exchange, rather than the token request being sent unauthenticated for the server to reject. +A registration response the server sends is no longer rejected on these fields: an echoed `""` for `token_endpoint_auth_method` or `application_type` reads as absent (as it already did for the optional URL fields), and a member serialized as an explicit `null` reads as an omitted key, so its default applies. Whether a substituted value is usable is judged where it matters, not at parse. When Dynamic Client Registration completes with a `token_endpoint_auth_method` the authorization-code flow cannot apply - anything other than `none`, `client_secret_post`, or `client_secret_basic`, including `private_key_jwt`, whose assertion that flow has no key to sign - the client raises `OAuthRegistrationError` naming the method, before the record is stored or authorization begins. Separately, a stored or pre-registered record carrying a method the SDK does not know at all raises `OAuthTokenError` when it reaches the token exchange (`private_key_jwt` on such a record is fine: `PrivateKeyJWTOAuthProvider` supplies the assertion, and its refresh path still works). The SDK's own registration endpoint also now returns all registered metadata in its 201 response (RFC 7591 §3.2.1), including the client's `application_type`, which v1 dropped from the echo (silently reporting the default in place of a client's `"web"`). diff --git a/src/mcp/client/auth/oauth2.py b/src/mcp/client/auth/oauth2.py index a67fe4bdf1..52d8e4b725 100644 --- a/src/mcp/client/auth/oauth2.py +++ b/src/mcp/client/auth/oauth2.py @@ -59,28 +59,38 @@ logger = logging.getLogger(__name__) -# Methods `OAuthContext.prepare_token_auth` recognizes on a registered client, derived from the -# same set the SDK is willing to request so the two cannot drift. `None`/"none" send no client -# secret; `private_key_jwt` adds no secret here (its assertion is supplied by -# `PrivateKeyJWTOAuthProvider`). Anything else is a method this client cannot apply. -_RECOGNIZED_TOKEN_ENDPOINT_AUTH_METHODS: tuple[str | None, ...] = (None, *get_args(TokenEndpointAuthMethod)) +# Methods a registered client's record may carry without a token request being an error, +# derived from the set the SDK is willing to request so the two cannot drift. `None`/"none" +# send no client secret; `private_key_jwt` sends none from here either, its assertion being +# added by `PrivateKeyJWTOAuthProvider` (whose inherited refresh path passes through +# `prepare_token_auth`). Anything else is a method no client here can apply. +_KNOWN_TOKEN_ENDPOINT_AUTH_METHODS: tuple[str | None, ...] = (None, *get_args(TokenEndpointAuthMethod)) + +# Methods a registration completed by the authorization-code flow can act on. That flow +# authenticates the token request with the minted client secret (or nothing); it holds no key +# to sign a `private_key_jwt` assertion, so a server assigning that method has registered a +# client this flow cannot use. `PrivateKeyJWTOAuthProvider` never registers dynamically. +_REGISTRATION_USABLE_TOKEN_ENDPOINT_AUTH_METHODS: tuple[str | None, ...] = tuple( + method for method in _KNOWN_TOKEN_ENDPOINT_AUTH_METHODS if method != "private_key_jwt" +) def check_registration_usable(client_info: OAuthClientInformationFull) -> None: - """Confirm a completed registration is one this client can act on. + """Confirm a registration this flow completed is one it can act on. RFC 7591 §3.2.1 lets the authorization server replace requested metadata and leaves it to the client to "check the values in the response to determine if the registration is sufficient for use". The one substitution that makes the minted credentials unusable is a - token-endpoint auth method this client cannot apply, so it is judged here - before the - record is persisted or any interactive authorization begins - rather than surfacing later - as an opaque failure at the token endpoint. + token-endpoint auth method the authorization-code flow cannot apply - one it does not + implement, or `private_key_jwt`, whose assertion this flow has no key to sign - so it is + judged here, before the record is persisted or any interactive authorization begins, + rather than surfacing later as an opaque failure at the token endpoint. Raises: OAuthRegistrationError: The server registered the client with a - `token_endpoint_auth_method` this client does not implement. + `token_endpoint_auth_method` this flow cannot apply. """ - if client_info.token_endpoint_auth_method not in _RECOGNIZED_TOKEN_ENDPOINT_AUTH_METHODS: + if client_info.token_endpoint_auth_method not in _REGISTRATION_USABLE_TOKEN_ENDPOINT_AUTH_METHODS: raise OAuthRegistrationError( "Authorization server registered the client with unsupported token_endpoint_auth_method " f"{client_info.token_endpoint_auth_method!r}" @@ -246,7 +256,7 @@ def prepare_token_auth( # Include client_id and client_secret in request body (RFC 6749 §2.3.1) data["client_id"] = self.client_info.client_id data["client_secret"] = self.client_info.client_secret - elif auth_method not in _RECOGNIZED_TOKEN_ENDPOINT_AUTH_METHODS: + elif auth_method not in _KNOWN_TOKEN_ENDPOINT_AUTH_METHODS: raise OAuthTokenError(f"Registered client uses unsupported token_endpoint_auth_method {auth_method!r}") # For "none" (or absent), don't add any client_secret; "private_key_jwt" adds its # assertion in the provider that implements it, not here. diff --git a/src/mcp/client/auth/utils.py b/src/mcp/client/auth/utils.py index 282f23b879..3911bace96 100644 --- a/src/mcp/client/auth/utils.py +++ b/src/mcp/client/auth/utils.py @@ -300,9 +300,13 @@ async def handle_registration_response(response: Response) -> OAuthClientInforma try: content = await response.aread() client_info = OAuthClientInformationFull.model_validate_json(content) - return client_info except ValidationError as e: raise OAuthRegistrationError(f"Invalid registration response: {e}") from e + # `issuer` is the SDK's own binding of these credentials to the server they were + # registered with (SEP-2352), stamped by the auth flow - never taken from the wire. + # A body echoing an "issuer" member must not seed the trust binding. + client_info.issuer = None + return client_info def is_valid_client_metadata_url(url: str | None) -> bool: diff --git a/src/mcp/server/auth/handlers/register.py b/src/mcp/server/auth/handlers/register.py index ed857ecfe7..8b8422e53a 100644 --- a/src/mcp/server/auth/handlers/register.py +++ b/src/mcp/server/auth/handlers/register.py @@ -106,11 +106,15 @@ async def handle(self, request: Request) -> Response: ) client_id_issued_at = int(time.time()) - client_secret_expires_at = ( - client_id_issued_at + self.options.client_secret_expiry_seconds - if self.options.client_secret_expiry_seconds is not None - else None - ) + # RFC 7591 §3.2.1: client_secret_expires_at is REQUIRED whenever a client_secret is + # issued, with 0 (not omission) meaning it never expires; a public client gets none. + client_secret_expires_at = None + if client_secret is not None: + client_secret_expires_at = ( + client_id_issued_at + self.options.client_secret_expiry_seconds + if self.options.client_secret_expiry_seconds is not None + else 0 + ) # RFC 7591 §3.2.1: the response returns all registered metadata about the client, so # the record is the whole validated request plus the credentials minted here - built diff --git a/src/mcp/shared/auth.py b/src/mcp/shared/auth.py index b338ca67df..6fc5cb38c9 100644 --- a/src/mcp/shared/auth.py +++ b/src/mcp/shared/auth.py @@ -1,6 +1,6 @@ -from typing import Any, Literal +from typing import Any, Literal, cast -from pydantic import AnyHttpUrl, AnyUrl, BaseModel, ConfigDict, Field, field_validator +from pydantic import AnyHttpUrl, AnyUrl, BaseModel, ConfigDict, Field, field_validator, model_validator # RFC 7523 JWT bearer grant; SEP-990 leg 2 uses this to present the ID-JAG. JWT_BEARER_GRANT_TYPE = "urn:ietf:params:oauth:grant-type:jwt-bearer" @@ -136,9 +136,11 @@ class OAuthClientInformationFull(OAuthClientMetadataBase): requested metadata values submitted during the registration and substitute them with suitable values", so `application_type`, `token_endpoint_auth_method`, and `grant_types` are typed to accept any string the server echoes, and `redirect_uris` may be absent or - empty. Whether a substituted value is usable is decided where the value is used, not at - parse. `redirect_uris` elements are still parsed as URLs, as the authorization server - compares them against a client's requested `redirect_uri`. + empty. A member the server serializes as an explicit `null` is read as omitted, so the + field's default applies rather than the parse failing. Whether a substituted value is + usable is decided where the value is used, not at parse. `redirect_uris` elements are + still parsed as URLs, as the authorization server compares them against a client's + requested `redirect_uri`. """ redirect_uris: list[AnyUrl] | None = None @@ -160,6 +162,17 @@ class OAuthClientInformationFull(OAuthClientMetadataBase): # RFC 7591 field) to detect authorization-server migration and avoid cross-AS credential reuse. issuer: str | None = None + @model_validator(mode="before") + @classmethod + def _explicit_null_reads_as_omitted(cls, data: object) -> object: + # Servers that serialize their client record dump unset members as null instead of + # omitting the keys; a null for a list field would otherwise fail the parse (and + # discard an already-provisioned registration). null and absent mean the same thing. + if isinstance(data, dict): + members = cast(dict[str, Any], data) + return {key: value for key, value in members.items() if value is not None} + return data + @field_validator("token_endpoint_auth_method", "application_type", mode="before") @classmethod def _empty_string_optional_metadata_to_none(cls, v: object) -> object: diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index 48ca608b8a..2f30ab4334 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -1030,6 +1030,19 @@ async def test_registration_response_with_substituted_metadata_yields_the_creden assert client_info.application_type == "confidential" +@pytest.mark.anyio +async def test_registration_response_does_not_seed_the_issuer_binding_from_the_body(): + """The issuer binding (SEP-2352) is the SDK's record of which server it registered with, + stamped by the auth flow; an "issuer" member in the untrusted response body must not + populate it, or a mismatched binding would discard the credentials on every 401.""" + body = b'{"client_id": "issued-id", "issuer": "https://not-the-flow.example"}' + + client_info = await handle_registration_response(httpx2.Response(201, content=body)) + + assert client_info.client_id == "issued-id" + assert client_info.issuer is None + + @pytest.mark.anyio async def test_a_2xx_body_that_is_not_client_information_is_an_oauth_registration_error(): """A success status whose body is not client information surfaces as OAuthRegistrationError.""" diff --git a/tests/interaction/_requirements.py b/tests/interaction/_requirements.py index 6c77af5a45..624f234ba1 100644 --- a/tests/interaction/_requirements.py +++ b/tests/interaction/_requirements.py @@ -2944,8 +2944,9 @@ def __post_init__(self) -> None: source="sdk", behavior=( "The bundled registration endpoint returns all registered metadata about the client " - "in its 201 response (RFC 7591 §3.2.1), including the client's `application_type` " - "rather than a substituted default." + "in its 201 response (RFC 7591 §3.2.1) - the client's `application_type` rather than a " + "substituted default, and `client_secret_expires_at` (0 when the secret never expires) " + "whenever a `client_secret` is issued." ), transports=("streamable-http",), note="Auth is enforced at the HTTP layer; the bundled AS is an ASGI app.", @@ -3675,8 +3676,9 @@ def __post_init__(self) -> None: behavior=( "A 201 registration response whose echoed metadata the server substituted (RFC 7591 §3.2.1) - " "an unregistered application_type, null redirect_uris, extra grant types - completes the flow; " - "a substituted token_endpoint_auth_method the client cannot apply is instead reported as an " - "OAuthRegistrationError before the record is persisted or authorization begins." + "a substituted token_endpoint_auth_method the authorization-code flow cannot apply (an " + "unimplemented method, or private_key_jwt, whose assertion it has no key to sign) is instead " + "reported as an OAuthRegistrationError before the record is persisted or authorization begins." ), transports=("streamable-http",), note="OAuth is HTTP-only.", diff --git a/tests/interaction/auth/test_as_handlers.py b/tests/interaction/auth/test_as_handlers.py index 7182a62e66..5535f06c33 100644 --- a/tests/interaction/auth/test_as_handlers.py +++ b/tests/interaction/auth/test_as_handlers.py @@ -258,8 +258,12 @@ async def test_registration_response_echoes_the_registered_application_type( response = await http.post("/register", json=body | {"application_type": application_type}) assert response.status_code == 201 - info = OAuthClientInformationFull.model_validate_json(response.content) - assert info.application_type == application_type + echoed = response.json() + assert echoed["application_type"] == application_type + # A secret was issued and no expiry is configured, so RFC 7591 §3.2.1 requires the + # response to carry client_secret_expires_at, with 0 (present, not omitted) for "never". + assert echoed["client_secret"] + assert echoed["client_secret_expires_at"] == 0 @requirement("hosting:auth:as:redirect-uri-binding") diff --git a/tests/interaction/auth/test_discovery.py b/tests/interaction/auth/test_discovery.py index 5a61a81032..9b195c1975 100644 --- a/tests/interaction/auth/test_discovery.py +++ b/tests/interaction/auth/test_discovery.py @@ -223,18 +223,23 @@ async def test_a_registration_response_with_substituted_metadata_completes_the_f @requirement("client-auth:dcr:substituted-metadata") -async def test_a_registration_assigning_an_unusable_auth_method_surfaces_as_a_registration_error() -> None: - """A 201 assigning an auth method the client cannot apply is a registration error. +@pytest.mark.parametrize("auth_method", ["client_secret_jwt", "private_key_jwt"]) +async def test_a_registration_assigning_an_unusable_auth_method_surfaces_as_a_registration_error( + auth_method: str, +) -> None: + """A 201 assigning an auth method this flow cannot apply is a registration error. RFC 7591 §3.2.1 leaves it to the client to judge whether a substituted value makes the - registration usable; an unimplemented token-endpoint auth method does not, and is - reported before the record is stored or any authorize/token request is made. + registration usable. The authorization-code flow authenticates with the minted secret, so + neither an unimplemented method nor `private_key_jwt` (an assertion it has no key to + sign) is usable; both are reported before the record is stored or any authorize/token + request is made. """ recorded, on_request = record_requests() provider = InMemoryAuthorizationServerProvider() server = Server("guarded", on_list_tools=list_tools) body = json.dumps( - {"client_id": "jwt-only", "client_secret": "s3cr3t", "token_endpoint_auth_method": "client_secret_jwt"} + {"client_id": "unusable", "client_secret": "s3cr3t", "token_endpoint_auth_method": auth_method} ).encode() app_shim = shim(serve={"/register": (201, body)}) storage = InMemoryTokenStorage() diff --git a/tests/shared/test_auth.py b/tests/shared/test_auth.py index e3403e914e..c818db98f3 100644 --- a/tests/shared/test_auth.py +++ b/tests/shared/test_auth.py @@ -176,6 +176,25 @@ def test_a_registration_response_without_a_client_id_is_rejected(): OAuthClientInformationFull.model_validate({"application_type": "web"}) +@pytest.mark.parametrize( + "nulled", + ["grant_types", "response_types", "redirect_uris", "application_type", "token_endpoint_auth_method", "scope"], +) +def test_client_information_reads_an_explicit_null_as_an_omitted_key(nulled: str): + """A server that dumps unset members as null (rather than omitting them) still yields a + usable registration: null and absent mean the same, so the field's default applies.""" + info = OAuthClientInformationFull.model_validate({"client_id": "abc123", nulled: None}) + defaults = OAuthClientInformationFull.model_validate({"client_id": "abc123"}) + assert getattr(info, nulled) == getattr(defaults, nulled) + + +def test_client_information_that_is_not_an_object_still_fails_the_parse(): + """The null-as-omitted coercion only touches JSON objects; a body that is not one is + passed through and rejected as a normal validation failure rather than swallowed.""" + with pytest.raises(ValidationError): + OAuthClientInformationFull.model_validate("not-an-object") + + @pytest.mark.parametrize("empty_field", ["token_endpoint_auth_method", "application_type"]) def test_client_information_empty_string_metadata_coerced_to_absent(empty_field: str): """An echoed "" reads as absent, matching the URL-field coercion, so it is neither From 40fe04630179b94148b965aaa92bbda24eaa8be0 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:43:57 +0000 Subject: [PATCH 3/4] Address review: seal the parse boundary and judge secret-less methods An SDK-internal field must never be sourced from the wire, not sourced then cleared: the registration response body is now parsed with issuer dropped before validation, so an echoed member neither seeds the SEP-2352 binding nor fails the parse when malformed. The parse goes through pydantic's JSON layer so malformed bytes still surface as OAuthRegistrationError. Both placeholder styles servers use for unset members - null, and "" - now read as omitted keys across the whole record, closing the gap for list fields and retiring the field-by-field empty-string coercions. An empty client_id is accordingly no client_id, so the unrepresentable empty-identifier record and its dead guards in prepare_token_auth go. check_registration_usable now also rejects a secret-based method for which the server issued no client_secret, before persistence and before any interactive authorization, instead of letting the token request go out unauthenticated after the browser round-trip. The bundled registration endpoint refuses private_key_jwt with 400 invalid_client_metadata: it authenticates token requests with the secret it mints and holds no client key to verify an assertion, so confirming the method would register a client it could never authenticate - which this SDK's own client would then immediately declare unusable, orphaning the record. Correct prose that overstated the private_key_jwt refresh path (the tolerance keeps prepare_token_auth from raising so a rejected refresh can fall back to a fresh, signed client-credentials exchange), the stale Raises text in prepare_token_auth, and the client failure-model page, which described OAuthRegistrationError as only a refusal. --- docs/client/oauth-clients.md | 2 +- docs/migration.md | 4 +- src/mcp/client/auth/oauth2.py | 48 ++++++++++++------- src/mcp/client/auth/utils.py | 19 +++++--- src/mcp/server/auth/handlers/register.py | 12 +++++ src/mcp/shared/auth.py | 34 ++++++------- .../extensions/test_client_credentials.py | 38 --------------- tests/client/test_auth.py | 30 ++++++++---- tests/interaction/_requirements.py | 7 +-- tests/interaction/auth/test_as_handlers.py | 17 +++++-- tests/interaction/auth/test_discovery.py | 27 +++++++---- tests/shared/test_auth.py | 29 +++++------ 12 files changed, 141 insertions(+), 126 deletions(-) diff --git a/docs/client/oauth-clients.md b/docs/client/oauth-clients.md index 08ccdca1df..2cd6663e0b 100644 --- a/docs/client/oauth-clients.md +++ b/docs/client/oauth-clients.md @@ -131,7 +131,7 @@ There is one more no-human situation: the client belongs to an enterprise whose ## When it fails -When the OAuth flow goes wrong, the provider raises an `OAuthFlowError` from `mcp.client.auth`. It has two subclasses. `OAuthRegistrationError` means the authorization server refused to register you. `OAuthTokenError` means the token endpoint said no. One `except OAuthFlowError:` covers discovery, registration, authorization, and exchange. +When the OAuth flow goes wrong, the provider raises an `OAuthFlowError` from `mcp.client.auth`. It has two subclasses. `OAuthRegistrationError` means registration did not yield a client you can use: the authorization server refused to register you, or it did register you but with credentials this flow cannot use (for instance an authentication method it does not implement). `OAuthTokenError` means the token endpoint said no. One `except OAuthFlowError:` covers discovery, registration, authorization, and exchange. Not everything is a flow error. The network can still fail; those are ordinary `httpx2` exceptions and pass through untouched. diff --git a/docs/migration.md b/docs/migration.md index 82bb37f52d..0f62018584 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -2196,9 +2196,9 @@ class OAuthClientInformationFull(OAuthClientMetadataBase): ... # server record On `OAuthClientInformationFull`, `application_type` and `token_endpoint_auth_method` are now `str | None`, `grant_types` is `list[str]`, and `redirect_uris` is optional (`list[AnyUrl] | None`, no minimum length). `client_id` is now required (`str`): [RFC 7591 §3.2.1](https://datatracker.ietf.org/doc/html/rfc7591#section-3.2.1) makes it mandatory in the response, and a record of a registered client without one was never meaningful. Code that only reads these fields is unaffected. Code that relied on `isinstance(client_info, OAuthClientMetadata)`, or passed an `OAuthClientInformationFull` where an `OAuthClientMetadata` is expected, must reference the record type directly. `validate_scope()` and `validate_redirect_uri()` moved with the record: they are methods of `OAuthClientInformationFull` (the type authorization-server code holds) and are no longer available on `OAuthClientMetadata`. -A registration response the server sends is no longer rejected on these fields: an echoed `""` for `token_endpoint_auth_method` or `application_type` reads as absent (as it already did for the optional URL fields), and a member serialized as an explicit `null` reads as an omitted key, so its default applies. Whether a substituted value is usable is judged where it matters, not at parse. When Dynamic Client Registration completes with a `token_endpoint_auth_method` the authorization-code flow cannot apply - anything other than `none`, `client_secret_post`, or `client_secret_basic`, including `private_key_jwt`, whose assertion that flow has no key to sign - the client raises `OAuthRegistrationError` naming the method, before the record is stored or authorization begins. Separately, a stored or pre-registered record carrying a method the SDK does not know at all raises `OAuthTokenError` when it reaches the token exchange (`private_key_jwt` on such a record is fine: `PrivateKeyJWTOAuthProvider` supplies the assertion, and its refresh path still works). +A registration response the server sends is no longer rejected on these fields: a member serialized as a placeholder - an explicit `null`, or `""` - reads as an omitted key, so its default applies. Whether a substituted value is usable is judged where it matters, not at parse. When Dynamic Client Registration completes with credentials the authorization-code flow cannot use - a `token_endpoint_auth_method` other than `none`, `client_secret_post`, or `client_secret_basic` (including `private_key_jwt`, whose assertion that flow has no key to sign), or a secret-based method for which the server issued no `client_secret` - the client raises `OAuthRegistrationError` naming the problem, before the record is stored or authorization begins. Separately, a stored or pre-registered record carrying a method the SDK does not know at all raises `OAuthTokenError` when it reaches the token exchange; `private_key_jwt` on such a record does not raise there, so `PrivateKeyJWTOAuthProvider`, which signs its assertion only in the client-credentials exchange, still recovers from a rejected refresh by exchanging afresh. -The SDK's own registration endpoint also now returns all registered metadata in its 201 response (RFC 7591 §3.2.1), including the client's `application_type`, which v1 dropped from the echo (silently reporting the default in place of a client's `"web"`). +The SDK's own registration endpoint now returns all registered metadata in its 201 response (RFC 7591 §3.2.1) - including the client's `application_type`, which v1 dropped from the echo (silently reporting the default in place of a client's `"web"`), and `client_secret_expires_at` (`0` when the secret never expires) whenever a `client_secret` is issued. It also now answers a `private_key_jwt` registration with `400 invalid_client_metadata` rather than confirming a method it authenticates no requests with. ### Stricter client authentication at `/token` and `/revoke` diff --git a/src/mcp/client/auth/oauth2.py b/src/mcp/client/auth/oauth2.py index 52d8e4b725..7dc62b52b9 100644 --- a/src/mcp/client/auth/oauth2.py +++ b/src/mcp/client/auth/oauth2.py @@ -61,11 +61,17 @@ # Methods a registered client's record may carry without a token request being an error, # derived from the set the SDK is willing to request so the two cannot drift. `None`/"none" -# send no client secret; `private_key_jwt` sends none from here either, its assertion being -# added by `PrivateKeyJWTOAuthProvider` (whose inherited refresh path passes through -# `prepare_token_auth`). Anything else is a method no client here can apply. +# send no client secret. `private_key_jwt` sends none from here either: only +# `PrivateKeyJWTOAuthProvider` signs the assertion, and only in its client-credentials +# exchange, so its inherited refresh path must pass through here without raising - a refresh +# the server then rejects falls back to a fresh client-credentials exchange, which signs. +# Anything else is a method no client here can apply. _KNOWN_TOKEN_ENDPOINT_AUTH_METHODS: tuple[str | None, ...] = (None, *get_args(TokenEndpointAuthMethod)) +# Methods that authenticate the token request with the minted `client_secret`; a +# registration assigning one is only usable if the server issued that secret. +_SECRET_TOKEN_ENDPOINT_AUTH_METHODS = ("client_secret_post", "client_secret_basic") + # Methods a registration completed by the authorization-code flow can act on. That flow # authenticates the token request with the minted client secret (or nothing); it holds no key # to sign a `private_key_jwt` assertion, so a server assigning that method has registered a @@ -80,20 +86,26 @@ def check_registration_usable(client_info: OAuthClientInformationFull) -> None: RFC 7591 §3.2.1 lets the authorization server replace requested metadata and leaves it to the client to "check the values in the response to determine if the registration is - sufficient for use". The one substitution that makes the minted credentials unusable is a - token-endpoint auth method the authorization-code flow cannot apply - one it does not - implement, or `private_key_jwt`, whose assertion this flow has no key to sign - so it is - judged here, before the record is persisted or any interactive authorization begins, - rather than surfacing later as an opaque failure at the token endpoint. + sufficient for use". Two substitutions make the minted credentials unusable, and both are + judged here - before the record is persisted or any interactive authorization begins - + rather than surfacing later as an opaque failure at the token endpoint: a token-endpoint + auth method the authorization-code flow cannot apply (one it does not implement, or + `private_key_jwt`, whose assertion this flow has no key to sign), and a secret-based + method the flow could apply but for which the server issued no `client_secret`. Raises: OAuthRegistrationError: The server registered the client with a - `token_endpoint_auth_method` this flow cannot apply. + `token_endpoint_auth_method` this flow cannot apply, or with a secret-based + method but no `client_secret`. """ - if client_info.token_endpoint_auth_method not in _REGISTRATION_USABLE_TOKEN_ENDPOINT_AUTH_METHODS: + method = client_info.token_endpoint_auth_method + if method not in _REGISTRATION_USABLE_TOKEN_ENDPOINT_AUTH_METHODS: + raise OAuthRegistrationError( + f"Authorization server registered the client with unsupported token_endpoint_auth_method {method!r}" + ) + if method in _SECRET_TOKEN_ENDPOINT_AUTH_METHODS and client_info.client_secret is None: raise OAuthRegistrationError( - "Authorization server registered the client with unsupported token_endpoint_auth_method " - f"{client_info.token_endpoint_auth_method!r}" + f"Authorization server registered the client for {method!r} but issued no client_secret" ) @@ -230,10 +242,10 @@ def prepare_token_auth( Tuple of (updated_data, updated_headers) Raises: - OAuthTokenError: The registered client's `token_endpoint_auth_method` is one this - client does not implement. The authorization server may assign a method other - than the one requested (RFC 7591 §3.2.1); the registration record accepts it, - and the mismatch is reported here, where the method is applied. + OAuthTokenError: The client record carries a `token_endpoint_auth_method` this + client does not know. A dynamic registration assigning an unusable method is + rejected earlier, by `check_registration_usable`; this fires for a stored or + pre-registered record that reaches a token request with such a method. """ if headers is None: headers = {} # pragma: no cover @@ -243,7 +255,7 @@ def prepare_token_auth( auth_method = self.client_info.token_endpoint_auth_method - if auth_method == "client_secret_basic" and self.client_info.client_id and self.client_info.client_secret: + if auth_method == "client_secret_basic" and self.client_info.client_secret: # URL-encode client ID and secret per RFC 6749 Section 2.3.1 encoded_id = quote(self.client_info.client_id, safe="") encoded_secret = quote(self.client_info.client_secret, safe="") @@ -252,7 +264,7 @@ def prepare_token_auth( headers["Authorization"] = f"Basic {encoded_credentials}" # Don't include client_secret in body for basic auth data = {k: v for k, v in data.items() if k != "client_secret"} - elif auth_method == "client_secret_post" and self.client_info.client_id and self.client_info.client_secret: + elif auth_method == "client_secret_post" and self.client_info.client_secret: # Include client_id and client_secret in request body (RFC 6749 §2.3.1) data["client_id"] = self.client_info.client_id data["client_secret"] = self.client_info.client_secret diff --git a/src/mcp/client/auth/utils.py b/src/mcp/client/auth/utils.py index 3911bace96..31e2e5cade 100644 --- a/src/mcp/client/auth/utils.py +++ b/src/mcp/client/auth/utils.py @@ -1,9 +1,11 @@ import re +from typing import Any, cast from urllib.parse import urljoin, urlparse from httpx2 import Request, Response from mcp_types import LATEST_PROTOCOL_VERSION from pydantic import AnyUrl, ValidationError +from pydantic_core import from_json from mcp.client.auth import OAuthFlowError, OAuthRegistrationError, OAuthTokenError from mcp.shared.auth import ( @@ -299,14 +301,17 @@ async def handle_registration_response(response: Response) -> OAuthClientInforma try: content = await response.aread() - client_info = OAuthClientInformationFull.model_validate_json(content) - except ValidationError as e: + body = from_json(content) + # `issuer` is the SDK's own binding of these credentials to the server they were + # registered with (SEP-2352), stamped by the auth flow - never sourced from the + # wire, so it is dropped before the body is parsed rather than trusted or cleared. + if isinstance(body, dict): + cast(dict[str, Any], body).pop("issuer", None) + return OAuthClientInformationFull.model_validate(body) + except ValueError as e: + # `from_json` reports malformed bytes/JSON as ValueError, and pydantic's + # ValidationError is itself a ValueError, so both parse layers surface here. raise OAuthRegistrationError(f"Invalid registration response: {e}") from e - # `issuer` is the SDK's own binding of these credentials to the server they were - # registered with (SEP-2352), stamped by the auth flow - never taken from the wire. - # A body echoing an "issuer" member must not seed the trust binding. - client_info.issuer = None - return client_info def is_valid_client_metadata_url(url: str | None) -> bool: diff --git a/src/mcp/server/auth/handlers/register.py b/src/mcp/server/auth/handlers/register.py index 8b8422e53a..7fb14b2c43 100644 --- a/src/mcp/server/auth/handlers/register.py +++ b/src/mcp/server/auth/handlers/register.py @@ -50,6 +50,18 @@ async def handle(self, request: Request) -> Response: # If auth method is None, default to client_secret_post if client_metadata.token_endpoint_auth_method is None: client_metadata.token_endpoint_auth_method = "client_secret_post" + # This server authenticates token requests with the client secret it mints; it holds + # no client key to verify a private_key_jwt assertion, so confirming that method would + # register a client whose every token request is then rejected. Refuse it instead + # (RFC 7591 §3.2.2), before minting credentials the client could never use. + if client_metadata.token_endpoint_auth_method == "private_key_jwt": + return PydanticJSONResponse( + content=RegistrationErrorResponse( + error="invalid_client_metadata", + error_description="token_endpoint_auth_method 'private_key_jwt' is not supported", + ), + status_code=400, + ) client_secret = None if client_metadata.token_endpoint_auth_method != "none": # pragma: no branch diff --git a/src/mcp/shared/auth.py b/src/mcp/shared/auth.py index 6fc5cb38c9..881379d381 100644 --- a/src/mcp/shared/auth.py +++ b/src/mcp/shared/auth.py @@ -15,9 +15,9 @@ def _empty_str_to_none(v: object) -> object: - # Some authorization servers echo omitted metadata back as "" instead of dropping the - # key. RFC 7591 §2 marks these fields OPTIONAL; treat "" as absent rather than rejecting - # an otherwise valid registration response over a placeholder. + # RFC 7591 §2 marks these URL fields OPTIONAL; a "" placeholder means absent, so it + # must not fail AnyHttpUrl validation. (The registered-client record applies the same + # rule to every member; this coercion serves the request model.) if v == "": return None return v @@ -136,11 +136,11 @@ class OAuthClientInformationFull(OAuthClientMetadataBase): requested metadata values submitted during the registration and substitute them with suitable values", so `application_type`, `token_endpoint_auth_method`, and `grant_types` are typed to accept any string the server echoes, and `redirect_uris` may be absent or - empty. A member the server serializes as an explicit `null` is read as omitted, so the - field's default applies rather than the parse failing. Whether a substituted value is - usable is decided where the value is used, not at parse. `redirect_uris` elements are - still parsed as URLs, as the authorization server compares them against a client's - requested `redirect_uri`. + empty. A member the server serializes as a placeholder - an explicit `null`, or `""` - + is read as an omitted key, so the field's default applies rather than the parse failing. + Whether a substituted value is usable is decided where the value is used, not at parse. + `redirect_uris` elements are still parsed as URLs, as the authorization server compares + them against a client's requested `redirect_uri`. """ redirect_uris: list[AnyUrl] | None = None @@ -164,22 +164,16 @@ class OAuthClientInformationFull(OAuthClientMetadataBase): @model_validator(mode="before") @classmethod - def _explicit_null_reads_as_omitted(cls, data: object) -> object: - # Servers that serialize their client record dump unset members as null instead of - # omitting the keys; a null for a list field would otherwise fail the parse (and - # discard an already-provisioned registration). null and absent mean the same thing. + def _placeholder_members_read_as_omitted(cls, data: object) -> object: + # Servers dump unset members of their client record as null, or echo them as "", + # instead of omitting the keys. Either placeholder would otherwise fail the parse of a + # list field (or read "" as an unrecognized method) and discard an already-provisioned + # registration; a placeholder and an absent key mean the same thing. if isinstance(data, dict): members = cast(dict[str, Any], data) - return {key: value for key, value in members.items() if value is not None} + return {key: value for key, value in members.items() if value is not None and value != ""} return data - @field_validator("token_endpoint_auth_method", "application_type", mode="before") - @classmethod - def _empty_string_optional_metadata_to_none(cls, v: object) -> object: - # An echoed "" here would otherwise read as an unrecognized method/type; treat it as - # absent, matching the URL-field coercion. - return _empty_str_to_none(v) - def validate_scope(self, requested_scope: str | None) -> list[str] | None: if requested_scope is None: return None diff --git a/tests/client/auth/extensions/test_client_credentials.py b/tests/client/auth/extensions/test_client_credentials.py index 84296eb6de..16336f8002 100644 --- a/tests/client/auth/extensions/test_client_credentials.py +++ b/tests/client/auth/extensions/test_client_credentials.py @@ -149,44 +149,6 @@ async def test_exchange_token_client_secret_post_includes_client_id(self, mock_s # Should NOT have Basic auth header assert "Authorization" not in request.headers - @pytest.mark.anyio - async def test_exchange_token_client_secret_post_without_client_id(self, mock_storage: MockTokenStorage): - """Test client_secret_post skips body credentials when client_id is empty.""" - provider = ClientCredentialsOAuthProvider( - server_url="https://api.example.com/v1/mcp", - storage=mock_storage, - client_id="placeholder", - client_secret="test-client-secret", - token_endpoint_auth_method="client_secret_post", - scope="read write", - ) - await provider._initialize() - provider.context.oauth_metadata = OAuthMetadata( - issuer=AnyHttpUrl("https://api.example.com"), - authorization_endpoint=AnyHttpUrl("https://api.example.com/authorize"), - token_endpoint=AnyHttpUrl("https://api.example.com/token"), - ) - provider.context.protocol_version = "2025-06-18" - # Override client_info to have an empty client_id (edge case) - provider.context.client_info = OAuthClientInformationFull( - redirect_uris=None, - client_id="", - client_secret="test-client-secret", - grant_types=["client_credentials"], - token_endpoint_auth_method="client_secret_post", - scope="read write", - ) - - request = await provider._perform_authorization() - - content = urllib.parse.unquote_plus(request.content.decode()) - assert "grant_type=client_credentials" in content - # Neither client_id nor client_secret should be in body since client_id is empty - # (RFC 6749 §2.3.1 requires both for client_secret_post) - assert "client_id=" not in content - assert "client_secret=" not in content - assert "Authorization" not in request.headers - @pytest.mark.anyio async def test_exchange_token_without_scopes(self, mock_storage: MockTokenStorage): """Test token exchange without scopes.""" diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index 2f30ab4334..be96cc8eec 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -1031,11 +1031,13 @@ async def test_registration_response_with_substituted_metadata_yields_the_creden @pytest.mark.anyio -async def test_registration_response_does_not_seed_the_issuer_binding_from_the_body(): +@pytest.mark.parametrize("echoed_issuer", ["https://not-the-flow.example", 12345], ids=["string", "not-a-string"]) +async def test_registration_response_does_not_seed_the_issuer_binding_from_the_body(echoed_issuer: object): """The issuer binding (SEP-2352) is the SDK's record of which server it registered with, - stamped by the auth flow; an "issuer" member in the untrusted response body must not - populate it, or a mismatched binding would discard the credentials on every 401.""" - body = b'{"client_id": "issued-id", "issuer": "https://not-the-flow.example"}' + stamped by the auth flow; an "issuer" member in the untrusted response body is dropped + before parsing - never populating the binding, and never failing the parse either, so a + mismatched or malformed value cannot discard the credentials on every 401.""" + body = json.dumps({"client_id": "issued-id", "issuer": echoed_issuer}).encode() client_info = await handle_registration_response(httpx2.Response(201, content=body)) @@ -1044,9 +1046,16 @@ async def test_registration_response_does_not_seed_the_issuer_binding_from_the_b @pytest.mark.anyio -async def test_a_2xx_body_that_is_not_client_information_is_an_oauth_registration_error(): - """A success status whose body is not client information surfaces as OAuthRegistrationError.""" - response = httpx2.Response(201, content=b"not json") +@pytest.mark.parametrize( + "content", + [b"not json", b'["json", "but", "not", "an", "object"]', '{"client_id": "café"}'.encode("latin-1")], + ids=["not-json", "not-an-object", "not-utf8"], +) +async def test_a_2xx_body_that_is_not_client_information_is_an_oauth_registration_error(content: bytes): + """A success status whose body is not client information - unparseable, not an object, or + not valid UTF-8 - surfaces as OAuthRegistrationError rather than a raw parse failure, so a + single OAuthFlowError handler still covers registration.""" + response = httpx2.Response(201, content=content) with pytest.raises(OAuthRegistrationError): await handle_registration_response(response) @@ -1068,9 +1077,10 @@ async def test_token_exchange_reports_an_unimplemented_registered_auth_method(oa def test_prepare_token_auth_leaves_a_private_key_jwt_client_to_its_provider(oauth_provider: OAuthClientProvider): - """private_key_jwt is recognized (PrivateKeyJWTOAuthProvider supplies the assertion), so - the base leaves the request untouched rather than reporting an unsupported method - the - refresh path a private-key-JWT client inherits keeps working.""" + """private_key_jwt is recognized, so the base leaves the request untouched rather than + raising - PrivateKeyJWTOAuthProvider's inherited refresh path passes through here, and a + refresh the server then rejects (no assertion is signed on it) falls back to a fresh, + signed client-credentials exchange instead of aborting the flow.""" oauth_provider.context.client_info = OAuthClientInformationFull( client_id="registered-id", client_secret="registered-secret", diff --git a/tests/interaction/_requirements.py b/tests/interaction/_requirements.py index 624f234ba1..fbc7c13846 100644 --- a/tests/interaction/_requirements.py +++ b/tests/interaction/_requirements.py @@ -3676,9 +3676,10 @@ def __post_init__(self) -> None: behavior=( "A 201 registration response whose echoed metadata the server substituted (RFC 7591 §3.2.1) - " "an unregistered application_type, null redirect_uris, extra grant types - completes the flow; " - "a substituted token_endpoint_auth_method the authorization-code flow cannot apply (an " - "unimplemented method, or private_key_jwt, whose assertion it has no key to sign) is instead " - "reported as an OAuthRegistrationError before the record is persisted or authorization begins." + "substituted credentials the authorization-code flow cannot apply (an unimplemented " + "token_endpoint_auth_method; private_key_jwt, whose assertion it has no key to sign; or a " + "secret-based method with no client_secret issued) are instead reported as an " + "OAuthRegistrationError before the record is persisted or authorization begins." ), transports=("streamable-http",), note="OAuth is HTTP-only.", diff --git a/tests/interaction/auth/test_as_handlers.py b/tests/interaction/auth/test_as_handlers.py index 5535f06c33..1876cd7181 100644 --- a/tests/interaction/auth/test_as_handlers.py +++ b/tests/interaction/auth/test_as_handlers.py @@ -239,10 +239,21 @@ async def test_registration_with_invalid_metadata_is_rejected_with_400( bad_scope = await http.post("/register", json=body | {"scope": "forbidden"}) assert bad_scope.status_code == 400 - body = bad_scope.json() - assert body["error"] == "invalid_client_metadata" + bad_scope_body = bad_scope.json() + assert bad_scope_body["error"] == "invalid_client_metadata" # The description embeds a set difference whose ordering is not stable, so assert the prefix. - assert body["error_description"].startswith("Requested scopes are not valid: ") + assert bad_scope_body["error_description"].startswith("Requested scopes are not valid: ") + + # The server holds no client key to verify a private_key_jwt assertion, so it refuses to + # confirm a registration whose every token request it would then reject (RFC 7591 §3.2.2). + unsignable = await http.post("/register", json=body | {"token_endpoint_auth_method": "private_key_jwt"}) + assert unsignable.status_code == 400 + assert unsignable.json() == snapshot( + { + "error": "invalid_client_metadata", + "error_description": "token_endpoint_auth_method 'private_key_jwt' is not supported", + } + ) @requirement("hosting:auth:as:register-echo") diff --git a/tests/interaction/auth/test_discovery.py b/tests/interaction/auth/test_discovery.py index 9b195c1975..28685bf8cd 100644 --- a/tests/interaction/auth/test_discovery.py +++ b/tests/interaction/auth/test_discovery.py @@ -223,24 +223,31 @@ async def test_a_registration_response_with_substituted_metadata_completes_the_f @requirement("client-auth:dcr:substituted-metadata") -@pytest.mark.parametrize("auth_method", ["client_secret_jwt", "private_key_jwt"]) -async def test_a_registration_assigning_an_unusable_auth_method_surfaces_as_a_registration_error( - auth_method: str, +@pytest.mark.parametrize( + "credentials", + [ + pytest.param( + {"client_secret": "s3cr3t", "token_endpoint_auth_method": "client_secret_jwt"}, id="unimplemented" + ), + pytest.param({"client_secret": "s3cr3t", "token_endpoint_auth_method": "private_key_jwt"}, id="unsignable"), + pytest.param({"token_endpoint_auth_method": "client_secret_post"}, id="secret-not-issued"), + ], +) +async def test_a_registration_assigning_unusable_credentials_surfaces_as_a_registration_error( + credentials: dict[str, str], ) -> None: - """A 201 assigning an auth method this flow cannot apply is a registration error. + """A 201 assigning credentials this flow cannot apply is a registration error. RFC 7591 §3.2.1 leaves it to the client to judge whether a substituted value makes the registration usable. The authorization-code flow authenticates with the minted secret, so - neither an unimplemented method nor `private_key_jwt` (an assertion it has no key to - sign) is usable; both are reported before the record is stored or any authorize/token - request is made. + an unimplemented method, `private_key_jwt` (an assertion it has no key to sign), and a + secret-based method with no secret issued are all unusable; each is reported before + the record is stored or any authorize/token request is made. """ recorded, on_request = record_requests() provider = InMemoryAuthorizationServerProvider() server = Server("guarded", on_list_tools=list_tools) - body = json.dumps( - {"client_id": "unusable", "client_secret": "s3cr3t", "token_endpoint_auth_method": auth_method} - ).encode() + body = json.dumps({"client_id": "unusable", **credentials}).encode() app_shim = shim(serve={"/register": (201, body)}) storage = InMemoryTokenStorage() diff --git a/tests/shared/test_auth.py b/tests/shared/test_auth.py index c818db98f3..5286b93834 100644 --- a/tests/shared/test_auth.py +++ b/tests/shared/test_auth.py @@ -176,16 +176,25 @@ def test_a_registration_response_without_a_client_id_is_rejected(): OAuthClientInformationFull.model_validate({"application_type": "web"}) +@pytest.mark.parametrize("placeholder", [None, ""], ids=["null", "empty-string"]) @pytest.mark.parametrize( - "nulled", + "member", ["grant_types", "response_types", "redirect_uris", "application_type", "token_endpoint_auth_method", "scope"], ) -def test_client_information_reads_an_explicit_null_as_an_omitted_key(nulled: str): - """A server that dumps unset members as null (rather than omitting them) still yields a - usable registration: null and absent mean the same, so the field's default applies.""" - info = OAuthClientInformationFull.model_validate({"client_id": "abc123", nulled: None}) +def test_client_information_reads_a_placeholder_member_as_an_omitted_key(member: str, placeholder: object): + """A server that dumps unset members as null, or echoes them as "", still yields a + usable registration: a placeholder and an absent key mean the same, so the field's + default applies - including for list fields, where the placeholder is not a valid list.""" + info = OAuthClientInformationFull.model_validate({"client_id": "abc123", member: placeholder}) defaults = OAuthClientInformationFull.model_validate({"client_id": "abc123"}) - assert getattr(info, nulled) == getattr(defaults, nulled) + assert getattr(info, member) == getattr(defaults, member) + + +def test_a_placeholder_client_id_is_a_missing_client_id(): + """The placeholder rule applies to the credential too: an empty client_id is no client_id, + so the body is rejected rather than parsing as a registration with an empty identifier.""" + with pytest.raises(ValidationError): + OAuthClientInformationFull.model_validate({"client_id": ""}) def test_client_information_that_is_not_an_object_still_fails_the_parse(): @@ -195,14 +204,6 @@ def test_client_information_that_is_not_an_object_still_fails_the_parse(): OAuthClientInformationFull.model_validate("not-an-object") -@pytest.mark.parametrize("empty_field", ["token_endpoint_auth_method", "application_type"]) -def test_client_information_empty_string_metadata_coerced_to_absent(empty_field: str): - """An echoed "" reads as absent, matching the URL-field coercion, so it is neither - stored as a value nor later mistaken for an unrecognized method or type.""" - info = OAuthClientInformationFull.model_validate({"client_id": "abc123", empty_field: ""}) - assert getattr(info, empty_field) is None - - @pytest.mark.parametrize("redirect_uris", [None, []], ids=["absent", "empty"]) @pytest.mark.parametrize( "redirect_uri", [None, AnyUrl("https://example.com/callback")], ids=["unspecified", "specified"] From cc4e9118b3d6b5110c8ad572e3050b81a5cbfc24 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:26:38 +0000 Subject: [PATCH 4/4] Describe OAuthTokenError's client-side raise on the client OAuth page OAuthTokenError is no longer only the token endpoint's rejection: a stored client record whose token_endpoint_auth_method this client cannot apply raises it while the token request is being built, before the endpoint is contacted. Say so, parallel to the OAuthRegistrationError half of the same sentence. No-Verification-Needed: doc-only change --- docs/client/oauth-clients.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/client/oauth-clients.md b/docs/client/oauth-clients.md index 2cd6663e0b..cd7de35626 100644 --- a/docs/client/oauth-clients.md +++ b/docs/client/oauth-clients.md @@ -131,7 +131,7 @@ There is one more no-human situation: the client belongs to an enterprise whose ## When it fails -When the OAuth flow goes wrong, the provider raises an `OAuthFlowError` from `mcp.client.auth`. It has two subclasses. `OAuthRegistrationError` means registration did not yield a client you can use: the authorization server refused to register you, or it did register you but with credentials this flow cannot use (for instance an authentication method it does not implement). `OAuthTokenError` means the token endpoint said no. One `except OAuthFlowError:` covers discovery, registration, authorization, and exchange. +When the OAuth flow goes wrong, the provider raises an `OAuthFlowError` from `mcp.client.auth`. It has two subclasses. `OAuthRegistrationError` means registration did not yield a client you can use: the authorization server refused to register you, or it did register you but with credentials this flow cannot use (for instance an authentication method it does not implement). `OAuthTokenError` means a token could not be obtained: the token endpoint said no, or a stored client record carries an authentication method this client cannot apply, which is reported while building the token request rather than sent. One `except OAuthFlowError:` covers discovery, registration, authorization, and exchange. Not everything is a flow error. The network can still fail; those are ordinary `httpx2` exceptions and pass through untouched.