From 9373eab0c5bafe0ef730dad597c6c9cb5a8363ae Mon Sep 17 00:00:00 2001 From: Marcelo Trylesinski Date: Sat, 20 Jun 2026 16:56:40 +0200 Subject: [PATCH 1/3] Validate the iss authorization-response parameter (RFC 9207 / SEP-2468) The OAuth client now validates the RFC 9207 iss parameter returned on the authorization-response redirect and the issuer of discovered authorization server metadata. iss is compared against the authorization server issuer with simple string comparison (RFC 3986 6.2.1): a mismatch is rejected, and a missing iss is rejected when the server advertised authorization_response_iss_parameter_supported. The callback_handler passed to OAuthClientProvider now returns an AuthorizationCodeResult (code/state/iss) instead of a tuple[str, str | None] so the redirect's iss reaches the validation. AnyHttpUrl appends a trailing slash to a bare authority, which would defeat the byte-exact comparison the spec requires, so the recorded issuer is compared with that lone slash stripped. Flips the auth/metadata-issuer-mismatch conformance scenario green; the auth/iss-* scenarios validate iss correctly but remain baselined because they reach DCR and trip the unimplemented SEP-837 application_type check. --- .github/actions/conformance/client.py | 15 +- .../expected-failures.2026-07-28.yml | 17 +-- .../actions/conformance/expected-failures.yml | 17 +-- README.v2.md | 10 +- docs/migration.md | 29 ++++ .../mcp_simple_auth_client/main.py | 17 ++- examples/snippets/clients/oauth_client.py | 10 +- src/mcp/client/auth/__init__.py | 2 + .../auth/extensions/client_credentials.py | 4 +- src/mcp/client/auth/oauth2.py | 23 +++- src/mcp/client/auth/utils.py | 59 +++++++- src/mcp/shared/auth.py | 13 ++ .../extensions/test_client_credentials.py | 12 +- tests/client/test_auth.py | 129 ++++++++++++++---- tests/client/test_scope_bug_1630.py | 11 +- tests/interaction/_requirements.py | 18 ++- tests/interaction/auth/_harness.py | 18 ++- tests/interaction/auth/_provider.py | 7 +- .../interaction/auth/test_authorize_token.py | 18 +++ tests/interaction/auth/test_discovery.py | 31 +++-- 20 files changed, 359 insertions(+), 101 deletions(-) diff --git a/.github/actions/conformance/client.py b/.github/actions/conformance/client.py index a438cb29ee..dc30951142 100644 --- a/.github/actions/conformance/client.py +++ b/.github/actions/conformance/client.py @@ -43,7 +43,7 @@ ) from mcp.client.context import ClientRequestContext from mcp.client.streamable_http import streamable_http_client -from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata, OAuthToken +from mcp.shared.auth import AuthorizationCodeResult, OAuthClientInformationFull, OAuthClientMetadata, OAuthToken # Set up logging to stderr (stdout is for conformance test output) logging.basicConfig( @@ -119,6 +119,7 @@ class ConformanceOAuthCallbackHandler: def __init__(self) -> None: self._auth_code: str | None = None self._state: str | None = None + self._iss: str | None = None async def handle_redirect(self, authorization_url: str) -> None: """Fetch the authorization URL and extract the auth code from the redirect.""" @@ -140,6 +141,8 @@ async def handle_redirect(self, authorization_url: str) -> None: self._auth_code = query_params["code"][0] state_values = query_params.get("state") self._state = state_values[0] if state_values else None + iss_values = query_params.get("iss") + self._iss = iss_values[0] if iss_values else None logger.debug(f"Got auth code from redirect: {self._auth_code[:10]}...") return else: @@ -149,15 +152,15 @@ async def handle_redirect(self, authorization_url: str) -> None: else: raise RuntimeError(f"Expected redirect response, got {response.status_code} from {authorization_url}") - async def handle_callback(self) -> tuple[str, str | None]: - """Return the captured auth code and state.""" + async def handle_callback(self) -> AuthorizationCodeResult: + """Return the captured auth code, state, and iss.""" if self._auth_code is None: raise RuntimeError("No authorization code available - was handle_redirect called?") - auth_code = self._auth_code - state = self._state + result = AuthorizationCodeResult(code=self._auth_code, state=self._state, iss=self._iss) self._auth_code = None self._state = None - return auth_code, state + self._iss = None + return result # --- Scenario Handlers --- diff --git a/.github/actions/conformance/expected-failures.2026-07-28.yml b/.github/actions/conformance/expected-failures.2026-07-28.yml index 1d010abc0d..3bbec3fe65 100644 --- a/.github/actions/conformance/expected-failures.2026-07-28.yml +++ b/.github/actions/conformance/expected-failures.2026-07-28.yml @@ -44,6 +44,15 @@ client: - auth/token-endpoint-auth-post - auth/token-endpoint-auth-none - auth/offline-access-not-supported + # SEP-2468 (authorization response iss parameter) is implemented, but these + # 2026-introduced scenarios reach DCR and so still fail the application_type + # check above; they unblock with SEP-837, not SEP-2468. + - auth/iss-supported + - auth/iss-not-advertised + - auth/iss-supported-missing + - auth/iss-wrong-issuer + - auth/iss-unexpected + - auth/iss-normalized # --- Auth scenarios cut short by the 2026 connection lifecycle --- # The auth fixture flow drives the 2025 stateful lifecycle; the 2026-mode @@ -65,14 +74,6 @@ client: - http-invalid-tool-headers # SEP-2106 (JSON Schema $ref handling): client still dereferences network $refs. - json-schema-ref-no-deref - # SEP-2468 (authorization response iss parameter): not implemented in the client. - - auth/iss-supported - - auth/iss-not-advertised - - auth/iss-supported-missing - - auth/iss-wrong-issuer - - auth/iss-unexpected - - auth/iss-normalized - - auth/metadata-issuer-mismatch # SEP-2352 (authorization server migration): client does not re-register when # PRM authorization_servers changes. - auth/authorization-server-migration diff --git a/.github/actions/conformance/expected-failures.yml b/.github/actions/conformance/expected-failures.yml index 9b676ea475..5d61bd7fc0 100644 --- a/.github/actions/conformance/expected-failures.yml +++ b/.github/actions/conformance/expected-failures.yml @@ -24,20 +24,21 @@ client: - http-invalid-tool-headers # SEP-2106 (JSON Schema $ref handling): client still dereferences network $refs. - json-schema-ref-no-deref - # SEP-2468 (authorization response iss parameter): not implemented in the client. + # SEP-2352 (authorization server migration): client does not re-register when + # PRM authorization_servers changes. + - auth/authorization-server-migration + # SEP-837 (application_type during DCR): the check fires on every non-legacy + # spec version (the default LATEST is 2026-07-28). The client omits + # application_type during Dynamic Client Registration, so every scenario that + # reaches DCR fails it. SEP-2468 iss validation is implemented, so these now + # fail only on the application_type check, not on iss. + - auth/offline-access-not-supported - auth/iss-supported - auth/iss-not-advertised - auth/iss-supported-missing - auth/iss-wrong-issuer - auth/iss-unexpected - auth/iss-normalized - - auth/metadata-issuer-mismatch - # SEP-2352 (authorization server migration): client does not re-register when - # PRM authorization_servers changes. - - auth/authorization-server-migration - # SEP-837 (application_type during DCR): the check only fires on draft-version - # runs; this draft scenario is the one place the client still hits it. - - auth/offline-access-not-supported # --- Pre-existing scenarios that fail on checks added after conformance 0.1.15 --- # SEP-2350 (scope step-up): WARNING-only; the expected-failures evaluator diff --git a/README.v2.md b/README.v2.md index 25cf5ac959..0a45c8e303 100644 --- a/README.v2.md +++ b/README.v2.md @@ -2323,7 +2323,7 @@ import httpx from pydantic import AnyUrl from mcp import ClientSession -from mcp.client.auth import OAuthClientProvider, TokenStorage +from mcp.client.auth import AuthorizationCodeResult, OAuthClientProvider, TokenStorage from mcp.client.streamable_http import streamable_http_client from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata, OAuthToken @@ -2356,10 +2356,14 @@ async def handle_redirect(auth_url: str) -> None: print(f"Visit: {auth_url}") -async def handle_callback() -> tuple[str, str | None]: +async def handle_callback() -> AuthorizationCodeResult: callback_url = input("Paste callback URL: ") params = parse_qs(urlparse(callback_url).query) - return params["code"][0], params.get("state", [None])[0] + return AuthorizationCodeResult( + code=params["code"][0], + state=params.get("state", [None])[0], + iss=params.get("iss", [None])[0], + ) async def main(): diff --git a/docs/migration.md b/docs/migration.md index 9fbbbf2ed2..725e0156fa 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -51,6 +51,35 @@ async with http_client: v1's internal client set `follow_redirects=True`; set it explicitly when supplying your own `httpx.AsyncClient` to preserve that behavior. +### OAuth `callback_handler` returns `AuthorizationCodeResult` + +The `callback_handler` passed to `OAuthClientProvider` now returns an `AuthorizationCodeResult` instead of a `tuple[str, str | None]` of `(code, state)`. The new object adds an `iss` field so the client can validate the RFC 9207 authorization-response issuer (SEP-2468): when the redirect carries an `iss` query parameter it must match the authorization server's issuer, and a missing `iss` is rejected when the server advertised `authorization_response_iss_parameter_supported`. + +**Before (v1):** + +```python +async def callback_handler() -> tuple[str, str | None]: + params = parse_qs(urlparse(await wait_for_redirect()).query) + return params["code"][0], params.get("state", [None])[0] +``` + +**After (v2):** + +```python +from mcp.client.auth import AuthorizationCodeResult + + +async def callback_handler() -> AuthorizationCodeResult: + params = parse_qs(urlparse(await wait_for_redirect()).query) + return AuthorizationCodeResult( + code=params["code"][0], + state=params.get("state", [None])[0], + iss=params.get("iss", [None])[0], + ) +``` + +Forward the `iss` query parameter from the redirect so the validation can run; omitting it disables the issuer check for servers that send `iss`. + ### `get_session_id` callback removed from `streamable_http_client` The `get_session_id` callback (third element of the returned tuple) has been removed from `streamable_http_client`. The function now returns a 2-tuple `(read_stream, write_stream)` instead of a 3-tuple. diff --git a/examples/clients/simple-auth-client/mcp_simple_auth_client/main.py b/examples/clients/simple-auth-client/mcp_simple_auth_client/main.py index 6ef2f0b113..501e5b011a 100644 --- a/examples/clients/simple-auth-client/mcp_simple_auth_client/main.py +++ b/examples/clients/simple-auth-client/mcp_simple_auth_client/main.py @@ -19,7 +19,7 @@ import httpx from mcp.client._transport import ReadStream, WriteStream -from mcp.client.auth import OAuthClientProvider, TokenStorage +from mcp.client.auth import AuthorizationCodeResult, OAuthClientProvider, TokenStorage from mcp.client.session import ClientSession from mcp.client.sse import sse_client from mcp.client.streamable_http import streamable_http_client @@ -69,6 +69,7 @@ def do_GET(self): if "code" in query_params: self.callback_data["authorization_code"] = query_params["code"][0] self.callback_data["state"] = query_params.get("state", [None])[0] + self.callback_data["iss"] = query_params.get("iss", [None])[0] self.send_response(200) self.send_header("Content-type", "text/html") self.end_headers() @@ -112,7 +113,7 @@ def __init__(self, port: int = 3000): self.port = port self.server = None self.thread = None - self.callback_data = {"authorization_code": None, "state": None, "error": None} + self.callback_data = {"authorization_code": None, "state": None, "iss": None, "error": None} def _create_handler_with_data(self): """Create a handler class with access to callback data.""" @@ -160,6 +161,10 @@ def get_state(self): """Get the received state parameter.""" return self.callback_data["state"] + def get_iss(self): + """Get the received iss parameter.""" + return self.callback_data["iss"] + class SimpleAuthClient: """Simple MCP client with auth support.""" @@ -183,12 +188,14 @@ async def connect(self): callback_server = CallbackServer(port=3030) callback_server.start() - async def callback_handler() -> tuple[str, str | None]: - """Wait for OAuth callback and return auth code and state.""" + async def callback_handler() -> AuthorizationCodeResult: + """Wait for OAuth callback and return auth code, state, and iss.""" print("⏳ Waiting for authorization callback...") try: auth_code = callback_server.wait_for_callback(timeout=300) - return auth_code, callback_server.get_state() + return AuthorizationCodeResult( + code=auth_code, state=callback_server.get_state(), iss=callback_server.get_iss() + ) finally: callback_server.stop() diff --git a/examples/snippets/clients/oauth_client.py b/examples/snippets/clients/oauth_client.py index 3887c5c8ca..2085b9a1db 100644 --- a/examples/snippets/clients/oauth_client.py +++ b/examples/snippets/clients/oauth_client.py @@ -13,7 +13,7 @@ from pydantic import AnyUrl from mcp import ClientSession -from mcp.client.auth import OAuthClientProvider, TokenStorage +from mcp.client.auth import AuthorizationCodeResult, OAuthClientProvider, TokenStorage from mcp.client.streamable_http import streamable_http_client from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata, OAuthToken @@ -46,10 +46,14 @@ async def handle_redirect(auth_url: str) -> None: print(f"Visit: {auth_url}") -async def handle_callback() -> tuple[str, str | None]: +async def handle_callback() -> AuthorizationCodeResult: callback_url = input("Paste callback URL: ") params = parse_qs(urlparse(callback_url).query) - return params["code"][0], params.get("state", [None])[0] + return AuthorizationCodeResult( + code=params["code"][0], + state=params.get("state", [None])[0], + iss=params.get("iss", [None])[0], + ) async def main(): diff --git a/src/mcp/client/auth/__init__.py b/src/mcp/client/auth/__init__.py index ab3179ecb9..9d00fc700f 100644 --- a/src/mcp/client/auth/__init__.py +++ b/src/mcp/client/auth/__init__.py @@ -9,8 +9,10 @@ PKCEParameters, TokenStorage, ) +from mcp.shared.auth import AuthorizationCodeResult __all__ = [ + "AuthorizationCodeResult", "OAuthClientProvider", "OAuthFlowError", "OAuthRegistrationError", diff --git a/src/mcp/client/auth/extensions/client_credentials.py b/src/mcp/client/auth/extensions/client_credentials.py index cb6dafb407..5efd596110 100644 --- a/src/mcp/client/auth/extensions/client_credentials.py +++ b/src/mcp/client/auth/extensions/client_credentials.py @@ -18,7 +18,7 @@ from pydantic import BaseModel, Field from mcp.client.auth import OAuthClientProvider, OAuthFlowError, OAuthTokenError, TokenStorage -from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata +from mcp.shared.auth import AuthorizationCodeResult, OAuthClientInformationFull, OAuthClientMetadata class ClientCredentialsOAuthProvider(OAuthClientProvider): @@ -405,7 +405,7 @@ def __init__( client_metadata: OAuthClientMetadata, storage: TokenStorage, redirect_handler: Callable[[str], Awaitable[None]] | None = None, - callback_handler: Callable[[], Awaitable[tuple[str, str | None]]] | None = None, + callback_handler: Callable[[], Awaitable[AuthorizationCodeResult]] | None = None, timeout: float = 300.0, jwt_parameters: JWTParameters | None = None, ) -> None: diff --git a/src/mcp/client/auth/oauth2.py b/src/mcp/client/auth/oauth2.py index 01bcc82347..786f64bb18 100644 --- a/src/mcp/client/auth/oauth2.py +++ b/src/mcp/client/auth/oauth2.py @@ -35,9 +35,12 @@ handle_token_response_scopes, is_valid_client_metadata_url, should_use_client_metadata_url, + validate_authorization_response_iss, + validate_metadata_issuer, ) from mcp.client.streamable_http import MCP_PROTOCOL_VERSION from mcp.shared.auth import ( + AuthorizationCodeResult, OAuthClientInformationFull, OAuthClientMetadata, OAuthMetadata, @@ -97,7 +100,7 @@ class OAuthContext: client_metadata: OAuthClientMetadata storage: TokenStorage redirect_handler: Callable[[str], Awaitable[None]] | None - callback_handler: Callable[[], Awaitable[tuple[str, str | None]]] | None + callback_handler: Callable[[], Awaitable[AuthorizationCodeResult]] | None timeout: float = 300.0 client_metadata_url: str | None = None @@ -227,7 +230,7 @@ def __init__( client_metadata: OAuthClientMetadata, storage: TokenStorage, redirect_handler: Callable[[str], Awaitable[None]] | None = None, - callback_handler: Callable[[], Awaitable[tuple[str, str | None]]] | None = None, + callback_handler: Callable[[], Awaitable[AuthorizationCodeResult]] | None = None, timeout: float = 300.0, client_metadata_url: str | None = None, validate_resource_url: Callable[[str, str | None], Awaitable[None]] | None = None, @@ -356,16 +359,19 @@ async def _perform_authorization_code_grant(self) -> tuple[str, str]: await self.context.redirect_handler(authorization_url) # Wait for callback - auth_code, returned_state = await self.context.callback_handler() + result = await self.context.callback_handler() - if returned_state is None or not secrets.compare_digest(returned_state, state): - raise OAuthFlowError(f"State parameter mismatch: {returned_state} != {state}") + if result.state is None or not secrets.compare_digest(result.state, state): + raise OAuthFlowError(f"State parameter mismatch: {result.state} != {state}") - if not auth_code: + # RFC 9207: validate the authorization-response issuer + validate_authorization_response_iss(result.iss, self.context.oauth_metadata) + + if not result.code: raise OAuthFlowError("No authorization code received") # Return auth code and code verifier for token exchange - return auth_code, pkce_params.code_verifier + return result.code, pkce_params.code_verifier def _get_token_endpoint(self) -> str: if self.context.oauth_metadata and self.context.oauth_metadata.token_endpoint: @@ -570,6 +576,9 @@ async def async_auth_flow(self, request: httpx.Request) -> AsyncGenerator[httpx. if not ok: break if ok and asm: + # SEP-2468: metadata issuer must match the discovery issuer + if self.context.auth_server_url is not None: + validate_metadata_issuer(asm, self.context.auth_server_url) self.context.oauth_metadata = asm break else: diff --git a/src/mcp/client/auth/utils.py b/src/mcp/client/auth/utils.py index 780a24e859..4bac6af9c2 100644 --- a/src/mcp/client/auth/utils.py +++ b/src/mcp/client/auth/utils.py @@ -4,7 +4,7 @@ from httpx import Request, Response from pydantic import AnyUrl, ValidationError -from mcp.client.auth import OAuthRegistrationError, OAuthTokenError +from mcp.client.auth import OAuthFlowError, OAuthRegistrationError, OAuthTokenError from mcp.client.streamable_http import MCP_PROTOCOL_VERSION from mcp.shared.auth import ( OAuthClientInformationFull, @@ -211,6 +211,63 @@ async def handle_auth_metadata_response(response: Response) -> tuple[bool, OAuth return True, None +def _strip_authority_trailing_slash(url: str) -> str: + """Drop the lone trailing slash `AnyHttpUrl` appends to a path-less authority. + + RFC 9207 / SEP-2468 mandate simple string comparison (RFC 3986 section 6.2.1), so issuers + must be compared as sent. `AnyHttpUrl` rewrites `https://as` to `https://as/`, which would + defeat the comparison; undo only that, leaving issuers with a real path untouched. + """ + if url.endswith("/") and urlparse(url).path == "/": + return url[:-1] + return url + + +def raw_issuer(oauth_metadata: OAuthMetadata) -> str: + """Return the issuer as the authorization server transmitted it, before URL normalization.""" + return _strip_authority_trailing_slash(str(oauth_metadata.issuer)) + + +def validate_authorization_response_iss(iss: str | None, oauth_metadata: OAuthMetadata | None) -> None: + """Validate the RFC 9207 `iss` authorization-response parameter. + + Per RFC 9207 section 2.4, the client compares `iss` against the issuer of the + authorization server the request was sent to, using simple string comparison + (RFC 3986 section 6.2.1, i.e. without URL normalization), and rejects on mismatch. + A response that omits `iss` is rejected only when the server advertised support via + `authorization_response_iss_parameter_supported`. + + Raises: + OAuthFlowError: If `iss` is present and does not match, or is absent when the + authorization server advertised support. + """ + expected = raw_issuer(oauth_metadata) if oauth_metadata else None + + if iss is not None: + if iss != expected: + raise OAuthFlowError(f"Authorization response iss mismatch: {iss} != {expected}") + return + + if oauth_metadata is not None and oauth_metadata.authorization_response_iss_parameter_supported: + raise OAuthFlowError("Authorization response missing iss parameter advertised by the authorization server") + + +def validate_metadata_issuer(oauth_metadata: OAuthMetadata, expected_issuer: str) -> None: + """Validate that authorization server metadata `issuer` matches the discovery issuer. + + Per RFC 8414 section 3.3 / SEP-2468, the `issuer` in the metadata must match the issuer + used to construct the well-known URL, compared as a simple string (RFC 3986 section 6.2.1). + + Raises: + OAuthFlowError: If the metadata issuer does not match `expected_issuer`. + """ + expected = _strip_authority_trailing_slash(expected_issuer) + if raw_issuer(oauth_metadata) != expected: + raise OAuthFlowError( + f"Authorization server metadata issuer mismatch: {raw_issuer(oauth_metadata)} != {expected}" + ) + + def create_oauth_metadata_request(url: str) -> Request: return Request("GET", url, headers={MCP_PROTOCOL_VERSION: LATEST_PROTOCOL_VERSION}) diff --git a/src/mcp/shared/auth.py b/src/mcp/shared/auth.py index 3b48152d5b..7fdf2b2e13 100644 --- a/src/mcp/shared/auth.py +++ b/src/mcp/shared/auth.py @@ -22,6 +22,18 @@ def normalize_token_type(cls, v: str | None) -> str | None: return v # pragma: no cover +class AuthorizationCodeResult(BaseModel): + """Authorization-code-grant redirect parameters returned by a callback handler. + + `iss` carries the RFC 9207 authorization-response issuer when the authorization server + includes it in the redirect; the client validates it against the expected issuer. + """ + + code: str + state: str | None = None + iss: str | None = None + + class InvalidScopeError(Exception): def __init__(self, message: str): self.message = message @@ -145,6 +157,7 @@ class OAuthMetadata(BaseModel): introspection_endpoint_auth_signing_alg_values_supported: list[str] | None = None code_challenge_methods_supported: list[str] | None = None client_id_metadata_document_supported: bool | None = None + authorization_response_iss_parameter_supported: bool | None = None class ProtectedResourceMetadata(BaseModel): diff --git a/tests/client/auth/extensions/test_client_credentials.py b/tests/client/auth/extensions/test_client_credentials.py index 09760f4530..a964891316 100644 --- a/tests/client/auth/extensions/test_client_credentials.py +++ b/tests/client/auth/extensions/test_client_credentials.py @@ -13,7 +13,13 @@ SignedJWTParameters, static_assertion_provider, ) -from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata, OAuthMetadata, OAuthToken +from mcp.shared.auth import ( + AuthorizationCodeResult, + OAuthClientInformationFull, + OAuthClientMetadata, + OAuthMetadata, + OAuthToken, +) class MockTokenStorage: @@ -57,9 +63,9 @@ async def redirect_handler(url: str) -> None: # pragma: no cover """Mock redirect handler.""" pass - async def callback_handler() -> tuple[str, str | None]: # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: # pragma: no cover """Mock callback handler.""" - return "test_auth_code", "test_state" + return AuthorizationCodeResult(code="test_auth_code", state="test_state") with warnings.catch_warnings(): warnings.simplefilter("ignore", DeprecationWarning) diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index ca7a495e6c..32b0f391cf 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -24,11 +24,15 @@ get_client_metadata_scopes, handle_registration_response, is_valid_client_metadata_url, + raw_issuer, should_use_client_metadata_url, + validate_authorization_response_iss, + validate_metadata_issuer, ) from mcp.server.auth.routes import build_metadata from mcp.server.auth.settings import ClientRegistrationOptions, RevocationOptions from mcp.shared.auth import ( + AuthorizationCodeResult, OAuthClientInformationFull, OAuthClientMetadata, OAuthMetadata, @@ -89,9 +93,9 @@ async def redirect_handler(url: str) -> None: """Mock redirect handler.""" pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: + async def callback_handler() -> AuthorizationCodeResult: """Mock callback handler.""" - return "test_auth_code", "test_state" # pragma: no cover + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover return OAuthClientProvider( server_url="https://api.example.com/v1/mcp", @@ -272,8 +276,8 @@ async def test_build_protected_resource_discovery_urls( async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover provider = OAuthClientProvider( server_url="https://api.example.com", @@ -1380,8 +1384,8 @@ async def capture_redirect(url: str) -> None: oauth_provider.context.redirect_handler = capture_redirect # Mock callback - async def mock_callback() -> tuple[str, str | None]: - return "auth_code", captured_state + async def mock_callback() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="auth_code", state=captured_state) oauth_provider.context.callback_handler = mock_callback @@ -1517,8 +1521,8 @@ async def test_legacy_server_no_prm_falls_back_to_root_oauth_discovery( async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover # Simulate a legacy server like Linear provider = OAuthClientProvider( @@ -1616,8 +1620,8 @@ async def test_legacy_server_with_different_prm_and_root_urls( async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover provider = OAuthClientProvider( server_url="https://api.example.com/v1/mcp", @@ -1721,8 +1725,8 @@ async def test_path_based_fallback_when_no_www_authenticate( async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover provider = OAuthClientProvider( server_url="https://api.example.com/v1/mcp", @@ -1756,8 +1760,8 @@ async def test_root_based_fallback_after_path_based_404( async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover provider = OAuthClientProvider( server_url="https://api.example.com/v1/mcp", @@ -1857,8 +1861,8 @@ async def test_www_authenticate_takes_priority_over_well_known( async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover provider = OAuthClientProvider( server_url="https://api.example.com/v1/mcp", @@ -2071,8 +2075,8 @@ def test_oauth_provider_with_valid_client_metadata_url( async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover provider = OAuthClientProvider( server_url="https://api.example.com/v1/mcp", @@ -2092,8 +2096,8 @@ def test_oauth_provider_with_invalid_client_metadata_url_raises_error( async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover with pytest.raises(ValueError) as exc_info: OAuthClientProvider( @@ -2115,8 +2119,8 @@ async def test_auth_flow_uses_cimd_when_server_supports( async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover provider = OAuthClientProvider( server_url="https://api.example.com/v1/mcp", @@ -2206,8 +2210,8 @@ async def test_auth_flow_falls_back_to_dcr_when_no_cimd_support( async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover provider = OAuthClientProvider( server_url="https://api.example.com/v1/mcp", @@ -2439,8 +2443,8 @@ async def redirect_handler(url: str) -> None: params = parse_qs(parsed.query) captured_state = params.get("state", [None])[0] - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", captured_state + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state=captured_state) provider = OAuthClientProvider( server_url="https://api.example.com/v1/mcp", @@ -2548,8 +2552,8 @@ async def redirect_handler(url: str) -> None: params = parse_qs(parsed.query) captured_state = params.get("state", [None])[0] - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", captured_state + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state=captured_state) provider = OAuthClientProvider( server_url="https://api.example.com/v1/mcp", @@ -2636,3 +2640,72 @@ async def callback_handler() -> tuple[str, str | None]: await auth_flow.asend(final_response) except StopAsyncIteration: pass + + +# A path-bearing issuer so a trailing slash is a genuine difference under simple string comparison +# (AnyHttpUrl normalizes a bare host to a trailing slash, which would hide the distinction). +_ISSUER = "https://as.example.com/tenant" + + +def _issuer_metadata(*, issuer: str = _ISSUER, iss_supported: bool | None = None) -> OAuthMetadata: + return OAuthMetadata( + issuer=AnyHttpUrl(issuer), + authorization_endpoint=AnyHttpUrl(f"{issuer}/authorize"), + token_endpoint=AnyHttpUrl(f"{issuer}/token"), + authorization_response_iss_parameter_supported=iss_supported, + ) + + +@pytest.mark.parametrize( + ("iss", "iss_supported"), + [ + pytest.param(_ISSUER, True, id="advertised-and-correct"), + pytest.param(None, None, id="not-advertised-and-omitted"), + pytest.param(_ISSUER, None, id="not-advertised-but-correct"), + ], +) +def test_validate_authorization_response_iss_accepts(iss: str | None, iss_supported: bool | None): + """RFC 9207: a matching or legitimately absent iss is accepted.""" + validate_authorization_response_iss(iss, _issuer_metadata(iss_supported=iss_supported)) + + +@pytest.mark.parametrize( + ("iss", "iss_supported", "match"), + [ + pytest.param(None, True, "missing iss", id="advertised-but-omitted"), + pytest.param("https://evil.example.com", True, "iss mismatch", id="wrong-issuer"), + pytest.param("https://evil.example.com", None, "iss mismatch", id="unexpected-when-not-advertised"), + pytest.param(f"{_ISSUER}/", True, "iss mismatch", id="trailing-slash-not-normalized"), + ], +) +def test_validate_authorization_response_iss_rejects(iss: str | None, iss_supported: bool | None, match: str): + """RFC 9207: a mismatched iss, or one missing when advertised, is rejected via simple string compare.""" + with pytest.raises(OAuthFlowError, match=match): + validate_authorization_response_iss(iss, _issuer_metadata(iss_supported=iss_supported)) + + +def test_validate_authorization_response_iss_without_metadata(): + """With no AS metadata, a present iss is rejected and an absent one is accepted.""" + validate_authorization_response_iss(None, None) + with pytest.raises(OAuthFlowError, match="iss mismatch"): + validate_authorization_response_iss(_ISSUER, None) + + +def test_validate_metadata_issuer_accepts_match(): + validate_metadata_issuer(_issuer_metadata(issuer=_ISSUER), str(AnyHttpUrl(_ISSUER))) + + +def test_validate_metadata_issuer_rejects_mismatch(): + with pytest.raises(OAuthFlowError, match="metadata issuer mismatch"): + validate_metadata_issuer(_issuer_metadata(issuer="https://attacker.example.com"), str(AnyHttpUrl(_ISSUER))) + + +def test_raw_issuer_strips_only_authority_trailing_slash(): + """The slash AnyHttpUrl adds to a bare authority is dropped; a real path keeps its slash.""" + assert raw_issuer(_issuer_metadata(issuer="https://as.example.com")) == "https://as.example.com" + assert raw_issuer(_issuer_metadata(issuer="https://as.example.com/tenant")) == "https://as.example.com/tenant" + + +def test_validate_metadata_issuer_ignores_authority_trailing_slash(): + """A bare-authority issuer matches whether or not the discovery URL carries the slash.""" + validate_metadata_issuer(_issuer_metadata(issuer="https://as.example.com"), "https://as.example.com") diff --git a/tests/client/test_scope_bug_1630.py b/tests/client/test_scope_bug_1630.py index fafa510075..338755dc68 100644 --- a/tests/client/test_scope_bug_1630.py +++ b/tests/client/test_scope_bug_1630.py @@ -11,7 +11,12 @@ from pydantic import AnyUrl from mcp.client.auth import OAuthClientProvider -from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata, OAuthToken +from mcp.shared.auth import ( + AuthorizationCodeResult, + OAuthClientInformationFull, + OAuthClientMetadata, + OAuthToken, +) class MockTokenStorage: @@ -48,8 +53,8 @@ async def test_401_uses_www_auth_scope_not_resource_metadata_url(): async def redirect_handler(url: str) -> None: pass # pragma: no cover - async def callback_handler() -> tuple[str, str | None]: - return "test_auth_code", "test_state" # pragma: no cover + async def callback_handler() -> AuthorizationCodeResult: + return AuthorizationCodeResult(code="test_auth_code", state="test_state") # pragma: no cover client_metadata = OAuthClientMetadata( redirect_uris=[AnyUrl("http://localhost:3030/callback")], diff --git a/tests/interaction/_requirements.py b/tests/interaction/_requirements.py index 0e7b2d25c0..01a5d19e7f 100644 --- a/tests/interaction/_requirements.py +++ b/tests/interaction/_requirements.py @@ -3281,16 +3281,10 @@ def __post_init__(self) -> None: source=f"{SPEC_BASE_URL}/basic/authorization#authorization-server-metadata-discovery", behavior=( "The client rejects authorization-server metadata whose issuer does not match the URL the " - "metadata was retrieved from (RFC 8414 section 3.3)." + "metadata was retrieved from (RFC 8414 section 3.3 / SEP-2468)." ), transports=("streamable-http",), note="OAuth is HTTP-only.", - divergence=Divergence( - note=( - "The SDK parses authorization-server metadata without comparing issuer to the discovery " - "URL; a mismatched issuer is accepted and the flow proceeds." - ), - ), ), "client-auth:authorize:error-surfaces": Requirement( source=f"{SPEC_BASE_URL}/basic/authorization#authorization-flow-steps", @@ -3494,6 +3488,16 @@ def __post_init__(self) -> None: transports=("streamable-http",), note="OAuth is HTTP-only.", ), + "client-auth:authorization-response:iss-verify": Requirement( + source=f"{SPEC_BASE_URL}/basic/authorization#authorization-server-metadata-discovery", + behavior=( + "The client validates the RFC 9207 iss authorization-response parameter against the " + "authorization server issuer (simple string comparison) and rejects a mismatch, or a " + "missing iss when the server advertises support (SEP-2468)." + ), + transports=("streamable-http",), + note="OAuth is HTTP-only.", + ), "client-auth:token-endpoint-auth-method": Requirement( source="sdk", behavior="The client authenticates to the token endpoint using the auth method established at registration.", diff --git a/tests/interaction/auth/_harness.py b/tests/interaction/auth/_harness.py index d013364f33..ab360addd4 100644 --- a/tests/interaction/auth/_harness.py +++ b/tests/interaction/auth/_harness.py @@ -26,7 +26,7 @@ from mcp.server import Server from mcp.server.auth.provider import AccessToken, ProviderTokenVerifier from mcp.server.auth.settings import AuthSettings, ClientRegistrationOptions, RevocationOptions -from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata, OAuthToken +from mcp.shared.auth import AuthorizationCodeResult, OAuthClientInformationFull, OAuthClientMetadata, OAuthToken from tests.interaction._connect import BASE_URL, NO_DNS_REBINDING_PROTECTION from tests.interaction.auth._provider import InMemoryAuthorizationServerProvider from tests.interaction.transports._bridge import StreamingASGITransport @@ -136,16 +136,21 @@ class HeadlessOAuth: `state_override`: when set, `callback_handler` returns this value as the state instead of the one parsed from the redirect, so tests can drive the state-mismatch path. + + `iss_override`: when set, `callback_handler` returns this value as the RFC 9207 issuer + instead of the one parsed from the redirect, so tests can drive the iss-mismatch path. """ - def __init__(self, *, state_override: str | None = None) -> None: + def __init__(self, *, state_override: str | None = None, iss_override: str | None = None) -> None: self.authorize_url: str | None = None self.authorize_urls: list[str] = [] self.error: str | None = None self._state_override = state_override + self._iss_override = iss_override self._http: httpx.AsyncClient | None = None self._code: str = "" self._state: str | None = None + self._iss: str | None = None def bind(self, http_client: httpx.AsyncClient) -> None: self._http = http_client @@ -161,10 +166,15 @@ async def redirect_handler(self, authorization_url: str) -> None: params = parse_qs(urlsplit(response.headers["location"]).query) self._code = params.get("code", [""])[0] self._state = params.get("state", [None])[0] + self._iss = params.get("iss", [None])[0] self.error = params.get("error", [None])[0] - async def callback_handler(self) -> tuple[str, str | None]: - return self._code, self._state_override if self._state_override is not None else self._state + async def callback_handler(self) -> AuthorizationCodeResult: + return AuthorizationCodeResult( + code=self._code, + state=self._state_override if self._state_override is not None else self._state, + iss=self._iss_override if self._iss_override is not None else self._iss, + ) def auth_settings( diff --git a/tests/interaction/auth/_provider.py b/tests/interaction/auth/_provider.py index 5c88995a30..65d520590d 100644 --- a/tests/interaction/auth/_provider.py +++ b/tests/interaction/auth/_provider.py @@ -21,6 +21,7 @@ construct_redirect_uri, ) from mcp.shared.auth import OAuthClientInformationFull, OAuthToken +from tests.interaction._connect import BASE_URL _TOKEN_LIFETIME_SECONDS = 3600 @@ -53,9 +54,13 @@ def __init__( issue_expired_first: bool = False, fail_next_refresh: bool = False, reject_all_tokens: bool = False, + issuer: str | None = None, ) -> None: self._default_scopes = list(default_scopes) if default_scopes is not None else ["mcp"] - self._issuer = "http://127.0.0.1:8000" + # The authorization-response iss must match the AS issuer the client recorded (RFC 9207 + # simple string comparison); the client strips the trailing slash AnyHttpUrl adds to a + # path-less issuer, so the redirect carries the bare origin. Path-issuer tests pass it. + self._issuer = issuer if issuer is not None else BASE_URL self._deny_authorize = deny_authorize self._issue_expired_first = issue_expired_first self._fail_next_refresh = fail_next_refresh diff --git a/tests/interaction/auth/test_authorize_token.py b/tests/interaction/auth/test_authorize_token.py index cb8524c097..62822ca804 100644 --- a/tests/interaction/auth/test_authorize_token.py +++ b/tests/interaction/auth/test_authorize_token.py @@ -187,6 +187,24 @@ async def test_a_mismatched_state_on_the_callback_aborts_the_flow() -> None: await connect_with_oauth(server, provider=provider, headless=headless).__aenter__() +@requirement("client-auth:authorization-response:iss-verify") +async def test_a_mismatched_iss_on_the_callback_aborts_the_flow() -> None: + """A callback whose RFC 9207 iss does not match the authorization server issuer aborts the flow. + + `iss_override` makes the headless callback return an issuer the AS never advertised; the SDK + compares it to `oauth_metadata.issuer` and raises `OAuthFlowError` before the token exchange. + """ + provider = InMemoryAuthorizationServerProvider() + server = Server("guarded", on_list_tools=list_tools) + headless = HeadlessOAuth(iss_override="https://attacker.example.com") + + with anyio.fail_after(5): + with pytest.RaisesGroup( + pytest.RaisesExc(OAuthFlowError, match="^Authorization response iss mismatch:"), flatten_subgroups=True + ): + await connect_with_oauth(server, provider=provider, headless=headless).__aenter__() + + @requirement("client-auth:resource-parameter") async def test_the_authorization_code_token_request_carries_grant_type_code_redirect_and_resource( recorded_oauth_flow: RecordedFlow, diff --git a/tests/interaction/auth/test_discovery.py b/tests/interaction/auth/test_discovery.py index 68c33c8a2d..3aba38fe4c 100644 --- a/tests/interaction/auth/test_discovery.py +++ b/tests/interaction/auth/test_discovery.py @@ -19,6 +19,7 @@ from mcp import types from mcp.client.auth import OAuthFlowError, OAuthRegistrationError +from mcp.client.auth.utils import raw_issuer from mcp.server import Server, ServerRequestContext from mcp.shared.auth import OAuthMetadata, ProtectedResourceMetadata from mcp.types import ListToolsResult, Tool @@ -236,7 +237,10 @@ async def test_as_metadata_discovery_falls_back_through_the_spec_endpoint_order( unrecognized field to prove the client's parser ignores unknown members (RFC 8414 §3.2). """ recorded, on_request = record_requests() - provider = InMemoryAuthorizationServerProvider() + asm = real_asm() + asm.issuer = AnyHttpUrl(authorization_server) + # The redirect iss must equal the issuer the client records from this metadata. + provider = InMemoryAuthorizationServerProvider(issuer=raw_issuer(asm)) server = Server("guarded", on_list_tools=list_tools) prm = ProtectedResourceMetadata( @@ -246,7 +250,7 @@ async def test_as_metadata_discovery_falls_back_through_the_spec_endpoint_order( not_found=not_found, serve={ PRM_PATH_SUFFIXED: metadata_body(prm), - serve_at: metadata_body(real_asm(), x_unknown_extension="ignored"), + serve_at: metadata_body(asm, x_unknown_extension="ignored"), }, ) @@ -311,23 +315,26 @@ async def test_as_metadata_advertises_authorize_token_registration_and_s256() -> @requirement("client-auth:as-metadata-discovery:issuer-validation") -async def test_as_metadata_with_a_mismatched_issuer_is_accepted_and_the_flow_proceeds() -> None: - """Authorization-server metadata whose `issuer` does not match the discovery URL is accepted. +async def test_as_metadata_with_a_mismatched_issuer_aborts_the_flow() -> None: + """Authorization-server metadata whose `issuer` does not match the discovery URL is rejected. - RFC 8414 §3.3 requires the client to reject the document; the SDK parses and uses it - without comparing `issuer` to the URL it was fetched from. See the divergence on the - requirement. The served body carries an unrecognized field as a fold-in proof of - unknown-field tolerance. + RFC 8414 §3.3 / SEP-2468 require the client to reject the document; the SDK compares `issuer` + to the URL the metadata was fetched from and raises `OAuthFlowError` before any authorize or + token request is made. """ + recorded, on_request = record_requests() provider = InMemoryAuthorizationServerProvider() server = Server("guarded", on_list_tools=list_tools) metadata = real_asm() metadata.issuer = AnyHttpUrl(f"{BASE_URL}/wrong-issuer") - app_shim = shim(serve={ASM_ROOT: metadata_body(metadata, x_unknown_extension="ignored")}) + app_shim = shim(serve={ASM_ROOT: metadata_body(metadata)}) with anyio.fail_after(5): - async with connect_with_oauth(server, provider=provider, app_shim=app_shim) as (client, _): - result = await client.list_tools() + with pytest.RaisesGroup( + pytest.RaisesExc(OAuthFlowError, match="^Authorization server metadata issuer mismatch"), + flatten_subgroups=True, + ): + await connect_with_oauth(server, provider=provider, app_shim=app_shim, on_request=on_request).__aenter__() - assert result.tools[0].name == "probe" + assert [r.path for r in recorded if r.path in ("/authorize", "/token")] == [] From 7eeee6cd3113f3daecbf7e2d1d5810022d8946e2 Mon Sep 17 00:00:00 2001 From: Marcelo Trylesinski Date: Sat, 20 Jun 2026 17:14:30 +0200 Subject: [PATCH 2/3] Address review: clarify iss migration note, make callback getters properties - migration.md: spell out that omitting iss raises OAuthFlowError against servers advertising authorization_response_iss_parameter_supported, rather than merely disabling the check. - simple-auth-client example: convert get_state/get_iss to @property. --- docs/migration.md | 2 +- .../mcp_simple_auth_client/main.py | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/migration.md b/docs/migration.md index 725e0156fa..30a1c9413a 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -78,7 +78,7 @@ async def callback_handler() -> AuthorizationCodeResult: ) ``` -Forward the `iss` query parameter from the redirect so the validation can run; omitting it disables the issuer check for servers that send `iss`. +Forward the `iss` query parameter from the redirect so the validation can run: omitting it makes the flow fail with `OAuthFlowError` against servers that advertise `authorization_response_iss_parameter_supported`, and silently skips the check for servers that send `iss` without advertising it. ### `get_session_id` callback removed from `streamable_http_client` diff --git a/examples/clients/simple-auth-client/mcp_simple_auth_client/main.py b/examples/clients/simple-auth-client/mcp_simple_auth_client/main.py index 501e5b011a..0d461d5d11 100644 --- a/examples/clients/simple-auth-client/mcp_simple_auth_client/main.py +++ b/examples/clients/simple-auth-client/mcp_simple_auth_client/main.py @@ -157,12 +157,14 @@ def wait_for_callback(self, timeout: int = 300): time.sleep(0.1) raise Exception("Timeout waiting for OAuth callback") - def get_state(self): - """Get the received state parameter.""" + @property + def state(self): + """The received state parameter.""" return self.callback_data["state"] - def get_iss(self): - """Get the received iss parameter.""" + @property + def iss(self): + """The received iss parameter.""" return self.callback_data["iss"] @@ -193,9 +195,7 @@ async def callback_handler() -> AuthorizationCodeResult: print("⏳ Waiting for authorization callback...") try: auth_code = callback_server.wait_for_callback(timeout=300) - return AuthorizationCodeResult( - code=auth_code, state=callback_server.get_state(), iss=callback_server.get_iss() - ) + return AuthorizationCodeResult(code=auth_code, state=callback_server.state, iss=callback_server.iss) finally: callback_server.stop() From 15f11a587afe2b01bddfc6377301e6b72bbda270 Mon Sep 17 00:00:00 2001 From: Marcelo Trylesinski Date: Sat, 20 Jun 2026 17:39:55 +0200 Subject: [PATCH 3/3] 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. --- src/mcp/client/auth/utils.py | 24 ++---------- tests/client/test_auth.py | 48 ++++++++++-------------- tests/interaction/auth/_provider.py | 9 +++-- tests/interaction/auth/test_discovery.py | 3 +- 4 files changed, 29 insertions(+), 55 deletions(-) diff --git a/src/mcp/client/auth/utils.py b/src/mcp/client/auth/utils.py index 4bac6af9c2..0395ad5575 100644 --- a/src/mcp/client/auth/utils.py +++ b/src/mcp/client/auth/utils.py @@ -211,23 +211,6 @@ async def handle_auth_metadata_response(response: Response) -> tuple[bool, OAuth return True, None -def _strip_authority_trailing_slash(url: str) -> str: - """Drop the lone trailing slash `AnyHttpUrl` appends to a path-less authority. - - RFC 9207 / SEP-2468 mandate simple string comparison (RFC 3986 section 6.2.1), so issuers - must be compared as sent. `AnyHttpUrl` rewrites `https://as` to `https://as/`, which would - defeat the comparison; undo only that, leaving issuers with a real path untouched. - """ - if url.endswith("/") and urlparse(url).path == "/": - return url[:-1] - return url - - -def raw_issuer(oauth_metadata: OAuthMetadata) -> str: - """Return the issuer as the authorization server transmitted it, before URL normalization.""" - return _strip_authority_trailing_slash(str(oauth_metadata.issuer)) - - def validate_authorization_response_iss(iss: str | None, oauth_metadata: OAuthMetadata | None) -> None: """Validate the RFC 9207 `iss` authorization-response parameter. @@ -241,7 +224,7 @@ def validate_authorization_response_iss(iss: str | None, oauth_metadata: OAuthMe OAuthFlowError: If `iss` is present and does not match, or is absent when the authorization server advertised support. """ - expected = raw_issuer(oauth_metadata) if oauth_metadata else None + expected = str(oauth_metadata.issuer) if oauth_metadata else None if iss is not None: if iss != expected: @@ -261,10 +244,9 @@ def validate_metadata_issuer(oauth_metadata: OAuthMetadata, expected_issuer: str Raises: OAuthFlowError: If the metadata issuer does not match `expected_issuer`. """ - expected = _strip_authority_trailing_slash(expected_issuer) - if raw_issuer(oauth_metadata) != expected: + if str(oauth_metadata.issuer) != expected_issuer: raise OAuthFlowError( - f"Authorization server metadata issuer mismatch: {raw_issuer(oauth_metadata)} != {expected}" + f"Authorization server metadata issuer mismatch: {oauth_metadata.issuer} != {expected_issuer}" ) diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index 24bb07af6f..2b7978b2e8 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -24,7 +24,6 @@ get_client_metadata_scopes, handle_registration_response, is_valid_client_metadata_url, - raw_issuer, should_use_client_metadata_url, validate_authorization_response_iss, validate_metadata_issuer, @@ -2642,31 +2641,35 @@ async def callback_handler() -> AuthorizationCodeResult: pass -# A path-bearing issuer so a trailing slash is a genuine difference under simple string comparison -# (AnyHttpUrl normalizes a bare host to a trailing slash, which would hide the distinction). -_ISSUER = "https://as.example.com/tenant" +_ISSUER = "https://as.example.com" def _issuer_metadata(*, issuer: str = _ISSUER, iss_supported: bool | None = None) -> OAuthMetadata: - return OAuthMetadata( - issuer=AnyHttpUrl(issuer), - authorization_endpoint=AnyHttpUrl(f"{issuer}/authorize"), - token_endpoint=AnyHttpUrl(f"{issuer}/token"), - authorization_response_iss_parameter_supported=iss_supported, + # Validate from string inputs so url_preserve_empty_path keeps the issuer as transmitted, + # matching the wire path (model_validate_json) rather than normalizing a bare authority. + return OAuthMetadata.model_validate( + { + "issuer": issuer, + "authorization_endpoint": f"{issuer}/authorize", + "token_endpoint": f"{issuer}/token", + "authorization_response_iss_parameter_supported": iss_supported, + } ) @pytest.mark.parametrize( - ("iss", "iss_supported"), + ("issuer", "iss", "iss_supported"), [ - pytest.param(_ISSUER, True, id="advertised-and-correct"), - pytest.param(None, None, id="not-advertised-and-omitted"), - pytest.param(_ISSUER, None, id="not-advertised-but-correct"), + pytest.param(_ISSUER, _ISSUER, True, id="advertised-and-correct"), + pytest.param(_ISSUER, None, None, id="not-advertised-and-omitted"), + pytest.param(_ISSUER, _ISSUER, None, id="not-advertised-but-correct"), + # An issuer that genuinely ends in a slash (e.g. Auth0) must match its own iss. + pytest.param("https://as.example.com/", "https://as.example.com/", True, id="trailing-slash-issuer"), ], ) -def test_validate_authorization_response_iss_accepts(iss: str | None, iss_supported: bool | None): +def test_validate_authorization_response_iss_accepts(issuer: str, iss: str | None, iss_supported: bool | None): """RFC 9207: a matching or legitimately absent iss is accepted.""" - validate_authorization_response_iss(iss, _issuer_metadata(iss_supported=iss_supported)) + validate_authorization_response_iss(iss, _issuer_metadata(issuer=issuer, iss_supported=iss_supported)) @pytest.mark.parametrize( @@ -2692,20 +2695,9 @@ def test_validate_authorization_response_iss_without_metadata(): def test_validate_metadata_issuer_accepts_match(): - validate_metadata_issuer(_issuer_metadata(issuer=_ISSUER), str(AnyHttpUrl(_ISSUER))) + validate_metadata_issuer(_issuer_metadata(issuer=_ISSUER), _ISSUER) def test_validate_metadata_issuer_rejects_mismatch(): with pytest.raises(OAuthFlowError, match="metadata issuer mismatch"): - validate_metadata_issuer(_issuer_metadata(issuer="https://attacker.example.com"), str(AnyHttpUrl(_ISSUER))) - - -def test_raw_issuer_strips_only_authority_trailing_slash(): - """The slash AnyHttpUrl adds to a bare authority is dropped; a real path keeps its slash.""" - assert raw_issuer(_issuer_metadata(issuer="https://as.example.com")) == "https://as.example.com" - assert raw_issuer(_issuer_metadata(issuer="https://as.example.com/tenant")) == "https://as.example.com/tenant" - - -def test_validate_metadata_issuer_ignores_authority_trailing_slash(): - """A bare-authority issuer matches whether or not the discovery URL carries the slash.""" - validate_metadata_issuer(_issuer_metadata(issuer="https://as.example.com"), "https://as.example.com") + validate_metadata_issuer(_issuer_metadata(issuer="https://attacker.example.com"), _ISSUER) diff --git a/tests/interaction/auth/_provider.py b/tests/interaction/auth/_provider.py index 65d520590d..422134becf 100644 --- a/tests/interaction/auth/_provider.py +++ b/tests/interaction/auth/_provider.py @@ -57,10 +57,11 @@ def __init__( issuer: str | None = None, ) -> None: self._default_scopes = list(default_scopes) if default_scopes is not None else ["mcp"] - # The authorization-response iss must match the AS issuer the client recorded (RFC 9207 - # simple string comparison); the client strips the trailing slash AnyHttpUrl adds to a - # path-less issuer, so the redirect carries the bare origin. Path-issuer tests pass it. - self._issuer = issuer if issuer is not None else BASE_URL + # The authorization-response iss must equal the AS metadata issuer the client recorded + # (RFC 9207 simple string comparison). `real_asm` builds the issuer from an AnyHttpUrl + # object, so it carries the trailing slash; the redirect iss matches it. Path-issuer + # tests pass the recorded issuer explicitly. + self._issuer = issuer if issuer is not None else f"{BASE_URL}/" self._deny_authorize = deny_authorize self._issue_expired_first = issue_expired_first self._fail_next_refresh = fail_next_refresh diff --git a/tests/interaction/auth/test_discovery.py b/tests/interaction/auth/test_discovery.py index 3aba38fe4c..5038fa8e65 100644 --- a/tests/interaction/auth/test_discovery.py +++ b/tests/interaction/auth/test_discovery.py @@ -19,7 +19,6 @@ from mcp import types from mcp.client.auth import OAuthFlowError, OAuthRegistrationError -from mcp.client.auth.utils import raw_issuer from mcp.server import Server, ServerRequestContext from mcp.shared.auth import OAuthMetadata, ProtectedResourceMetadata from mcp.types import ListToolsResult, Tool @@ -240,7 +239,7 @@ async def test_as_metadata_discovery_falls_back_through_the_spec_endpoint_order( asm = real_asm() asm.issuer = AnyHttpUrl(authorization_server) # The redirect iss must equal the issuer the client records from this metadata. - provider = InMemoryAuthorizationServerProvider(issuer=raw_issuer(asm)) + provider = InMemoryAuthorizationServerProvider(issuer=str(asm.issuer)) server = Server("guarded", on_list_tools=list_tools) prm = ProtectedResourceMetadata(