fix(auth): coerce empty-string optional URL fields to None in OAuthClientMetadata (#2404)
This commit is contained in:
parent
941089e06a
commit
2dfb51a4d1
3 changed files with 102 additions and 2 deletions
|
|
@ -45,7 +45,9 @@
|
||||||
|
|
||||||
- Framework: `uv run --frozen pytest`
|
- Framework: `uv run --frozen pytest`
|
||||||
- Async testing: use anyio, not asyncio
|
- 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;
|
- 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.
|
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
|
- For end-to-end behavior, an in-memory `Client(server)` is usually the
|
||||||
|
|
|
||||||
|
|
@ -67,6 +67,24 @@ class OAuthClientMetadata(BaseModel):
|
||||||
software_id: str | None = None
|
software_id: str | None = None
|
||||||
software_version: 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:
|
def validate_scope(self, requested_scope: str | None) -> list[str] | None:
|
||||||
if requested_scope is None:
|
if requested_scope is None:
|
||||||
return None
|
return None
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,9 @@
|
||||||
"""Tests for OAuth 2.0 shared code."""
|
"""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():
|
def test_oauth():
|
||||||
|
|
@ -58,3 +61,80 @@ def test_oauth_with_jarm():
|
||||||
"token_endpoint_auth_methods_supported": ["client_secret_basic", "client_secret_post"],
|
"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…
Reference in a new issue