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:
czlonkowski
2025-11-24 18:28:13 +01:00
parent f5cf1e2934
commit 5d2c5df53e
3 changed files with 74 additions and 23 deletions

View File

@@ -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
}
}

View File

@@ -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)

View File

@@ -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)