Skip to content

Commit 523b32b

Browse files
KludexKludex
committed
Harden SEP-2352 issuer binding (Codex review)
- Stamp the bound issuer from the discovered oauth_metadata.issuer when PRM did not advertise an authorization server (legacy no-PRM path), instead of leaving it None — otherwise migrated resources could reuse the old DCR client_id. - Detect CIMD portability by the client_id equaling the configured client_metadata_url, not by URL shape, so a registration server that issues a URL-shaped client_id is still treated as bound to its issuer.
1 parent b8fc48d commit 523b32b

3 files changed

Lines changed: 40 additions & 18 deletions

File tree

‎src/mcp/client/auth/oauth2.py‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,9 @@ async def async_auth_flow(self, request: httpx.Request) -> AsyncGenerator[httpx.
571571
if (
572572
self.context.client_info is not None
573573
and self.context.auth_server_url is not None
574-
and not credentials_match_issuer(self.context.client_info, self.context.auth_server_url)
574+
and not credentials_match_issuer(
575+
self.context.client_info, self.context.auth_server_url, self.context.client_metadata_url
576+
)
575577
):
576578
logger.debug("Authorization server changed; discarding bound credentials and re-registering")
577579
self.context.client_info = None
@@ -608,6 +610,13 @@ async def async_auth_flow(self, request: httpx.Request) -> AsyncGenerator[httpx.
608610

609611
# Step 4: Register client or use URL-based client ID (CIMD)
610612
if not self.context.client_info:
613+
# SEP-2352: bind the credentials to the issuing AS. Prefer the PRM-advertised
614+
# authorization server; on the legacy no-PRM path fall back to the issuer from
615+
# the discovered metadata so the binding is still recorded.
616+
bound_issuer = self.context.auth_server_url
617+
if bound_issuer is None and self.context.oauth_metadata is not None:
618+
bound_issuer = str(self.context.oauth_metadata.issuer)
619+
611620
if should_use_client_metadata_url(
612621
self.context.oauth_metadata, self.context.client_metadata_url
613622
):
@@ -617,8 +626,7 @@ async def async_auth_flow(self, request: httpx.Request) -> AsyncGenerator[httpx.
617626
self.context.client_metadata_url, # type: ignore[arg-type]
618627
redirect_uris=self.context.client_metadata.redirect_uris,
619628
)
620-
# SEP-2352: bind the credentials to the issuing authorization server
621-
client_information.issuer = self.context.auth_server_url
629+
client_information.issuer = bound_issuer
622630
self.context.client_info = client_information
623631
await self.context.storage.set_client_info(client_information)
624632
else:
@@ -630,8 +638,7 @@ async def async_auth_flow(self, request: httpx.Request) -> AsyncGenerator[httpx.
630638
)
631639
registration_response = yield registration_request
632640
client_information = await handle_registration_response(registration_response)
633-
# SEP-2352: bind the credentials to the issuing authorization server
634-
client_information.issuer = self.context.auth_server_url
641+
client_information.issuer = bound_issuer
635642
self.context.client_info = client_information
636643
await self.context.storage.set_client_info(client_information)
637644

‎src/mcp/client/auth/utils.py‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -325,16 +325,20 @@ def is_valid_client_metadata_url(url: str | None) -> bool:
325325
return False
326326

327327

328-
def credentials_match_issuer(client_info: OAuthClientInformationFull, issuer: str) -> bool:
328+
def credentials_match_issuer(
329+
client_info: OAuthClientInformationFull, issuer: str, client_metadata_url: str | None
330+
) -> bool:
329331
"""Whether stored client credentials may be reused against `issuer` (SEP-2352).
330332
331-
URL-based client IDs (CIMD) are portable across authorization servers — the same self-hosted
332-
document is resolved by whichever server is in use — so they always match. Credentials with a
333-
recorded issuer match only when it equals `issuer` (simple string comparison). Credentials
334-
with no recorded issuer (pre-registered, or stored before issuer binding existed) carry no
335-
binding to enforce and are left as-is.
333+
A URL-based client ID (CIMD) is portable across authorization servers — the same self-hosted
334+
document is resolved by whichever server is in use — so it always matches; CIMD is identified
335+
by the client ID being the configured `client_metadata_url`, not by URL shape (a registration
336+
server may also issue URL-shaped IDs that are bound to it). Credentials with a recorded issuer
337+
match only when it equals `issuer` (simple string comparison). Credentials with no recorded
338+
issuer (pre-registered, or stored before issuer binding existed) carry no binding to enforce
339+
and are left as-is.
336340
"""
337-
if is_valid_client_metadata_url(client_info.client_id):
341+
if client_metadata_url is not None and client_info.client_id == client_metadata_url:
338342
return True
339343
if client_info.issuer is None:
340344
return True

‎tests/client/test_auth.py‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2798,26 +2798,37 @@ def test_union_scopes(previous: str | None, new: str | None, expected: str | Non
27982798

27992799
def test_credentials_match_issuer_same_issuer():
28002800
info = OAuthClientInformationFull(client_id="c", redirect_uris=[AnyUrl("http://localhost/cb")], issuer="https://as")
2801-
assert credentials_match_issuer(info, "https://as") is True
2801+
assert credentials_match_issuer(info, "https://as", None) is True
28022802

28032803

28042804
def test_credentials_match_issuer_different_issuer():
28052805
info = OAuthClientInformationFull(client_id="c", redirect_uris=[AnyUrl("http://localhost/cb")], issuer="https://as")
2806-
assert credentials_match_issuer(info, "https://other") is False
2806+
assert credentials_match_issuer(info, "https://other", None) is False
28072807

28082808

28092809
def test_credentials_match_issuer_no_recorded_issuer_is_left_alone():
28102810
"""Credentials with no bound issuer (pre-registered / legacy) carry no binding to enforce."""
28112811
info = OAuthClientInformationFull(client_id="c", redirect_uris=[AnyUrl("http://localhost/cb")])
2812-
assert credentials_match_issuer(info, "https://as") is True
2812+
assert credentials_match_issuer(info, "https://as", None) is True
28132813

28142814

28152815
def test_credentials_match_issuer_cimd_is_portable():
2816-
"""A URL-based client_id (CIMD) is portable across authorization servers."""
2816+
"""A client_id equal to the configured client_metadata_url (CIMD) is portable across servers."""
2817+
cimd_url = "https://client.example/metadata.json"
28172818
info = OAuthClientInformationFull(
2818-
client_id="https://client.example/metadata.json",
2819+
client_id=cimd_url,
28192820
redirect_uris=[AnyUrl("http://localhost/cb")],
28202821
token_endpoint_auth_method="none",
28212822
issuer="https://as",
28222823
)
2823-
assert credentials_match_issuer(info, "https://other") is True
2824+
assert credentials_match_issuer(info, "https://other", cimd_url) is True
2825+
2826+
2827+
def test_credentials_match_issuer_url_shaped_dcr_id_is_not_portable():
2828+
"""A URL-shaped client_id from DCR (not the configured CIMD URL) stays bound to its issuer."""
2829+
info = OAuthClientInformationFull(
2830+
client_id="https://as.example.com/clients/123",
2831+
redirect_uris=[AnyUrl("http://localhost/cb")],
2832+
issuer="https://as.example.com",
2833+
)
2834+
assert credentials_match_issuer(info, "https://other", "https://client.example/metadata.json") is False

0 commit comments

Comments
 (0)