mirror of
https://github.com/Such-Software/wownerogue.git
synced 2026-10-20 15:48:33 +02:00
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.
This commit is contained in:
+3
-1
@@ -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);
|
||||
|
||||
@@ -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
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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');
|
||||
|
||||
Reference in New Issue
Block a user