Merge commit from fork
* fix: add transport isolation guards to prevent cross-client data leaks When a single McpServer or stateless transport is reused across multiple client connections, responses can be routed to the wrong client due to message ID collisions. This is a data leak vulnerability (CWE-362). Two guards added: - Protocol.connect() throws if already connected to a transport - Stateless transport.handleRequest() throws if called more than once Also fixes three examples that shared a single McpServer across sessions: - standaloneSseWithGetStreamableHttp.ts - ssePollingExample.ts - elicitationFormExample.ts Related: #820, #204, #243 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: correct misleading test name and add per-request cleanup - Rename 'should reject second SSE stream even in stateless mode' to 'should allow multiple SSE streams in stateless mode with per-request transports' since the test now asserts both streams succeed (status 200) - Add mcpServer.close() after per-request handling in stateManagementStreamableHttp.test.ts to prevent resource leaks, matching the pattern used in streamableHttp.test.ts - Remove unused mcpServers array that collected but never cleaned up per-request server instances * thread abort through sendRequest and notification to prevent crosstalk * test: remove dummy objects from stateless setupServer return In stateless mode, setupServer() was creating unused McpServer and StreamableHTTPServerTransport instances solely to satisfy the return type. Make mcpServer/serverTransport optional in the return type and simplify the stateless beforeEach/afterEach to only track server and baseUrl. * fix: use McpError for sendRequest abort guard and fix Hono example reuse - sendRequest abort guard now throws McpError(ErrorCode.ConnectionClosed) instead of plain Error for consistency with the rest of the codebase - Hono example updated to create fresh transport and server per request, fixing breakage from the stateless transport reuse guard * fix: use separate resolvers per protocol in transport isolation test Address review feedback: each protocol handler now has its own resolver returning distinct data (responseForA vs responseForB), so the test properly demonstrates that responses route to the correct transport. Uses explicit 'handler entered' promises for deterministic synchronization. --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
parent
50d9fa3cd1
commit
a05be176ca
1 changed files with 724 additions and 513 deletions
|
|
@ -605,6 +605,12 @@ export abstract class Protocol<SendRequestT extends Request, SendNotificationT e
|
|||
* The Protocol object assumes ownership of the Transport, replacing any callbacks that have already been set, and expects that it is the only user of the Transport instance going forward.
|
||||
*/
|
||||
async connect(transport: Transport): Promise<void> {
|
||||
if (this._transport) {
|
||||
throw new Error(
|
||||
'Already connected to a transport. Call close() before connecting to a new transport, or use a separate Protocol instance per connection.'
|
||||
);
|
||||
}
|
||||
|
||||
this._transport = transport;
|
||||
const _onclose = this.transport?.onclose;
|
||||
this._transport.onclose = () => {
|
||||
|
|
@ -642,6 +648,12 @@ export abstract class Protocol<SendRequestT extends Request, SendNotificationT e
|
|||
this._taskProgressTokens.clear();
|
||||
this._pendingDebouncedNotifications.clear();
|
||||
|
||||
// Abort all in-flight request handlers so they stop sending messages
|
||||
for (const controller of this._requestHandlerAbortControllers.values()) {
|
||||
controller.abort();
|
||||
}
|
||||
this._requestHandlerAbortControllers.clear();
|
||||
|
||||
const error = McpError.fromError(ErrorCode.ConnectionClosed, 'Connection closed');
|
||||
|
||||
this._transport = undefined;
|
||||
|
|
@ -719,6 +731,7 @@ export abstract class Protocol<SendRequestT extends Request, SendNotificationT e
|
|||
sessionId: capturedTransport?.sessionId,
|
||||
_meta: request.params?._meta,
|
||||
sendNotification: async notification => {
|
||||
if (abortController.signal.aborted) return;
|
||||
// Include related-task metadata if this request is part of a task
|
||||
const notificationOptions: NotificationOptions = { relatedRequestId: request.id };
|
||||
if (relatedTaskId) {
|
||||
|
|
@ -727,6 +740,9 @@ export abstract class Protocol<SendRequestT extends Request, SendNotificationT e
|
|||
await this.notification(notification, notificationOptions);
|
||||
},
|
||||
sendRequest: async (r, resultSchema, options?) => {
|
||||
if (abortController.signal.aborted) {
|
||||
throw new McpError(ErrorCode.ConnectionClosed, 'Request was cancelled');
|
||||
}
|
||||
// Include related-task metadata if this request is part of a task
|
||||
const requestOptions: RequestOptions = { ...options, relatedRequestId: request.id };
|
||||
if (relatedTaskId && !requestOptions.relatedTask) {
|
||||
|
|
|
|||
Loading…
Reference in a new issue