Skip to content

Commit a4b48e1

Browse files
committed
fix(client): accumulate OAuth scopes on 401/403 instead of overwriting
Replace direct this._scope = scope assignments with mergeScopes() that unions existing and incoming scope strings via Set deduplication. Prevents infinite re-auth loops when servers use per-operation progressive authorization (RFC 6750 §3.1). Fixes #1582
1 parent e86b183 commit a4b48e1

5 files changed

Lines changed: 325 additions & 6 deletions

File tree

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
'@modelcontextprotocol/client': patch
3+
---
4+
5+
Fix: accumulate OAuth scopes on 401/403 instead of overwriting
6+
7+
When an HTTP transport receives a 401 or 403 with a `WWW-Authenticate` header
8+
containing new scopes, the scopes are now merged with any previously-acquired scopes
9+
rather than replacing them. The previous behaviour could cause an infinite re-auth loop
10+
where the client repeatedly lost its original scopes each time it attempted to upscope.

packages/client/src/client/sse.ts

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,21 @@ import { EventSource } from 'eventsource';
66
import type { AuthProvider, OAuthClientProvider } from './auth.js';
77
import { adaptOAuthProvider, auth, extractWWWAuthenticateParams, isOAuthClientProvider, UnauthorizedError } from './auth.js';
88

9+
/**
10+
* Merges two space-separated OAuth scope strings into a deduplicated union.
11+
* Returns undefined when the resulting set is empty.
12+
* Preserves insertion order of first occurrence for determinism.
13+
*/
14+
function mergeScopes(existing: string | undefined, incoming: string | undefined): string | undefined {
15+
const existingTokens = existing?.split(/\s+/).filter(Boolean) ?? [];
16+
const incomingTokens = incoming?.split(/\s+/).filter(Boolean) ?? [];
17+
const merged = new Set<string>([...existingTokens, ...incomingTokens]);
18+
if (merged.size === 0) {
19+
return undefined;
20+
}
21+
return [...merged].join(' ');
22+
}
23+
924
export class SseError extends Error {
1025
constructor(
1126
public readonly code: number | undefined,
@@ -136,7 +151,7 @@ export class SSEClientTransport implements Transport {
136151
if (response.headers.has('www-authenticate')) {
137152
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
138153
this._resourceMetadataUrl = resourceMetadataUrl;
139-
this._scope = scope;
154+
this._scope = mergeScopes(this._scope, scope);
140155
}
141156
}
142157

@@ -271,7 +286,7 @@ export class SSEClientTransport implements Transport {
271286
if (response.headers.has('www-authenticate')) {
272287
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
273288
this._resourceMetadataUrl = resourceMetadataUrl;
274-
this._scope = scope;
289+
this._scope = mergeScopes(this._scope, scope);
275290
}
276291

277292
if (this._authProvider.onUnauthorized && !isAuthRetry) {

packages/client/src/client/streamableHttp.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,21 @@ import { EventSourceParserStream } from 'eventsource-parser/stream';
1616
import type { AuthProvider, OAuthClientProvider } from './auth.js';
1717
import { adaptOAuthProvider, auth, extractWWWAuthenticateParams, isOAuthClientProvider, UnauthorizedError } from './auth.js';
1818

19+
/**
20+
* Merges two space-separated OAuth scope strings into a deduplicated union.
21+
* Returns undefined when the resulting set is empty.
22+
* Preserves insertion order of first occurrence for determinism.
23+
*/
24+
function mergeScopes(existing: string | undefined, incoming: string | undefined): string | undefined {
25+
const existingTokens = existing?.split(/\s+/).filter(Boolean) ?? [];
26+
const incomingTokens = incoming?.split(/\s+/).filter(Boolean) ?? [];
27+
const merged = new Set<string>([...existingTokens, ...incomingTokens]);
28+
if (merged.size === 0) {
29+
return undefined;
30+
}
31+
return [...merged].join(' ');
32+
}
33+
1934
// Default reconnection options for StreamableHTTP connections
2035
const DEFAULT_STREAMABLE_HTTP_RECONNECTION_OPTIONS: StreamableHTTPReconnectionOptions = {
2136
initialReconnectionDelay: 1000,
@@ -222,7 +237,7 @@ export class StreamableHTTPClientTransport implements Transport {
222237
if (response.headers.has('www-authenticate')) {
223238
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
224239
this._resourceMetadataUrl = resourceMetadataUrl;
225-
this._scope = scope;
240+
this._scope = mergeScopes(this._scope, scope);
226241
}
227242

228243
if (this._authProvider.onUnauthorized && !isAuthRetry) {
@@ -515,7 +530,7 @@ export class StreamableHTTPClientTransport implements Transport {
515530
if (response.headers.has('www-authenticate')) {
516531
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
517532
this._resourceMetadataUrl = resourceMetadataUrl;
518-
this._scope = scope;
533+
this._scope = mergeScopes(this._scope, scope);
519534
}
520535

521536
if (this._authProvider.onUnauthorized && !isAuthRetry) {
@@ -554,7 +569,7 @@ export class StreamableHTTPClientTransport implements Transport {
554569
}
555570

556571
if (scope) {
557-
this._scope = scope;
572+
this._scope = mergeScopes(this._scope, scope);
558573
}
559574

560575
if (resourceMetadataUrl) {

packages/client/test/client/sse.test.ts

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1171,6 +1171,87 @@ describe('SSEClientTransport', () => {
11711171
await expect(() => transport.start()).rejects.toMatchObject(expectedError);
11721172
expect(mockAuthProvider.invalidateCredentials).toHaveBeenCalledWith('tokens');
11731173
});
1174+
1175+
it('accumulates scopes from sequential 401 responses in send()', async () => {
1176+
// Create server that accepts SSE connection but returns 401 on POST
1177+
// with different scopes on successive requests
1178+
resourceServer.close();
1179+
1180+
let postCallCount = 0;
1181+
resourceServer = createServer((req, res) => {
1182+
lastServerRequest = req;
1183+
1184+
if (req.method === 'GET') {
1185+
if (req.url !== '/') {
1186+
res.writeHead(404).end();
1187+
return;
1188+
}
1189+
1190+
res.writeHead(200, {
1191+
'Content-Type': 'text/event-stream',
1192+
'Cache-Control': 'no-cache, no-transform',
1193+
Connection: 'keep-alive'
1194+
});
1195+
res.write('event: endpoint\n');
1196+
res.write(`data: ${resourceBaseUrl.href}\n\n`);
1197+
return;
1198+
}
1199+
1200+
if (req.method === 'POST') {
1201+
postCallCount++;
1202+
if (postCallCount === 1) {
1203+
// First POST: 401 with scope="read:op1"
1204+
res.writeHead(401, {
1205+
'WWW-Authenticate': 'Bearer scope="read:op1"'
1206+
});
1207+
res.end();
1208+
} else if (postCallCount === 2) {
1209+
// Second POST: 401 with scope="read:op2"
1210+
res.writeHead(401, {
1211+
'WWW-Authenticate': 'Bearer scope="read:op2"'
1212+
});
1213+
res.end();
1214+
} else {
1215+
res.writeHead(200);
1216+
res.end();
1217+
}
1218+
}
1219+
});
1220+
1221+
resourceBaseUrl = await listenOnRandomPort(resourceServer);
1222+
1223+
// Use a minimal AuthProvider (not OAuthClientProvider) so onUnauthorized
1224+
// is not set and 401 throws UnauthorizedError, letting us inspect _scope.
1225+
const minimalAuthProvider: AuthProvider = {
1226+
token: vi.fn().mockResolvedValue('test-token')
1227+
};
1228+
1229+
transport = new SSEClientTransport(resourceBaseUrl, {
1230+
authProvider: minimalAuthProvider
1231+
});
1232+
1233+
await transport.start();
1234+
1235+
const message: JSONRPCMessage = {
1236+
jsonrpc: '2.0',
1237+
id: '1',
1238+
method: 'test',
1239+
params: {}
1240+
};
1241+
1242+
// First send: 401 with scope="read:op1" — throws UnauthorizedError
1243+
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
1244+
expect((transport as unknown as { _scope: string | undefined })['_scope']).toBe('read:op1');
1245+
1246+
// Second send: 401 with scope="read:op2" — scope should accumulate
1247+
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
1248+
1249+
// Verify _scope has accumulated both tokens
1250+
const finalScope = (transport as unknown as { _scope: string | undefined })['_scope'];
1251+
expect(finalScope).toBeDefined();
1252+
const finalScopeTokens = String(finalScope).split(' ').toSorted();
1253+
expect(finalScopeTokens).toEqual(['read:op1', 'read:op2']);
1254+
});
11741255
});
11751256

11761257
describe('custom fetch in auth code paths', () => {

0 commit comments

Comments
 (0)