Skip to content
Merged
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
4 changes: 3 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions src/mcp/shared/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,24 @@
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

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

View check run for this annotation

Claude / Claude Code Review

OAuthMetadata and ProtectedResourceMetadata missing empty-string URL coercion

This PR correctly fixes empty-string coercion for OAuthClientMetadata, but OAuthMetadata (RFC 8414) and ProtectedResourceMetadata (RFC 9728) in the same file have the identical failure mode for their optional URL fields — a pre-existing gap now made visible by the inconsistency. The same non-compliant servers that echo "" in DCR responses could equally send "" in their /.well-known/oauth-authorization-server or protected resource metadata responses, causing ValidationError when parsing those ser
Comment on lines +70 to +86
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 fixes empty-string coercion for OAuthClientMetadata, but OAuthMetadata (RFC 8414) and ProtectedResourceMetadata (RFC 9728) in the same file have the identical failure mode for their optional URL fields — a pre-existing gap now made visible by the inconsistency. The same non-compliant servers that echo "" in DCR responses could equally send "" in their /.well-known/oauth-authorization-server or protected resource metadata responses, causing ValidationError when parsing those server-returned payloads.

Extended reasoning...

What the bug is and how it manifests

The PR correctly identifies that some real-world OAuth servers echo omitted optional URL fields as empty strings rather than dropping the keys, and that Pydantic's AnyHttpUrl rejects "" — causing ValidationError at parse time. The fix (a before-mode field_validator that maps "" → None) is applied to OAuthClientMetadata, but two sibling models in the same file are left unpatched:

  • OAuthMetadata (RFC 8414 AS metadata): optional URL fields registration_endpoint, service_documentation, op_policy_uri, op_tos_uri, revocation_endpoint, introspection_endpoint
  • ProtectedResourceMetadata (RFC 9728): optional URL fields jwks_uri, resource_documentation, resource_policy_uri, resource_tos_uri

The specific code path that triggers it

When the MCP client fetches /.well-known/oauth-authorization-server (or equivalent), the response JSON is parsed via OAuthMetadata.model_validate(). If the server returns {"issuer": "https://example.com", "authorization_endpoint": "...", "token_endpoint": "...", "registration_endpoint": ""}, Pydantic's AnyHttpUrl validator fires on registration_endpoint before any field_validator runs (there is none), sees an empty string, and raises ValidationError — even though registration_endpoint is marked OPTIONAL in RFC 8414 §2. The same applies to the ProtectedResourceMetadata parse at the protected resource metadata endpoint.

Why existing code doesn't prevent it

OAuthMetadata and ProtectedResourceMetadata have no field_validator at all for their optional AnyHttpUrl fields. The PR adds one only to OAuthClientMetadata. Pydantic's default behavior for AnyHttpUrl | None fields is to accept None or a valid URL; an empty string is neither, so it raises. There is no upstream fallback.

Impact

Any MCP client talking to a non-compliant-but-widely-deployed authorization server that echoes "" for omitted AS-metadata or resource-metadata fields will fail during the discovery phase — before client registration even begins. This is a harder failure than the DCR failure fixed by the PR, because it blocks all OAuth flows, not just re-registration.

How to fix it

Apply the same field_validator pattern used for OAuthClientMetadata to OAuthMetadata (covering registration_endpoint, service_documentation, op_policy_uri, op_tos_uri, revocation_endpoint, introspection_endpoint) and to ProtectedResourceMetadata (covering jwks_uri, resource_documentation, resource_policy_uri, resource_tos_uri).

Step-by-step proof

  1. A server returns HTTP 200 with body: {"issuer": "https://as.example.com", "authorization_endpoint": "https://as.example.com/auth", "token_endpoint": "https://as.example.com/token", "registration_endpoint": ""}
  2. Client calls OAuthMetadata.model_validate(response_json)
  3. Pydantic processes field registration_endpoint with value ""
  4. Field type is AnyHttpUrl | None; "" is not None and not a valid URL
  5. ValidationError is raised: registration_endpoint → URL input should be a string or URL
  6. The MCP client's OAuth discovery flow fails, and no connection can be established

Addressing the refutation

The refuting verifier correctly notes the PR does not introduce this bug — it pre-dates the PR. However, this PR directly modifies auth.py and adds the exact fix pattern needed. The PR's description explicitly cites Postel's law and real-world non-compliant servers as motivation. Those same servers' AS metadata and resource metadata endpoints are equally likely to echo "" for optional fields. The code review is the appropriate venue to flag that the fix was applied incompletely across the three related models in the same file.


def validate_scope(self, requested_scope: str | None) -> list[str] | None:
if requested_scope is None:
return None
Expand Down
82 changes: 81 additions & 1 deletion tests/shared/test_auth.py
Original file line number Diff line number Diff line change
@@ -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():
Expand Down Expand Up @@ -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)
Loading