Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/mcp/shared/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,27 @@
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

Check notice on line 94 in src/mcp/shared/auth.py

View check run for this annotation

Claude / Claude Code Review

Missing empty-string coercion in OAuthMetadata and ProtectedResourceMetadata

This PR correctly adds empty-string coercion for `OAuthClientMetadata`, but `OAuthMetadata` and `ProtectedResourceMetadata` have the same defect for their optional `AnyHttpUrl` fields and are left unprotected. This is a pre-existing gap — the PR does not introduce or modify these classes — but the same Postel's law rationale applies equally to them.
Comment on lines +74 to 94
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 This PR correctly adds empty-string coercion for OAuthClientMetadata, but OAuthMetadata and ProtectedResourceMetadata have the same defect for their optional AnyHttpUrl fields and are left unprotected. This is a pre-existing gap — the PR does not introduce or modify these classes — but the same Postel's law rationale applies equally to them.

Extended reasoning...

What the bug is

The PR adds a field_validator to OAuthClientMetadata that coerces empty strings to None for five optional AnyHttpUrl fields. The same class of defect exists in two sibling models that were not touched by this PR:

  • OAuthMetadata (RFC 8414 Authorization Server Metadata): registration_endpoint, service_documentation, op_policy_uri, op_tos_uri, revocation_endpoint, introspection_endpoint — all declared AnyHttpUrl | None at lines 135, 142–149, with no empty-string validator.
  • ProtectedResourceMetadata (RFC 9728 Protected Resource Metadata): jwks_uri, resource_documentation, resource_policy_uri, resource_tos_uri — all declared AnyHttpUrl | None at lines 164, 169–171, with no empty-string validator.

How it manifests

If a non-compliant authorization server or resource server echoes any of these optional URL fields as "" (rather than omitting the key), Pydantic's AnyHttpUrl validator rejects the empty string and raises ValidationError. The entire discovery response is discarded — the same failure mode this PR was authored to fix.

Step-by-step proof

  1. A non-compliant AS returns its metadata with "op_tos_uri": "".
  2. The client calls OAuthMetadata.model_validate(data).
  3. Pydantic attempts to coerce "" through AnyHttpUrl — no field_validator intercepts it.
  4. AnyHttpUrl raises ValidationError because an empty string is not a valid URL.
  5. The caller discards the response and OAuth discovery fails entirely.

The same sequence applies to any of the unprotected fields in ProtectedResourceMetadata.

Why existing code does not prevent it

The new validator is defined only on OAuthClientMetadata and is not inherited by OAuthMetadata or ProtectedResourceMetadata (they are not subclasses). Neither of those models defines any field_validator for their URL fields.

Impact

Any MCP client connecting to a non-compliant server that echoes empty-string URL fields in its authorization server metadata or protected resource metadata response will fail OAuth discovery entirely. The behavior is identical to what the PR describes for DCR responses — a valid discovery document is thrown away because of a cosmetic serialization quirk in a single optional field.

How to fix

Add the same _empty_string_optional_url_to_none field_validator (or a shared mixin) to OAuthMetadata and ProtectedResourceMetadata, covering all their optional AnyHttpUrl fields.

Pre-existing status

This is a pre-existing issue that predates this PR. The PR neither introduces these models nor adds new call sites for them — it is scoped entirely to DCR response parsing via OAuthClientMetadata. The gap in coverage is worth addressing as a follow-up given the PR's stated motivation.

requested_scopes = requested_scope.split(" ")
allowed_scopes = [] if self.scope is None else self.scope.split(" ")
for scope in requested_scopes:
Expand Down
75 changes: 74 additions & 1 deletion tests/client/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,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
Expand Down Expand Up @@ -857,6 +857,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."""

Expand Down
Loading