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.

Author: maxisbey

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.
 

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`
 

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

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:

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

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

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