Skip to content

Commit 470bff3

Browse files
committed
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.
1 parent 64fc3f9 commit 470bff3

10 files changed

Lines changed: 109 additions & 35 deletions

File tree

docs/migration.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2196,7 +2196,7 @@ class OAuthClientInformationFull(OAuthClientMetadataBase): ... # server record
21962196

21972197
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`.
21982198

2199-
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.
2199+
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).
22002200

22012201
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"`).
22022202

src/mcp/client/auth/oauth2.py

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -59,28 +59,38 @@
5959

6060
logger = logging.getLogger(__name__)
6161

62-
# Methods `OAuthContext.prepare_token_auth` recognizes on a registered client, derived from the
63-
# same set the SDK is willing to request so the two cannot drift. `None`/"none" send no client
64-
# secret; `private_key_jwt` adds no secret here (its assertion is supplied by
65-
# `PrivateKeyJWTOAuthProvider`). Anything else is a method this client cannot apply.
66-
_RECOGNIZED_TOKEN_ENDPOINT_AUTH_METHODS: tuple[str | None, ...] = (None, *get_args(TokenEndpointAuthMethod))
62+
# Methods a registered client's record may carry without a token request being an error,
63+
# derived from the set the SDK is willing to request so the two cannot drift. `None`/"none"
64+
# send no client secret; `private_key_jwt` sends none from here either, its assertion being
65+
# added by `PrivateKeyJWTOAuthProvider` (whose inherited refresh path passes through
66+
# `prepare_token_auth`). Anything else is a method no client here can apply.
67+
_KNOWN_TOKEN_ENDPOINT_AUTH_METHODS: tuple[str | None, ...] = (None, *get_args(TokenEndpointAuthMethod))
68+
69+
# Methods a registration completed by the authorization-code flow can act on. That flow
70+
# authenticates the token request with the minted client secret (or nothing); it holds no key
71+
# to sign a `private_key_jwt` assertion, so a server assigning that method has registered a
72+
# client this flow cannot use. `PrivateKeyJWTOAuthProvider` never registers dynamically.
73+
_REGISTRATION_USABLE_TOKEN_ENDPOINT_AUTH_METHODS: tuple[str | None, ...] = tuple(
74+
method for method in _KNOWN_TOKEN_ENDPOINT_AUTH_METHODS if method != "private_key_jwt"
75+
)
6776

6877

6978
def check_registration_usable(client_info: OAuthClientInformationFull) -> None:
70-
"""Confirm a completed registration is one this client can act on.
79+
"""Confirm a registration this flow completed is one it can act on.
7180
7281
RFC 7591 §3.2.1 lets the authorization server replace requested metadata and leaves it to
7382
the client to "check the values in the response to determine if the registration is
7483
sufficient for use". The one substitution that makes the minted credentials unusable is a
75-
token-endpoint auth method this client cannot apply, so it is judged here - before the
76-
record is persisted or any interactive authorization begins - rather than surfacing later
77-
as an opaque failure at the token endpoint.
84+
token-endpoint auth method the authorization-code flow cannot apply - one it does not
85+
implement, or `private_key_jwt`, whose assertion this flow has no key to sign - so it is
86+
judged here, before the record is persisted or any interactive authorization begins,
87+
rather than surfacing later as an opaque failure at the token endpoint.
7888
7989
Raises:
8090
OAuthRegistrationError: The server registered the client with a
81-
`token_endpoint_auth_method` this client does not implement.
91+
`token_endpoint_auth_method` this flow cannot apply.
8292
"""
83-
if client_info.token_endpoint_auth_method not in _RECOGNIZED_TOKEN_ENDPOINT_AUTH_METHODS:
93+
if client_info.token_endpoint_auth_method not in _REGISTRATION_USABLE_TOKEN_ENDPOINT_AUTH_METHODS:
8494
raise OAuthRegistrationError(
8595
"Authorization server registered the client with unsupported token_endpoint_auth_method "
8696
f"{client_info.token_endpoint_auth_method!r}"
@@ -246,7 +256,7 @@ def prepare_token_auth(
246256
# Include client_id and client_secret in request body (RFC 6749 §2.3.1)
247257
data["client_id"] = self.client_info.client_id
248258
data["client_secret"] = self.client_info.client_secret
249-
elif auth_method not in _RECOGNIZED_TOKEN_ENDPOINT_AUTH_METHODS:
259+
elif auth_method not in _KNOWN_TOKEN_ENDPOINT_AUTH_METHODS:
250260
raise OAuthTokenError(f"Registered client uses unsupported token_endpoint_auth_method {auth_method!r}")
251261
# For "none" (or absent), don't add any client_secret; "private_key_jwt" adds its
252262
# assertion in the provider that implements it, not here.

src/mcp/client/auth/utils.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,9 +300,13 @@ async def handle_registration_response(response: Response) -> OAuthClientInforma
300300
try:
301301
content = await response.aread()
302302
client_info = OAuthClientInformationFull.model_validate_json(content)
303-
return client_info
304303
except ValidationError as e:
305304
raise OAuthRegistrationError(f"Invalid registration response: {e}") from e
305+
# `issuer` is the SDK's own binding of these credentials to the server they were
306+
# registered with (SEP-2352), stamped by the auth flow - never taken from the wire.
307+
# A body echoing an "issuer" member must not seed the trust binding.
308+
client_info.issuer = None
309+
return client_info
306310

307311

308312
def is_valid_client_metadata_url(url: str | None) -> bool:

src/mcp/server/auth/handlers/register.py

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -106,11 +106,15 @@ async def handle(self, request: Request) -> Response:
106106
)
107107

108108
client_id_issued_at = int(time.time())
109-
client_secret_expires_at = (
110-
client_id_issued_at + self.options.client_secret_expiry_seconds
111-
if self.options.client_secret_expiry_seconds is not None
112-
else None
113-
)
109+
# RFC 7591 §3.2.1: client_secret_expires_at is REQUIRED whenever a client_secret is
110+
# issued, with 0 (not omission) meaning it never expires; a public client gets none.
111+
client_secret_expires_at = None
112+
if client_secret is not None:
113+
client_secret_expires_at = (
114+
client_id_issued_at + self.options.client_secret_expiry_seconds
115+
if self.options.client_secret_expiry_seconds is not None
116+
else 0
117+
)
114118

115119
# RFC 7591 §3.2.1: the response returns all registered metadata about the client, so
116120
# the record is the whole validated request plus the credentials minted here - built

src/mcp/shared/auth.py

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
from typing import Any, Literal
1+
from typing import Any, Literal, cast
22

3-
from pydantic import AnyHttpUrl, AnyUrl, BaseModel, ConfigDict, Field, field_validator
3+
from pydantic import AnyHttpUrl, AnyUrl, BaseModel, ConfigDict, Field, field_validator, model_validator
44

55
# RFC 7523 JWT bearer grant; SEP-990 leg 2 uses this to present the ID-JAG.
66
JWT_BEARER_GRANT_TYPE = "urn:ietf:params:oauth:grant-type:jwt-bearer"
@@ -136,9 +136,11 @@ class OAuthClientInformationFull(OAuthClientMetadataBase):
136136
requested metadata values submitted during the registration and substitute them with
137137
suitable values", so `application_type`, `token_endpoint_auth_method`, and `grant_types`
138138
are typed to accept any string the server echoes, and `redirect_uris` may be absent or
139-
empty. Whether a substituted value is usable is decided where the value is used, not at
140-
parse. `redirect_uris` elements are still parsed as URLs, as the authorization server
141-
compares them against a client's requested `redirect_uri`.
139+
empty. A member the server serializes as an explicit `null` is read as omitted, so the
140+
field's default applies rather than the parse failing. Whether a substituted value is
141+
usable is decided where the value is used, not at parse. `redirect_uris` elements are
142+
still parsed as URLs, as the authorization server compares them against a client's
143+
requested `redirect_uri`.
142144
"""
143145

144146
redirect_uris: list[AnyUrl] | None = None
@@ -160,6 +162,17 @@ class OAuthClientInformationFull(OAuthClientMetadataBase):
160162
# RFC 7591 field) to detect authorization-server migration and avoid cross-AS credential reuse.
161163
issuer: str | None = None
162164

165+
@model_validator(mode="before")
166+
@classmethod
167+
def _explicit_null_reads_as_omitted(cls, data: object) -> object:
168+
# Servers that serialize their client record dump unset members as null instead of
169+
# omitting the keys; a null for a list field would otherwise fail the parse (and
170+
# discard an already-provisioned registration). null and absent mean the same thing.
171+
if isinstance(data, dict):
172+
members = cast(dict[str, Any], data)
173+
return {key: value for key, value in members.items() if value is not None}
174+
return data
175+
163176
@field_validator("token_endpoint_auth_method", "application_type", mode="before")
164177
@classmethod
165178
def _empty_string_optional_metadata_to_none(cls, v: object) -> object:

tests/client/test_auth.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1030,6 +1030,19 @@ async def test_registration_response_with_substituted_metadata_yields_the_creden
10301030
assert client_info.application_type == "confidential"
10311031

10321032

1033+
@pytest.mark.anyio
1034+
async def test_registration_response_does_not_seed_the_issuer_binding_from_the_body():
1035+
"""The issuer binding (SEP-2352) is the SDK's record of which server it registered with,
1036+
stamped by the auth flow; an "issuer" member in the untrusted response body must not
1037+
populate it, or a mismatched binding would discard the credentials on every 401."""
1038+
body = b'{"client_id": "issued-id", "issuer": "https://not-the-flow.example"}'
1039+
1040+
client_info = await handle_registration_response(httpx2.Response(201, content=body))
1041+
1042+
assert client_info.client_id == "issued-id"
1043+
assert client_info.issuer is None
1044+
1045+
10331046
@pytest.mark.anyio
10341047
async def test_a_2xx_body_that_is_not_client_information_is_an_oauth_registration_error():
10351048
"""A success status whose body is not client information surfaces as OAuthRegistrationError."""

tests/interaction/_requirements.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2944,8 +2944,9 @@ def __post_init__(self) -> None:
29442944
source="sdk",
29452945
behavior=(
29462946
"The bundled registration endpoint returns all registered metadata about the client "
2947-
"in its 201 response (RFC 7591 §3.2.1), including the client's `application_type` "
2948-
"rather than a substituted default."
2947+
"in its 201 response (RFC 7591 §3.2.1) - the client's `application_type` rather than a "
2948+
"substituted default, and `client_secret_expires_at` (0 when the secret never expires) "
2949+
"whenever a `client_secret` is issued."
29492950
),
29502951
transports=("streamable-http",),
29512952
note="Auth is enforced at the HTTP layer; the bundled AS is an ASGI app.",
@@ -3675,8 +3676,9 @@ def __post_init__(self) -> None:
36753676
behavior=(
36763677
"A 201 registration response whose echoed metadata the server substituted (RFC 7591 §3.2.1) - "
36773678
"an unregistered application_type, null redirect_uris, extra grant types - completes the flow; "
3678-
"a substituted token_endpoint_auth_method the client cannot apply is instead reported as an "
3679-
"OAuthRegistrationError before the record is persisted or authorization begins."
3679+
"a substituted token_endpoint_auth_method the authorization-code flow cannot apply (an "
3680+
"unimplemented method, or private_key_jwt, whose assertion it has no key to sign) is instead "
3681+
"reported as an OAuthRegistrationError before the record is persisted or authorization begins."
36803682
),
36813683
transports=("streamable-http",),
36823684
note="OAuth is HTTP-only.",

tests/interaction/auth/test_as_handlers.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,8 +258,12 @@ async def test_registration_response_echoes_the_registered_application_type(
258258
response = await http.post("/register", json=body | {"application_type": application_type})
259259

260260
assert response.status_code == 201
261-
info = OAuthClientInformationFull.model_validate_json(response.content)
262-
assert info.application_type == application_type
261+
echoed = response.json()
262+
assert echoed["application_type"] == application_type
263+
# A secret was issued and no expiry is configured, so RFC 7591 §3.2.1 requires the
264+
# response to carry client_secret_expires_at, with 0 (present, not omitted) for "never".
265+
assert echoed["client_secret"]
266+
assert echoed["client_secret_expires_at"] == 0
263267

264268

265269
@requirement("hosting:auth:as:redirect-uri-binding")

tests/interaction/auth/test_discovery.py

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -223,18 +223,23 @@ async def test_a_registration_response_with_substituted_metadata_completes_the_f
223223

224224

225225
@requirement("client-auth:dcr:substituted-metadata")
226-
async def test_a_registration_assigning_an_unusable_auth_method_surfaces_as_a_registration_error() -> None:
227-
"""A 201 assigning an auth method the client cannot apply is a registration error.
226+
@pytest.mark.parametrize("auth_method", ["client_secret_jwt", "private_key_jwt"])
227+
async def test_a_registration_assigning_an_unusable_auth_method_surfaces_as_a_registration_error(
228+
auth_method: str,
229+
) -> None:
230+
"""A 201 assigning an auth method this flow cannot apply is a registration error.
228231
229232
RFC 7591 §3.2.1 leaves it to the client to judge whether a substituted value makes the
230-
registration usable; an unimplemented token-endpoint auth method does not, and is
231-
reported before the record is stored or any authorize/token request is made.
233+
registration usable. The authorization-code flow authenticates with the minted secret, so
234+
neither an unimplemented method nor `private_key_jwt` (an assertion it has no key to
235+
sign) is usable; both are reported before the record is stored or any authorize/token
236+
request is made.
232237
"""
233238
recorded, on_request = record_requests()
234239
provider = InMemoryAuthorizationServerProvider()
235240
server = Server("guarded", on_list_tools=list_tools)
236241
body = json.dumps(
237-
{"client_id": "jwt-only", "client_secret": "s3cr3t", "token_endpoint_auth_method": "client_secret_jwt"}
242+
{"client_id": "unusable", "client_secret": "s3cr3t", "token_endpoint_auth_method": auth_method}
238243
).encode()
239244
app_shim = shim(serve={"/register": (201, body)})
240245
storage = InMemoryTokenStorage()

tests/shared/test_auth.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,25 @@ def test_a_registration_response_without_a_client_id_is_rejected():
176176
OAuthClientInformationFull.model_validate({"application_type": "web"})
177177

178178

179+
@pytest.mark.parametrize(
180+
"nulled",
181+
["grant_types", "response_types", "redirect_uris", "application_type", "token_endpoint_auth_method", "scope"],
182+
)
183+
def test_client_information_reads_an_explicit_null_as_an_omitted_key(nulled: str):
184+
"""A server that dumps unset members as null (rather than omitting them) still yields a
185+
usable registration: null and absent mean the same, so the field's default applies."""
186+
info = OAuthClientInformationFull.model_validate({"client_id": "abc123", nulled: None})
187+
defaults = OAuthClientInformationFull.model_validate({"client_id": "abc123"})
188+
assert getattr(info, nulled) == getattr(defaults, nulled)
189+
190+
191+
def test_client_information_that_is_not_an_object_still_fails_the_parse():
192+
"""The null-as-omitted coercion only touches JSON objects; a body that is not one is
193+
passed through and rejected as a normal validation failure rather than swallowed."""
194+
with pytest.raises(ValidationError):
195+
OAuthClientInformationFull.model_validate("not-an-object")
196+
197+
179198
@pytest.mark.parametrize("empty_field", ["token_endpoint_auth_method", "application_type"])
180199
def test_client_information_empty_string_metadata_coerced_to_absent(empty_field: str):
181200
"""An echoed "" reads as absent, matching the URL-field coercion, so it is neither

0 commit comments

Comments
 (0)