fix(client): validate discovered AS metadata by document shape, not well-known path - #2734
fix(client): validate discovered AS metadata by document shape, not well-known path#2734claude[bot] wants to merge 7 commits into
Conversation
…ell-known path discoverAuthorizationServerMetadata() picked its validation schema from which well-known filename resolved, so conforming RFC 8414 metadata served at /.well-known/openid-configuration (permitted by RFC 8414 §5) was parsed against the OIDC Discovery schema and rejected for lacking jwks_uri, subject_types_supported and id_token_signing_alg_values_supported — and the parse threw out of the candidate loop, aborting discovery entirely. - Try the schema implied by the path first, then the other one; a document that fits neither skips to the next candidate URL like the existing 4xx / 502 / CORS failures. - Make OpenIdProviderDiscoveryMetadataSchema a loose object like its component schemas so mixed OIDC/OAuth documents keep their RFC 8414 fields (revocation_endpoint, introspection_endpoint). - Correct the discovery doc comment; issuer validation is unchanged. Fixes #2733 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9hQUruXNrCqCqmMjfMCBZ
🦋 Changeset detectedLatest commit: 2112499 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ry diagnosable - Declare the RFC 8414 revocation/introspection fields on OpenIdProviderDiscoveryMetadataSchema with their OAuth validators so a successful OIDC parse validates them instead of passing them through the loose object's catchall. - On a fallback parse, drop the top-level fields the path-implied schema rejected so values that failed their declared validators (e.g. an unsafe jwks_uri) cannot ride through the fallback schema's passthrough. - Guard response.json() so a 200 with a non-JSON body skips to the next candidate like the other per-candidate failures. - When every candidate fails schema validation, throw an error naming the URL and the schema issues instead of returning undefined and letting auth() silently guess default endpoints. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9hQUruXNrCqCqmMjfMCBZ
…rospection_endpoint - Run the sibling schema's parse on BOTH branches and drop the top-level fields it rejected, so a mixed document served at the oauth-authorization-server path gets the same sanitization as at the openid-configuration path (an unsafe jwks_uri / op_policy_uri / mis-typed subject_types_supported can no longer ride through the OAuth schema's passthrough by choice of well-known path). This also covers service_documentation, which the OIDC shape declares as a plain string while the OAuth schema requires a safe URL. - Validate introspection_endpoint with SafeUrlSchema in OAuthMetadataSchema (matching revocation_endpoint), so the field newly declared on the discovery schema rejects javascript:/data:/vbscript: values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9hQUruXNrCqCqmMjfMCBZ
…ip validated ones - Invalid values in optional metadata fields now drop the field instead of failing the document: the parse retries once with the rejected top-level fields removed, so a relative revocation_endpoint or unsafe introspection_endpoint no longer aborts an auth flow that never uses them; invalid required fields still fail the parse. - The sibling-schema sanitization only removes fields the accepting schema does NOT declare (looseObject passthrough keys): a field the accepting schema itself declared and validated (e.g. OIDC's string service_documentation) is never removed. - The response.json() guard now swallows only SyntaxError (a non-JSON body); network errors reading the body and aborts propagate again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9hQUruXNrCqCqmMjfMCBZ
There was a problem hiding this comment.
This pull request has now been reviewed several times and this review found new issues. Before patching these one by one, step back: would one root-cause fix close several of them? Is the pull request's scope growing with each push? Prefer root-cause fixes, keep scope fixed, and note out-of-scope improvements as follow-ups.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
packages/client/src/client/auth.ts— nit: The JSDoc still promises "undefined if discovery fails" and lists only IssuerMismatchError under @ throws, but the function now throws a plain Error when every candidate fails schema validation (line 1950), so callers coded against the documented undefined-on-failure contract (e.g. discoverAndRequestJwtAuthGrant'sif (!metadata?.token_endpoint)fallback in crossAppAccess.ts:211) get an unexpected exception instead of their fallback path. Fix: update @ returns/@ throws to document the new schema-mismatch throw so the contract matches the implementation, per REVIEW.md's doc-vs-implementation check.Extended reasoning...
The diff adds
if (schemaFailure) throw new Error(...)at packages/client/src/client/auth.ts:1949-1953: when any candidate returned valid JSON that fits neither metadata schema and no candidate succeeds, discovery now throws instead of returning undefined. The JSDoc directly above (updated by this same diff for the fallback-strategy bullets) still says at line 1826 "@ returns Promise resolving to authorization server metadata, or undefined if discovery fails" and at line 1827 "@ throws {IssuerMismatchError}" only. This is exported public API (packages/client/src/index.ts:25). A consumer following the documented contract — e.g. the SDK's own discoverAndRequestJwtAuthGrant (packages/client/src/client/crossAppAccess.ts:209-213), which handles failure viaif (!metadata?.token_endpoint) throw new Error('Failed to discover token endpoint for IdP: ...')— instead gets the raw schema-mismatch Error for a near-miss document; any downstream code that catches/branches on the documented error surface misses it. On base the JSDoc matched more closely (only ZodError/HTTP throws leaked). REVIEW.md's Documentation…Verification: nit — triggered whenever every discovery candidate fails and at least one returned valid JSON fitting neither schema; the doc/code mismatch itself is unconditional in the checkout. Verified: packages/client/src/client/auth.ts:1826 still reads "@ returns Promise resolving to authorization server metadata, or undefined if discovery fails" and line 1827 lists only "@ throws…
| function parseMetadataDroppingInvalidOptionalFields(schema: AuthorizationServerMetadataSchema, json: unknown) { | ||
| const first = schema.safeParse(json); | ||
| if (first.success || typeof json !== 'object' || json === null) { | ||
| return first; | ||
| } | ||
| const failedPresentKeys = new Set<string>(); | ||
| for (const issue of first.error.issues) { | ||
| const key = issue.path[0]; | ||
| if (typeof key === 'string' && key in json) { | ||
| failedPresentKeys.add(key); | ||
| } | ||
| } | ||
| if (failedPresentKeys.size === 0) { | ||
| return first; | ||
| } | ||
| const stripped: Record<string, unknown> = { ...(json as Record<string, unknown>) }; | ||
| for (const key of failedPresentKeys) { | ||
| delete stripped[key]; | ||
| } | ||
| const second = schema.safeParse(stripped); |
There was a problem hiding this comment.
🟡 nit: parseMetadataDroppingInvalidOptionalFields drops ANY invalid optional field, including ones the SDK actively consumes — a malformed registration_endpoint (e.g. relative "/register") is silently removed, so dynamic registration later fails with the misleading "Incompatible auth server: does not support dynamic client registration" instead of the base's ZodError naming the bad field; a mistyped scopes_supported or token_endpoint_auth_methods_supported likewise silently alters scope/auth-method selection. Fix: only drop fields the client never dereferences (allowlist), or attach the dropped-field issues to downstream errors so consumed fields still fail diagnosably.
Extended reasoning...
The inline comment at auth.ts:1899-1902 justifies the drop as safe for "an auth flow that never uses those fields", but the loop at 1979-1993 collects every top-level key with a Zod issue and strips it, with no restriction to unused fields. Trigger: an AS publishes otherwise-valid RFC 8414 metadata with registration_endpoint: "/register" (relative — SafeUrlSchema rejects it; both schemas declare the field, so neither accepts it as passthrough). After merge: first safeParse fails on registration_endpoint, the retry at 1989-1992 succeeds with the field deleted, discovery returns metadata without registration_endpoint. Later, registerClient (auth.ts:2511-2512) sees metadata present but no registration_endpoint and throws "Incompatible auth server: does not support dynamic client registration" — a wrong diagnosis (the server DOES support DCR, its URL was just relative). On base, OAuthMetadataSchema.parse threw a ZodError naming registration_endpoint, pointing the operator at the actual defect. Same mechanism for other SDK-consumed optional fields: scopes_supported or…
Verification: nit — triggered when an authorization server (external, unconstrained input) publishes otherwise-valid metadata with an invalid value in an optional field the SDK actually consumes, e.g. registration_endpoint: "/register" (relative). Mechanism verified in /home/claude/typescript-sdk/packages/client/src/client/auth.ts:1974-1995: parseMetadataDroppingInvalidOptionalFields collects EVERY…
There was a problem hiding this comment.
Standing pat on this one, deliberately, so the field policy stays uniform (95d81ce states it once: URL fields scheme-guarded everywhere; invalid optional fields drop; required fields reject). Two reasons: (1) silent-drop of invalid optional fields is exactly what the OIDC path's z.object strip did on base for every one of these fields, so this is not a new failure mode — and on the OAuth path, base hard-failed the entire auth flow on a relative registration_endpoint, which is strictly worse than a later, admittedly less precise, DCR error; (2) an allowlist of "fields the SDK dereferences" goes stale the day a new consumer lands. Carrying the dropped-field issues into downstream errors is a real improvement but is diagnostics plumbing beyond this fix's scope — noting it as a follow-up candidate (attach dropped-field summaries to the discovery result or an optional warning hook) for the maintainers to weigh.
Generated by Claude Code
…s, JSDoc Unified policy, stated once: every URL-carrying optional field is validated with SafeUrlSchema on both discovery schemas (service_documentation joins the pick, closing the javascript:-scheme hole the OIDC z.string() left); invalid optional fields are dropped, mirroring the strip semantics the OIDC path always had; invalid or missing required fields reject the document. - explicit MetadataParseResult return type on parseMetadataDroppingInvalidOptionalFields (repo convention) - discoverAuthorizationServerMetadata JSDoc: @returns/@throws now state the undefined-vs-throw contract (crossAppAccess's discoverAndRequestJwtAuthGrant checked: it throws its own error on undefined and has no catch-and-continue, so the new, more specific throw propagates the same way) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9hQUruXNrCqCqmMjfMCBZ
|
Round-4 findings addressed in 95d81ce with one policy stated once, per the review's step-back note: every URL-carrying optional field is validated with SafeUrlSchema on both discovery schemas; an invalid optional field is dropped (the strip semantics the OIDC path always had); an invalid or missing required field rejects the document; a document fitting neither schema skips to the next candidate, and exhausting all candidates with only schema failures throws a diagnosable error. On the two body-level notes: the One flagged item is deliberately not changed (rationale in its thread): invalid SDK-consumed optional fields (e.g. a relative Generated by Claude Code |
There was a problem hiding this comment.
This pull request has now been reviewed several times and this review found new issues. Before patching these one by one, step back: would one root-cause fix close several of them? Is the pull request's scope growing with each push? Prefer root-cause fixes, keep scope fixed, and note out-of-scope improvements as follow-ups.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
packages/client/src/client/auth.ts— Theintrospection_endpointtightening (z.string() → SafeUrlSchema in packages/core/src/auth.ts:65) is only softened inside discoverAuthorizationServerMetadata's drop-invalid-optional-fields retry; the deprecated but still-exporteddiscoverOAuthMetadatahere still callsOAuthMetadataSchema.parseraw, so a server whose otherwise-valid metadata carries a relative or emptyintrospection_endpointnow throws a ZodError and aborts the caller's auth flow where base returned the metadata. Fix: give every parse site reached by the tightened schema the same invalid-optional-field tolerance (safeParse + drop), not just the new discovery path.Extended reasoning...
Trigger: an authorization server publishes RFC 8414 metadata with
introspection_endpoint: "/oauth/introspect"(relative) or an empty string — a field the client SDK never dereferences. Path: a consumer still on the deprecated public API calls discoverOAuthMetadata (packages/client/src/client/auth.ts:1698); line 1735 runsOAuthMetadataSchema.parse(await response.json())with no safeParse/drop retry. Because this diff tightensintrospection_endpointfromz.string().optional()toSafeUrlSchema.optional()(packages/core/src/auth.ts:65), the parse now throws a ZodError, propagating out and aborting the auth flow. On base the same document parsed successfully (any string accepted). The PR's mitigation — parseMetadataDroppingInvalidOptionalFields (auth.ts:1981) — is wired only into discoverAuthorizationServerMetadata (lines 1906/1913), so this sibling site is left failing exactly the way the resolved core/auth.ts:65 finding described, but on a path that fix did not cover. Same exposure applies to any external consumer of the publishedOAuthMetadataSchemafrom @…Verification: normal — triggered when an authorization server (external, unconstrained input) publishes otherwise-valid RFC 8414 metadata whose optional
introspection_endpointis a non-absolute-URL string (e.g. relative "/oauth/introspect" or ""), and a consumer uses the still-exported deprecated API. Mechanism verified: the diff tightens packages/core/src/auth.ts:65 from `introspection_endpoint:…
The introspection_endpoint tightening made the deprecated (still exported) discoverOAuthMetadata throw on documents base accepted — its raw OAuthMetadataSchema.parse was the one call site left outside the drop-invalid-optional-fields policy (repo-wide grep: no others). It now reuses parseMetadataDroppingInvalidOptionalFields, throwing the original ZodError only when required fields fail, with a regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9hQUruXNrCqCqmMjfMCBZ
|
Round-5 finding (review body, no inline anchor) fixed in a55e373: the deprecated but still-exported Per the earlier note, this closes the review-driven pushes on this PR: any further findings will be summarized in a comment rather than patched, and the maintainer review has the final word. Generated by Claude Code |
There was a problem hiding this comment.
This pull request has now been reviewed several times and this review found new issues. Before patching these one by one, step back: would one root-cause fix close several of them? Is the pull request's scope growing with each push? Prefer root-cause fixes, keep scope fixed, and note out-of-scope improvements as follow-ups.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
packages/core/src/auth.ts— This PR's scheme-guard sweep (introspection_endpoint → SafeUrlSchema; discovery-schema comment "every URL-carrying field is scheme-guarded uniformly") skips the sibling URL fields in this same module: OAuthProtectedResourceMetadataSchema's jwks_uri, resource_policy_uri and resource_tos_uri stay z.string().url(), which accepts javascript:/data:/vbscript: URLs, so discoverOAuthProtectedResourceMetadata still returns e.g. resource_policy_uri: "javascript:alert(1)" as validated metadata — the exact hole this PR closes for AS metadata. Fix: validate every URL-typed field in this module with SafeUrlSchema (jwks_uri, resource_policy_uri, resource_tos_uri), matching authorization_servers on line 32.Extended reasoning...
Path: discoverOAuthProtectedResourceMetadata (packages/client/src/client/auth.ts) parses the RFC 9728 document with OAuthProtectedResourceMetadataSchema (packages/core/src/auth.ts:30-45). A malicious or compromised MCP resource server serves a PRM document with resource_policy_uri: "javascript:alert(1)" (or the same in resource_tos_uri or jwks_uri). z.string().url() (lines 33, 39, 40) validates any WHATWG-parseable URL including javascript:/data:/vbscript: schemes, so the parse succeeds and auth()/discoverOAuthServerInfo hand the value to providers and host apps as validated resourceMetadata — consent/authorization UIs commonly render the policy and ToS links from PRM, giving a script-scheme navigation target that the SDK's own SafeUrlSchema exists to block (see authorization_servers on line 32 and the fields this PR tightened at line 65 and 133-145). Base behaves identically for the PRM schema, but this PR is precisely the sweep that migrates URL metadata fields in this module from plain string/url validators to SafeUrlSchema (changeset: introspection_endpoint "validated as a safe…
Verification: normal — security-relevant and reachable once merged (a malicious or compromised MCP resource server serving a crafted RFC 9728 protected-resource metadata document is the trigger; that document is untrusted external input). Mechanism verified: packages/core/src/auth.ts:33,39,40 leave
jwks_uri: z.string().url().optional(),resource_policy_uri: z.string().url().optional(), `resource_tos_uri:…
|
Round-6 findings noted but not patched — they expand scope beyond this fix, and the review's own step-back guidance says to note such items as follow-ups:
Final state: head a55e373, CI fully green, client 816/816 + core + core-internal tests, typecheck and lint clean. Ready for human review. Generated by Claude Code |
…deltas in the migration guide - discoverOAuthMetadata now applies the same sibling-schema sanitization as discoverAuthorizationServerMetadata, so OIDC-declared fields it only passes through (jwks_uri et al.) cannot reach callers unvalidated — making the deprecated path consistent with the PR's stated policy. - docs/migration/upgrade-to-v2.md 'OAuth client flow — behavioral changes' gains a bullet for the discovery deltas (validate-by-shape, drop invalid optional fields, URL guards on introspection_endpoint / service_documentation, throw on all-candidates schema failure), per the repo's breaking-changes documentation rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9hQUruXNrCqCqmMjfMCBZ
|
Addendum to the round-6 summary above, on reflection (2112499): two of the three were cheap in-scope completions of this PR's own stated policy, so they are now done rather than declined — the deprecated Final state: head 2112499, client 817/817, typecheck and lint clean. This is the last change on this PR ahead of human review. Generated by Claude Code |
There was a problem hiding this comment.
I reviewed the latest push and found no new bugs — the points raised in earlier review rounds now all appear addressed. Since this rewrites OAuth discovery validation, a security-sensitive path, a human look is still worthwhile before merge.
What was reviewed this round:
- The sibling-schema sanitization added to the deprecated
discoverOAuthMetadatapath, including its new unsafe-jwks_uritest — consistent with the main discovery path. - The new migration-guide bullet in
docs/migration/upgrade-to-v2.mdagainst the implementation — each claim (drop-invalid-optional-fields, URL guards, throw-on-all-candidates-failing, undefined reserved for no-metadata) matches the code. - Edge cases in
dropPassthroughFieldsRejectedBySibling— sibling issues for missing required keys delete nonexistent keys (no-op), and accepting-schema-declared fields are exempt, so validated fields cannot be stripped.
Extended reasoning...
Overview
The PR (7 commits) changes authorization server metadata discovery in packages/client/src/client/auth.ts and the schemas in packages/core/src/auth.ts: discovery now validates fetched documents by shape (primary schema implied by the well-known path, falling back to the sibling schema, per RFC 8414 §5), drops invalid optional fields instead of rejecting the document, sanitizes looseObject passthrough fields the sibling schema declares and rejects, skips non-JSON 200 bodies, and throws a diagnostic error when every JSON candidate fails both schemas. OpenIdProviderDiscoveryMetadataSchema became a looseObject that explicitly declares the RFC 8414 revocation/introspection fields plus service_documentation, and introspection_endpoint gained SafeUrlSchema. Tests (+270 lines), a changeset, and a migration-guide bullet accompany the code.
Security risks
This is squarely security-sensitive: the schemas gate which URLs (token, authorization, revocation, jwks) a client will trust from a remote authorization server, and earlier rounds of this review caught real gaps (unsafe javascript: URLs riding through looseObject passthrough on various paths). The final state closes every gap I identified — both well-known paths and the deprecated discoverOAuthMetadata path now run the same sanitize-and-drop policy, and I re-verified the helper logic: fields the accepting schema declared and validated are never removed, and sibling issues for absent keys are no-ops. This run's multi-agent hunt exited on a dry streak with only duplicate candidates, refuted as repeats of already-reported items.
Level of scrutiny
High. The change alters OAuth client trust decisions and tightens a published Zod schema (introspection_endpoint) that external consumers of @ modelcontextprotocol/core may parse with directly. Six review rounds of iteration also mean the final design (uniform field policy, sibling sanitization, error-vs-undefined contract) deserves a human sign-off on the policy itself, not just its implementation — e.g., whether silently dropping invalid optional fields is the desired posture versus hard-failing. That design question is a judgment call the guidelines reserve for humans, so approval is not appropriate even with zero open findings.
Other factors
The latest commit resolved both findings from my previous review (deprecated-path sanitization; migration-guide documentation), and I confirmed the new migration prose matches the shipped behavior line by line, satisfying the repo's Documentation & Changesets recurring-catch rule. Test coverage is substantial and targets the exact adversarial cases raised earlier (unsafe endpoint values, mixed documents on either path, all-candidates-fail diagnostics, non-JSON bodies). Given the converged state, a short defer acknowledging that the back-and-forth is resolved adds real information for the human who ultimately merges.
1 participant
Requested by Felix Weinberger · Slack thread
Fixes #2733
Problem
discoverAuthorizationServerMetadata()picks its validation schema from which well-known filename resolved, not from the document that came back: everyopenid-configurationcandidate is parsed withOpenIdProviderDiscoveryMetadataSchema, which requiresjwks_uri,subject_types_supportedandid_token_signing_alg_values_supported— fields OpenID Connect Discovery 1.0 requires and RFC 8414 does not. A plain OAuth 2.0 authorization server publishing conforming RFC 8414 metadata at/.well-known/openid-configuration(explicitly permitted by RFC 8414 §5) is therefore rejected — and because the.parse()throws instead of continuing the candidate loop (unlike the existing 4xx/502/CORScontinues), the whole connection fails. Reproduced against@modelcontextprotocol/client@2.0.0with the issue's injected-fetchFnscript before writing the fix (ZodError on exactly those three fields).Changes
packages/client/src/client/auth.ts— validate by document shape: the schema implied by the well-known path is tried first (safeParse), then the other one; a document that fits neither is treated like the other per-candidate failures and the next candidate URL is tried instead of aborting discovery. Issuer validation (RFC 8414 §3.3 / OIDC Discovery §4.3) is unchanged and still runs on fallback-parsed documents. The doc comment now describes the actual behavior (it previously claimed a schema fallback that only the URL ordering had).packages/core/src/auth.ts—OpenIdProviderDiscoveryMetadataSchemais nowz.looseObject(...)like the component schemas it is built from, so a successful OIDC parse no longer strips RFC 8414 fields absent from the OIDC shape (revocation_endpoint,introspection_endpoint, …) — the issue's second finding.@modelcontextprotocol/coreand@modelcontextprotocol/client.Tests
Four regression tests in
packages/client/test/client/auth.test.ts, all verified red without the source change (4 failed / 804 passed) and green with it:openid-configurationpath is returned intact (the issue's repro shape);IssuerMismatchError).Verification
@modelcontextprotocol/client: 808/808 tests;@modelcontextprotocol/core: 2/2;@modelcontextprotocol/core-internal: 1447/1447pnpm typecheck:allclean; ESLint + Prettier clean on the touched packagesGenerated by Claude Code