Skip to content

Allow empty string as valid URL in DCR workflow#987

Merged
pcarleton merged 2 commits intomodelcontextprotocol:mainfrom
fredericbarthelet:allow-empty-string-as-safe-url
Oct 13, 2025
Merged

Allow empty string as valid URL in DCR workflow#987
pcarleton merged 2 commits intomodelcontextprotocol:mainfrom
fredericbarthelet:allow-empty-string-as-safe-url

Conversation

@fredericbarthelet
Copy link
Copy Markdown
Contributor

@fredericbarthelet fredericbarthelet commented Oct 1, 2025

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
domdomegg
domdomegg requested changes Oct 3, 2025
View reviewed changes
Copy link
Copy Markdown
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()?

@domdomegg domdomegg mentioned this pull request Oct 3, 2025
Closed
@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
Copy Markdown
Contributor Author

fredericbarthelet commented Oct 6, 2025 •
edited
Loading

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

@fredericbarthelet
Copy link
Copy Markdown
Contributor Author

fredericbarthelet commented Oct 8, 2025

Hey @pcarleton - sorry for the ping, any chance this can get merged :)?

@pcarleton pcarleton merged commit caa2550 into modelcontextprotocol:main Oct 13, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pcarleton pcarleton pcarleton approved these changes

@domdomegg domdomegg domdomegg approved these changes

@ochafik ochafik Awaiting requested review from ochafik

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

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

3 participants

@fredericbarthelet @pcarleton @domdomegg