-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: validate clientMetadataUrl at construction time (fail-fast) #1653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
pcarleton
merged 6 commits into
modelcontextprotocol:main
from
rechedev9:fix/validate-client-metadata-url
Apr 14, 2026
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
738ef45
fix(auth): add validateClientMetadataUrl utility for fail-fast valida…
f464827
merge: resolve conflicts with upstream/main
rechedev9 276b439
style(client): add blank line between test describe blocks
rechedev9 117d2d2
Merge branch 'main' into fix/validate-client-metadata-url
felixweinberger 5e48ff0
fix(changeset): classify client export as minor
rechedev9 5fa6d12
Merge branch 'main' into fix/validate-client-metadata-url
pcarleton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': patch | ||
| --- | ||
|
|
||
| Add `validateClientMetadataUrl()` utility for early validation of `clientMetadataUrl` | ||
|
|
||
| Exports a `validateClientMetadataUrl()` function that `OAuthClientProvider` implementations | ||
| can call in their constructors to fail fast on invalid URL-based client IDs, instead of | ||
| discovering the error deep in the auth flow. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The changeset declares
patchfor@modelcontextprotocol/client, but this PR adds a new publicly-exported symbol (validateClientMetadataUrl) topackages/client/src/index.ts. Per semver, adding new backwards-compatible functionality is aminorchange. The changeset should declareminorinstead ofpatch.Extended reasoning...
What the bug is
The changeset file
.changeset/fix-validate-client-metadata-url.mdusespatchas the bump type for@modelcontextprotocol/client. However, the PR addsvalidateClientMetadataUrlas a new named export inpackages/client/src/index.ts(line 41). Semver defines aminorbump as any backwards-compatible addition of new functionality, and a new public export is exactly that.The specific code path
The new function
validateClientMetadataUrlis defined inpackages/client/src/client/auth.tsand explicitly added to the public surface inpackages/client/src/index.ts:This is unambiguously a new API addition visible to all downstream consumers.
Why existing patterns contradict this choice
The repo has a clear, consistent convention for when to use
minorvspatchin changesets. Examining.changeset/history:expose-auth-server-discovery.mdusesminorfor addingdiscoverOAuthServerInfo,OAuthDiscoveryState, andOAuthServerInfo;token-provider-composable-auth.mdusesminorfor addingAuthProvider,adaptOAuthProvider,handleOAuthUnauthorized, etc. Conversely,patchchangesets are reserved for bug fixes and behavioral tweaks (fix-oauth-5xx-discovery.md,oauth-error-http200.md,finish-sdkerror-capability.md). This PR breaks that pattern by usingpatchfor an additive API change.Proof by example
Step-by-step: (1) The PR adds
export function validateClientMetadataUrl(...)inauth.ts. (2) It is listed in the named exports inindex.ts. (3) The changeset declares this aspatch. (4) Per semver §7, aMINORversion increment is required when "new, backwards compatible functionality is introduced to the public API". (5) Apatchincrement is only correct for "backwards compatible bug fixes" (semver §6). SincevalidateClientMetadataUrlis purely additive and introduces no behavioral change to existing code, it does not qualify as a bug fix — it is new functionality.Impact and fix
The functional impact is nil — consumers using
^x.y.zranges will receive the new export regardless of whether it ships as a patch or minor bump. However, this violates both the strict semver specification and the repo's own established convention, which can confuse consumers tracking API surface changes across release boundaries. The fix is a one-character change in the changeset file: replacepatchwithminor.