Skip to content

Commit 992dced

Browse files
committed
fix(server): reject duplicate in-flight request ids in streamable HTTP
The transport registered every request via _requestToStreamMapping.set(message.id, streamId) with no duplicate check, so a concurrent POST reusing an in-flight id overwrote the entry, cross-wired the first request's response onto the second POST's stream, and left the first POST hanging. Reject a POST containing a request whose id is already in flight, or duplicated within the same batch, with HTTP 400 and JSON-RPC -32600. Sequential reuse after completion stays allowed since deployed clients send a constant id for every request. Cancelled requests never produce a response, so notifications/cancelled now retires the transport bookkeeping for its target id; without that, a cancelled id would stay in flight forever and lock out cancel-then-reuse clients. Adds the missing isCancelledNotification guard to core-internal alongside the existing notification guards. Fixes #2433.
1 parent ddba550 commit 992dced

3 files changed

Lines changed: 216 additions & 0 deletions

File tree

packages/core-internal/src/types/guards.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import {
22
CallToolResultSchema,
3+
CancelledNotificationSchema,
34
InitializedNotificationSchema,
45
InitializeRequestSchema,
56
JSONRPCErrorResponseSchema,
@@ -12,6 +13,7 @@ import {
1213
} from './schemas';
1314
import type {
1415
CallToolResult,
16+
CancelledNotification,
1517
CompleteRequest,
1618
CompleteRequestPrompt,
1719
CompleteRequestResourceTemplate,
@@ -123,6 +125,9 @@ export const isInitializeRequest = (value: unknown): value is InitializeRequest
123125
export const isInitializedNotification = (value: unknown): value is InitializedNotification =>
124126
InitializedNotificationSchema.safeParse(value).success;
125127

128+
export const isCancelledNotification = (value: unknown): value is CancelledNotification =>
129+
CancelledNotificationSchema.safeParse(value).success;
130+
126131
export function assertCompleteRequestPrompt(request: CompleteRequest): asserts request is CompleteRequestPrompt {
127132
if (request.params.ref.type !== 'ref/prompt') {
128133
throw new TypeError(`Expected CompleteRequestPrompt, but got ${request.params.ref.type}`);

packages/server/src/server/streamableHttp.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import type { AuthInfo, JSONRPCMessage, MessageExtraInfo, RequestId, Transport } from '@modelcontextprotocol/core-internal';
1111
import {
1212
DEFAULT_NEGOTIATED_PROTOCOL_VERSION,
13+
isCancelledNotification,
1314
isInitializeRequest,
1415
isJSONRPCErrorResponse,
1516
isJSONRPCRequest,
@@ -763,6 +764,38 @@ export class WebStandardStreamableHTTPServerTransport implements Transport {
763764
}
764765
}
765766

767+
// A cancelled request never produces a response (the protocol layer
768+
// suppresses responses for aborted handlers), so retire its transport
769+
// bookkeeping here. Otherwise its _requestToStreamMapping entry would
770+
// pin the request ID as in-flight forever and the duplicate-ID guard
771+
// below would reject any later reuse of that ID.
772+
for (const message of messages) {
773+
if (isCancelledNotification(message) && message.params.requestId !== undefined) {
774+
this._requestToStreamMapping.delete(message.params.requestId);
775+
this._requestResponseMap.delete(message.params.requestId);
776+
}
777+
}
778+
779+
// Request IDs MUST be unique within a session. Registering a duplicate
780+
// would overwrite the in-flight entry in _requestToStreamMapping and
781+
// cross-wire the original request's response onto this POST's stream,
782+
// leaving the original POST hanging - so reject the duplicate while the
783+
// first request is still in flight. Sequential reuse stays allowed:
784+
// entries are retired when the response is delivered or the request is
785+
// cancelled.
786+
const incomingRequestIds = new Set<RequestId>();
787+
for (const message of messages) {
788+
if (!isJSONRPCRequest(message)) {
789+
continue;
790+
}
791+
if (this._requestToStreamMapping.has(message.id) || incomingRequestIds.has(message.id)) {
792+
const error = `Invalid Request: Request ID ${String(message.id)} is already in flight`;
793+
this.onerror?.(new Error(error));
794+
return this.createJsonErrorResponse(400, -32_600, error);
795+
}
796+
incomingRequestIds.add(message.id);
797+
}
798+
766799
// check if it contains requests
767800
const hasRequests = messages.some(element => isJSONRPCRequest(element));
768801

packages/server/test/server/streamableHttp.test.ts

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,184 @@ describe('Zod v4', () => {
561561
});
562562
});
563563

564+
describe('HTTPServerTransport - Duplicate request IDs', () => {
565+
let transport: WebStandardStreamableHTTPServerTransport;
566+
let mcpServer: McpServer;
567+
let sessionId: string;
568+
let slowToolStarted: Promise<void>;
569+
let releaseSlowTool: () => void;
570+
571+
function setupServer(options?: { enableJsonResponse?: boolean }): void {
572+
mcpServer = new McpServer({ name: 'test-server', version: '1.0.0' }, { capabilities: { logging: {} } });
573+
574+
mcpServer.registerTool(
575+
'greet',
576+
{ description: 'Greeting tool', inputSchema: z.object({ name: z.string() }) },
577+
async ({ name }): Promise<CallToolResult> => {
578+
return { content: [{ type: 'text', text: `Hello, ${name}!` }] };
579+
}
580+
);
581+
582+
let started!: () => void;
583+
slowToolStarted = new Promise<void>(resolve => {
584+
started = resolve;
585+
});
586+
const gate = new Promise<void>(resolve => {
587+
releaseSlowTool = resolve;
588+
});
589+
mcpServer.registerTool(
590+
'slow',
591+
{ description: 'A tool that blocks until released by the test', inputSchema: z.object({}) },
592+
async (): Promise<CallToolResult> => {
593+
started();
594+
await gate;
595+
return { content: [{ type: 'text', text: 'slow-done' }] };
596+
}
597+
);
598+
599+
transport = new WebStandardStreamableHTTPServerTransport({
600+
sessionIdGenerator: () => randomUUID(),
601+
enableJsonResponse: options?.enableJsonResponse ?? false
602+
});
603+
}
604+
605+
beforeEach(async () => {
606+
setupServer();
607+
await mcpServer.connect(transport);
608+
});
609+
610+
afterEach(async () => {
611+
releaseSlowTool();
612+
await transport.close();
613+
});
614+
615+
async function initializeServer(): Promise<string> {
616+
const request = createRequest('POST', TEST_MESSAGES.initialize);
617+
const response = await transport.handleRequest(request);
618+
619+
expect(response.status).toBe(200);
620+
const newSessionId = response.headers.get('mcp-session-id');
621+
expect(newSessionId).toBeDefined();
622+
return newSessionId as string;
623+
}
624+
625+
function callTool(name: string, id: string): JSONRPCMessage {
626+
return {
627+
jsonrpc: '2.0',
628+
method: 'tools/call',
629+
params: { name, arguments: name === 'greet' ? { name: 'Test' } : {} },
630+
id
631+
} as JSONRPCMessage;
632+
}
633+
634+
it('should reject a request whose ID collides with one still in flight', async () => {
635+
sessionId = await initializeServer();
636+
637+
// First request occupies the ID and blocks inside the tool handler
638+
const firstResponse = await transport.handleRequest(createRequest('POST', callTool('slow', 'dup-1'), { sessionId }));
639+
expect(firstResponse.status).toBe(200);
640+
await slowToolStarted;
641+
642+
// Concurrent request reusing the same ID must be rejected, not cross-wired
643+
const secondResponse = await transport.handleRequest(createRequest('POST', callTool('greet', 'dup-1'), { sessionId }));
644+
expect(secondResponse.status).toBe(400);
645+
const errorData = await secondResponse.json();
646+
expectErrorResponse(errorData, -32_600, /already in flight/);
647+
648+
// The original request must still receive its own response
649+
releaseSlowTool();
650+
const text = await readSSEEvent(firstResponse);
651+
const eventData = parseSSEData(text);
652+
expect(eventData).toMatchObject({
653+
jsonrpc: '2.0',
654+
result: { content: [{ type: 'text', text: 'slow-done' }] },
655+
id: 'dup-1'
656+
});
657+
});
658+
659+
it('should reject duplicate request IDs within a single batch', async () => {
660+
sessionId = await initializeServer();
661+
662+
const batch: JSONRPCMessage[] = [callTool('greet', 'batch-dup'), callTool('greet', 'batch-dup')];
663+
const response = await transport.handleRequest(createRequest('POST', batch, { sessionId }));
664+
665+
expect(response.status).toBe(400);
666+
const errorData = await response.json();
667+
expectErrorResponse(errorData, -32_600, /already in flight/);
668+
});
669+
670+
it('should allow sequential reuse of a request ID after the response is delivered', async () => {
671+
sessionId = await initializeServer();
672+
673+
for (let attempt = 0; attempt < 2; attempt++) {
674+
const response = await transport.handleRequest(createRequest('POST', callTool('greet', 'reuse-1'), { sessionId }));
675+
expect(response.status).toBe(200);
676+
677+
const eventData = parseSSEData(await readSSEEvent(response));
678+
expect(eventData).toMatchObject({
679+
jsonrpc: '2.0',
680+
result: { content: [{ type: 'text', text: 'Hello, Test!' }] },
681+
id: 'reuse-1'
682+
});
683+
}
684+
});
685+
686+
it('should allow reuse of a request ID after the original request was cancelled', async () => {
687+
sessionId = await initializeServer();
688+
689+
const firstResponse = await transport.handleRequest(createRequest('POST', callTool('slow', 'cancel-1'), { sessionId }));
690+
expect(firstResponse.status).toBe(200);
691+
await slowToolStarted;
692+
693+
const cancelNotification: JSONRPCMessage = {
694+
jsonrpc: '2.0',
695+
method: 'notifications/cancelled',
696+
params: { requestId: 'cancel-1', reason: 'test cancellation' }
697+
};
698+
const cancelResponse = await transport.handleRequest(createRequest('POST', cancelNotification, { sessionId }));
699+
expect(cancelResponse.status).toBe(202);
700+
701+
// The cancelled request will never produce a response, so its ID must be reusable
702+
const secondResponse = await transport.handleRequest(createRequest('POST', callTool('greet', 'cancel-1'), { sessionId }));
703+
expect(secondResponse.status).toBe(200);
704+
705+
const eventData = parseSSEData(await readSSEEvent(secondResponse));
706+
expect(eventData).toMatchObject({
707+
jsonrpc: '2.0',
708+
result: { content: [{ type: 'text', text: 'Hello, Test!' }] },
709+
id: 'cancel-1'
710+
});
711+
});
712+
713+
it('should reject duplicate in-flight request IDs in JSON response mode', async () => {
714+
await transport.close();
715+
setupServer({ enableJsonResponse: true });
716+
await mcpServer.connect(transport);
717+
718+
sessionId = await initializeServer();
719+
720+
// In JSON mode handleRequest resolves only when the response is ready - do not await
721+
const firstResponsePromise = transport.handleRequest(createRequest('POST', callTool('slow', 'dup-json'), { sessionId }));
722+
await slowToolStarted;
723+
724+
const secondResponse = await transport.handleRequest(createRequest('POST', callTool('greet', 'dup-json'), { sessionId }));
725+
expect(secondResponse.status).toBe(400);
726+
const errorData = await secondResponse.json();
727+
expectErrorResponse(errorData, -32_600, /already in flight/);
728+
729+
// The original request must still resolve with its own result
730+
releaseSlowTool();
731+
const firstResponse = await firstResponsePromise;
732+
expect(firstResponse.status).toBe(200);
733+
const data = await firstResponse.json();
734+
expect(data).toMatchObject({
735+
jsonrpc: '2.0',
736+
result: { content: [{ type: 'text', text: 'slow-done' }] },
737+
id: 'dup-json'
738+
});
739+
});
740+
});
741+
564742
describe('HTTPServerTransport - Session Callbacks', () => {
565743
it('should call onsessioninitialized callback', async () => {
566744
const onInitialized = vi.fn();

0 commit comments

Comments
 (0)