Skip to content

Commit 15f11a5

Browse files
committed
Drop iss slash-stripping workaround now that empty paths are preserved
With url_preserve_empty_path on the OAuth metadata models (#2925), the issuer parsed from the wire keeps its empty path, so str(oauth_metadata.issuer) is already the byte-exact value the authorization server transmitted. Remove _strip_authority_trailing_slash / raw_issuer and compare directly. This also fixes the false rejection the heuristic introduced for an issuer that genuinely ends in a trailing slash (e.g. Auth0's https://tenant.auth0.com/): its redirect iss now matches its advertised issuer instead of being stripped.
1 parent 109c391 commit 15f11a5

4 files changed

Lines changed: 29 additions & 55 deletions

File tree

src/mcp/client/auth/utils.py

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -211,23 +211,6 @@ async def handle_auth_metadata_response(response: Response) -> tuple[bool, OAuth
211211
return True, None
212212

213213

214-
def _strip_authority_trailing_slash(url: str) -> str:
215-
"""Drop the lone trailing slash `AnyHttpUrl` appends to a path-less authority.
216-
217-
RFC 9207 / SEP-2468 mandate simple string comparison (RFC 3986 section 6.2.1), so issuers
218-
must be compared as sent. `AnyHttpUrl` rewrites `https://as` to `https://as/`, which would
219-
defeat the comparison; undo only that, leaving issuers with a real path untouched.
220-
"""
221-
if url.endswith("/") and urlparse(url).path == "/":
222-
return url[:-1]
223-
return url
224-
225-
226-
def raw_issuer(oauth_metadata: OAuthMetadata) -> str:
227-
"""Return the issuer as the authorization server transmitted it, before URL normalization."""
228-
return _strip_authority_trailing_slash(str(oauth_metadata.issuer))
229-
230-
231214
def validate_authorization_response_iss(iss: str | None, oauth_metadata: OAuthMetadata | None) -> None:
232215
"""Validate the RFC 9207 `iss` authorization-response parameter.
233216
@@ -241,7 +224,7 @@ def validate_authorization_response_iss(iss: str | None, oauth_metadata: OAuthMe
241224
OAuthFlowError: If `iss` is present and does not match, or is absent when the
242225
authorization server advertised support.
243226
"""
244-
expected = raw_issuer(oauth_metadata) if oauth_metadata else None
227+
expected = str(oauth_metadata.issuer) if oauth_metadata else None
245228

246229
if iss is not None:
247230
if iss != expected:
@@ -261,10 +244,9 @@ def validate_metadata_issuer(oauth_metadata: OAuthMetadata, expected_issuer: str
261244
Raises:
262245
OAuthFlowError: If the metadata issuer does not match `expected_issuer`.
263246
"""
264-
expected = _strip_authority_trailing_slash(expected_issuer)
265-
if raw_issuer(oauth_metadata) != expected:
247+
if str(oauth_metadata.issuer) != expected_issuer:
266248
raise OAuthFlowError(
267-
f"Authorization server metadata issuer mismatch: {raw_issuer(oauth_metadata)} != {expected}"
249+
f"Authorization server metadata issuer mismatch: {oauth_metadata.issuer} != {expected_issuer}"
268250
)
269251

270252

tests/client/test_auth.py

Lines changed: 20 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
get_client_metadata_scopes,
2525
handle_registration_response,
2626
is_valid_client_metadata_url,
27-
raw_issuer,
2827
should_use_client_metadata_url,
2928
validate_authorization_response_iss,
3029
validate_metadata_issuer,
@@ -2642,31 +2641,35 @@ async def callback_handler() -> AuthorizationCodeResult:
26422641
pass
26432642

26442643

2645-
# A path-bearing issuer so a trailing slash is a genuine difference under simple string comparison
2646-
# (AnyHttpUrl normalizes a bare host to a trailing slash, which would hide the distinction).
2647-
_ISSUER = "https://as.example.com/tenant"
2644+
_ISSUER = "https://as.example.com"
26482645

26492646

26502647
def _issuer_metadata(*, issuer: str = _ISSUER, iss_supported: bool | None = None) -> OAuthMetadata:
2651-
return OAuthMetadata(
2652-
issuer=AnyHttpUrl(issuer),
2653-
authorization_endpoint=AnyHttpUrl(f"{issuer}/authorize"),
2654-
token_endpoint=AnyHttpUrl(f"{issuer}/token"),
2655-
authorization_response_iss_parameter_supported=iss_supported,
2648+
# Validate from string inputs so url_preserve_empty_path keeps the issuer as transmitted,
2649+
# matching the wire path (model_validate_json) rather than normalizing a bare authority.
2650+
return OAuthMetadata.model_validate(
2651+
{
2652+
"issuer": issuer,
2653+
"authorization_endpoint": f"{issuer}/authorize",
2654+
"token_endpoint": f"{issuer}/token",
2655+
"authorization_response_iss_parameter_supported": iss_supported,
2656+
}
26562657
)
26572658

26582659

26592660
@pytest.mark.parametrize(
2660-
("iss", "iss_supported"),
2661+
("issuer", "iss", "iss_supported"),
26612662
[
2662-
pytest.param(_ISSUER, True, id="advertised-and-correct"),
2663-
pytest.param(None, None, id="not-advertised-and-omitted"),
2664-
pytest.param(_ISSUER, None, id="not-advertised-but-correct"),
2663+
pytest.param(_ISSUER, _ISSUER, True, id="advertised-and-correct"),
2664+
pytest.param(_ISSUER, None, None, id="not-advertised-and-omitted"),
2665+
pytest.param(_ISSUER, _ISSUER, None, id="not-advertised-but-correct"),
2666+
# An issuer that genuinely ends in a slash (e.g. Auth0) must match its own iss.
2667+
pytest.param("https://as.example.com/", "https://as.example.com/", True, id="trailing-slash-issuer"),
26652668
],
26662669
)
2667-
def test_validate_authorization_response_iss_accepts(iss: str | None, iss_supported: bool | None):
2670+
def test_validate_authorization_response_iss_accepts(issuer: str, iss: str | None, iss_supported: bool | None):
26682671
"""RFC 9207: a matching or legitimately absent iss is accepted."""
2669-
validate_authorization_response_iss(iss, _issuer_metadata(iss_supported=iss_supported))
2672+
validate_authorization_response_iss(iss, _issuer_metadata(issuer=issuer, iss_supported=iss_supported))
26702673

26712674

26722675
@pytest.mark.parametrize(
@@ -2692,20 +2695,9 @@ def test_validate_authorization_response_iss_without_metadata():
26922695

26932696

26942697
def test_validate_metadata_issuer_accepts_match():
2695-
validate_metadata_issuer(_issuer_metadata(issuer=_ISSUER), str(AnyHttpUrl(_ISSUER)))
2698+
validate_metadata_issuer(_issuer_metadata(issuer=_ISSUER), _ISSUER)
26962699

26972700

26982701
def test_validate_metadata_issuer_rejects_mismatch():
26992702
with pytest.raises(OAuthFlowError, match="metadata issuer mismatch"):
2700-
validate_metadata_issuer(_issuer_metadata(issuer="https://attacker.example.com"), str(AnyHttpUrl(_ISSUER)))
2701-
2702-
2703-
def test_raw_issuer_strips_only_authority_trailing_slash():
2704-
"""The slash AnyHttpUrl adds to a bare authority is dropped; a real path keeps its slash."""
2705-
assert raw_issuer(_issuer_metadata(issuer="https://as.example.com")) == "https://as.example.com"
2706-
assert raw_issuer(_issuer_metadata(issuer="https://as.example.com/tenant")) == "https://as.example.com/tenant"
2707-
2708-
2709-
def test_validate_metadata_issuer_ignores_authority_trailing_slash():
2710-
"""A bare-authority issuer matches whether or not the discovery URL carries the slash."""
2711-
validate_metadata_issuer(_issuer_metadata(issuer="https://as.example.com"), "https://as.example.com")
2703+
validate_metadata_issuer(_issuer_metadata(issuer="https://attacker.example.com"), _ISSUER)

tests/interaction/auth/_provider.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,11 @@ def __init__(
5757
issuer: str | None = None,
5858
) -> None:
5959
self._default_scopes = list(default_scopes) if default_scopes is not None else ["mcp"]
60-
# The authorization-response iss must match the AS issuer the client recorded (RFC 9207
61-
# simple string comparison); the client strips the trailing slash AnyHttpUrl adds to a
62-
# path-less issuer, so the redirect carries the bare origin. Path-issuer tests pass it.
63-
self._issuer = issuer if issuer is not None else BASE_URL
60+
# The authorization-response iss must equal the AS metadata issuer the client recorded
61+
# (RFC 9207 simple string comparison). `real_asm` builds the issuer from an AnyHttpUrl
62+
# object, so it carries the trailing slash; the redirect iss matches it. Path-issuer
63+
# tests pass the recorded issuer explicitly.
64+
self._issuer = issuer if issuer is not None else f"{BASE_URL}/"
6465
self._deny_authorize = deny_authorize
6566
self._issue_expired_first = issue_expired_first
6667
self._fail_next_refresh = fail_next_refresh

tests/interaction/auth/test_discovery.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919

2020
from mcp import types
2121
from mcp.client.auth import OAuthFlowError, OAuthRegistrationError
22-
from mcp.client.auth.utils import raw_issuer
2322
from mcp.server import Server, ServerRequestContext
2423
from mcp.shared.auth import OAuthMetadata, ProtectedResourceMetadata
2524
from mcp.types import ListToolsResult, Tool
@@ -240,7 +239,7 @@ async def test_as_metadata_discovery_falls_back_through_the_spec_endpoint_order(
240239
asm = real_asm()
241240
asm.issuer = AnyHttpUrl(authorization_server)
242241
# The redirect iss must equal the issuer the client records from this metadata.
243-
provider = InMemoryAuthorizationServerProvider(issuer=raw_issuer(asm))
242+
provider = InMemoryAuthorizationServerProvider(issuer=str(asm.issuer))
244243
server = Server("guarded", on_list_tools=list_tools)
245244

246245
prm = ProtectedResourceMetadata(

0 commit comments

Comments
 (0)