Skip to content

Conversation

fredericbarthelet
Copy link
Contributor

Fixes #940

Motivation and Context

Allow empty URL as valid URL in DCR payload

How Has This Been Tested?

Added a specific test case for empty string

Breaking Changes

None, this actually reverts breaking change introduced by #877

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

@fredericbarthelet fredericbarthelet requested review from a team and ochafik October 1, 2025 10:27
Copy link
Member

@domdomegg domdomegg left a comment

Choose a reason for hiding this comment

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

I may be wrong, please push back if so. But I think we might want to allow empty strings only on properties that are not required.

E.g. we should not allow empty string for token_endpoint. But we should allow it for op_tos_uri.

Maybe more broadly - potentially back to the issue - is should we really be accepting an empty string? Should the endpoints just not be returning anything for this property, which is already allowed by the .optional()?

@fredericbarthelet fredericbarthelet force-pushed the allow-empty-string-as-safe-url branch from daf1ea5 to f2ffcbd Compare October 6, 2025 12:44
@fredericbarthelet fredericbarthelet requested a review from a team as a code owner October 6, 2025 12:44
@fredericbarthelet
Copy link
Contributor Author

fredericbarthelet commented Oct 6, 2025

Thanks for your review @domdomegg. I updated my PR with a few modifications that might address some of your concern :

  • keep the existing SafeUrlSchema untouched so that all URLs are checked to be valid (and deny empty strings)
  • add a new OptionalSafeUrlSchema allowing optional SafeUrlSchema and empty string, but transforming the parsed value to undefined to preserve functionality down the line
  • use this new Schema only on logo_uri and tos_uri in order to keep this fix for existing clients scoped to the only 2 problematic properties (and not to all optional URIs in DCR)

Maybe this new schema could be removed during next protocol version release in November in order to keep it contained within a certain time frame

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.

Empty string for tos_uri and logo_uri causing validation errors during DCR as of 1.17.4
2 participants