fix(client,core): tighten OAuth PRM resource validation per RFC 8707 §2#2092
Open
bishnubista wants to merge 1 commit into
Open
Conversation
- Reject Protected Resource Metadata `resource` values that include a fragment component in `OAuthProtectedResourceMetadataSchema`. RFC 8707 §2 hard-requires absolute URIs without a fragment; the prior `z.string().url()` accepted both empty and non-empty fragments. - When `selectResourceURL` rejects a PRM `resource` because the origin or path does not match the MCP server URL, point integrators at the documented escape hatch: `OAuthClientProvider.validateResourceURL`. - Tighten the `validateResourceURL` JSDoc to describe what the default validator actually enforces (URL `resource` values with no fragment that share origin with, and are equal to or a parent path of, the MCP server URL), and to document the override use cases for non-URL RFC 8707 indicators (e.g. `urn:`, `api://`) and cross-origin identifiers. Reference RFC 9728 §3.3 / §7.3 for the strict-default security rationale. Motivated by user reports against the Inspector consumer of this SDK: - modelcontextprotocol/inspector#1304 (URN identifier rejected) - modelcontextprotocol/inspector#855 (Entra ID `api://` identifier rejected) This PR does not relax the origin check; that is a separate design decision left for maintainer input. The intent here is to (a) close a real RFC 8707 §2 gap (fragment validation), and (b) make the existing `validateResourceURL` override discoverable from the error surface so Entra-ID and URN-based deployments can opt in without confusion. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
🦋 Changeset detectedLatest commit: de1800a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
OAuthProtectedResourceMetadataSchema.resourceis currentlyz.string().url(). RFC 8707 §2 requires that resource indicators MUST NOT include a fragment component, butz.string().url()accepts bothhttps://example.com/mcp#section(non-empty fragment) andhttps://example.com/mcp#(empty fragment). PRM publishers can emit non-conformant identifiers and they parse silently.Separately,
selectResourceURLrejects PRMresourcevalues that don't share the MCP server's origin/path — a defensible default per RFC 9728 §3.3 / §7.3 (metadata impersonation defense) — but the error message is opaque. Two downstream Inspector reports (modelcontextprotocol/inspector#1304, modelcontextprotocol/inspector#855) show users hit this wall with legitimate Entra IDapi://and URN identifiers and don't realise the existingOAuthClientProvider.validateResourceURLhook is the supported escape valve.Fix
Three small changes:
packages/core/src/shared/auth.ts— add.refine(value => !value.includes('#'))toOAuthProtectedResourceMetadataSchema.resource. Thevalue.includes('#')check catches both empty and non-empty fragments per RFC 3986 §3.5; percent-encoded%23inside the path/query is correctly preserved.packages/client/src/client/auth.ts— inselectResourceURL, append actionable guidance to the existing mismatch error pointing atOAuthClientProvider.validateResourceURLas the documented opt-in for non-URL RFC 8707 indicators (urn:,api://) or cross-origin identifiers. Same throw point; only the message changes.packages/client/src/client/auth.ts— tighten thevalidateResourceURLJSDoc to describe what the default validator actually enforces, distinguish it from RFC 8707 §2 (looser) and RFC 9728 §3.3 (stricter), and document the override use cases. Add the RFC 9728 §3.3 / §7.3 security rationale for the strict default.Tests
packages/core/test/shared/auth.test.ts— newdescribe('OAuthProtectedResourceMetadataSchema')block covering: no-fragment accept, non-empty-fragment reject, empty-fragment reject, percent-encoded%23accept.packages/client/test/client/auth.test.ts— two new directdiscoverOAuthProtectedResourceMetadatacases for non-empty and empty fragment rejection (added next to the existing schema-validation test, sinceauth()swallows PRM errors and would mask the assertion). One newauth()integration test asserting the actionable error message includesvalidateResourceURLwhen the default rejection path runs.Standards
resourcemismatch defense against metadata impersonation, which is why the strict default inselectResourceURLis correct as policy even though it surprises Entra/URN users.Out of scope (deliberate)
This PR does not relax
selectResourceURL's origin check. Whether to introduce an opt-in compatibility policy for accepting non-httpsPRM identifiers (e.g. anOAuthClientProviderflag, or a constructor option) is a public-API design decision. Per CONTRIBUTING.md that needs maintainer alignment first. Happy to open a follow-up issue framing the design tradeoffs (status-quo + override hook vs. opt-in policy) once direction here is clear.Related
resourcerejectedapi://resourcerejectedBoth are downstream of this SDK's
selectResourceURLbehavior. This PR makes the existing escape valve discoverable from the error surface; the upstream fix (if desired) is the follow-up discussed above.Validation
pnpm --filter @modelcontextprotocol/core test— 556/556 ✅pnpm --filter @modelcontextprotocol/client test— 367/367 ✅pnpm typecheck:all— ✅pnpm lint:all— ✅Changeset:
.changeset/oauth-prm-resource-fragment-validation.md(patch oncore+client).