Skip to content

Commit 731ffae

Browse files
rechedev9rechedev9
committed
fix(client): preserve OAuth metadata across repeated 401s
1 parent c62ccfc commit 731ffae

5 files changed

Lines changed: 167 additions & 38 deletions

File tree

‎.changeset/fix-scope-overwrite-infinite-reauth.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
---
22
'@modelcontextprotocol/client': patch
3+
'@modelcontextprotocol/core': patch
34
---
45

56
Fix: accumulate OAuth scopes on 401/403 instead of overwriting

‎packages/client/src/client/sse.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,9 @@ export class SSEClientTransport implements Transport {
142142
this._last401Response = response;
143143
if (response.headers.has('www-authenticate')) {
144144
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
145-
this._resourceMetadataUrl = resourceMetadataUrl;
145+
if (resourceMetadataUrl) {
146+
this._resourceMetadataUrl = resourceMetadataUrl;
147+
}
146148
this._scope = mergeScopes(this._scope, scope);
147149
}
148150
}
@@ -277,7 +279,9 @@ export class SSEClientTransport implements Transport {
277279
if (response.status === 401 && this._authProvider) {
278280
if (response.headers.has('www-authenticate')) {
279281
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
280-
this._resourceMetadataUrl = resourceMetadataUrl;
282+
if (resourceMetadataUrl) {
283+
this._resourceMetadataUrl = resourceMetadataUrl;
284+
}
281285
this._scope = mergeScopes(this._scope, scope);
282286
}
283287

‎packages/client/src/client/streamableHttp.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,9 @@ export class StreamableHTTPClientTransport implements Transport {
222222
if (response.status === 401 && this._authProvider) {
223223
if (response.headers.has('www-authenticate')) {
224224
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
225-
this._resourceMetadataUrl = resourceMetadataUrl;
225+
if (resourceMetadataUrl) {
226+
this._resourceMetadataUrl = resourceMetadataUrl;
227+
}
226228
this._scope = mergeScopes(this._scope, scope);
227229
}
228230

@@ -515,7 +517,9 @@ export class StreamableHTTPClientTransport implements Transport {
515517
// Store WWW-Authenticate params for interactive finishAuth() path
516518
if (response.headers.has('www-authenticate')) {
517519
const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
518-
this._resourceMetadataUrl = resourceMetadataUrl;
520+
if (resourceMetadataUrl) {
521+
this._resourceMetadataUrl = resourceMetadataUrl;
522+
}
519523
this._scope = mergeScopes(this._scope, scope);
520524
}
521525

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

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1252,6 +1252,79 @@ describe('SSEClientTransport', () => {
12521252
const finalScopeTokens = String(finalScope).split(' ').toSorted();
12531253
expect(finalScopeTokens).toEqual(['read:op1', 'read:op2']);
12541254
});
1255+
1256+
it('preserves resource metadata URL across repeated 401 POST responses without resource_metadata', async () => {
1257+
resourceServer.close();
1258+
1259+
let postCallCount = 0;
1260+
const resourceMetadataUrl = 'http://example.com/.well-known/oauth-protected-resource';
1261+
resourceServer = createServer((req, res) => {
1262+
lastServerRequest = req;
1263+
1264+
if (req.method === 'GET') {
1265+
if (req.url !== '/') {
1266+
res.writeHead(404).end();
1267+
return;
1268+
}
1269+
1270+
res.writeHead(200, {
1271+
'Content-Type': 'text/event-stream',
1272+
'Cache-Control': 'no-cache, no-transform',
1273+
Connection: 'keep-alive'
1274+
});
1275+
res.write('event: endpoint\n');
1276+
res.write(`data: ${resourceBaseUrl.href}\n\n`);
1277+
return;
1278+
}
1279+
1280+
if (req.method === 'POST') {
1281+
postCallCount++;
1282+
if (postCallCount === 1) {
1283+
res.writeHead(401, {
1284+
'WWW-Authenticate': `Bearer resource_metadata="${resourceMetadataUrl}", scope="read:op1"`
1285+
});
1286+
res.end();
1287+
} else if (postCallCount === 2) {
1288+
res.writeHead(401, {
1289+
'WWW-Authenticate': 'Bearer scope="read:op2"'
1290+
});
1291+
res.end();
1292+
} else {
1293+
res.writeHead(200);
1294+
res.end();
1295+
}
1296+
}
1297+
});
1298+
1299+
resourceBaseUrl = await listenOnRandomPort(resourceServer);
1300+
1301+
const minimalAuthProvider: AuthProvider = {
1302+
token: vi.fn().mockResolvedValue('test-token')
1303+
};
1304+
1305+
transport = new SSEClientTransport(resourceBaseUrl, {
1306+
authProvider: minimalAuthProvider
1307+
});
1308+
1309+
await transport.start();
1310+
1311+
const message: JSONRPCMessage = {
1312+
jsonrpc: '2.0',
1313+
id: '1',
1314+
method: 'test',
1315+
params: {}
1316+
};
1317+
1318+
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
1319+
expect((transport as unknown as { _resourceMetadataUrl: URL | undefined })['_resourceMetadataUrl']).toEqual(
1320+
new URL(resourceMetadataUrl)
1321+
);
1322+
1323+
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
1324+
expect((transport as unknown as { _resourceMetadataUrl: URL | undefined })['_resourceMetadataUrl']).toEqual(
1325+
new URL(resourceMetadataUrl)
1326+
);
1327+
});
12551328
});
12561329

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

‎packages/client/test/client/streamableHttp.test.ts‎

Lines changed: 81 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -723,21 +723,23 @@ describe('StreamableHTTPClientTransport', () => {
723723
const authSpy = vi.spyOn(authModule, 'auth');
724724
authSpy.mockResolvedValue('AUTHORIZED');
725725

726-
await transport.send(message);
727-
728-
// Verify fetch was called twice
729-
expect(fetchMock).toHaveBeenCalledTimes(2);
726+
try {
727+
await transport.send(message);
730728

731-
// Verify auth was called with the new scope
732-
expect(authSpy).toHaveBeenCalledWith(
733-
mockAuthProvider,
734-
expect.objectContaining({
735-
scope: 'new_scope',
736-
resourceMetadataUrl: new URL('http://example.com/resource')
737-
})
738-
);
729+
// Verify fetch was called twice
730+
expect(fetchMock).toHaveBeenCalledTimes(2);
739731

740-
authSpy.mockRestore();
732+
// Verify auth was called with the new scope
733+
expect(authSpy).toHaveBeenCalledWith(
734+
mockAuthProvider,
735+
expect.objectContaining({
736+
scope: 'new_scope',
737+
resourceMetadataUrl: new URL('http://example.com/resource')
738+
})
739+
);
740+
} finally {
741+
authSpy.mockRestore();
742+
}
741743
});
742744

743745
it('prevents infinite upscoping on repeated 403', async () => {
@@ -765,21 +767,23 @@ describe('StreamableHTTPClientTransport', () => {
765767
const authSpy = vi.spyOn(authModule as typeof import('../../src/client/auth.js'), 'auth');
766768
authSpy.mockResolvedValue('AUTHORIZED');
767769

768-
// First send: should trigger upscoping
769-
await expect(transport.send(message)).rejects.toThrow('Server returned 403 after trying upscoping');
770+
try {
771+
// First send: should trigger upscoping
772+
await expect(transport.send(message)).rejects.toThrow('Server returned 403 after trying upscoping');
770773

771-
expect(fetchMock).toHaveBeenCalledTimes(2); // Initial call + one retry after auth
772-
expect(authSpy).toHaveBeenCalledTimes(1); // Auth called once
774+
expect(fetchMock).toHaveBeenCalledTimes(2); // Initial call + one retry after auth
775+
expect(authSpy).toHaveBeenCalledTimes(1); // Auth called once
773776

774-
// Second send: should fail immediately without re-calling auth
775-
fetchMock.mockClear();
776-
authSpy.mockClear();
777-
await expect(transport.send(message)).rejects.toThrow('Server returned 403 after trying upscoping');
777+
// Second send: should fail immediately without re-calling auth
778+
fetchMock.mockClear();
779+
authSpy.mockClear();
780+
await expect(transport.send(message)).rejects.toThrow('Server returned 403 after trying upscoping');
778781

779-
expect(fetchMock).toHaveBeenCalledTimes(1); // Only one fetch call
780-
expect(authSpy).not.toHaveBeenCalled(); // Auth not called again
781-
782-
authSpy.mockRestore();
782+
expect(fetchMock).toHaveBeenCalledTimes(1); // Only one fetch call
783+
expect(authSpy).not.toHaveBeenCalled(); // Auth not called again
784+
} finally {
785+
authSpy.mockRestore();
786+
}
783787
});
784788

785789
describe('mergeScopes behavior (via transport)', () => {
@@ -965,17 +969,60 @@ describe('StreamableHTTPClientTransport', () => {
965969
const authSpy = vi.spyOn(authModule, 'auth');
966970
authSpy.mockResolvedValue('AUTHORIZED');
967971

968-
await expect(
969-
transport.send({
970-
jsonrpc: '2.0',
971-
method: 'test',
972-
params: {},
973-
id: 'test-id'
972+
try {
973+
await expect(
974+
transport.send({
975+
jsonrpc: '2.0',
976+
method: 'test',
977+
params: {},
978+
id: 'test-id'
979+
})
980+
).rejects.toThrow('Server returned 403 after trying upscoping');
981+
982+
expect(authSpy).toHaveBeenCalledTimes(1);
983+
} finally {
984+
authSpy.mockRestore();
985+
}
986+
});
987+
988+
it('preserves resource metadata URL across 401 responses without resource_metadata', async () => {
989+
const message: JSONRPCMessage = {
990+
jsonrpc: '2.0',
991+
method: 'test',
992+
params: {},
993+
id: 'test-id'
994+
};
995+
996+
const resourceMetadataUrl = 'http://example.com/.well-known/oauth-protected-resource';
997+
(globalThis.fetch as Mock)
998+
.mockResolvedValueOnce({
999+
ok: false,
1000+
status: 401,
1001+
statusText: 'Unauthorized',
1002+
headers: new Headers({
1003+
'WWW-Authenticate': `Bearer resource_metadata="${resourceMetadataUrl}", scope="read:op1"`
1004+
}),
1005+
text: () => Promise.resolve('Unauthorized')
9741006
})
975-
).rejects.toThrow('Server returned 403 after trying upscoping');
1007+
.mockResolvedValueOnce({
1008+
ok: false,
1009+
status: 401,
1010+
statusText: 'Unauthorized',
1011+
headers: new Headers({
1012+
'WWW-Authenticate': 'Bearer scope="read:op2"'
1013+
}),
1014+
text: () => Promise.resolve('Unauthorized')
1015+
});
1016+
1017+
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
1018+
expect((transport as unknown as { _resourceMetadataUrl: URL | undefined })._resourceMetadataUrl).toEqual(
1019+
new URL(resourceMetadataUrl)
1020+
);
9761021

977-
expect(authSpy).toHaveBeenCalledTimes(1);
978-
authSpy.mockRestore();
1022+
await expect(transport.send(message)).rejects.toThrow(UnauthorizedError);
1023+
expect((transport as unknown as { _resourceMetadataUrl: URL | undefined })._resourceMetadataUrl).toEqual(
1024+
new URL(resourceMetadataUrl)
1025+
);
9791026
});
9801027

9811028
describe('Reconnection Logic', () => {

0 commit comments

Comments
 (0)