Compare commits

...

3 Commits

Author SHA1 Message Date
czlonkowski
4d4ef64bf6 fix: data tables available on self-hosted too, not just enterprise/cloud
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-22 22:26:24 +01:00
czlonkowski
984a49d4ec fix: address code review - add spec-compliance comment and mixed-batch test
- 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) <noreply@anthropic.com>
2026-03-22 19:45:57 +01:00
czlonkowski
e52a04a291 fix: return 202 for notifications with stale sessions, warn on updateTable columns (#654)
**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) <noreply@anthropic.com>
2026-03-22 19:21:29 +01:00
8 changed files with 203 additions and 13 deletions

View File

@@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased] ## [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 ## [2.40.2] - 2026-03-22
### Fixed ### Fixed

View File

@@ -1,6 +1,6 @@
{ {
"name": "n8n-mcp", "name": "n8n-mcp",
"version": "2.40.2", "version": "2.40.3",
"description": "Integration between n8n workflow automation and Model Context Protocol (MCP)", "description": "Integration between n8n workflow automation and Model Context Protocol (MCP)",
"main": "dist/index.js", "main": "dist/index.js",
"types": "dist/index.d.ts", "types": "dist/index.d.ts",

View File

@@ -253,6 +253,22 @@ export class SingleSessionHTTPServer {
// This ensures compatibility with all MCP clients and proxies // This ensures compatibility with all MCP clients and proxies
return Boolean(sessionId && sessionId.length > 0); 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 * Sanitize error information for client responses
@@ -614,6 +630,22 @@ export class SingleSessionHTTPServer {
logger.info('handleRequest: Reusing existing transport for session', { sessionId }); logger.info('handleRequest: Reusing existing transport for session', { sessionId });
transport = this.transports[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 // In multi-tenant shared mode, update instance context if provided
const isMultiTenantEnabled = process.env.ENABLE_MULTI_TENANT === 'true'; const isMultiTenantEnabled = process.env.ENABLE_MULTI_TENANT === 'true';
const sessionStrategy = process.env.MULTI_TENANT_SESSION_STRATEGY || 'instance'; const sessionStrategy = process.env.MULTI_TENANT_SESSION_STRATEGY || 'instance';
@@ -627,23 +659,33 @@ export class SingleSessionHTTPServer {
this.updateSessionAccess(sessionId); this.updateSessionAccess(sessionId);
} else { } 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 = { const errorDetails = {
hasSessionId: !!sessionId, hasSessionId: !!sessionId,
isInitialize: isInitialize, isInitialize: isInitialize,
sessionIdValid: sessionId ? this.isValidSessionId(sessionId) : false, sessionIdValid: sessionId ? this.isValidSessionId(sessionId) : false,
sessionExists: sessionId ? !!this.transports[sessionId] : false sessionExists: sessionId ? !!this.transports[sessionId] : false
}; };
logger.warn('handleRequest: Invalid request - no session ID and not initialize', errorDetails); 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'; let errorMessage = 'Bad Request: No valid session ID provided and not an initialize request';
if (sessionId && !this.isValidSessionId(sessionId)) { if (sessionId && !this.isValidSessionId(sessionId)) {
errorMessage = 'Bad Request: Invalid session ID format'; errorMessage = 'Bad Request: Invalid session ID format';
} else if (sessionId && !this.transports[sessionId]) { } else if (sessionId && !this.transports[sessionId]) {
errorMessage = 'Bad Request: Session not found or expired'; errorMessage = 'Bad Request: Session not found or expired';
} }
res.status(400).json({ res.status(400).json({
jsonrpc: '2.0', jsonrpc: '2.0',
error: { error: {

View File

@@ -2834,10 +2834,13 @@ export async function handleUpdateTable(args: unknown, context?: InstanceContext
const client = ensureApiConfigured(context); const client = ensureApiConfigured(context);
const { tableId, name } = updateTableSchema.parse(args); const { tableId, name } = updateTableSchema.parse(args);
const dataTable = await client.updateDataTable(tableId, { name }); const dataTable = await client.updateDataTable(tableId, { name });
const rawArgs = args as Record<string, unknown>;
const hasColumns = rawArgs && typeof rawArgs === 'object' && 'columns' in rawArgs;
return { return {
success: true, success: true,
data: dataTable, 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) { } catch (error) {
return handleDataTableError(error); return handleDataTableError(error);

View File

@@ -14,7 +14,7 @@ export const n8nManageDatatableDoc: ToolDocumentation = {
'Use dryRun: true to preview update/upsert/delete before applying', 'Use dryRun: true to preview update/upsert/delete before applying',
'Filter supports: eq, neq, like, ilike, gt, gte, lt, lte conditions', 'Filter supports: eq, neq, like, ilike, gt, gte, lt, lte conditions',
'Use returnData: true to get affected rows back from update/upsert/delete', 'Use returnData: true to get affected rows back from update/upsert/delete',
'Requires n8n enterprise or cloud with data tables feature' 'Requires n8n instance with data tables feature enabled'
] ]
}, },
full: { full: {
@@ -97,7 +97,7 @@ export const n8nManageDatatableDoc: ToolDocumentation = {
], ],
pitfalls: [ pitfalls: [
'Requires N8N_API_URL and N8N_API_KEY configured', 'Requires N8N_API_URL and N8N_API_KEY configured',
'Feature only available on n8n enterprise or cloud plans', 'Requires n8n instance with data tables feature enabled (available on cloud, enterprise, and self-hosted)',
'deleteTable permanently deletes all rows — cannot be undone', 'deleteTable permanently deletes all rows — cannot be undone',
'deleteRows requires a filter — cannot delete all rows without one', 'deleteRows requires a filter — cannot delete all rows without one',
'Column types cannot be changed after table creation via API', 'Column types cannot be changed after table creation via API',

View File

@@ -609,7 +609,7 @@ export const n8nManagementTools: ToolDefinition[] = [
}, },
{ {
name: 'n8n_manage_datatable', name: 'n8n_manage_datatable',
description: `Manage n8n data tables and rows. Actions: createTable, listTables, getTable, updateTable, deleteTable, getRows, insertRows, updateRows, upsertRows, deleteRows. Requires n8n enterprise/cloud with data tables feature.`, description: `Manage n8n data tables and rows. Actions: createTable, listTables, getTable, updateTable, deleteTable, getRows, insertRows, updateRows, upsertRows, deleteRows.`,
inputSchema: { inputSchema: {
type: 'object', type: 'object',
properties: { properties: {
@@ -619,10 +619,10 @@ export const n8nManagementTools: ToolDefinition[] = [
description: 'Operation to perform', description: 'Operation to perform',
}, },
tableId: { type: 'string', description: 'Data table ID (required for all actions except createTable and listTables)' }, 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: { columns: {
type: 'array', type: 'array',
description: 'For createTable: column definitions', description: 'For createTable only: column definitions (schema is immutable after creation via public API)',
items: { items: {
type: 'object', type: 'object',
properties: { properties: {

View File

@@ -219,6 +219,7 @@ describe('HTTP Server Session Management', () => {
status: vi.fn().mockReturnThis(), status: vi.fn().mockReturnThis(),
json: vi.fn().mockReturnThis(), json: vi.fn().mockReturnThis(),
send: vi.fn().mockReturnThis(), send: vi.fn().mockReturnThis(),
end: vi.fn().mockReturnThis(),
setHeader: vi.fn((key: string, value: string) => { setHeader: vi.fn((key: string, value: string) => {
headers[key.toLowerCase()] = value; headers[key.toLowerCase()] = value;
}), }),
@@ -1170,7 +1171,7 @@ describe('HTTP Server Session Management', () => {
it('should show legacy SSE session when present', async () => { it('should show legacy SSE session when present', async () => {
server = new SingleSessionHTTPServer(); server = new SingleSessionHTTPServer();
// Mock legacy session // Mock legacy session
const mockSession = { const mockSession = {
sessionId: 'sse-session-123', sessionId: 'sse-session-123',
@@ -1180,10 +1181,127 @@ describe('HTTP Server Session Management', () => {
(server as any).session = mockSession; (server as any).session = mockSession;
const sessionInfo = server.getSessionInfo(); const sessionInfo = server.getSessionInfo();
expect(sessionInfo.active).toBe(true); expect(sessionInfo.active).toBe(true);
expect(sessionInfo.sessionId).toBe('sse-session-123'); expect(sessionInfo.sessionId).toBe('sse-session-123');
expect(sessionInfo.age).toBeGreaterThanOrEqual(0); 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<any>) => {
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);
});
});
}); });

View File

@@ -363,6 +363,22 @@ describe('Data Table Handlers (n8n_manage_datatable)', () => {
expect(result.success).toBe(false); expect(result.success).toBe(false);
expect(result.error).toBe('Update failed'); 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' });
});
}); });
// ======================================================================== // ========================================================================