From e52a04a291b1a02de47360703ec01fd1fc9a6cf3 Mon Sep 17 00:00:00 2001 From: czlonkowski Date: Sun, 22 Mar 2026 19:21:29 +0100 Subject: [PATCH] fix: return 202 for notifications with stale sessions, warn on updateTable columns (#654) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Issue #654 (critical):** handleRequest() returned HTTP 400 for JSON-RPC notifications with stale/expired session IDs. Per JSON-RPC 2.0 spec, notifications don't expect responses. Returning 400 caused Claude's proxy to interpret the connection as broken, triggering reconnection storms (930 errors/day, 216 users affected). Fix: Added isJsonRpcNotification() helper that detects single and batch notifications (messages with method but no id field). Path C in handleRequest() now returns 202 Accepted for notifications with stale sessions instead of 400. Also added TOCTOU null guard in Path B. **Issue 1 (medium):** updateTable silently accepted columns parameter via the shared MCP tool schema. Zod stripped it but gave no feedback. Fix: handleUpdateTable now detects columns in raw args and appends a warning to the response. Tool schema descriptions clarified that columns are for createTable only and schema is immutable after creation. Conceived by Romuald Członkowski - https://www.aiadvisors.pl/en Co-Authored-By: Claude Opus 4.6 (1M context) --- CHANGELOG.md | 11 ++ package.json | 2 +- src/http-server-single-session.ts | 54 ++++++++- src/mcp/handlers-n8n-manager.ts | 5 +- src/mcp/tools-n8n-manager.ts | 4 +- .../http-server-session-management.test.ts | 106 +++++++++++++++++- .../mcp/handlers-manage-datatable.test.ts | 16 +++ 7 files changed, 188 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..017d1ce 100644 --- a/src/http-server-single-session.ts +++ b/src/http-server-single-session.ts @@ -253,6 +253,21 @@ 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). + * Notifications have a `method` field but no `id` field per JSON-RPC 2.0 spec. + */ + private isJsonRpcNotification(body: unknown): boolean { + if (!body || typeof body !== 'object') return false; + if (Array.isArray(body)) { + return body.length > 0 && body.every( + (msg: any) => msg && typeof msg.method === 'string' && !('id' in msg) + ); + } + const msg = body as Record; + return typeof msg.method === 'string' && !('id' in msg); + } /** * Sanitize error information for client responses @@ -614,6 +629,24 @@ 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 + 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 +660,36 @@ export class SingleSessionHTTPServer { this.updateSessionAccess(sessionId); } else { - // Invalid request - no session ID and not an initialize request + // 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). + 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..3483c9d 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,111 @@ 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); + }); + }); }); \ 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' }); + }); }); // ========================================================================