mirror of
https://github.com/czlonkowski/n8n-mcp.git
synced 2026-01-30 06:22:04 +00:00
feat: implement 7 critical session persistence API fixes for production readiness
This commit implements all 7 critical fixes identified in the code review to make the session persistence API production-ready for zero-downtime container deployments in multi-tenant environments. Fixes implemented: 1. Made instanceId optional in SessionState interface 2. Removed redundant validation, properly using validateInstanceContext() 3. Fixed race condition in MAX_SESSIONS check using real-time count 4. Added comprehensive security logging with logSecurityEvent() helper 5. Added duplicate session ID detection during export with Set tracking 6. Added date parsing validation with isNaN checks for Invalid Date objects 7. Restructured null checks for proper TypeScript type narrowing Changes: - src/types/session-state.ts: Made instanceId optional - src/http-server-single-session.ts: Implemented all validation and security fixes - tests/unit/http-server/session-persistence.test.ts: Fixed MAX_SESSIONS test All 13 session persistence unit tests passing. All 9 MCP engine session persistence tests passing. Conceived by Romuald Członkowski - https://www.aiadvisors.pl/en 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -72,6 +72,30 @@ function extractMultiTenantHeaders(req: express.Request): MultiTenantHeaders {
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Security logging helper for audit trails
|
||||
* Provides structured logging for security-relevant events
|
||||
*/
|
||||
function logSecurityEvent(
|
||||
event: 'session_export' | 'session_restore' | 'session_restore_failed' | 'max_sessions_reached',
|
||||
details: {
|
||||
sessionId?: string;
|
||||
reason?: string;
|
||||
count?: number;
|
||||
instanceId?: string;
|
||||
}
|
||||
): void {
|
||||
const timestamp = new Date().toISOString();
|
||||
const logEntry = {
|
||||
timestamp,
|
||||
event,
|
||||
...details
|
||||
};
|
||||
|
||||
// Log to standard logger with [SECURITY] prefix for easy filtering
|
||||
logger.info(`[SECURITY] ${event}`, logEntry);
|
||||
}
|
||||
|
||||
export class SingleSessionHTTPServer {
|
||||
// Map to store transports by session ID (following SDK pattern)
|
||||
private transports: { [sessionId: string]: StreamableHTTPServerTransport } = {};
|
||||
@@ -1443,9 +1467,16 @@ export class SingleSessionHTTPServer {
|
||||
*/
|
||||
public exportSessionState(): SessionState[] {
|
||||
const sessions: SessionState[] = [];
|
||||
const seenSessionIds = new Set<string>();
|
||||
|
||||
// Iterate over all sessions with metadata (source of truth for active sessions)
|
||||
for (const sessionId of Object.keys(this.sessionMetadata)) {
|
||||
// Check for duplicates (defensive programming)
|
||||
if (seenSessionIds.has(sessionId)) {
|
||||
logger.warn(`Duplicate sessionId detected during export: ${sessionId}`);
|
||||
continue;
|
||||
}
|
||||
|
||||
// Skip expired sessions - they're not worth persisting
|
||||
if (this.isSessionExpired(sessionId)) {
|
||||
continue;
|
||||
@@ -1461,6 +1492,7 @@ export class SingleSessionHTTPServer {
|
||||
continue;
|
||||
}
|
||||
|
||||
seenSessionIds.add(sessionId);
|
||||
sessions.push({
|
||||
sessionId,
|
||||
metadata: {
|
||||
@@ -1478,6 +1510,7 @@ export class SingleSessionHTTPServer {
|
||||
}
|
||||
|
||||
logger.info(`Exported ${sessions.length} session(s) for persistence`);
|
||||
logSecurityEvent('session_export', { count: sessions.length });
|
||||
return sessions;
|
||||
}
|
||||
|
||||
@@ -1502,7 +1535,6 @@ export class SingleSessionHTTPServer {
|
||||
*/
|
||||
public restoreSessionState(sessions: SessionState[]): number {
|
||||
let restoredCount = 0;
|
||||
const currentSessionCount = Object.keys(this.transports).length;
|
||||
|
||||
for (const sessionState of sessions) {
|
||||
try {
|
||||
@@ -1512,11 +1544,12 @@ export class SingleSessionHTTPServer {
|
||||
continue;
|
||||
}
|
||||
|
||||
// Check if we've hit the MAX_SESSIONS limit
|
||||
if (currentSessionCount + restoredCount >= MAX_SESSIONS) {
|
||||
// Check if we've hit the MAX_SESSIONS limit (check real-time count)
|
||||
if (Object.keys(this.sessionMetadata).length >= MAX_SESSIONS) {
|
||||
logger.warn(
|
||||
`Reached MAX_SESSIONS limit (${MAX_SESSIONS}), skipping remaining sessions`
|
||||
);
|
||||
logSecurityEvent('max_sessions_reached', { count: MAX_SESSIONS });
|
||||
break;
|
||||
}
|
||||
|
||||
@@ -1526,10 +1559,19 @@ export class SingleSessionHTTPServer {
|
||||
continue;
|
||||
}
|
||||
|
||||
// Validate session isn't expired
|
||||
// Parse and validate dates first
|
||||
const createdAt = new Date(sessionState.metadata.createdAt);
|
||||
const lastAccess = new Date(sessionState.metadata.lastAccess);
|
||||
const age = Date.now() - lastAccess.getTime();
|
||||
|
||||
if (isNaN(createdAt.getTime()) || isNaN(lastAccess.getTime())) {
|
||||
logger.warn(
|
||||
`Skipping session ${sessionState.sessionId} - invalid date format`
|
||||
);
|
||||
continue;
|
||||
}
|
||||
|
||||
// Validate session isn't expired
|
||||
const age = Date.now() - lastAccess.getTime();
|
||||
if (age > this.sessionTimeout) {
|
||||
logger.debug(
|
||||
`Skipping session ${sessionState.sessionId} - expired (age: ${Math.round(age / 1000)}s)`
|
||||
@@ -1537,33 +1579,30 @@ export class SingleSessionHTTPServer {
|
||||
continue;
|
||||
}
|
||||
|
||||
// Validate required context fields
|
||||
if (
|
||||
!sessionState.context ||
|
||||
!sessionState.context.n8nApiUrl ||
|
||||
!sessionState.context.n8nApiKey
|
||||
) {
|
||||
logger.warn(
|
||||
`Skipping session ${sessionState.sessionId} - missing required context fields`
|
||||
);
|
||||
// Validate context exists (TypeScript null narrowing)
|
||||
if (!sessionState.context) {
|
||||
logger.warn(`Skipping session ${sessionState.sessionId} - missing context`);
|
||||
continue;
|
||||
}
|
||||
|
||||
// Validate context structure using existing validation
|
||||
try {
|
||||
validateInstanceContext(sessionState.context);
|
||||
} catch (error) {
|
||||
const validation = validateInstanceContext(sessionState.context);
|
||||
if (!validation.valid) {
|
||||
const reason = validation.errors?.join(', ') || 'invalid context';
|
||||
logger.warn(
|
||||
`Skipping session ${sessionState.sessionId} - invalid context:`,
|
||||
error
|
||||
`Skipping session ${sessionState.sessionId} - invalid context: ${reason}`
|
||||
);
|
||||
logSecurityEvent('session_restore_failed', {
|
||||
sessionId: sessionState.sessionId,
|
||||
reason
|
||||
});
|
||||
continue;
|
||||
}
|
||||
|
||||
// Restore session metadata
|
||||
this.sessionMetadata[sessionState.sessionId] = {
|
||||
createdAt: new Date(sessionState.metadata.createdAt),
|
||||
lastAccess: new Date(sessionState.metadata.lastAccess)
|
||||
createdAt,
|
||||
lastAccess
|
||||
};
|
||||
|
||||
// Restore session context
|
||||
@@ -1576,9 +1615,17 @@ export class SingleSessionHTTPServer {
|
||||
};
|
||||
|
||||
logger.debug(`Restored session ${sessionState.sessionId}`);
|
||||
logSecurityEvent('session_restore', {
|
||||
sessionId: sessionState.sessionId,
|
||||
instanceId: sessionState.context.instanceId
|
||||
});
|
||||
restoredCount++;
|
||||
} catch (error) {
|
||||
logger.error(`Failed to restore session ${sessionState.sessionId}:`, error);
|
||||
logSecurityEvent('session_restore_failed', {
|
||||
sessionId: sessionState.sessionId,
|
||||
reason: error instanceof Error ? error.message : 'unknown error'
|
||||
});
|
||||
// Continue with next session - don't let one failure break the entire restore
|
||||
}
|
||||
}
|
||||
|
||||
@@ -75,7 +75,7 @@ export interface SessionState {
|
||||
* Instance identifier (optional)
|
||||
* Custom identifier for tracking which n8n instance this session belongs to
|
||||
*/
|
||||
instanceId: string;
|
||||
instanceId?: string;
|
||||
|
||||
/**
|
||||
* Session-specific ID (optional)
|
||||
|
||||
@@ -429,8 +429,12 @@ describe('SingleSessionHTTPServer - Session Persistence', () => {
|
||||
it('should respect MAX_SESSIONS limit during restore', () => {
|
||||
// Create 99 existing sessions (MAX_SESSIONS is 100)
|
||||
const serverAny = server as any;
|
||||
const now = new Date();
|
||||
for (let i = 0; i < 99; i++) {
|
||||
serverAny.transports[`existing-${i}`] = {}; // Mock transport
|
||||
serverAny.sessionMetadata[`existing-${i}`] = {
|
||||
createdAt: now,
|
||||
lastAccess: now
|
||||
};
|
||||
}
|
||||
|
||||
// Try to restore 3 sessions (should only restore 1 due to limit)
|
||||
|
||||
Reference in New Issue
Block a user