Skip to content

fix: validate clientMetadataUrl at construction time (fail-fast)#1653

Merged
pcarleton merged 6 commits intomodelcontextprotocol:mainfrom
rechedev9:fix/validate-client-metadata-url
Apr 14, 2026
Merged

fix: validate clientMetadataUrl at construction time (fail-fast)#1653
pcarleton merged 6 commits intomodelcontextprotocol:mainfrom
rechedev9:fix/validate-client-metadata-url

Conversation

@rechedev9
Copy link
Copy Markdown
Contributor

@rechedev9 rechedev9 commented Mar 9, 2026

Summary

  • Adds validateClientMetadataUrl() utility that reuses the existing isHttpsUrl() helper to validate clientMetadataUrl early
  • Exports validateClientMetadataUrl() so OAuthClientProvider implementations can call it in their constructors for fail-fast validation
  • Demonstrates usage in the example InMemoryOAuthClientProvider constructor
  • Preserves existing defense-in-depth validation in authInternal() (near-zero cost redundant check)

Motivation

clientMetadataUrl was only validated deep inside authInternal() after network discovery requests had already been made. This meant invalid URL-based client IDs weren't caught until runtime during authentication, producing confusing stack traces far from the configuration source.

This change provides a public utility function that provider implementations can call at construction time, giving developers fail-fast behavior with clear error messages pointing to where the provider was configured.

Since OAuthClientProvider is an interface, the SDK cannot enforce constructor-time validation on external implementations. The exported utility makes it trivial for any implementation to opt in.

Test plan

  • Unit tests for validateClientMetadataUrl() — valid HTTPS URLs, HTTP rejection, root-path rejection, non-URL strings, undefined passthrough, error format
  • Verified existing defense-in-depth validation in authInternal() is untouched
  • All client, core, and server tests pass
  • TypeScript typecheck passes with zero errors

Closes #1159

@rechedev9 rechedev9 requested a review from a team as a code owner March 9, 2026 23:21
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 9, 2026

🦋 Changeset detected

Latest commit: 5fa6d12

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@modelcontextprotocol/client Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Mar 9, 2026

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@1653

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@1653

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@1653

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@1653

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@1653

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@1653

commit: 5fa6d12

@felixweinberger
Copy link
Copy Markdown
Contributor

@claude review

Copy link
Copy Markdown

@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 — straightforward fail-fast validation with comprehensive tests.

Extended reasoning...

Overview

This PR adds a validateClientMetadataUrl() utility function in auth.ts that reuses the existing isHttpsUrl() helper to validate clientMetadataUrl early. It is called from four SDK entry points: StreamableHTTPClientTransport constructor, SSEClientTransport constructor, auth() function, and withOAuth middleware factory. The existing defense-in-depth check in authInternal() is preserved. Total production code added is roughly 15 lines across 4 files, plus a changeset.

Security risks

None. This PR strictly adds input validation at construction time — it does not weaken or bypass any existing checks. The validation function throws an OAuthError for non-HTTPS or root-path URLs, matching the existing behavior deeper in the auth flow. No auth logic, crypto, or permissions are modified.

Level of scrutiny

Low scrutiny is appropriate. The production changes are small, mechanical, and follow the established pattern of calling isHttpsUrl(). The middleware.ts change restructures withOAuth from an implicit return to a block body, which is a straightforward refactor. All entry points have corresponding test coverage.

Other factors

The two bug reports from the bug hunting system are both minor test-quality nits (a test that could pass vacuously, and a duplicate test case) — neither affects production correctness. Test coverage is thorough across all four entry points. No CODEOWNERS file conflicts, no outstanding reviewer comments, and CI appears green.

@rechedev9 rechedev9 force-pushed the fix/validate-client-metadata-url branch from ee6e5bf to 43e9892 Compare March 27, 2026 22:17
@felixweinberger felixweinberger added the auth Issues and PRs related to Authentication / OAuth label Mar 30, 2026
Copy link
Copy Markdown
Member

@pcarleton pcarleton left a comment

Choose a reason for hiding this comment

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

left a comment on #1159

…tion

Export validateClientMetadataUrl() so OAuthClientProvider implementations
can validate clientMetadataUrl in their constructors. Demonstrate usage
in InMemoryOAuthClientProvider example.

Closes modelcontextprotocol#1159
@rechedev9 rechedev9 force-pushed the fix/validate-client-metadata-url branch from 43e9892 to 738ef45 Compare March 30, 2026 16:18
pcarleton
pcarleton previously approved these changes Mar 30, 2026
@pcarleton
Copy link
Copy Markdown
Member

lgtm, thikn we just need tests passing and conflicts fixe

@pcarleton pcarleton enabled auto-merge (squash) March 30, 2026 19:13
Merge upstream main into fix/validate-client-metadata-url,
keeping both validateClientMetadataUrl and determineScope test suites.
auto-merge was automatically disabled March 30, 2026 22:14

Head branch was pushed to by a user without write access

@rechedev9
Copy link
Copy Markdown
Contributor Author

Rebased on main and resolved merge conflicts — the new determineScope tests from upstream and the validateClientMetadataUrl tests now coexist cleanly.

All checks pass locally: build, lint, typecheck, and 894 tests (354 client + 488 core + 52 server).

@felixweinberger
Copy link
Copy Markdown
Contributor

@claude review

@rechedev9
Copy link
Copy Markdown
Contributor Author

rechedev9 commented Apr 1, 2026

Addressed the review feedback:

What changed:

  1. PR description updated — The description was stale from the original implementation (which had entry-point validation calls). Rewrote it to accurately reflect the current approach: exported utility function + example usage, per maintainer feedback.

  2. Test file formatting — Added a missing blank line between the validateClientMetadataUrl and determineScope describe blocks.

Review comments addressed inline:

  • 🔴 Entry-point mismatch → stale description, now fixed. Entry-point calls were intentionally removed per maintainer request.
  • 🟡 Vacuous test → valid on original commit (8a34337). The .toThrow() guard was added during the rebase to f464827. Already resolved.
  • 🟡 Duplicate test → valid on original commit (8a34337). The duplicate was removed during the rebase to f464827. Already resolved.

All pre-push checks pass (typecheck, build, lint). 354 client tests pass.

@felixweinberger
Copy link
Copy Markdown
Contributor

@claude review

Copy link
Copy Markdown

@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 — all prior concerns resolved, code is correct and well-tested.

Extended reasoning...

Overview

This PR adds validateClientMetadataUrl() as a new exported utility function in packages/client/src/client/auth.ts, exports it via index.ts, demonstrates usage in the example InMemoryOAuthClientProvider constructor, and adds comprehensive unit tests. The approach was deliberately scoped to an opt-in utility rather than mandatory SDK-level enforcement, per maintainer guidance.

Security risks

No security risks. The change is purely additive — a validation helper that fails fast on invalid HTTPS URLs. It reuses the existing isHttpsUrl() helper and the defense-in-depth check in authInternal() is preserved.

Level of scrutiny

Low. This is a small, focused addition: one new exported function (~10 lines), one export line, one example call site, and test coverage. The logic is a thin wrapper around an existing helper. No SDK internals or auth flow logic was modified.

Other factors

My previous 🔴 concern (PR description claimed entry-point calls that were not present) has been resolved — the author clarified the description was stale from an earlier iteration, and updated it to accurately reflect the current opt-in utility approach. My two 🟡 test nits (vacuous test, duplicate test) were already fixed in the rebase prior to my comment. The maintainer pcarleton has approved. The only remaining finding is a semver nit in the changeset (patch vs minor for a new public export), which is already noted as an inline comment.

@@ -0,0 +1,9 @@
---
'@modelcontextprotocol/client': patch
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 The changeset declares patch for @modelcontextprotocol/client, but this PR adds a new publicly-exported symbol (validateClientMetadataUrl) to packages/client/src/index.ts. Per semver, adding new backwards-compatible functionality is a minor change. The changeset should declare minor instead of patch.

Extended reasoning...

What the bug is

The changeset file .changeset/fix-validate-client-metadata-url.md uses patch as the bump type for @modelcontextprotocol/client. However, the PR adds validateClientMetadataUrl as a new named export in packages/client/src/index.ts (line 41). Semver defines a minor bump as any backwards-compatible addition of new functionality, and a new public export is exactly that.

The specific code path

The new function validateClientMetadataUrl is defined in packages/client/src/client/auth.ts and explicitly added to the public surface in packages/client/src/index.ts:

export {
    ...
    UnauthorizedError,
    validateClientMetadataUrl   // <-- new public export
} from ./client/auth.js;

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 minor vs patch in changesets. Examining .changeset/ history: expose-auth-server-discovery.md uses minor for adding discoverOAuthServerInfo, OAuthDiscoveryState, and OAuthServerInfo; token-provider-composable-auth.md uses minor for adding AuthProvider, adaptOAuthProvider, handleOAuthUnauthorized, etc. Conversely, patch changesets 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 using patch for an additive API change.

Proof by example

Step-by-step: (1) The PR adds export function validateClientMetadataUrl(...) in auth.ts. (2) It is listed in the named exports in index.ts. (3) The changeset declares this as patch. (4) Per semver §7, a MINOR version increment is required when "new, backwards compatible functionality is introduced to the public API". (5) A patch increment is only correct for "backwards compatible bug fixes" (semver §6). Since validateClientMetadataUrl is 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.z ranges 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: replace patch with minor.

Copy link
Copy Markdown
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

Thanks for addressing! Please make it a minor version instead of patch, otherwise LGTM but will defer to @pcarleton for final approval.

@pcarleton pcarleton enabled auto-merge (squash) April 14, 2026 10:15
@pcarleton pcarleton merged commit 6bec24a into modelcontextprotocol:main Apr 14, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auth Issues and PRs related to Authentication / OAuth

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate clientMetadataUrl at construction time rather than during auth flow (SEP-991/CIMD)

3 participants