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

[rechedev9]

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 5e48ff0

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]

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: 5e48ff0

[felixweinberger]

@claude review

[claude]

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.

[pcarleton]

left a comment on #1159

[pcarleton]

lgtm, thikn we just need tests passing and conflicts fixe

[rechedev9]

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]

@claude review

[rechedev9]

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]

@claude review

[claude]

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.

[claude]

🟡 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.

[felixweinberger]

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