Skip to content

feat(client): validate discovery and endpoint URLs in the OAuth flow#2447

Open
felixweinberger wants to merge 1 commit into
mainfrom
fweinberger/oauth-discovery-url-validation
Open

feat(client): validate discovery and endpoint URLs in the OAuth flow#2447
felixweinberger wants to merge 1 commit into
mainfrom
fweinberger/oauth-discovery-url-validation

Conversation

@felixweinberger

Copy link
Copy Markdown
Contributor

The client OAuth flow consumes URLs produced by the previous step of discovery — the WWW-Authenticate header names the metadata document, the metadata names the authorization server, its metadata names the endpoints — and previously used each one as-is. RFC 9728 §7.6 requires clients to verify trust in the authorization server before use, and RFC 8414 §2 constrains issuer syntax. This adds a single URL-policy checkpoint (assertAllowedDiscoveryUrl) applied at each step of the flow.

Motivation and Context

Align client discovery with the URL-handling requirements of RFC 9728 and RFC 8414, and make the policy explicit, consistent, and configurable rather than implicit per call site.

What's included:

  • A policy module (assertAllowedDiscoveryUrl, DiscoveryUrlBlockedError) with documented defaults: https or loopback targets; same-origin metadata fallback with a logged warning when the announced metadata URL is cross-origin; RFC 8414 issuer-syntax check; discovery redirects validated per hop and bounded at 3.
  • A provider hook (validateDiscoveryURL, trustedAuthorizationServers) for deployments that pin their authorization servers.
  • Transport request headers no longer apply to OAuth requests; a new oauthRequestInit transport option configures those explicitly.
  • A lint rule keeping future fetch sites in the auth flow on the same path, tests, a changeset, and a migration-guide entry.

Opened as a draft for discussion of the default policy choices (scheme rule, cross-origin metadata fallback, redirect handling) before finalizing.

How Has This Been Tested?

  • New unit matrix for the policy module (59 tests) and 29 wiring tests covering each call site, plus header-scoping tests on both transports.
  • Full suite, typecheck, lint, docs build, and all four conformance runs pass; exercised end-to-end against a local HTTP server through the built packages.

Breaking Changes

Documented defaults with opt-outs (allowHttpDiscovery, allowCrossOriginResourceMetadata, allowPrivateAddressTargets); transport requestInit headers no longer reach OAuth requests (use oauthRequestInit). See the migration guide entry.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@changeset-bot

changeset-bot Bot commented Jul 7, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0887f38

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

This PR includes changesets to release 2 packages
Name Type
@modelcontextprotocol/client Patch
@modelcontextprotocol/core-internal Patch

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

pkg-pr-new Bot commented Jul 7, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

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

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2447

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2447

@modelcontextprotocol/server

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

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2447

@modelcontextprotocol/express

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

@modelcontextprotocol/fastify

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

@modelcontextprotocol/hono

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

@modelcontextprotocol/node

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

commit: 0887f38

@felixweinberger felixweinberger marked this pull request as ready for review July 7, 2026 12:43
@felixweinberger felixweinberger requested a review from a team as a code owner July 7, 2026 12:43
Comment thread packages/client/src/client/auth.ts Outdated
Comment thread packages/client/src/client/streamableHttp.ts
Comment thread packages/client/src/client/auth.ts
Comment thread packages/client/src/client/auth.ts
Comment thread packages/core-internal/src/shared/discoveryPolicy.ts
@felixweinberger felixweinberger force-pushed the fweinberger/oauth-discovery-url-validation branch from fee5a0b to 47fd105 Compare July 7, 2026 17:05
Comment thread packages/client/test/client/authDiscoveryPolicy.test.ts Fixed
Comment thread packages/client/test/client/authDiscoveryPolicy.test.ts Fixed
Comment thread packages/client/test/client/authDiscoveryPolicy.test.ts Fixed
Comment thread packages/client/src/client/auth.ts
Comment on lines +1229 to +1250
#### Discovery URL policy (RFC 9728 §3 / §7.6, RFC 8414 §2)

Every URL the client OAuth flow derives from the network — the `WWW-Authenticate`
`resource_metadata` URL, the protected-resource metadata's `authorization_servers[0]`
(including restore from cached `discoveryState`), each well-known discovery URL, the
authorization, registration, and token endpoints, and every redirect `Location` on a
discovery request — is now validated by `assertAllowedDiscoveryUrl` **before** any
request is made or the URL is adopted. The default policy: `https:` everywhere (`http:` only for
loopback hosts, matching the SEP-2207 token-endpoint rule), no userinfo, no
loopback/private/link-local IP-literal targets unless the step that produced the URL
(`ctx.producer` — the MCP server for URLs adopted or derived during discovery, or the
authorization server that published an endpoint) is itself local,
RFC 8414 §2 issuer syntax (no query/fragment) for authorization server identifiers, and
same-origin-with-the-server for protected-resource metadata. An explicit metadata URL
carries its provenance (`resourceMetadataUrlSource`): the transports label URLs
extracted from a `WWW-Authenticate` challenge `'www-authenticate'`, and a
non-conformant challenge-relayed URL is ignored with a `console.warn` while discovery
falls back to the SDK's own same-origin well-known derivation; a caller-configured
`resourceMetadataUrl` (`'caller'`, the default) fails closed instead — like every
other violation, with `DiscoveryUrlBlockedError` (a rejected
`authorization_servers[0]` does **not** fall back to the legacy server-derived
authorization server). Discovery

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 new client OAuth surface added by this PR — OAuthClientProvider.discoveryPolicy (with allowHttpDiscovery / allowCrossOriginResourceMetadata / allowPrivateAddressTargets), trustedAuthorizationServers, validateDiscoveryURL, DiscoveryUrlBlockedError, and the requestInitoauthRequestInit header isolation on both client transports — is documented only in this migration-guide entry and the changeset. The client OAuth guide page docs/clients/oauth.md (which documents the OAuthClientProvider surface and transport-driven auth) and docs/clients/machine-auth.md (which documents the Cross-App Access helpers now subject to the URL policy) are not updated, so users configuring OAuth via the guide pages won't discover the new fail-closed defaults, the DiscoveryUrlBlockedError failure mode, or the remedies for previously-working deployments.

Extended reasoning...

What's missing. This PR adds a substantial amount of new public API to the client OAuth flow: the fail-closed discovery URL policy (OAuthClientProvider.discoveryPolicy / AuthOptions.discoveryPolicy with the allowHttpDiscovery, allowCrossOriginResourceMetadata, and allowPrivateAddressTargets opt-outs), the RFC 9728 §7.6 trust members trustedAuthorizationServers and validateDiscoveryURL(ctx), the new DiscoveryUrlBlockedError failure type, and the requestInitoauthRequestInit header isolation on StreamableHTTPClientTransport / SSEClientTransport. The only prose documentation for all of this lives in docs/migration/upgrade-to-v2.md (this section) and the changeset.

Which pages should cover it. docs/clients/oauth.md is the guide page that documents the OAuthClientProvider surface (clientInformation, tokens, discoveryState, validateResourceURL, skipIssuerMetadataValidation) and the transport-driven auth flow via connect() / finishAuth(), including the failure modes callers may see (UnauthorizedError, IssuerMismatchError). A grep across docs/**/*.md shows none of the new symbols — discoveryPolicy, trustedAuthorizationServers, validateDiscoveryURL, oauthRequestInit, DiscoveryUrlBlockedError — appear anywhere except the migration guide; the OAuth guide page is untouched by this PR. Similarly, docs/clients/machine-auth.md documents requestJwtAuthorizationGrant / discoverAndRequestJwtAuthGrant / exchangeJwtAuthGrant / CrossAppAccessProvider without mentioning that those token exchanges now go through the URL policy or that they accept a per-call discoveryPolicy option.

Why it matters. The new defaults are behavior changes that a user following the guide pages will hit directly: the fail-closed policy can reject previously-working deployments (e.g. cross-origin resource_metadata behind a gateway, or a private-IP-literal authorization server), and transport requestInit headers no longer reach OAuth requests at all. The remedies — setting provider.discoveryPolicy, listing trustedAuthorizationServers, or moving gateway headers to oauthRequestInit — are currently only discoverable from the migration guide, which is not the page a user configuring an OAuthClientProvider from scratch reads. The repo's own review checklist asks that new features carry prose documentation (not just JSDoc / migration notes) and that behavior changes be reflected where existing docs describe the old behavior.

Concrete example. A user reading docs/clients/oauth.md today implements a provider and connects with StreamableHTTPClientTransport + requestInit: { headers: { 'X-Api-Key': ... } } against a server whose WWW-Authenticate challenge points at a cross-origin metadata URL. After this PR: (1) the gateway header silently stops being sent on the well-known / token / registration requests (they need oauthRequestInit now), and (2) if the discovery flow adopts a URL that violates the policy, connect() rejects with DiscoveryUrlBlockedError — a failure type the guide never mentions. Nothing on the page they read explains either behavior or its fix.

How to fix. Add a section to docs/clients/oauth.md covering the new provider members (discoveryPolicy, trustedAuthorizationServers, validateDiscoveryURL), the default fail-closed rules and their opt-outs, DiscoveryUrlBlockedError as a failure callers of connect() / finishAuth() may see, and the requestInit vs oauthRequestInit split on the transports. Add a short note to docs/clients/machine-auth.md that the Cross-App Access token exchanges enforce the same URL policy and accept a discoveryPolicy option. This is a docs-completeness gap only — nothing breaks at runtime if merged as-is, and the migration guide/changeset do document the changes — so it is a nit, not a blocker.

Comment thread packages/client/src/client/auth.ts Outdated
@felixweinberger felixweinberger force-pushed the fweinberger/oauth-discovery-url-validation branch from 47fd105 to 78050d0 Compare July 7, 2026 17:31
Comment on lines 2947 to +2952
): Promise<OAuthTokens> {
// Effective policy resolved once at this flow entry: per-call options take
// precedence over the provider's durable policy. The provider's trust hook is
// threaded to the token-endpoint checkpoint the same way.
discoveryPolicy ??= provider.discoveryPolicy;
const validateDiscoveryURL = provider.validateDiscoveryURL?.bind(provider);

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 migration guide added by this PR names fetchToken() as one of the entries a caller drives with "a minimal provider carrying these members" to get RFC 9728 §7.6 trust verification, but fetchToken() only reads provider.discoveryPolicy and provider.validateDiscoveryURL — it never consults provider.trustedAuthorizationServers, so a pin configured only via that list has zero effect on the fetchToken path. Either apply the trust-list match against metadata.issuer ?? authorizationServerUrl in fetchToken(), or reword the guide sentence to scope trustedAuthorizationServers to the flows that adopt an authorization server (auth() / the transports).

Extended reasoning...

What the mismatch is. The migration-guide entry added by this PR (docs/migration/upgrade-to-v2.md, "Discovery URL policy" section) says: "Trust is configured on the provider only — there is no per-call trust override; a caller that needs trust verification constructs a minimal provider carrying these members and drives the flow through auth(), fetchToken(), or a transport (the standalone discovery functions apply the URL policy but run no trust verification)." "These members" are OAuthClientProvider.trustedAuthorizationServers and validateDiscoveryURL. For auth() and the transports the sentence is accurate — but fetchToken() honors only half of the trust configuration.

The code path. fetchToken() (packages/client/src/client/auth.ts:2947-2952) resolves discoveryPolicy ??= provider.discoveryPolicy and binds provider.validateDiscoveryURL, then threads both into executeTokenRequest (lines 2985-2997). It never reads provider.trustedAuthorizationServers. verifyAuthorizationServerTrust — the only place the list is enforced — is called only at the three adoption sites: the cached-state restore in authInternal (line 1244), the PRM-published adoption (line 2501), and the legacy fallback (line 2522) inside discoverOAuthServerInfoInternal. Nothing on the fetchToken path compares the caller-supplied authorizationServerUrl or metadata.issuer/metadata.token_endpoint against the list.

Concrete walkthrough. The documented composition for machine/refresh flows outside auth() is: discover metadata with the standalone functions (which the same guide says run no trust verification), then call fetchToken() with a minimal provider. A deployment that follows this and sets trustedAuthorizationServers: ['https://idp.corp.example'] on that minimal provider gets zero enforcement of the pin: fetchToken(provider, 'https://anything.example', { metadata }) will POST the grant/credentials to whatever token_endpoint the (network-derived) metadata names, as long as the mechanical URL policy passes — only the validateDiscoveryURL hook (which the deployment did not implement, because the guide presents the list as an equivalent declarative alternative) would have gated it.

Why nothing else prevents it. The mechanical policy (assertAllowedDiscoveryUrl) checks URL structure and locality only; it has no knowledge of the trust list. The changeset's own scoping ("the exhaustive trustedAuthorizationServers list applies to every RFC 9728 §7.6 adoption path") is technically consistent because fetchToken performs no adoption — the AS URL is caller-supplied — but the migration-guide sentence explicitly names fetchToken() as a trust-verified entry, which is what a security-conscious integrator will read.

Impact. No runtime breakage for default deployments, and the practical exposure is limited: the caller controls the authorization-server URL on this path, and the validateDiscoveryURL hook member IS honored at the token-endpoint checkpoint. The defect is misleading guide prose promising behavior the code does not ship for one of the two named members.

How to fix. Either (a) run the trustedAuthorizationServers match (e.g. via verifyAuthorizationServerTrust) against metadata.issuer ?? authorizationServerUrl in fetchToken() when the provider configures a list, mirroring how the hook is already threaded; or (b) correct the migration-guide sentence (and cross-check the trustedAuthorizationServers JSDoc) to say the list applies only to flows that adopt an authorization server (auth()/transports) and that fetchToken() honors only the validateDiscoveryURL hook.

Step-by-step proof.

  1. Provider: { trustedAuthorizationServers: ['https://idp.corp.example'], clientInformation, codeVerifier, redirectUrl, ... } (no validateDiscoveryURL hook).
  2. Metadata is obtained via discoverAuthorizationServerMetadata('https://rogue.example') — per the guide, standalone discovery runs no trust verification, so this succeeds and returns token_endpoint: 'https://rogue.example/token'.
  3. Call fetchToken(provider, 'https://rogue.example', { metadata, authorizationCode }).
  4. Lines 2951-2952 pick up discoveryPolicy and validateDiscoveryURL (both undefined here); trustedAuthorizationServers is never read.
  5. executeTokenRequest validates https://rogue.example/token with the mechanical policy only (public https → passes) and POSTs the authorization code and client credentials there — despite the pin naming a different issuer. Expected per the guide sentence: a DiscoveryUrlBlockedError rejection.

Comment thread packages/client/src/client/auth.ts
Comment on lines +42 to +51
// OAuth discovery URL policy (RFC 9728 §7.6 trust verification / RFC 8414 conformance).
// Types are public because the OAuthClientProvider validation hook is typed against them.
export type {
DiscoveryUrlContext,
DiscoveryUrlPolicyOptions,
DiscoveryUrlProducer,
DiscoveryUrlPurpose,
DiscoveryUrlSource
} from '../../shared/discoveryPolicy';
export { assertAllowedDiscoveryUrl, DiscoveryUrlBlockedError } from '../../shared/discoveryPolicy';

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 new client-only OAuth discovery-policy exports (assertAllowedDiscoveryUrl, DiscoveryUrlBlockedError, and the DiscoveryUrl* types) were added to core-internal/public, which the server package re-exports wholesale — so the server's public API now carries client-OAuth-flow policy machinery with no server-side consumer (the PR itself had to add DiscoveryUrlBlockedError to the server's errorBrandConformance CORE_PINNED set). Since the stated justification (typing OAuthClientProvider.validateDiscoveryURL) only requires the client package to expose these, consider exporting them explicitly from packages/client/src/index.ts (alongside ResourceMetadataUrlSource) instead of routing them through the shared public barrel.

Extended reasoning...

What the concern is. packages/core-internal/src/exports/public/index.ts (lines 42–51) now exports assertAllowedDiscoveryUrl, DiscoveryUrlBlockedError, and the DiscoveryUrlContext / DiscoveryUrlPolicyOptions / DiscoveryUrlProducer / DiscoveryUrlPurpose / DiscoveryUrlSource types. Both @modelcontextprotocol/client and @modelcontextprotocol/server do export * from '@modelcontextprotocol/core-internal/public' (packages/client/src/index.ts:111, packages/server/src/index.ts:103), so every symbol added to the curated barrel becomes public API of both packages. The result is that the server package's public surface gains a client-OAuth-flow policy function and error class.

Why the server exposure is unnecessary. Grepping packages/server/src (and the middleware packages) shows zero consumers of any DiscoveryUrl* symbol or assertAllowedDiscoveryUrl — they are consumed exclusively by the client OAuth flow (auth.ts, crossAppAccess.ts, and the client transports). The barrel comment justifies the placement with "the OAuthClientProvider validation hook is typed against them", but that hook lives on the client package, so only the client needs to export the types. The existing auth exports in core-internal/public (OAuthError, OAuthProtectedResourceMetadata, etc.) are different: the server auth implementation genuinely consumes those.

Concrete evidence the exposure is real. Step-by-step: (1) DiscoveryUrlBlockedError is exported from core-internal/public; (2) packages/server/src/index.ts:103 re-exports the barrel wholesale, so import { DiscoveryUrlBlockedError } from '@modelcontextprotocol/server' type-checks and resolves at runtime; (3) the server's errorBrandConformance.test.ts walks the server export surface and found the new error class, which is exactly why this PR adds 'DiscoveryUrlBlockedError' to that test's CORE_PINNED set — the test change is a mechanical acknowledgment that the server surface widened, not a design justification for it.

Why it matters. Per CLAUDE.md's Public API Exports guidance ('Adding a symbol to a package index makes it public API — do so intentionally'; internal helpers should not be added to core-internal/public), every new export should be deliberate, and removing a symbol from a published surface later is a breaking change while not adding it costs nothing today. Shipping a client-only OAuth policy function on the server package invites confusion (server authors may assume it participates in server-side auth) and locks in surface that has no server use case.

How to fix. Move the DiscoveryUrl* type exports plus assertAllowedDiscoveryUrl / DiscoveryUrlBlockedError out of core-internal/public and export them explicitly from packages/client/src/index.ts — the client index already uses this pattern for ResourceMetadataUrlSource (added in this PR) and for other symbols pulled directly off the internal barrel (e.g. withInputRequired). Then drop the DiscoveryUrlBlockedError entry from the server errorBrandConformance CORE_PINNED set (the core-internal errorSurfacePins brand pin stays). Alternatively, if there is a deliberate reason to keep these on the shared barrel (e.g. anticipated server-side reuse of the URL policy), a short note on the barrel comment explaining that would resolve the concern.

Why this is a nit. Nothing malfunctions if the PR merges as-is — this is purely an API-surface/placement concern. It is worth flagging pre-merge because widening a published surface is much easier to prevent than to undo, but it does not block the feature.

The client OAuth flow consumes URLs produced by the previous discovery
step and used them as-is. This adds a URL policy checkpoint applied to
every discovery, endpoint, and redirect target before any request is
issued (RFC 9728 §7.6 trust verification, RFC 8414 issuer syntax,
https-or-loopback with locality anchored on the producing step, fail
closed), routes all auth-flow requests through one fetch path with
explicit redirect handling, scopes transport request headers away from
OAuth requests (with an explicit oauthRequestInit opt-in), and adds
provider-level policy and trust configuration (discoveryPolicy,
trustedAuthorizationServers, validateDiscoveryURL).
@felixweinberger felixweinberger force-pushed the fweinberger/oauth-discovery-url-validation branch from 78050d0 to 0887f38 Compare July 7, 2026 19:39
Comment on lines +157 to +186
/**
* Thrown by the discovery fetch path (`fetchDiscoveryUrl`) when a response is a
* redirect the runtime filtered — Fetch `Response.type === 'opaqueredirect'`
* (status 0, no readable `Location` header), the shape browser runtimes resolve
* a `redirect: 'manual'` request with when the response status is a redirect.
* The redirect target cannot be observed, so it can neither be checked nor
* followed; see `throwIfRedirectFiltered` in `auth.ts` for the mechanism and
* the per-purpose handling the discovery flows apply.
*
* Intentionally **not** part of the package's public exports: the flows that can
* proceed without the redirected document handle this error internally (the
* SDK-derived well-known probes degrade exactly like a 404, and a
* challenge-relayed metadata URL falls back to the well-known derivation), so it
* only surfaces where the request's outcome is required — a caller-configured
* metadata URL, or a token/registration POST.
*/
export class RedirectFilteredResponseError extends OAuthClientFlowError {
static {
Object.defineProperty(this, 'mcpBrand', { value: 'mcp.RedirectFilteredResponseError' });
}

constructor(
/**
* The URL whose response was the filtered redirect. The redirect target
* itself is not observable and is never part of this error.
*/
public readonly url: URL,
/**
* What the request was for. `'redirect-hop'` when the filtered response
* answered a followed redirect hop — {@linkcode RedirectFilteredResponseError.redirectHop | redirectHop}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 RedirectFilteredResponseError surfaces to callers of public API (a caller-configured resourceMetadataUrl, redirected token/registration POSTs via exchangeAuthorization/refreshAuthorization/fetchToken/registerClient, the Cross-App Access helpers, and the transports' 401/finishAuth flows), and withOAuth was changed to rethrow it unwrapped so callers can "branch on" its structured context — but the class is not exported from packages/client/src/index.ts, unlike its siblings (InsecureTokenEndpointError, IssuerMismatchError, OAuthClientFlowError) and DiscoveryUrlBlockedError, so external consumers cannot instanceof-check it. Either export it from the client index (and pin it in errorBrandConformance), or amend the JSDoc/middleware comment/migration prose so they stop implying callers can catch this specific type.

Extended reasoning...

What the inconsistency is. RedirectFilteredResponseError (packages/client/src/client/authErrors.ts:173) carries a JSDoc note saying it is "Intentionally not part of the package's public exports" because the flows that can proceed without the redirected document handle it internally. But the same JSDoc, the changeset, and the migration guide all state it does surface to callers on several paths: a caller-configured resourceMetadataUrl whose response is a filtered redirect, and any redirected/filtered token or registration POST. Those paths are reached from public entry points added or changed in this PR — auth(), discoverOAuthProtectedResourceMetadata, discoverOAuthServerInfo, exchangeAuthorization, refreshAuthorization, fetchToken, registerClient, the Cross-App Access helpers, and the transports' 401/finishAuth flows.\n\nThe code paths. throwIfRedirectFiltered throws it from fetchDiscoveryUrl, which now backs every OAuth-flow request (the eslint rule added in this PR enforces that). executeTokenRequest and registerClientInternal route their POSTs through it, so a filtered redirect on the token or registration endpoint propagates out of exchangeAuthorization/refreshAuthorization/fetchToken/registerClient and out of the transports' code-exchange and refresh legs. isFilteredCallerMetadataUrl deliberately rethrows it for a caller-configured metadata URL in authInternal and discoverOAuthServerInfoInternal, and auth()'s refresh branch rethrows it explicitly. Most tellingly, withOAuth (packages/client/src/client/middleware.ts:93-97) was changed in this PR to propagate it unwrapped with the comment that these "typed URL-policy and redirect-handling failures carry structured context callers branch on".\n\nWhy callers cannot actually branch on it. packages/client/src/index.ts:48-55 exports every sibling in the same error family — AuthorizationServerMismatchError, InsecureTokenEndpointError, InsufficientScopeError, IssuerMismatchError, OAuthClientFlowError, RegistrationRejectedError — and the PR exports DiscoveryUrlBlockedError via the core public barrel, but RedirectFilteredResponseError appears nowhere on the client index or the core public barrel. A consumer of @modelcontextprotocol/client therefore cannot write error instanceof RedirectFilteredResponseError; the PR's own tests can only reach it via a deep relative import ('../../src/client/authErrors'), which external consumers cannot do. The authErrors module docstring says these dedicated error classes exist "so callers can instanceof-dispatch on the failure mode without string-matching messages", which this class cannot deliver as shipped.\n\nConcrete walkthrough. 1) A consumer follows the migration guide and calls fetchToken(provider, asUrl, { metadata }) in a browser runtime. 2) The token endpoint answers with a redirect; the browser filters it into an opaqueredirect (status 0). 3) throwIfRedirectFiltered throws RedirectFilteredResponseError with purpose: 'token-endpoint' and the endpoint URL — exactly the structured context the PR built for this failure. 4) The consumer wants to distinguish "endpoint misconfigured to redirect" from other auth failures, but the only tools available are catching the base OAuthClientFlowError (which also matches five unrelated failure modes) or string-matching error.name/error.message — the thing the error-class design explicitly set out to avoid.\n\nOn the refutation. One reviewer argued the non-export is deliberate (the JSDoc says so), that not exporting is the conservative default, and that callers can duck-type on error.name or catch the base class. That is fair as far as runtime behavior goes — nothing breaks if this merges as-is, which is why this is a nit rather than a blocker. But the design as shipped is not internally coherent: the middleware comment justifies unwrapped propagation by saying callers branch on the structured context, the migration guide and changeset describe the caller-observable failure, and the sibling DiscoveryUrlBlockedError was exported for precisely the same reason ("a single failure type callers observe"). If the non-export stands, the prose should stop implying instanceof-dispatch is available for this type; if the prose stands, the class should be exported.\n\nHow to fix. Either (a) export RedirectFilteredResponseError from packages/client/src/index.ts alongside its siblings and add its brand to the client errorBrandConformance pinned set, or (b) keep it internal and adjust the JSDoc "only surfaces where..." note, the withOAuth comment, and the changeset/migration wording so they direct callers to catch OAuthClientFlowError (or DiscoveryUrlBlockedError where applicable) instead of implying this specific type is catchable. Option (a) is a two-line change and matches how every other member of the family is handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants