Skip to content

fix(auth): coerce empty-string optional URL fields to None in OAuthClientMetadata#2404

Open
felixweinberger wants to merge 1 commit intomainfrom
fweinberger/dcr-empty-url-coerce
Open

fix(auth): coerce empty-string optional URL fields to None in OAuthClientMetadata#2404
felixweinberger wants to merge 1 commit intomainfrom
fweinberger/dcr-empty-url-coerce

Conversation

@felixweinberger
Copy link
Copy Markdown
Contributor

@felixweinberger felixweinberger commented Apr 8, 2026

Coerce "" to None for the five OPTIONAL URL fields on OAuthClientMetadata so that DCR responses from servers that echo omitted metadata as empty strings parse successfully.

Motivation and Context

RFC 7591 §2 marks client_uri, logo_uri, tos_uri, policy_uri, and jwks_uri as OPTIONAL. Some authorization servers in the wild (observed: Udemy, deepsense.ai, Apify, windsor.ai, firecrawl, and others) echo the client's omitted metadata back as "" instead of dropping the keys:

{"client_id": "abc123", "client_uri": "", "logo_uri": "", "tos_uri": "", "policy_uri": "", ...}

AnyHttpUrl rejects "", so handle_registration_response raises ValidationError and callers discard an otherwise valid registration — the server returned 201 with a real client_id.

The servers are technically non-compliant (an empty string is not a valid URL), but the only reasonable interpretation of "" for an OPTIONAL field is "absent". This applies Postel's law at the deserialization boundary, matching the existing normalize_token_type validator on OAuthToken in the same file. Because OAuthClientInformationFull inherits from OAuthClientMetadata, the coercion applies to parsed DCR responses as well.

How Has This Been Tested?

New TestOAuthClientMetadataEmptyUrlCoercion covers per-field coercion (parametrized over all five fields), all-fields-together, valid-URL passthrough, inheritance through OAuthClientInformationFull (the DCR-response model), and a negative case confirming non-empty invalid URLs still raise.

Breaking Changes

None. This only widens accepted input — previously-rejected "" values now succeed as None. Valid URLs, None, and omitted keys are unchanged.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

RFC 7591 §2: https://datatracker.ietf.org/doc/html/rfc7591#section-2

v1.x backport: #2405

Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

LGTM — clean, well-tested Postel's-law fix that exactly mirrors the existing normalize_token_type pattern in the same file.

Extended reasoning...

Overview

The PR adds a single field_validator on OAuthClientMetadata that coerces empty-string values to None for five optional URL fields (client_uri, logo_uri, tos_uri, policy_uri, jwks_uri). Because OAuthClientInformationFull inherits from OAuthClientMetadata, the coercion automatically covers parsed DCR responses. The diff touches two files: the model definition and its test suite.

Security risks

None. The five fields affected are informational metadata; they are not used in any authentication or authorization decision. The critical redirect_uris field is intentionally excluded. The validator only widens accepted input for values that were previously rejected with a ValidationError (i.e., ""), and the negative-case test confirms that non-empty invalid URLs still raise.

Level of scrutiny

Low. This is a small, non-breaking compatibility shim consistent with the existing normalize_token_type validator already present in the same file. The pattern is idiomatic Pydantic v2 (mode="before", @classmethod). Test coverage is thorough: parametrized per-field, all-at-once, valid-URL passthrough, subclass inheritance, and a negative case.

Other factors

No outstanding reviewer comments; no prior reviews from any reviewer. The bug hunting system found no issues. This is a direct fix for a real-world interoperability problem observed with multiple production OAuth servers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant