From 895aa2af3d671ce5a7a29f79382baf612caccf4e Mon Sep 17 00:00:00 2001 From: jwinterm Date: Sun, 31 May 2026 13:46:53 -0400 Subject: [PATCH] fix: input validation + memory-leak/concurrency hardening - register_client now validates the client-supplied clientId (type, length, charset) before it enters clientSocketMap as both key and value, closing a map-poisoning / mapping-impersonation vector. - mempoolNotified is now a Map with real TTL eviction owned by PaymentHandlers (the actual dedup set). The dead duplicate Set in SocketHandlers and its random-10%-per-cycle eviction (which could drop live entries) are removed; addresses are also evicted promptly when monitoring stops. - The SessionManager `sessions` cache is evicted on disconnect (removeSocket, called after the suspend logic reads it) so it no longer grows unbounded with every socket ever seen. - sessionManager.dispose(), suspendedGameManager.cleanup() and paymentHandlers.dispose() are wired into SocketHandlers.shutdown() so their timers/maps don't leak on shutdown. - checkGamesTimeout snapshots activeGames before iterating and awaits each game-over sequentially (was mutating the Map during forEach and firing async game-overs concurrently, which could skip games / double-process). Adds inputValidation.test.js. Full suite: 19 suites / 149 tests. --- docs/REMEDIATION_PLAN.md | 2 +- src/index.js | 4 ++- src/network/connectionHandler.js | 27 ++++++++++------ src/network/paymentHandlers.js | 14 ++++++-- src/network/sessionManager.js | 9 ++++++ src/network/socketHandlers.js | 44 ++++++++++++++----------- test/inputValidation.test.js | 55 ++++++++++++++++++++++++++++++++ 7 files changed, 124 insertions(+), 31 deletions(-) create mode 100644 test/inputValidation.test.js diff --git a/docs/REMEDIATION_PLAN.md b/docs/REMEDIATION_PLAN.md index 1ca839c..6681a75 100644 --- a/docs/REMEDIATION_PLAN.md +++ b/docs/REMEDIATION_PLAN.md @@ -35,7 +35,7 @@ Key: `S` ≤ half day · `M` ~1–3 days · `L` ~1 week+ · 🔴 launch-blocker ## Phase 2 — Identity, abuse & concurrency (🟠) - [x] **2.1 L — Unify identity resolution on stable `user_id`.** ✅ `getOrCreateUser` now resolves through SessionManager's stable `anon_token`→`id` identity first (re-selecting a fresh row by stable id), falling back to the legacy `socket_id` lookup only when no session exists — eliminating the duplicate "orphan" user row that caused credit/payout desync. `gameModeManager.sessionManager` wired in SocketHandlers. `anon_token` is already `UNIQUE` (migration 003), so the stable-identity constraint exists. New `identityResolution.test.js` proves no duplicate row is created when a session exists. _Deferred: re-keying the in-memory `activeGames` map by `user_id` (invasive; already mitigated in 0.5 by stamping `dbUserId` on the game) — tracked as a follow-up._ - [x] **2.2 M — Reconnect-proof rate limiting.** ✅ New `rateLimitContext` helper resolves a STABLE id (`anon_token`→`id`, falling back to socket id) + the real client IP. All socket-event limiters (game:start, address:set, chat:message) now pass both — previously they passed `socket.id` and **no IP at all**, so reconnecting reset every limit and the "IP-limited" prefixes never actually applied. `chat:` added to IP-limited prefixes; `connection:new` uses the real IP. Express `trust proxy` + XFF honored only when `TRUST_PROXY=true` (documented in `.env.example`). Contradictory default limit values aligned. New `rateLimit.test.js` (reconnect can't bypass, IP backstop, XFF gating). -- [ ] **2.3 M — Input validation + leak fixes.** Validate socket payloads (`connectionHandler.js:90`); TTL-evict `mempoolNotified` (`socketHandlers.js:223`); evict `sessions` on disconnect; wire all `dispose()` into `shutdown()`; snapshot in `checkGamesTimeout` (`socketHandlers.js:1012`). +- [x] **2.3 M — Input validation + leak fixes.** ✅ `register_client` validates `clientId` (type/length/charset) before it enters `clientSocketMap` — closes the map-poisoning vector. `mempoolNotified` is now a Map with real TTL eviction in PaymentHandlers (the dead duplicate Set + its random-10% eviction removed from SocketHandlers). `sessions` cache evicted on disconnect (`removeSocket`, called after suspend reads it). `sessionManager.dispose()` / `suspendedGameManager.cleanup()` / `paymentHandlers.dispose()` wired into `shutdown()`. `checkGamesTimeout` snapshots before iterating + awaits sequentially (was mutate-during-`forEach` + fire-and-forget). New `inputValidation.test.js`. ## Phase 3 — Game integrity (🟠) - [ ] **3.1 S — Uniform time budget** (elapsed/fixed-block, not "next block"); fix early-entry (`src/network/queueHandler.js:241`). diff --git a/src/index.js b/src/index.js index d97d119..477afdd 100644 --- a/src/index.js +++ b/src/index.js @@ -1816,7 +1816,9 @@ debugManager.onNewBlockCallback((blockHeight) => { // Start games for waiting players when new block detected socketHandlers.startGamesForWaiting(blockHeight); // Check active games for timeout - socketHandlers.checkGamesTimeout(blockHeight); + Promise.resolve(socketHandlers.checkGamesTimeout(blockHeight)).catch(err => { + console.error('❌ checkGamesTimeout error:', err.message); + }); // Flush any pending payouts (safety net for batch timer) gameModeManager._processPendingPayouts().catch(err => { console.error('❌ Block-triggered payout processing error:', err.message); diff --git a/src/network/connectionHandler.js b/src/network/connectionHandler.js index 1f9cfe8..1010217 100644 --- a/src/network/connectionHandler.js +++ b/src/network/connectionHandler.js @@ -82,18 +82,27 @@ class ConnectionHandler { * Handle client registration with cleanup */ handleRegisterClient(socket, data) { - if (this.debugManager.CONSOLE_LOGGING) { - console.log(`Client registered: ${socket.id} (server) <-> ${data.clientId} (client)`); + // Validate the client-supplied id before it goes into clientSocketMap as both key + // and value. Without this a client could send a huge or colliding id to poison the + // map (and getUserBySocket follows that mapping). + const clientId = data && data.clientId; + if (typeof clientId !== 'string' || clientId.length === 0 || clientId.length > 64 || !/^[A-Za-z0-9_-]+$/.test(clientId)) { + socket.emit('socket_registered', { serverId: socket.id, success: false, error: 'Invalid clientId' }); + return; } - + + if (this.debugManager.CONSOLE_LOGGING) { + console.log(`Client registered: ${socket.id} (server) <-> ${clientId} (client)`); + } + // Clean up any existing mappings for these IDs to prevent memory leaks - this._cleanupExistingMappings(socket.id, data.clientId); - - this.clientSocketMap.set(data.clientId, socket.id); - this.clientSocketMap.set(socket.id, data.clientId); - + this._cleanupExistingMappings(socket.id, clientId); + + this.clientSocketMap.set(clientId, socket.id); + this.clientSocketMap.set(socket.id, clientId); + socket.emit('socket_registered', { - clientId: data.clientId, + clientId: clientId, serverId: socket.id, success: true }); diff --git a/src/network/paymentHandlers.js b/src/network/paymentHandlers.js index 90f37bd..fc9a8ba 100644 --- a/src/network/paymentHandlers.js +++ b/src/network/paymentHandlers.js @@ -15,7 +15,10 @@ class PaymentHandlers { this.queueManager = queueManager; this.broadcastManager = broadcastManager; this.sessionManager = sessionManager; - this.mempoolNotified = new Set(); + // Address -> timestamp of the first mempool notification, so we notify/queue once + // per payment address. A Map with TTL eviction (below) instead of an unbounded Set. + this.mempoolNotified = new Map(); + this._mempoolNotifiedTtlMs = 60 * 60 * 1000; // 1 hour // Track underpaid payments we've already warned about (notify once, keep monitoring for top-up) this.underpaidNotified = new Set(); this.paymentMonitors = new Map(); @@ -37,6 +40,12 @@ class PaymentHandlers { this.confirmedPayments.delete(pid); } } + // TTL-evict stale mempool-notified addresses (replaces the old random eviction). + for (const [addr, ts] of this.mempoolNotified.entries()) { + if (now - ts > this._mempoolNotifiedTtlMs) { + this.mempoolNotified.delete(addr); + } + } }, 30 * 60 * 1000); // sweep every 30 min if (this._cleanupInterval && this._cleanupInterval.unref) { // Allow process (including Jest) to exit naturally without waiting for this interval @@ -280,7 +289,7 @@ class PaymentHandlers { this.walletService.startPaymentMonitoring(paymentRequest.address, async (status) => { if (status.in_mempool && !status.confirmed) { if (this.mempoolNotified.has(paymentRequest.address)) return; - this.mempoolNotified.add(paymentRequest.address); + this.mempoolNotified.set(paymentRequest.address, Date.now()); // Check payment type — credits_package should NOT queue for a game const mapping = this.socketPaymentMap.get(socket.id); @@ -496,6 +505,7 @@ class PaymentHandlers { if (mapping) { this.walletService.stopPaymentMonitoring(mapping.address); this.socketPaymentMap.delete(socketId); + this.mempoolNotified.delete(mapping.address); if (mapping.paymentId != null) { this.underpaidNotified.delete(mapping.paymentId); } diff --git a/src/network/sessionManager.js b/src/network/sessionManager.js index 8a2a65a..7758a13 100644 --- a/src/network/sessionManager.js +++ b/src/network/sessionManager.js @@ -413,6 +413,15 @@ class SessionManager { return newToken; } + /** + * Evict a socket's cached session row. Called on disconnect so the in-memory + * `sessions` map doesn't grow unbounded with every socket ever seen (and so stale + * cached rows aren't served on reconnect). Safe: a later getBySocket re-reads from DB. + */ + removeSocket(socketId) { + this.sessions.delete(socketId); + } + dispose() { if (this.cleanupInterval) { clearInterval(this.cleanupInterval); diff --git a/src/network/socketHandlers.js b/src/network/socketHandlers.js index 6c67ff0..53f9ca9 100644 --- a/src/network/socketHandlers.js +++ b/src/network/socketHandlers.js @@ -59,8 +59,7 @@ class SocketHandlers { }); // Legacy WAITING_PLAYERS removed; queueManager owns queue state - this.mempoolNotified = new Set(); // track payment addresses already queued - + // Initialize session manager first if (this.gameModeManager && this.gameModeManager.db) { this.sessionManager = new SessionManager({ @@ -233,15 +232,8 @@ class SocketHandlers { 300000 ); - // Cleanup mempool notifications (keep for 10 minutes) - this.memoryManager.registerCleanup( - 'mempoolNotifications', - MemoryManager.createSetCleanup(this.mempoolNotified, (addr) => { - // This is a simple heuristic - in production you'd want to track timestamps - return Math.random() < 0.1; // Randomly clean ~10% each cycle - }), - 600000 - ); + // (mempool-notification dedup + TTL eviction lives in PaymentHandlers, which owns + // the actual dedup set; the old SocketHandlers copy here was unused and is removed.) // Let other components register their cleanups if (this.rateLimiter) { @@ -849,6 +841,12 @@ class SocketHandlers { if (this.spectatorManager) { this.spectatorManager.handleDisconnect(socketId); } + + // Evict the cached session row LAST (after the suspend logic above read it), + // so the sessions map doesn't grow unbounded with every socket ever seen. + if (this.sessionManager?.removeSocket) { + this.sessionManager.removeSocket(socketId); + } }); } @@ -1036,20 +1034,26 @@ class SocketHandlers { /** * Check for game timeouts based on block height */ - checkGamesTimeout(currentHeight) { - this.activeGames.forEach((game, socketId) => { + async checkGamesTimeout(currentHeight) { + // Snapshot the entries first: handleGameOver mutates activeGames (it deletes the + // entry), so iterating the live Map could skip games. Process sequentially and + // await each game-over rather than firing them all off concurrently on one tick. + const snapshot = Array.from(this.activeGames.entries()); + for (const [socketId, game] of snapshot) { const user = this.connectionHandler.getUserBySocket(socketId); - if (user && user.blockRec && currentHeight > user.blockRec) { if (this.debugManager.CONSOLE_LOGGING) { console.log(`💀 GAME TIMEOUT for player ${socketId}: entered on block ${user.blockRec}, died on block ${currentHeight}`); } - game.gameState = 'lost'; const fakeSocket = { id: socketId }; - this.handleGameOver(fakeSocket, game, 'lost', 'timeout', 'You didn\'t escape before the block time limit!'); + try { + await this.handleGameOver(fakeSocket, game, 'lost', 'timeout', 'You didn\'t escape before the block time limit!'); + } catch (e) { + console.error(`checkGamesTimeout: handleGameOver failed for ${socketId}:`, e.message); + } } - }); + } } // Payment system handlers (placeholder for compatibility) @@ -1192,10 +1196,14 @@ class SocketHandlers { this.memoryManager.shutdown(); } + // Dispose components whose timers/maps would otherwise leak on shutdown. + if (this.sessionManager?.dispose) this.sessionManager.dispose(); + if (this.suspendedGameManager?.cleanup) this.suspendedGameManager.cleanup(); + if (this.paymentHandlers?.dispose) this.paymentHandlers.dispose(); + // Clear remaining data structures this.activeGames.clear(); this.playerMoveTimestamps.clear(); - this.mempoolNotified.clear(); if (this.debugManager.CONSOLE_LOGGING) { console.log('✅ SocketHandlers shutdown complete'); diff --git a/test/inputValidation.test.js b/test/inputValidation.test.js new file mode 100644 index 0000000..2f9bbd3 --- /dev/null +++ b/test/inputValidation.test.js @@ -0,0 +1,55 @@ +/** + * Input validation / hardening tests (Phase 2.3). + * + * register_client must validate the client-supplied id before it goes into clientSocketMap + * (as both key and value), or a client could poison the map / impersonate mappings. + */ + +const ConnectionHandler = require('../src/network/connectionHandler'); + +function buildHandler() { + const ch = new ConnectionHandler({ + io: { sockets: { sockets: new Map() } }, + broadcastManager: { broadcastUserCount: () => {} }, + debugManager: { CONSOLE_LOGGING: false }, + sessionManager: null, + rateLimiter: null + }); + return ch; +} + +function fakeSocket(id) { + const emitted = []; + return { id, emit: (event, data) => emitted.push({ event, data }), _emitted: emitted }; +} + +describe('handleRegisterClient validation', () => { + let ch; + afterEach(() => ch && ch.shutdown && ch.shutdown()); + + test('accepts a valid clientId and records the mapping', () => { + ch = buildHandler(); + const sock = fakeSocket('sock1'); + ch.handleRegisterClient(sock, { clientId: 'client-abc_123' }); + + expect(ch.clientSocketMap.get('client-abc_123')).toBe('sock1'); + expect(ch.clientSocketMap.get('sock1')).toBe('client-abc_123'); + expect(sock._emitted.find(e => e.event === 'socket_registered').data.success).toBe(true); + }); + + test.each([ + ['missing', undefined], + ['empty', ''], + ['too long', 'x'.repeat(65)], + ['bad chars', 'evil