mirror of
https://github.com/czlonkowski/n8n-mcp.git
synced 2026-03-24 11:23:08 +00:00
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>
This commit is contained in:
@@ -256,17 +256,18 @@ export class SingleSessionHTTPServer {
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Checks if a request body is a JSON-RPC notification (or batch of only notifications).
|
* 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 {
|
private isJsonRpcNotification(body: unknown): boolean {
|
||||||
if (!body || typeof body !== 'object') return false;
|
if (!body || typeof body !== 'object') return false;
|
||||||
|
const isSingleNotification = (msg: any): boolean =>
|
||||||
|
msg && typeof msg.method === 'string' && !('id' in msg);
|
||||||
if (Array.isArray(body)) {
|
if (Array.isArray(body)) {
|
||||||
return body.length > 0 && body.every(
|
return body.length > 0 && body.every(isSingleNotification);
|
||||||
(msg: any) => msg && typeof msg.method === 'string' && !('id' in msg)
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
const msg = body as Record<string, unknown>;
|
return isSingleNotification(body);
|
||||||
return typeof msg.method === 'string' && !('id' in msg);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -629,12 +630,10 @@ 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 and this line
|
// TOCTOU guard: session may have been removed between the check above and here
|
||||||
if (!transport) {
|
if (!transport) {
|
||||||
if (this.isJsonRpcNotification(req.body)) {
|
if (this.isJsonRpcNotification(req.body)) {
|
||||||
logger.info('handleRequest: Session removed during lookup, accepting notification', {
|
logger.info('handleRequest: Session removed during lookup, accepting notification', { sessionId });
|
||||||
sessionId,
|
|
||||||
});
|
|
||||||
res.status(202).end();
|
res.status(202).end();
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
@@ -660,10 +659,7 @@ export class SingleSessionHTTPServer {
|
|||||||
this.updateSessionAccess(sessionId);
|
this.updateSessionAccess(sessionId);
|
||||||
|
|
||||||
} else {
|
} else {
|
||||||
// Check if this is a JSON-RPC notification (no "id" field = fire-and-forget)
|
// Notifications are fire-and-forget; returning 400 triggers reconnection storms (#654)
|
||||||
// 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)) {
|
if (this.isJsonRpcNotification(req.body)) {
|
||||||
logger.info('handleRequest: Accepting notification for stale/missing session', {
|
logger.info('handleRequest: Accepting notification for stale/missing session', {
|
||||||
method: req.body?.method,
|
method: req.body?.method,
|
||||||
|
|||||||
@@ -1287,5 +1287,21 @@ describe('HTTP Server Session Management', () => {
|
|||||||
|
|
||||||
expect(res.status).toHaveBeenCalledWith(400);
|
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);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
Reference in New Issue
Block a user