From ec19c9dade49d89995b77af364d7310c1665c1b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romuald=20Cz=C5=82onkowski?= <56956555+czlonkowski@users.noreply.github.com> Date: Sun, 22 Mar 2026 19:59:57 +0100 Subject: [PATCH] fix: return 202 for stale-session notifications, warn on updateTable columns (#654) (#655) --- CHANGELOG.md | 11 ++ package.json | 2 +- src/http-server-single-session.ts | 50 ++++++- src/mcp/handlers-n8n-manager.ts | 5 +- src/mcp/tools-n8n-manager.ts | 4 +- .../http-server-session-management.test.ts | 122 +++++++++++++++++- .../mcp/handlers-manage-datatable.test.ts | 16 +++ 7 files changed, 200 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b862813..30cde08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [2.40.3] - 2026-03-22 + +### Fixed + +- **Notification 400 disconnect storms (#654)**: `handleRequest()` now returns 202 Accepted for JSON-RPC notifications with stale/expired session IDs instead of 400. Per JSON-RPC 2.0 spec, notifications don't expect responses — returning 400 caused Claude's proxy to trigger reconnection storms (930 errors/day, 216 users affected) +- **TOCTOU race in session lookup**: Added null guard after transport assignment to handle sessions removed between the existence check and use +- **`updateTable` silently ignoring `columns` parameter**: Now returns a warning message when `columns` is passed to `updateTable`, clarifying that table schema is immutable after creation via the public API +- **Tool schema descriptions clarified**: `name` and `columns` parameter descriptions now explicitly document that `updateTable` is rename-only and columns are for `createTable` only + +Conceived by Romuald Członkowski - https://www.aiadvisors.pl/en + ## [2.40.2] - 2026-03-22 ### Fixed diff --git a/package.json b/package.json index 238cfee..2818522 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "n8n-mcp", - "version": "2.40.2", + "version": "2.40.3", "description": "Integration between n8n workflow automation and Model Context Protocol (MCP)", "main": "dist/index.js", "types": "dist/index.d.ts", diff --git a/src/http-server-single-session.ts b/src/http-server-single-session.ts index 63cd72c..effd3bb 100644 --- a/src/http-server-single-session.ts +++ b/src/http-server-single-session.ts @@ -253,6 +253,22 @@ export class SingleSessionHTTPServer { // This ensures compatibility with all MCP clients and proxies return Boolean(sessionId && sessionId.length > 0); } + + /** + * Checks if a request body is a JSON-RPC notification (or batch of only notifications). + * 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(isSingleNotification); + } + return isSingleNotification(body); + } /** * Sanitize error information for client responses @@ -614,6 +630,22 @@ 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 above and here + if (!transport) { + if (this.isJsonRpcNotification(req.body)) { + logger.info('handleRequest: Session removed during lookup, accepting notification', { sessionId }); + res.status(202).end(); + return; + } + logger.warn('handleRequest: Session removed between check and use (TOCTOU)', { sessionId }); + res.status(400).json({ + jsonrpc: '2.0', + error: { code: -32000, message: 'Bad Request: Session not found or expired' }, + id: req.body?.id || null, + }); + return; + } + // In multi-tenant shared mode, update instance context if provided const isMultiTenantEnabled = process.env.ENABLE_MULTI_TENANT === 'true'; const sessionStrategy = process.env.MULTI_TENANT_SESSION_STRATEGY || 'instance'; @@ -627,23 +659,33 @@ export class SingleSessionHTTPServer { this.updateSessionAccess(sessionId); } else { - // Invalid request - no session ID and not an initialize request + // 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, + sessionId: sessionId || 'none', + }); + res.status(202).end(); + return; + } + + // Only return 400 for actual requests that need a valid session const errorDetails = { hasSessionId: !!sessionId, isInitialize: isInitialize, sessionIdValid: sessionId ? this.isValidSessionId(sessionId) : false, sessionExists: sessionId ? !!this.transports[sessionId] : false }; - + logger.warn('handleRequest: Invalid request - no session ID and not initialize', errorDetails); - + let errorMessage = 'Bad Request: No valid session ID provided and not an initialize request'; if (sessionId && !this.isValidSessionId(sessionId)) { errorMessage = 'Bad Request: Invalid session ID format'; } else if (sessionId && !this.transports[sessionId]) { errorMessage = 'Bad Request: Session not found or expired'; } - + res.status(400).json({ jsonrpc: '2.0', error: { diff --git a/src/mcp/handlers-n8n-manager.ts b/src/mcp/handlers-n8n-manager.ts index c3e2dd7..b0761f1 100644 --- a/src/mcp/handlers-n8n-manager.ts +++ b/src/mcp/handlers-n8n-manager.ts @@ -2834,10 +2834,13 @@ export async function handleUpdateTable(args: unknown, context?: InstanceContext const client = ensureApiConfigured(context); const { tableId, name } = updateTableSchema.parse(args); const dataTable = await client.updateDataTable(tableId, { name }); + const rawArgs = args as Record; + const hasColumns = rawArgs && typeof rawArgs === 'object' && 'columns' in rawArgs; return { success: true, data: dataTable, - message: `Data table renamed to "${dataTable.name}"`, + message: `Data table renamed to "${dataTable.name}"` + + (hasColumns ? '. Note: columns parameter was ignored — table schema is immutable after creation via the public API' : ''), }; } catch (error) { return handleDataTableError(error); diff --git a/src/mcp/tools-n8n-manager.ts b/src/mcp/tools-n8n-manager.ts index 16bea4d..0b364a4 100644 --- a/src/mcp/tools-n8n-manager.ts +++ b/src/mcp/tools-n8n-manager.ts @@ -619,10 +619,10 @@ export const n8nManagementTools: ToolDefinition[] = [ description: 'Operation to perform', }, tableId: { type: 'string', description: 'Data table ID (required for all actions except createTable and listTables)' }, - name: { type: 'string', description: 'For createTable/updateTable: table name' }, + name: { type: 'string', description: 'For createTable: table name. For updateTable: new name (rename only — schema is immutable after creation)' }, columns: { type: 'array', - description: 'For createTable: column definitions', + description: 'For createTable only: column definitions (schema is immutable after creation via public API)', items: { type: 'object', properties: { diff --git a/tests/unit/http-server-session-management.test.ts b/tests/unit/http-server-session-management.test.ts index 22b49d5..af4fb0c 100644 --- a/tests/unit/http-server-session-management.test.ts +++ b/tests/unit/http-server-session-management.test.ts @@ -219,6 +219,7 @@ describe('HTTP Server Session Management', () => { status: vi.fn().mockReturnThis(), json: vi.fn().mockReturnThis(), send: vi.fn().mockReturnThis(), + end: vi.fn().mockReturnThis(), setHeader: vi.fn((key: string, value: string) => { headers[key.toLowerCase()] = value; }), @@ -1170,7 +1171,7 @@ describe('HTTP Server Session Management', () => { it('should show legacy SSE session when present', async () => { server = new SingleSessionHTTPServer(); - + // Mock legacy session const mockSession = { sessionId: 'sse-session-123', @@ -1180,10 +1181,127 @@ describe('HTTP Server Session Management', () => { (server as any).session = mockSession; const sessionInfo = server.getSessionInfo(); - + expect(sessionInfo.active).toBe(true); expect(sessionInfo.sessionId).toBe('sse-session-123'); expect(sessionInfo.age).toBeGreaterThanOrEqual(0); }); }); + + describe('Notification handling for stale sessions (#654)', () => { + beforeEach(() => { + // Re-apply mockImplementation after vi.clearAllMocks() resets it + mockConsoleManager.wrapOperation.mockImplementation(async (fn: () => Promise) => { + return await fn(); + }); + }); + + it('should return 202 for notification with stale session ID', 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', + }; + + await server.handleRequest(req as any, res as any); + + expect(res.status).toHaveBeenCalledWith(202); + expect(res.end).toHaveBeenCalled(); + }); + + it('should return 202 for notification batch with stale session ID', 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: 'notifications/cancelled' }, + ]; + + await server.handleRequest(req as any, res as any); + + expect(res.status).toHaveBeenCalledWith(202); + expect(res.end).toHaveBeenCalled(); + }); + + it('should return 400 for request (with id) with stale session ID', 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: 'tools/call', + params: { name: 'search_nodes', arguments: { query: 'http' } }, + id: 42, + }; + + await server.handleRequest(req as any, res as any); + + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ + error: expect.objectContaining({ + message: 'Bad Request: Session not found or expired', + }), + })); + }); + + it('should return 202 for notification with no session ID', async () => { + server = new SingleSessionHTTPServer(); + + const { req, res } = createMockReqRes(); + + req.method = 'POST'; + req.body = { + jsonrpc: '2.0', + method: 'notifications/cancelled', + }; + + await server.handleRequest(req as any, res as any); + + expect(res.status).toHaveBeenCalledWith(202); + expect(res.end).toHaveBeenCalled(); + }); + + it('should return 400 for request with no session ID and not initialize', async () => { + server = new SingleSessionHTTPServer(); + + const { req, res } = createMockReqRes(); + req.method = 'POST'; + req.body = { + jsonrpc: '2.0', + method: 'tools/list', + id: 1, + }; + + await server.handleRequest(req as any, res as any); + + 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 diff --git a/tests/unit/mcp/handlers-manage-datatable.test.ts b/tests/unit/mcp/handlers-manage-datatable.test.ts index 98809bf..76f07c9 100644 --- a/tests/unit/mcp/handlers-manage-datatable.test.ts +++ b/tests/unit/mcp/handlers-manage-datatable.test.ts @@ -363,6 +363,22 @@ describe('Data Table Handlers (n8n_manage_datatable)', () => { expect(result.success).toBe(false); expect(result.error).toBe('Update failed'); }); + + it('should warn when columns parameter is passed', async () => { + const updatedTable = { id: 'dt-1', name: 'Renamed' }; + mockApiClient.updateDataTable.mockResolvedValue(updatedTable); + + const result = await handlers.handleUpdateTable({ + tableId: 'dt-1', + name: 'Renamed', + columns: [{ name: 'phone', type: 'string' }], + }); + + expect(result.success).toBe(true); + expect(result.message).toContain('columns parameter was ignored'); + expect(result.message).toContain('immutable after creation'); + expect(mockApiClient.updateDataTable).toHaveBeenCalledWith('dt-1', { name: 'Renamed' }); + }); }); // ========================================================================