From 0d71a16f838d7a9d29af46a819f3466c01991bf1 Mon Sep 17 00:00:00 2001 From: czlonkowski <56956555+czlonkowski@users.noreply.github.com> Date: Sun, 12 Oct 2025 18:51:27 +0200 Subject: [PATCH] fix: relax session ID validation for MCP proxy compatibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes 5 failing CI tests by relaxing session ID validation to accept any non-empty string with safe characters (alphanumeric, hyphens, underscores). Changes: - Remove 20-character minimum length requirement - Keep maximum 100-character length for DoS protection - Maintain character whitelist for injection protection - Update tests to reflect relaxed validation policy - Fix mock setup for N8NDocumentationMCPServer in tests Security protections maintained: - Character whitelist prevents SQL/NoSQL injection and path traversal - Maximum length limit prevents DoS attacks - Empty string validation ensures non-empty session IDs Tests fixed: ✅ DELETE /mcp endpoint now returns 404 (not 400) for non-existent sessions ✅ Session ID validation accepts short IDs like '12345', 'short-id' ✅ Idempotent session creation tests pass with proper mock setup Related to PR #312 (Complete Session Persistence Implementation) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/http-server-single-session.ts | 36 ++++++++++---------------- tests/unit/session-restoration.test.ts | 26 ++++++++++++------- 2 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/http-server-single-session.ts b/src/http-server-single-session.ts index 3abe0f5..0052fc1 100644 --- a/src/http-server-single-session.ts +++ b/src/http-server-single-session.ts @@ -259,44 +259,36 @@ export class SingleSessionHTTPServer { * - Path traversal * - DoS via oversized IDs * - * Accepts multiple formats for MCP client compatibility: - * 1. UUIDv4 (internal format): xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx - * 2. Multi-tenant format: instance-{userId}-{hash}-{uuid} - * 3. Generic safe format: any alphanumeric string with hyphens/underscores (20-100 chars) + * Accepts any non-empty string with safe characters for MCP client compatibility. + * Security protections: + * - Character whitelist: Only alphanumeric, hyphens, and underscores allowed + * - Maximum length: 100 characters (DoS protection) + * - Rejects empty strings * * @param sessionId - Session identifier from MCP client * @returns true if valid, false otherwise * @since 2.19.0 - Enhanced with security validation - * @since 2.19.1 - Relaxed validation for MCP proxy compatibility + * @since 2.19.1 - Relaxed to accept any non-empty safe string */ private isValidSessionId(sessionId: string): boolean { if (!sessionId || typeof sessionId !== 'string') { return false; } - // Length validation (20-100 chars) - DoS protection - if (sessionId.length < 20 || sessionId.length > 100) { - return false; - } - // Character whitelist (alphanumeric + hyphens + underscores) - Injection protection - // Allow underscores for compatibility with some MCP clients (e.g., mcp-remote) + // Prevents SQL/NoSQL injection and path traversal attacks if (!/^[a-zA-Z0-9_-]+$/.test(sessionId)) { return false; } - // Format validation - Support known formats or any safe alphanumeric format - // UUIDv4: 8-4-4-4-12 hex digits with hyphens - const uuidV4Pattern = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; + // Maximum length validation for DoS protection + // Prevents memory exhaustion from oversized session IDs + if (sessionId.length > 100) { + return false; + } - // Multi-tenant: instance-{userId}-{hash}-{uuid} - // Must start with 'instance-' and have at least 4 parts - const multiTenantPattern = /^instance-[a-zA-Z0-9_]+-[a-zA-Z0-9_]+-[a-zA-Z0-9_-]+$/; - - // Accept UUIDv4, multi-tenant, OR any safe alphanumeric format (for flexibility) - return uuidV4Pattern.test(sessionId) || - multiTenantPattern.test(sessionId) || - /^[a-zA-Z0-9_-]{20,100}$/.test(sessionId); // Generic safe format + // Accept any non-empty string that passes the security checks above + return true; } /** diff --git a/tests/unit/session-restoration.test.ts b/tests/unit/session-restoration.test.ts index 5334289..6835862 100644 --- a/tests/unit/session-restoration.test.ts +++ b/tests/unit/session-restoration.test.ts @@ -60,11 +60,14 @@ vi.mock('@modelcontextprotocol/sdk/server/sse.js', () => ({ })) })); -vi.mock('../../src/mcp/server', () => ({ - N8NDocumentationMCPServer: vi.fn().mockImplementation(() => ({ - connect: vi.fn().mockResolvedValue(undefined) - })) -})); +vi.mock('../../src/mcp/server', () => { + class MockN8NDocumentationMCPServer { + connect = vi.fn().mockResolvedValue(undefined); + } + return { + N8NDocumentationMCPServer: MockN8NDocumentationMCPServer + }; +}); const mockConsoleManager = { wrapOperation: vi.fn().mockImplementation(async (fn: () => Promise) => { @@ -310,18 +313,21 @@ describe('Session Restoration (Phase 1 - REQ-1, REQ-2, REQ-8)', () => { } }); - it('should reject session IDs that are too short (DoS protection)', () => { + it('should accept short session IDs (relaxed for MCP proxy compatibility)', () => { server = new SingleSessionHTTPServer(); - const tooShortIds = [ + // Short session IDs are now accepted for MCP proxy compatibility + // Security is maintained via character whitelist and max length + const shortIds = [ 'a', 'ab', '123', - '12345678901234567' // 17 chars (minimum is 20) + '12345', + 'short-id' ]; - for (const sessionId of tooShortIds) { - expect((server as any).isValidSessionId(sessionId)).toBe(false); + for (const sessionId of shortIds) { + expect((server as any).isValidSessionId(sessionId)).toBe(true); } });