Skip to content

Commit 7b8f3ec

Browse files
fix(client): brand the OAuth flow error family and UnauthorizedError; enforce participation with conformance tests
The branding PR covered the classes duplicated across the client and server bundles but left seven client-only error classes unbranded — OAuthClientFlowError and its five subclasses, and UnauthorizedError — even though the docs steer callers into instanceof on them (including the IssuerMismatchError mix-up-attack path). Two copies of the client package (version skew, bundler duplication) left those checks silently dead. Brand them with the same static-block pattern, and give UnauthorizedError a real .name. Make participation enforceable instead of remembered: per-package errorBrandConformance tests walk the export surface and fail naming any exported Error subclass without an own brand, pin package-local brand strings (core brands stay pinned in errorSurfacePins.test.ts — single ownership), and verify cross-copy matching with distinct-copy premise guards. Fix the version-negotiation probe's dead auth-gated-server branch: the name-string check could never fire because UnauthorizedError did not set .name. Match by brand-aware instanceof with the name check retained as the fallback for skewed copies and middleware-authored auth errors; pin the classification fork (legacy fallback vs typed EraNegotiationFailed) with tests. Also: cover the changeset's abort-reason passthrough behavior (foreign-bundle SdkError rethrown as-is), document the brand evolution escape hatch and the participation criterion in crossBundleBrand.ts, and give the migration guide's fine print a concrete identity-free recognition recipe for skewed rollouts.
1 parent dbf9b54 commit 7b8f3ec

11 files changed

Lines changed: 421 additions & 5 deletions

File tree

.changeset/cross-bundle-error-instanceof.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@
33
'@modelcontextprotocol/server': patch
44
---
55

6-
`instanceof` on the SDK error classes (`ProtocolError` and its typed subclasses, `SdkError`/`SdkHttpError`, `OAuthError`, and the client's `SseError`) now works across separately bundled copies of the SDK. The classes match by a stable brand (via `Symbol.hasInstance` and a registry symbol) instead of prototype identity, so a process that uses both `@modelcontextprotocol/client` and `@modelcontextprotocol/server` - a gateway, host, or in-process test - can check errors constructed by either package against the class re-exported by the other. Ordinary prototype-based `instanceof` is preserved as a fallback; user-defined subclasses keep plain prototype semantics. Notes: cross-bundle matching requires both copies to be at or after this release; brands assert identity, not field shape, across versions - keep reading fields defensively. As a side effect, a foreign-bundle `SdkError` used as an abort reason is now rethrown as-is instead of being wrapped as a `RequestTimeout`.
6+
`instanceof` on the SDK error classes (`ProtocolError` and its typed subclasses, `SdkError`/`SdkHttpError`, `OAuthError`, and the client's `SseError`, `UnauthorizedError`, and OAuth-client-flow error family — `OAuthClientFlowError` and its subclasses) now works across separately bundled copies of the SDK. The classes match by a stable brand (via `Symbol.hasInstance` and a registry symbol) instead of prototype identity, so a process that uses both `@modelcontextprotocol/client` and `@modelcontextprotocol/server` - a gateway, host, or in-process test - can check errors constructed by either package against the class re-exported by the other. Ordinary prototype-based `instanceof` is preserved as a fallback; user-defined subclasses keep plain prototype semantics. Notes: cross-bundle matching requires both copies to be at or after this release; brands assert identity, not field shape, across versions - keep reading fields defensively. As a side effect, a foreign-bundle `SdkError` used as an abort reason is now rethrown as-is instead of being wrapped as a `RequestTimeout`. Also: `UnauthorizedError` now sets `error.name` to `'UnauthorizedError'` (previously `'Error'`), and per-package conformance tests enforce that every exported error class participates in branding. Version-negotiation probing now recognizes `UnauthorizedError` (previously a dead name-string check) and classifies an auth-gated server as an HTTP 401 outcome — taking the conservative legacy fallback instead of failing negotiation with a network error.

docs/migration/upgrade-to-v2.md

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -823,9 +823,23 @@ The SDK now distinguishes three error kinds:
823823
3. **`SdkHttpError`** (extends `SdkError`) — HTTP transport errors with typed `.status`
824824
and `.statusText`.
825825
826-
These classes (and `OAuthError`) brand-match under `instanceof`, so checks work across
826+
These classes (and `OAuthError`, the client's `SseError`, `UnauthorizedError`, and the
827+
OAuth-client-flow error family) brand-match under `instanceof`, so checks work across
827828
separately bundled copies of the SDK — e.g. a process using both
828-
`@modelcontextprotocol/client` and `@modelcontextprotocol/server`.
829+
`@modelcontextprotocol/client` and `@modelcontextprotocol/server`. Fine print:
830+
831+
- **Version skew** — matching needs *both* copies at a brand-aware release; against an
832+
older copy, behavior degrades to plain prototype `instanceof` (false across bundles).
833+
During mixed-version rollouts, recognize errors without class identity: match
834+
`error.name` plus the class's discriminant field (`code`, `status`), or reconstruct
835+
typed protocol errors with `ProtocolError.fromError(code, message, data)`.
836+
- **Worker boundaries** — `structuredClone`/`postMessage` drop the (symbol-keyed) brand,
837+
so a rehydrated error no longer brand-matches; recognize forwarded errors by
838+
`code`/`data` instead.
839+
- **Brands assert identity, not shape** — a matched instance from another SDK version
840+
may lack newer fields; read fields defensively.
841+
- **Re-bundling with property mangling** (`mangle.props` and similar) breaks the brand
842+
statics; default esbuild/webpack/terser settings are safe.
829843
830844
The codemod renames `McpError` → `ProtocolError`, `ErrorCode` → `ProtocolErrorCode`
831845
(routing `RequestTimeout` / `ConnectionClosed` to `SdkErrorCode`), and

packages/client/src/client/auth.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ import type {
1313
StoredOAuthTokens
1414
} from '@modelcontextprotocol/core-internal';
1515
import {
16+
brandedHasInstance,
1617
checkResourceAllowed,
18+
stampErrorBrands,
1719
LATEST_PROTOCOL_VERSION,
1820
OAuthClientInformationFullSchema,
1921
OAuthError,
@@ -486,8 +488,18 @@ export interface OAuthDiscoveryState extends OAuthServerInfo {
486488
export type AuthResult = 'AUTHORIZED' | 'REDIRECT';
487489

488490
export class UnauthorizedError extends Error {
491+
static {
492+
Object.defineProperty(this, 'mcpBrand', { value: 'mcp.UnauthorizedError' });
493+
}
494+
495+
static override [Symbol.hasInstance](value: unknown): boolean {
496+
return brandedHasInstance(this, value);
497+
}
498+
489499
constructor(message?: string) {
490500
super(message ?? 'Unauthorized');
501+
this.name = 'UnauthorizedError';
502+
stampErrorBrands(this, new.target);
491503
}
492504
}
493505

packages/client/src/client/authErrors.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
*/
88

99
import type { OAuthClientMetadata } from '@modelcontextprotocol/core-internal';
10+
import { brandedHasInstance, stampErrorBrands } from '@modelcontextprotocol/core-internal';
1011

1112
/**
1213
* Base class for the OAuth-client-flow error family. Concrete subclasses are
@@ -19,9 +20,18 @@ import type { OAuthClientMetadata } from '@modelcontextprotocol/core-internal';
1920
* hook and will not match anything until the first behavior change ships.
2021
*/
2122
export class OAuthClientFlowError extends Error {
23+
static {
24+
Object.defineProperty(this, 'mcpBrand', { value: 'mcp.OAuthClientFlowError' });
25+
}
26+
27+
static override [Symbol.hasInstance](value: unknown): boolean {
28+
return brandedHasInstance(this, value);
29+
}
30+
2231
constructor(message: string) {
2332
super(message);
2433
this.name = new.target.name;
34+
stampErrorBrands(this, new.target);
2535
}
2636
}
2737

@@ -45,6 +55,10 @@ export class OAuthClientFlowError extends Error {
4555
* end users. The values are JSON-encoded in the message to neutralize log-injection.
4656
*/
4757
export class IssuerMismatchError extends OAuthClientFlowError {
58+
static {
59+
Object.defineProperty(this, 'mcpBrand', { value: 'mcp.IssuerMismatchError' });
60+
}
61+
4862
/** Which check failed — metadata echo (RFC 8414 §3.3) or authorization-response `iss` (RFC 9207). */
4963
readonly kind: 'metadata' | 'authorization_response';
5064
/** The issuer the client expected (from validated metadata / discovery input). */
@@ -79,6 +93,10 @@ export class IssuerMismatchError extends OAuthClientFlowError {
7993
* path.
8094
*/
8195
export class RegistrationRejectedError extends OAuthClientFlowError {
96+
static {
97+
Object.defineProperty(this, 'mcpBrand', { value: 'mcp.RegistrationRejectedError' });
98+
}
99+
82100
/** HTTP status code returned by the registration endpoint. */
83101
public readonly status: number;
84102
/** Raw response body text (typically an RFC 7591 error JSON document). */
@@ -102,6 +120,10 @@ export class RegistrationRejectedError extends OAuthClientFlowError {
102120
* of falling through to a fresh `/authorize` redirect.
103121
*/
104122
export class InsecureTokenEndpointError extends OAuthClientFlowError {
123+
static {
124+
Object.defineProperty(this, 'mcpBrand', { value: 'mcp.InsecureTokenEndpointError' });
125+
}
126+
105127
/** The token endpoint URL that was rejected. */
106128
public readonly tokenEndpoint: string;
107129

@@ -145,6 +167,10 @@ export class InsecureTokenEndpointError extends OAuthClientFlowError {
145167
* client credentials are protected structurally by the `issuer` stamp instead.
146168
*/
147169
export class AuthorizationServerMismatchError extends OAuthClientFlowError {
170+
static {
171+
Object.defineProperty(this, 'mcpBrand', { value: 'mcp.AuthorizationServerMismatchError' });
172+
}
173+
148174
constructor(
149175
/** The issuer recorded in `discoveryState()` when the authorization redirect was issued. */
150176
public readonly recordedIssuer: string,
@@ -160,6 +186,10 @@ export class AuthorizationServerMismatchError extends OAuthClientFlowError {
160186
}
161187

162188
export class InsufficientScopeError extends OAuthClientFlowError {
189+
static {
190+
Object.defineProperty(this, 'mcpBrand', { value: 'mcp.InsufficientScopeError' });
191+
}
192+
163193
/** The `scope` value from the `WWW-Authenticate` challenge — the scopes the resource server says are required. */
164194
readonly requiredScope?: string;
165195
/** The `resource_metadata` URL from the `WWW-Authenticate` challenge, if present. */

packages/client/src/client/versionNegotiation.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
SUPPORTED_MODERN_PROTOCOL_VERSIONS
2626
} from '@modelcontextprotocol/core-internal';
2727

28+
import { UnauthorizedError } from './auth';
2829
import type { ProbeEnvironment, ProbeOutcome, ProbeTransportKind, ProbeVerdict } from './probeClassifier';
2930
import { classifyProbeOutcome } from './probeClassifier';
3031

@@ -293,9 +294,17 @@ function normalizeReply(reply: RawProbeReply, timeoutMs: number): ProbeOutcome {
293294
const text = (error.data as { text?: unknown } | undefined)?.text;
294295
return { kind: 'http-error', status: error.data.status, body: typeof text === 'string' ? text : undefined };
295296
}
296-
if (error instanceof Error && error.name === 'UnauthorizedError') {
297+
const isUnauthorized =
298+
error instanceof UnauthorizedError ||
299+
// Name fallback for auth errors the brand cannot reach: an
300+
// UnauthorizedError from a differently bundled SDK copy at a
301+
// skewed version, or an auth middleware's own class.
302+
(error instanceof Error && error.name === 'UnauthorizedError');
303+
if (isUnauthorized) {
297304
// Auth-gated server: not era evidence — the conservative legacy
298305
// fallback re-runs the auth flow through the plain connect path.
306+
// (The pre-branding name-string check alone could never fire
307+
// for the SDK's own class — it did not set `.name`.)
299308
return { kind: 'http-error', status: 401 };
300309
}
301310
return { kind: 'network-error', error };
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
import { describe, expect, it, vi } from 'vitest';
2+
3+
import * as barrel from '../../src/index';
4+
5+
/**
6+
* Enforces the cross-bundle branding participation criterion from
7+
* `core-internal/src/errors/crossBundleBrand.ts`: every error class exported
8+
* from this package's public surface must carry an own `mcpBrand` static and
9+
* resolve `instanceof` through a branded `Symbol.hasInstance`.
10+
*
11+
* This is the infrastructure that replaces "remember the static block": adding
12+
* a new exported error class without branding turns this test red naming the
13+
* class, instead of shipping a subclass whose cross-bundle `instanceof`
14+
* silently returns false while its parent still matches.
15+
*/
16+
17+
/** Error classes exported on purpose without a brand. Justify every entry. */
18+
const UNBRANDED_ALLOWLIST: ReadonlySet<string> = new Set([]);
19+
20+
function exportedErrorClasses(mod: Record<string, unknown>): Array<[string, Function]> {
21+
return Object.entries(mod)
22+
.filter((entry): entry is [string, Function] => {
23+
const v = entry[1];
24+
return typeof v === 'function' && v !== Error && v.prototype instanceof Error;
25+
})
26+
.sort(([a], [b]) => a.localeCompare(b));
27+
}
28+
29+
describe('error brand conformance (client export surface)', () => {
30+
const classes = exportedErrorClasses(barrel as Record<string, unknown>);
31+
32+
it('finds the export surface (guards against a broken walker)', () => {
33+
expect(classes.length).toBeGreaterThanOrEqual(15);
34+
expect(classes.map(([n]) => n)).toContain('ProtocolError');
35+
expect(classes.map(([n]) => n)).toContain('OAuthClientFlowError');
36+
});
37+
38+
it('every exported error class is branded (or explicitly allowlisted)', () => {
39+
const unbranded = classes
40+
.filter(([name]) => !UNBRANDED_ALLOWLIST.has(name))
41+
.filter(([, cls]) => !Object.prototype.hasOwnProperty.call(cls, 'mcpBrand'))
42+
.map(([name]) => name);
43+
expect(
44+
unbranded,
45+
`unbranded exported error classes (add the static mcpBrand block, or allowlist with a justification): ${unbranded.join(', ')}`
46+
).toEqual([]);
47+
});
48+
49+
it('every branded class resolves instanceof through a branded hasInstance', () => {
50+
const missing = classes
51+
.filter(([, cls]) => Object.prototype.hasOwnProperty.call(cls, 'mcpBrand'))
52+
.filter(([, cls]) => (cls as never as Record<symbol, unknown>)[Symbol.hasInstance] === Function.prototype[Symbol.hasInstance])
53+
.map(([name]) => name);
54+
expect(missing, `branded classes whose hierarchy root does not install brandedHasInstance: ${missing.join(', ')}`).toEqual([]);
55+
});
56+
57+
// Core-internal brand strings are pinned once, in core-internal's
58+
// errorSurfacePins.test.ts — this test only asserts the core re-export SET
59+
// so a rename there is not double-maintained here.
60+
const CORE_PINNED = new Set([
61+
'MissingRequiredClientCapabilityError',
62+
'OAuthError',
63+
'ProtocolError',
64+
'ResourceNotFoundError',
65+
'SdkError',
66+
'SdkHttpError',
67+
'UnsupportedProtocolVersionError',
68+
'UrlElicitationRequiredError'
69+
]);
70+
71+
it('pins every client-local brand string (renaming one severs cross-version matching — must be deliberate)', () => {
72+
const brands = Object.fromEntries(
73+
classes
74+
.filter(([name, cls]) => !CORE_PINNED.has(name) && Object.prototype.hasOwnProperty.call(cls, 'mcpBrand'))
75+
.map(([name, cls]) => [name, (cls as never as { mcpBrand: string }).mcpBrand])
76+
);
77+
expect(brands).toEqual({
78+
AuthorizationServerMismatchError: 'mcp.AuthorizationServerMismatchError',
79+
InsecureTokenEndpointError: 'mcp.InsecureTokenEndpointError',
80+
InsufficientScopeError: 'mcp.InsufficientScopeError',
81+
IssuerMismatchError: 'mcp.IssuerMismatchError',
82+
OAuthClientFlowError: 'mcp.OAuthClientFlowError',
83+
RegistrationRejectedError: 'mcp.RegistrationRejectedError',
84+
SseError: 'mcp.SseError',
85+
UnauthorizedError: 'mcp.UnauthorizedError'
86+
});
87+
});
88+
89+
it('core re-exported error classes are exactly the set pinned in errorSurfacePins.test.ts', () => {
90+
const coreExported = classes.map(([name]) => name).filter(name => CORE_PINNED.has(name));
91+
expect(coreExported).toEqual([...CORE_PINNED].sort((a, b) => a.localeCompare(b)));
92+
});
93+
94+
it('the OAuth flow family and UnauthorizedError match across module copies', async () => {
95+
vi.resetModules();
96+
const foreignAuthErrors = await import('../../src/client/authErrors');
97+
const foreignAuth = await import('../../src/client/auth');
98+
// Guard the premise: a cached module would make every check below pass
99+
// trivially through prototype identity.
100+
expect(foreignAuthErrors.IssuerMismatchError).not.toBe(barrel.IssuerMismatchError);
101+
expect(foreignAuth.UnauthorizedError).not.toBe(barrel.UnauthorizedError);
102+
103+
const issuer = new foreignAuthErrors.IssuerMismatchError('metadata', 'a', 'b');
104+
expect(issuer instanceof barrel.IssuerMismatchError).toBe(true);
105+
expect(issuer instanceof barrel.OAuthClientFlowError).toBe(true);
106+
expect(issuer instanceof barrel.UnauthorizedError).toBe(false);
107+
108+
const unauthorized = new foreignAuth.UnauthorizedError('nope');
109+
expect(unauthorized instanceof barrel.UnauthorizedError).toBe(true);
110+
expect(unauthorized.name).toBe('UnauthorizedError');
111+
});
112+
});

packages/client/test/client/versionNegotiation.test.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
} from '@modelcontextprotocol/core-internal';
1919
import { describe, expect, test } from 'vitest';
2020

21+
import { UnauthorizedError } from '../../src/client/auth';
2122
import { Client } from '../../src/client/client';
2223
import type { StreamableHTTPClientTransportOptions } from '../../src/client/streamableHttp';
2324
import type { StdioServerParameters } from '../../src/client/stdio';
@@ -706,3 +707,62 @@ describe('era scope discipline', () => {
706707
expect(noCachedEra).toBe(true);
707708
});
708709
});
710+
711+
/* ------------------------------------------------------------------------- *
712+
* Probe send-error classification: auth-gated servers take the legacy
713+
* fallback; other send failures stay typed negotiation errors.
714+
* ------------------------------------------------------------------------- */
715+
716+
describe('probe send-error classification', () => {
717+
/** Rejects the probe send with `probeError`, then serves legacy initialize. */
718+
class AuthGatedTransport extends ScriptedTransport {
719+
constructor(private readonly probeError: Error) {
720+
super(legacyServerScript);
721+
}
722+
723+
override async send(message: JSONRPCMessage): Promise<void> {
724+
if (isJSONRPCRequest(message) && message.method === 'server/discover') {
725+
throw this.probeError;
726+
}
727+
await super.send(message);
728+
}
729+
}
730+
731+
test('UnauthorizedError from the probe send is an auth-gated server — legacy fallback on the same connection', async () => {
732+
const transport = new AuthGatedTransport(new UnauthorizedError());
733+
const client = new Client({ name: 'c', version: '0' }, { versionNegotiation: { mode: 'auto' } });
734+
735+
await client.connect(transport);
736+
737+
expect(requests(transport.sent).some(r => r.method === 'initialize')).toBe(true);
738+
expect(client.getNegotiatedProtocolVersion()).toBe('2025-11-25');
739+
});
740+
741+
test("a foreign auth error matching only by name === 'UnauthorizedError' also takes the legacy fallback", async () => {
742+
class ForeignUnauthorizedError extends Error {
743+
override readonly name = 'UnauthorizedError';
744+
}
745+
const transport = new AuthGatedTransport(new ForeignUnauthorizedError('401 from middleware'));
746+
const client = new Client({ name: 'c', version: '0' }, { versionNegotiation: { mode: 'auto' } });
747+
748+
await client.connect(transport);
749+
750+
expect(requests(transport.sent).some(r => r.method === 'initialize')).toBe(true);
751+
expect(client.getNegotiatedProtocolVersion()).toBe('2025-11-25');
752+
});
753+
754+
test('a plain send failure stays a typed negotiation error — no fallback runs', async () => {
755+
const transport = new AuthGatedTransport(new Error('connection refused'));
756+
const client = new Client({ name: 'c', version: '0' }, { versionNegotiation: { mode: 'auto' } });
757+
758+
const rejection = await client.connect(transport).then(
759+
() => {
760+
throw new Error('connect unexpectedly resolved');
761+
},
762+
(e: unknown) => e
763+
);
764+
expect(rejection).toBeInstanceOf(SdkError);
765+
expect((rejection as SdkError).code).toBe(SdkErrorCode.EraNegotiationFailed);
766+
expect(requests(transport.sent).some(r => r.method === 'initialize')).toBe(false);
767+
});
768+
});

0 commit comments

Comments
 (0)