From 984a49d4ec3b0df4379bd5669fb8aba1e873c70f Mon Sep 17 00:00:00 2001 From: czlonkowski Date: Sun, 22 Mar 2026 19:45:57 +0100 Subject: [PATCH] fix: address code review - add spec-compliance comment and mixed-batch test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Document design choice: !('id' in msg) is strict per JSON-RPC 2.0 §4.1 - Add test: mixed batch (notification + request) with stale session → 400 Co-Authored-By: Claude Opus 4.6 (1M context) --- src/http-server-single-session.ts | 24 ++++++++----------- .../http-server-session-management.test.ts | 16 +++++++++++++ 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/src/http-server-single-session.ts b/src/http-server-single-session.ts index 017d1ce..effd3bb 100644 --- a/src/http-server-single-session.ts +++ b/src/http-server-single-session.ts @@ -256,17 +256,18 @@ export class SingleSessionHTTPServer { /** * Checks if a request body is a JSON-RPC notification (or batch of only notifications). - * Notifications have a `method` field but no `id` field per JSON-RPC 2.0 spec. + * Per JSON-RPC 2.0 §4.1, a notification is a request without an "id" member. + * Note: `!('id' in msg)` is strict — messages with `id: null` are treated as + * requests, not notifications. This is spec-compliant. */ private isJsonRpcNotification(body: unknown): boolean { if (!body || typeof body !== 'object') return false; + const isSingleNotification = (msg: any): boolean => + msg && typeof msg.method === 'string' && !('id' in msg); if (Array.isArray(body)) { - return body.length > 0 && body.every( - (msg: any) => msg && typeof msg.method === 'string' && !('id' in msg) - ); + return body.length > 0 && body.every(isSingleNotification); } - const msg = body as Record; - return typeof msg.method === 'string' && !('id' in msg); + return isSingleNotification(body); } /** @@ -629,12 +630,10 @@ export class SingleSessionHTTPServer { logger.info('handleRequest: Reusing existing transport for session', { sessionId }); transport = this.transports[sessionId]; - // TOCTOU guard: session may have been removed between the check and this line + // TOCTOU guard: session may have been removed between the check above and here if (!transport) { if (this.isJsonRpcNotification(req.body)) { - logger.info('handleRequest: Session removed during lookup, accepting notification', { - sessionId, - }); + logger.info('handleRequest: Session removed during lookup, accepting notification', { sessionId }); res.status(202).end(); return; } @@ -660,10 +659,7 @@ export class SingleSessionHTTPServer { this.updateSessionAccess(sessionId); } else { - // Check if this is a JSON-RPC notification (no "id" field = fire-and-forget) - // Per JSON-RPC 2.0 spec, notifications don't expect responses. - // Returning 400 for stale-session notifications causes Claude's proxy to - // interpret the connection as broken, triggering reconnection storms (#654). + // Notifications are fire-and-forget; returning 400 triggers reconnection storms (#654) if (this.isJsonRpcNotification(req.body)) { logger.info('handleRequest: Accepting notification for stale/missing session', { method: req.body?.method, diff --git a/tests/unit/http-server-session-management.test.ts b/tests/unit/http-server-session-management.test.ts index 3483c9d..af4fb0c 100644 --- a/tests/unit/http-server-session-management.test.ts +++ b/tests/unit/http-server-session-management.test.ts @@ -1287,5 +1287,21 @@ describe('HTTP Server Session Management', () => { expect(res.status).toHaveBeenCalledWith(400); }); + + it('should return 400 for mixed batch (notification + request) with stale session', async () => { + server = new SingleSessionHTTPServer(); + + const { req, res } = createMockReqRes(); + req.headers = { 'mcp-session-id': 'stale-session-that-does-not-exist' }; + req.method = 'POST'; + req.body = [ + { jsonrpc: '2.0', method: 'notifications/initialized' }, + { jsonrpc: '2.0', method: 'tools/list', id: 1 }, + ]; + + await server.handleRequest(req as any, res as any); + + expect(res.status).toHaveBeenCalledWith(400); + }); }); }); \ No newline at end of file