From fef29a5fdcd0629bf5d77ae8046f9bcab90a06a8 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Wed, 8 Apr 2026 15:45:00 +0000 Subject: [PATCH 1/3] fix(auth): coerce empty-string optional URL fields to None in OAuthClientMetadata --- src/mcp/shared/auth.py | 18 ++++++++++ tests/client/test_auth.py | 75 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/src/mcp/shared/auth.py b/src/mcp/shared/auth.py index ca5b7b45a..ebf534d79 100644 --- a/src/mcp/shared/auth.py +++ b/src/mcp/shared/auth.py @@ -67,6 +67,24 @@ class OAuthClientMetadata(BaseModel): software_id: str | None = None software_version: str | None = None + @field_validator( + "client_uri", + "logo_uri", + "tos_uri", + "policy_uri", + "jwks_uri", + mode="before", + ) + @classmethod + def _empty_string_optional_url_to_none(cls, v: object) -> object: + # RFC 7591 §2 marks these URL fields OPTIONAL. Some authorization servers + # echo omitted metadata back as "" instead of dropping the keys, which + # AnyHttpUrl would otherwise reject — throwing away an otherwise valid + # registration response. Treat "" as absent. + if v == "": + return None + return v + def validate_scope(self, requested_scope: str | None) -> list[str] | None: if requested_scope is None: return None diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index 5aa985e36..4bb0c2a44 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -8,7 +8,7 @@ import httpx import pytest from inline_snapshot import Is, snapshot -from pydantic import AnyHttpUrl, AnyUrl +from pydantic import AnyHttpUrl, AnyUrl, ValidationError from mcp.client.auth import OAuthClientProvider, PKCEParameters from mcp.client.auth.exceptions import OAuthFlowError @@ -985,6 +985,79 @@ def text(self): assert "Registration failed: 400" in str(exc_info.value) +class TestOAuthClientMetadataEmptyUrlCoercion: + """RFC 7591 §2 marks client_uri/logo_uri/tos_uri/policy_uri/jwks_uri as OPTIONAL. + Some authorization servers echo the client's omitted metadata back as "" + instead of dropping the keys; without coercion, AnyHttpUrl rejects "" and + the whole registration response is thrown away even though the server + returned a valid client_id.""" + + @pytest.mark.parametrize( + "empty_field", + ["client_uri", "logo_uri", "tos_uri", "policy_uri", "jwks_uri"], + ) + def test_optional_url_empty_string_coerced_to_none(self, empty_field: str): + data = { + "redirect_uris": ["https://example.com/callback"], + empty_field: "", + } + metadata = OAuthClientMetadata.model_validate(data) + assert getattr(metadata, empty_field) is None + + def test_all_optional_urls_empty_together(self): + data = { + "redirect_uris": ["https://example.com/callback"], + "client_uri": "", + "logo_uri": "", + "tos_uri": "", + "policy_uri": "", + "jwks_uri": "", + } + metadata = OAuthClientMetadata.model_validate(data) + assert metadata.client_uri is None + assert metadata.logo_uri is None + assert metadata.tos_uri is None + assert metadata.policy_uri is None + assert metadata.jwks_uri is None + + def test_valid_url_passes_through_unchanged(self): + data = { + "redirect_uris": ["https://example.com/callback"], + "client_uri": "https://udemy.com/", + } + metadata = OAuthClientMetadata.model_validate(data) + assert str(metadata.client_uri) == "https://udemy.com/" + + def test_information_full_inherits_coercion(self): + """OAuthClientInformationFull subclasses OAuthClientMetadata, so the + same coercion applies to DCR responses parsed via the full model.""" + data = { + "client_id": "abc123", + "redirect_uris": ["https://example.com/callback"], + "client_uri": "", + "logo_uri": "", + "tos_uri": "", + "policy_uri": "", + "jwks_uri": "", + } + info = OAuthClientInformationFull.model_validate(data) + assert info.client_id == "abc123" + assert info.client_uri is None + assert info.logo_uri is None + assert info.tos_uri is None + assert info.policy_uri is None + assert info.jwks_uri is None + + def test_invalid_non_empty_url_still_rejected(self): + """Coercion must only touch empty strings — garbage URLs still raise.""" + data = { + "redirect_uris": ["https://example.com/callback"], + "client_uri": "not a url", + } + with pytest.raises(ValidationError): + OAuthClientMetadata.model_validate(data) + + class TestCreateClientRegistrationRequest: """Test client registration request creation.""" From b0c83a84dc094b97ad2c2a9905a1fc2ad25dc3c0 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Mon, 13 Apr 2026 14:08:29 +0000 Subject: [PATCH 2/3] test: move OAuthClientMetadata empty-URL tests to tests/shared, drop Test class --- tests/client/test_auth.py | 75 +---------------------------------- tests/shared/test_auth.py | 82 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 75 deletions(-) diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index 4bb0c2a44..5aa985e36 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -8,7 +8,7 @@ import httpx import pytest from inline_snapshot import Is, snapshot -from pydantic import AnyHttpUrl, AnyUrl, ValidationError +from pydantic import AnyHttpUrl, AnyUrl from mcp.client.auth import OAuthClientProvider, PKCEParameters from mcp.client.auth.exceptions import OAuthFlowError @@ -985,79 +985,6 @@ def text(self): assert "Registration failed: 400" in str(exc_info.value) -class TestOAuthClientMetadataEmptyUrlCoercion: - """RFC 7591 §2 marks client_uri/logo_uri/tos_uri/policy_uri/jwks_uri as OPTIONAL. - Some authorization servers echo the client's omitted metadata back as "" - instead of dropping the keys; without coercion, AnyHttpUrl rejects "" and - the whole registration response is thrown away even though the server - returned a valid client_id.""" - - @pytest.mark.parametrize( - "empty_field", - ["client_uri", "logo_uri", "tos_uri", "policy_uri", "jwks_uri"], - ) - def test_optional_url_empty_string_coerced_to_none(self, empty_field: str): - data = { - "redirect_uris": ["https://example.com/callback"], - empty_field: "", - } - metadata = OAuthClientMetadata.model_validate(data) - assert getattr(metadata, empty_field) is None - - def test_all_optional_urls_empty_together(self): - data = { - "redirect_uris": ["https://example.com/callback"], - "client_uri": "", - "logo_uri": "", - "tos_uri": "", - "policy_uri": "", - "jwks_uri": "", - } - metadata = OAuthClientMetadata.model_validate(data) - assert metadata.client_uri is None - assert metadata.logo_uri is None - assert metadata.tos_uri is None - assert metadata.policy_uri is None - assert metadata.jwks_uri is None - - def test_valid_url_passes_through_unchanged(self): - data = { - "redirect_uris": ["https://example.com/callback"], - "client_uri": "https://udemy.com/", - } - metadata = OAuthClientMetadata.model_validate(data) - assert str(metadata.client_uri) == "https://udemy.com/" - - def test_information_full_inherits_coercion(self): - """OAuthClientInformationFull subclasses OAuthClientMetadata, so the - same coercion applies to DCR responses parsed via the full model.""" - data = { - "client_id": "abc123", - "redirect_uris": ["https://example.com/callback"], - "client_uri": "", - "logo_uri": "", - "tos_uri": "", - "policy_uri": "", - "jwks_uri": "", - } - info = OAuthClientInformationFull.model_validate(data) - assert info.client_id == "abc123" - assert info.client_uri is None - assert info.logo_uri is None - assert info.tos_uri is None - assert info.policy_uri is None - assert info.jwks_uri is None - - def test_invalid_non_empty_url_still_rejected(self): - """Coercion must only touch empty strings — garbage URLs still raise.""" - data = { - "redirect_uris": ["https://example.com/callback"], - "client_uri": "not a url", - } - with pytest.raises(ValidationError): - OAuthClientMetadata.model_validate(data) - - class TestCreateClientRegistrationRequest: """Test client registration request creation.""" diff --git a/tests/shared/test_auth.py b/tests/shared/test_auth.py index cd3c35332..7463bc5a8 100644 --- a/tests/shared/test_auth.py +++ b/tests/shared/test_auth.py @@ -1,6 +1,9 @@ """Tests for OAuth 2.0 shared code.""" -from mcp.shared.auth import OAuthMetadata +import pytest +from pydantic import ValidationError + +from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata, OAuthMetadata def test_oauth(): @@ -58,3 +61,80 @@ def test_oauth_with_jarm(): "token_endpoint_auth_methods_supported": ["client_secret_basic", "client_secret_post"], } ) + + +# RFC 7591 §2 marks client_uri/logo_uri/tos_uri/policy_uri/jwks_uri as OPTIONAL. +# Some authorization servers echo the client's omitted metadata back as "" +# instead of dropping the keys; without coercion, AnyHttpUrl rejects "" and +# the whole registration response is thrown away even though the server +# returned a valid client_id. + + +@pytest.mark.parametrize( + "empty_field", + ["client_uri", "logo_uri", "tos_uri", "policy_uri", "jwks_uri"], +) +def test_optional_url_empty_string_coerced_to_none(empty_field: str): + data = { + "redirect_uris": ["https://example.com/callback"], + empty_field: "", + } + metadata = OAuthClientMetadata.model_validate(data) + assert getattr(metadata, empty_field) is None + + +def test_all_optional_urls_empty_together(): + data = { + "redirect_uris": ["https://example.com/callback"], + "client_uri": "", + "logo_uri": "", + "tos_uri": "", + "policy_uri": "", + "jwks_uri": "", + } + metadata = OAuthClientMetadata.model_validate(data) + assert metadata.client_uri is None + assert metadata.logo_uri is None + assert metadata.tos_uri is None + assert metadata.policy_uri is None + assert metadata.jwks_uri is None + + +def test_valid_url_passes_through_unchanged(): + data = { + "redirect_uris": ["https://example.com/callback"], + "client_uri": "https://udemy.com/", + } + metadata = OAuthClientMetadata.model_validate(data) + assert str(metadata.client_uri) == "https://udemy.com/" + + +def test_information_full_inherits_coercion(): + """OAuthClientInformationFull subclasses OAuthClientMetadata, so the + same coercion applies to DCR responses parsed via the full model.""" + data = { + "client_id": "abc123", + "redirect_uris": ["https://example.com/callback"], + "client_uri": "", + "logo_uri": "", + "tos_uri": "", + "policy_uri": "", + "jwks_uri": "", + } + info = OAuthClientInformationFull.model_validate(data) + assert info.client_id == "abc123" + assert info.client_uri is None + assert info.logo_uri is None + assert info.tos_uri is None + assert info.policy_uri is None + assert info.jwks_uri is None + + +def test_invalid_non_empty_url_still_rejected(): + """Coercion must only touch empty strings — garbage URLs still raise.""" + data = { + "redirect_uris": ["https://example.com/callback"], + "client_uri": "not a url", + } + with pytest.raises(ValidationError): + OAuthClientMetadata.model_validate(data) From bfc7da345db594f47869d9c6304173a4adddf3d0 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Mon, 13 Apr 2026 14:21:29 +0000 Subject: [PATCH 3/3] docs: clarify Test-class convention applies even in legacy files --- AGENTS.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 969227104..307bd81b3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,7 +45,9 @@ - Framework: `uv run --frozen pytest` - Async testing: use anyio, not asyncio -- Do not use `Test` prefixed classes, use functions +- Do not use `Test` prefixed classes — write plain top-level `test_*` functions. + Legacy files still contain `Test*` classes; do NOT follow that pattern for new + tests even when adding to such a file. - IMPORTANT: Tests should be fast and deterministic. Prefer in-memory async execution; reach for threads only when necessary, and subprocesses only as a last resort. - For end-to-end behavior, an in-memory `Client(server)` is usually the