fix: relax session ID validation for MCP proxy compatibility

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 <noreply@anthropic.com>
This commit is contained in:
czlonkowski
2025-10-12 18:51:27 +02:00
parent 085f6db7a2
commit 0d71a16f83
2 changed files with 30 additions and 32 deletions

View File

@@ -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;
}
/**

View File

@@ -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<any>) => {
@@ -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);
}
});