From 6fa42279fecaba423635072f716bbb2f6f7c77f3 Mon Sep 17 00:00:00 2001 From: LuckTerence <156219145+LuckTerence@users.noreply.github.com> Date: Thu, 3 Sep 2026 22:32:08 +0800 Subject: [PATCH 1/2] fix(client): surface underlying network error via Error.cause on probe failures (#2657) (#2726) Co-authored-by: Konstantin Konstantinov Co-authored-by: Konstantin Konstantinov --- .changeset/brave-donkeys-listen.md | 7 ++++ packages/client/src/client/probeClassifier.ts | 11 +++-- .../client/test/client/probeAuthSeam.test.ts | 3 ++ .../test/client/probeClassifier.test.ts | 19 +++++++++ .../core-internal/src/errors/sdkErrors.ts | 22 ++++++++-- .../test/types/errorSurfacePins.test.ts | 42 +++++++++++++++++++ 6 files changed, 97 insertions(+), 7 deletions(-) create mode 100644 .changeset/brave-donkeys-listen.md diff --git a/.changeset/brave-donkeys-listen.md b/.changeset/brave-donkeys-listen.md new file mode 100644 index 0000000000..c6b92902e1 --- /dev/null +++ b/.changeset/brave-donkeys-listen.md @@ -0,0 +1,7 @@ +--- +'@modelcontextprotocol/core-internal': patch +'@modelcontextprotocol/client': patch +'@modelcontextprotocol/server': patch +--- + +`SdkError` and `SdkHttpError` accept standard `ErrorOptions` as an optional fourth constructor argument and forward it to `Error`, so a wrapped error is reachable through the standard `Error.cause` chain. Version-negotiation probe failures (`SdkErrorCode.EraNegotiationFailed`) now use it: the underlying `TypeError: fetch failed` and the DNS or socket error beneath it surface via `error.cause`, so pino, Sentry, and `util.inspect` render `ENOTFOUND` / `ECONNREFUSED` / `ETIMEDOUT` instead of stopping at the `SdkError` (#2657). The previous `error.data.cause` slot is still populated for compatibility but is deprecated and slated for removal; read `error.cause` instead. diff --git a/packages/client/src/client/probeClassifier.ts b/packages/client/src/client/probeClassifier.ts index e23d011a2a..f73d57b393 100644 --- a/packages/client/src/client/probeClassifier.ts +++ b/packages/client/src/client/probeClassifier.ts @@ -311,9 +311,14 @@ function classifyNetworkError(error: unknown, context: ProbeClassifierContext): } return { kind: 'error', - error: new SdkError(SdkErrorCode.EraNegotiationFailed, `Version negotiation probe failed: ${describeError(error)}`, { - cause: error - }) + error: new SdkError( + SdkErrorCode.EraNegotiationFailed, + `Version negotiation probe failed: ${describeError(error)}`, + // Keep data.cause for existing consumers while also exposing the + // standard Error.cause chain (#2657). + { cause: error }, + { cause: error } + ) }; } diff --git a/packages/client/test/client/probeAuthSeam.test.ts b/packages/client/test/client/probeAuthSeam.test.ts index a347d71cd5..a26276b11a 100644 --- a/packages/client/test/client/probeAuthSeam.test.ts +++ b/packages/client/test/client/probeAuthSeam.test.ts @@ -283,6 +283,9 @@ describe('stamped-seam fault injection (identity-preserving auth outcomes, never expect(out.settled).toBe('rejected'); expect(out.error).toBeInstanceOf(SdkError); expect((out.error as SdkError).code).toBe(SdkErrorCode.EraNegotiationFailed); + // The failure rides the standard cause chain (#2657); the legacy data.cause + // slot is kept populated for compatibility until it is removed. + expect((out.error as SdkError).cause).toBe(netError); expect(((out.error as SdkError).data as { cause?: unknown }).cause).toBe(netError); }); }); diff --git a/packages/client/test/client/probeClassifier.test.ts b/packages/client/test/client/probeClassifier.test.ts index 3a65f240b7..ccc1efd54a 100644 --- a/packages/client/test/client/probeClassifier.test.ts +++ b/packages/client/test/client/probeClassifier.test.ts @@ -270,6 +270,25 @@ describe('row: network outage โ†’ typed connect error (Node)', () => { const verdict = classify({ kind: 'network-error', error: new TypeError('fetch failed') }, { environment: 'node' }); expect(verdict.kind).toBe('error'); }); + + test('the underlying network error is reachable via Error.cause (#2657)', () => { + // Node's fetch wraps the socket/DNS failure: `TypeError: fetch failed` with + // the error that actually names the failure (ENOTFOUND / ECONNREFUSED / + // ETIMEDOUT) on its own `cause`. + const dnsError = Object.assign(new Error('getaddrinfo ENOTFOUND unreachable.invalid'), { code: 'ENOTFOUND' }); + const fetchError = new TypeError('fetch failed', { cause: dnsError }); + const verdict = classify({ kind: 'network-error', error: fetchError }); + expect(verdict.kind).toBe('error'); + if (verdict.kind === 'error') { + // Walking `.cause` (what loggers and error reporters do) must reach the + // error that names the failure instead of dead-ending on the SdkError. + expect(verdict.error.cause).toBe(fetchError); + expect((verdict.error.cause as Error).cause).toBe(dnsError); + // The legacy data.cause slot stays populated too (kept for compatibility, + // slated for removal). + expect(((verdict.error as SdkError).data as { cause?: unknown }).cause).toBe(fetchError); + } + }); }); describe('row: timeout โ€” transport-aware verdict', () => { diff --git a/packages/core-internal/src/errors/sdkErrors.ts b/packages/core-internal/src/errors/sdkErrors.ts index 0bc8f9a1ad..3ad48cc7f6 100644 --- a/packages/core-internal/src/errors/sdkErrors.ts +++ b/packages/core-internal/src/errors/sdkErrors.ts @@ -144,12 +144,23 @@ export class SdkError extends Error { return brandedHasInstance(this, value); } + /** + * @param code - Stable string code identifying the failure ({@linkcode SdkErrorCode}). + * @param message - Human-readable description. + * @param data - Optional structured payload (for example the HTTP status carried by + * {@linkcode SdkHttpError}). Opaque to the SDK: a `cause` key inside `data` is not + * promoted to `Error.cause`. + * @param options - Standard `ErrorOptions`, forwarded to `Error`. Pass the underlying + * failure as `{ cause }` so it is reachable through the `Error.cause` chain that + * loggers and error trackers walk. + */ constructor( public readonly code: SdkErrorCode, message: string, - public readonly data?: unknown + public readonly data?: unknown, + options?: ErrorOptions ) { - super(message); + super(message, options); this.name = 'SdkError'; stampErrorBrands(this, new.target); } @@ -187,8 +198,11 @@ export class SdkHttpError extends SdkError { declare readonly data: SdkHttpErrorData; - constructor(code: SdkErrorCode, message: string, data: SdkHttpErrorData) { - super(code, message, data); + /** + * @param options - Standard `ErrorOptions`, forwarded to `Error` (see {@linkcode SdkError}). + */ + constructor(code: SdkErrorCode, message: string, data: SdkHttpErrorData, options?: ErrorOptions) { + super(code, message, data, options); this.name = 'SdkHttpError'; } diff --git a/packages/core-internal/test/types/errorSurfacePins.test.ts b/packages/core-internal/test/types/errorSurfacePins.test.ts index cc01cf4c57..e2f52c2410 100644 --- a/packages/core-internal/test/types/errorSurfacePins.test.ts +++ b/packages/core-internal/test/types/errorSurfacePins.test.ts @@ -205,6 +205,48 @@ describe('SdkError', () => { expect(error.code).toBe('CLIENT_HTTP_FAILED_TO_OPEN_STREAM'); expect(error.data).toMatchObject({ status: 404 }); }); + + // Cause plumbing (#2657): a wrapped error travels on the standard `Error.cause` + // chain via `ErrorOptions`, never through the opaque `data` payload, so pino / + // Sentry / `util.inspect` reach the root failure without SDK-specific handling. + test('forwards ErrorOptions.cause onto Error.cause without touching data', () => { + const root = new TypeError('fetch failed'); + const error = new SdkError(SdkErrorCode.EraNegotiationFailed, 'Version negotiation probe failed', undefined, { + cause: root + }); + expect(error.cause).toBe(root); + expect(error.data).toBeUndefined(); + // Same non-enumerable own property the native Error constructor installs, + // so serializers that copy enumerable fields do not emit it twice. + expect(Object.getOwnPropertyDescriptor(error, 'cause')?.enumerable).toBe(false); + }); + + test('does not promote a `cause` key inside data to Error.cause', () => { + const root = new Error('boom'); + const error = new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout: 60_000, cause: root }); + expect(error.cause).toBeUndefined(); + expect(error.data).toEqual({ timeout: 60_000, cause: root }); + }); + + test('carries data and cause independently when both are passed', () => { + const root = new Error('boom'); + const error = new SdkError(SdkErrorCode.RequestTimeout, 'Request timed out', { timeout: 60_000 }, { cause: root }); + expect(error.cause).toBe(root); + expect(error.data).toEqual({ timeout: 60_000 }); + }); + + test('SdkHttpError forwards ErrorOptions.cause and keeps the HTTP status', () => { + const root = new Error('socket hang up'); + const error = new SdkHttpError( + SdkErrorCode.ClientHttpFailedToOpenStream, + 'Failed to open SSE stream: Bad Gateway', + { status: 502, statusText: 'Bad Gateway' }, + { cause: root } + ); + expect(error.cause).toBe(root); + expect(error.status).toBe(502); + expect(error.statusText).toBe('Bad Gateway'); + }); }); describe('protocol version constants', () => { From 5119ee7fd7790e335a3fb60ef36f85334e2a6326 Mon Sep 17 00:00:00 2001 From: Hugo Moreira <38438788+hugosmoreira@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:36:13 -0700 Subject: [PATCH 2/2] fix: preserve exact OAuth resource indicators (#2581) Co-authored-by: Konstantin Konstantinov --- .changeset/plenty-plums-sip.md | 5 ++ packages/client/src/client/auth.ts | 39 ++++++++--- packages/client/test/client/auth.test.ts | 87 +++++++++++++++++++++++- 3 files changed, 120 insertions(+), 11 deletions(-) create mode 100644 .changeset/plenty-plums-sip.md diff --git a/.changeset/plenty-plums-sip.md b/.changeset/plenty-plums-sip.md new file mode 100644 index 0000000000..334cf24a33 --- /dev/null +++ b/.changeset/plenty-plums-sip.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/client': patch +--- + +Preserve the exact OAuth resource indicator from protected resource metadata when building authorization and token requests. Previously a pathless `resource` such as `https://example.com` was normalized to `https://example.com/` via `URL.href`, which breaks authorization servers that require the `resource` parameter to match the published value exactly (Microsoft Entra ID rejects it with `AADSTS9010010`). The exported OAuth helpers (`startAuthorization`, `exchangeAuthorization`, `refreshAuthorization`, `fetchToken`, `executeTokenRequest`) now also accept a `string` for `resource`; `selectResourceURL` still returns a `URL`, and a provider's `validateResourceURL` result is used unchanged. Fixes #1968. diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 2086f0677d..4c37339297 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -1243,11 +1243,17 @@ async function authInternal( await provider.saveDiscoveryState?.(freshDiscoveryState); } - const resource: URL | undefined = await selectResourceURL(serverUrl, provider, resourceMetadata); + // Send the metadata's resource indicator verbatim: `selectResourceURL` returns a parsed + // `URL`, and `URL.href` appends "/" to a pathless indicator such as `https://example.com`, + // which exact-match authorization servers reject (#1968). A URL returned by the + // provider's own `validateResourceURL` is used as returned. + const selectedResource = await selectResourceURL(serverUrl, provider, resourceMetadata); + const resource: string | URL | undefined = + selectedResource && resourceMetadata && !provider.validateResourceURL ? resourceMetadata.resource : selectedResource; // Save resource URL for providers that need it (e.g., CrossAppAccessProvider) if (resource) { - await provider.saveResourceUrl?.(String(resource)); + await provider.saveResourceUrl?.(resourceIndicatorToString(resource)); } // Scope selection used consistently for DCR and the authorization request. @@ -1460,6 +1466,17 @@ export function isHttpsUrl(value?: string): boolean { } } +/** + * Selects the RFC 8707 resource indicator for an MCP server: the provider's + * {@linkcode OAuthClientProvider.validateResourceURL | validateResourceURL} result when + * implemented, otherwise the protected resource metadata's `resource` (checked against the + * server URL with `checkResourceAllowed`), or `undefined` when there is no metadata. + * + * The result is a parsed `URL`, so a pathless indicator such as `https://example.com` has + * the `href` `https://example.com/`. {@linkcode auth} therefore sends the metadata string + * verbatim instead of this URL's `href` (#1968); callers that emit the `resource` + * parameter themselves should do the same. + */ export async function selectResourceURL( serverUrl: string | URL, provider: OAuthClientProvider, @@ -2032,6 +2049,10 @@ export async function discoverOAuthServerInfo( }; } +function resourceIndicatorToString(resource: string | URL): string { + return typeof resource === 'string' ? resource : resource.href; +} + /** * Begins the authorization flow with the given server, by generating a PKCE challenge and constructing the authorization URL. */ @@ -2050,7 +2071,7 @@ export async function startAuthorization( redirectUrl: string | URL; scope?: string; state?: string; - resource?: URL; + resource?: string | URL; } ): Promise<{ authorizationUrl: URL; codeVerifier: string }> { let authorizationUrl: URL; @@ -2098,7 +2119,7 @@ export async function startAuthorization( } if (resource) { - authorizationUrl.searchParams.set('resource', resource.href); + authorizationUrl.searchParams.set('resource', resourceIndicatorToString(resource)); } return { authorizationUrl, codeVerifier }; @@ -2147,7 +2168,7 @@ export async function executeTokenRequest( tokenRequestParams: URLSearchParams; clientInformation?: OAuthClientInformationMixed; addClientAuthentication?: OAuthClientProvider['addClientAuthentication']; - resource?: URL; + resource?: string | URL; /** * SEP-1932 / RFC 9449 ยง5: when set, signs a DPoP proof into the token request's `DPoP` * header โ€” the prerequisite for obtaining a DPoP-bound access token. On a `400 @@ -2166,7 +2187,7 @@ export async function executeTokenRequest( }); if (resource) { - tokenRequestParams.set('resource', resource.href); + tokenRequestParams.set('resource', resourceIndicatorToString(resource)); } if (!addClientAuthentication && clientInformation) { @@ -2269,7 +2290,7 @@ export async function exchangeAuthorization( iss?: string; codeVerifier: string; redirectUri: string | URL; - resource?: URL; + resource?: string | URL; addClientAuthentication?: OAuthClientProvider['addClientAuthentication']; /** SEP-1932 / RFC 9449: see {@linkcode executeTokenRequest}'s `dpop` option. */ dpop?: DpopSession; @@ -2321,7 +2342,7 @@ export async function refreshAuthorization( metadata?: AuthorizationServerMetadata; clientInformation: OAuthClientInformationMixed; refreshToken: string; - resource?: URL; + resource?: string | URL; addClientAuthentication?: OAuthClientProvider['addClientAuthentication']; /** SEP-1932 / RFC 9449: see {@linkcode executeTokenRequest}'s `dpop` option. */ dpop?: DpopSession; @@ -2386,7 +2407,7 @@ export async function fetchToken( fetchFn }: { metadata?: AuthorizationServerMetadata; - resource?: URL; + resource?: string | URL; /** Authorization code for the default `authorization_code` grant flow */ authorizationCode?: string; /** diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 7329ded3ed..3ac9c7ddff 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -1496,7 +1496,8 @@ describe('OAuth Authorization', () => { it('calls saveDiscoveryState after discovery when provider implements it', async () => { const saveDiscoveryState = vi.fn(); - const provider = createMockProvider({ saveDiscoveryState }); + const saveResourceUrl = vi.fn(); + const provider = createMockProvider({ saveDiscoveryState, saveResourceUrl }); mockFetch.mockImplementation(url => { const urlString = url.toString(); @@ -1529,6 +1530,9 @@ describe('OAuth Authorization', () => { authorizationServerMetadata: validAuthMetadata }) ); + expect(saveResourceUrl).toHaveBeenCalledWith('https://resource.example.com'); + const authorizationUrl = vi.mocked(provider.redirectToAuthorization).mock.calls[0]![0]; + expect(authorizationUrl.searchParams.get('resource')).toBe('https://resource.example.com'); }); it('restores full discovery state from cache including resource metadata', async () => { @@ -1580,7 +1584,7 @@ describe('OAuth Authorization', () => { const tokenCall = mockFetch.mock.calls.find(call => call[0].toString().includes('/token')); expect(tokenCall).toBeDefined(); const body = tokenCall![1].body as URLSearchParams; - expect(body.get('resource')).toBe('https://resource.example.com/'); + expect(body.get('resource')).toBe('https://resource.example.com'); }); it('re-saves enriched state when partial cache is supplemented with fetched metadata', async () => { @@ -1787,6 +1791,16 @@ describe('OAuth Authorization', () => { expect(codeVerifier).toBe('test_verifier'); }); + it('preserves a string resource indicator without URL normalization', async () => { + const { authorizationUrl } = await startAuthorization('https://auth.example.com', { + clientInformation: validClientInfo, + redirectUrl: 'http://localhost:3000/callback', + resource: 'https://api.example.com' + }); + + expect(authorizationUrl.searchParams.get('resource')).toBe('https://api.example.com'); + }); + it('includes scope parameter when provided', async () => { const { authorizationUrl } = await startAuthorization('https://auth.example.com', { clientInformation: validClientInfo, @@ -3353,6 +3367,75 @@ describe('OAuth Authorization', () => { expect(authUrl.searchParams.get('resource')).toBe('https://api.example.com/'); }); + it('sends a pathless PRM resource verbatim on the authorization and token requests (#1968)', async () => { + // RFC 9728 publishes the resource identifier and RFC 8707 requires it to be + // sent unchanged. `new URL('https://example.com').href` is 'https://example.com/', + // and authorization servers that match the indicator exactly (Microsoft Entra + // ID: AADSTS9010010) reject the extra slash. + mockFetch.mockImplementation(url => { + const urlString = url.toString(); + + if (urlString.includes('/.well-known/oauth-protected-resource')) { + return Promise.resolve({ + ok: true, + status: 200, + json: async () => ({ + resource: 'https://example.com', + authorization_servers: ['https://auth.example.com'], + scopes_supported: ['https://example.com/mcp:tools'] + }) + }); + } else if (urlString.includes('/.well-known/oauth-authorization-server')) { + return Promise.resolve({ + ok: true, + status: 200, + json: async () => ({ + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/authorize', + token_endpoint: 'https://auth.example.com/token', + response_types_supported: ['code'], + code_challenge_methods_supported: ['S256'] + }) + }); + } else if (urlString.includes('/token')) { + return Promise.resolve({ + ok: true, + status: 200, + json: async () => ({ access_token: 'access123', token_type: 'bearer', expires_in: 3600 }) + }); + } + + return Promise.resolve({ ok: false, status: 404 }); + }); + + (mockProvider.clientInformation as Mock).mockResolvedValue({ + client_id: 'test-client', + client_secret: 'test-secret' + }); + (mockProvider.tokens as Mock).mockResolvedValue(undefined); + (mockProvider.saveCodeVerifier as Mock).mockResolvedValue(undefined); + (mockProvider.redirectToAuthorization as Mock).mockResolvedValue(undefined); + (mockProvider.codeVerifier as Mock).mockResolvedValue('verifier123'); + (mockProvider.saveTokens as Mock).mockResolvedValue(undefined); + + // Authorization request: the redirect carries the metadata value byte for byte. + const redirectResult = await auth(mockProvider, { serverUrl: 'https://example.com/mcp' }); + expect(redirectResult).toBe('REDIRECT'); + const authUrl: URL = (mockProvider.redirectToAuthorization as Mock).mock.calls[0]![0]; + expect(authUrl.searchParams.get('resource')).toBe('https://example.com'); + + // Token request: the authorization-code exchange sends the same value. + const exchangeResult = await auth(mockProvider, { + serverUrl: 'https://example.com/mcp', + authorizationCode: 'code123' + }); + expect(exchangeResult).toBe('AUTHORIZED'); + const tokenCall = mockFetch.mock.calls.find(call => call[0].toString().includes('/token')); + expect(tokenCall).toBeDefined(); + const body = tokenCall![1].body as URLSearchParams; + expect(body.get('resource')).toBe('https://example.com'); + }); + it('excludes resource parameter when Protected Resource Metadata is not present', async () => { // Mock metadata discovery where protected resource metadata is not available (404) // but authorization server metadata is available