From 448cb2f5283d07d9af20bc0738a9a2084fac87f9 Mon Sep 17 00:00:00 2001 From: jwinterm Date: Sun, 31 May 2026 10:21:28 -0400 Subject: [PATCH] fix: reconnect-proof rate limiting (stable id + real client IP) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Socket-event rate limits keyed on socket.id (new on every reconnect) and passed NO ip argument, so the "IP-limited" prefixes never actually applied — reconnecting reset every limit, and chat had no IP limit at all. - New rateLimitContext helper resolves a STABLE identity (anon_token -> id, falling back to socket id) and the real client IP (X-Forwarded-For honoured only when TRUST_PROXY=true, else handshake address — no spoofing on direct exposure). - game:start, address:set and chat:message now pass (stableId, action, ip); connection:new uses the real IP. 'chat:' added to IP-limited prefixes. - Express 'trust proxy' enabled when TRUST_PROXY=true; documented in .env.example. Aligned the contradictory default limit values with the production override. Adds rateLimit.test.js (reconnect can't bypass, IP backstop for fresh identities, XFF gating). Full suite: 18 suites / 143 tests. --- docs/REMEDIATION_PLAN.md | 2 +- src/.env.example | 5 +++ src/index.js | 5 +++ src/network/chatHandler.js | 25 +++++++------ src/network/connectionHandler.js | 6 ++-- src/network/rateLimitContext.js | 35 ++++++++++++++++++ src/network/rateLimiter.js | 11 +++--- src/network/socketHandlers.js | 20 +++++++---- test/rateLimit.test.js | 61 ++++++++++++++++++++++++++++++++ 9 files changed, 146 insertions(+), 24 deletions(-) create mode 100644 src/network/rateLimitContext.js create mode 100644 test/rateLimit.test.js diff --git a/docs/REMEDIATION_PLAN.md b/docs/REMEDIATION_PLAN.md index 2346383..1ca839c 100644 --- a/docs/REMEDIATION_PLAN.md +++ b/docs/REMEDIATION_PLAN.md @@ -34,7 +34,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._ -- [ ] **2.2 M — Reconnect-proof rate limiting.** Key on `user_id`/IP; IP-limit chat; set `trust proxy`; de-dup limit configs (`src/network/rateLimiter.js:80`). +- [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`). ## Phase 3 — Game integrity (🟠) diff --git a/src/.env.example b/src/.env.example index 84bcdb5..63d9075 100644 --- a/src/.env.example +++ b/src/.env.example @@ -343,6 +343,11 @@ RPC_POLL_INTERVAL=2000 # Same-origin requests are always allowed. Leave unset if admin panel is on same host. # ALLOWED_ORIGINS=https://admin.example.com +# Set to 'true' ONLY when running behind a trusted reverse proxy (Nginx/Caddy). When +# true, the real client IP is read from the X-Forwarded-For header for rate limiting and +# req.ip. Leave unset/false for direct exposure, otherwise clients could spoof their IP. +# TRUST_PROXY=true + # ============================================================================= # SECTION 12: SERVER & MULTI-INSTANCE # ============================================================================= diff --git a/src/index.js b/src/index.js index c92e7de..d97d119 100644 --- a/src/index.js +++ b/src/index.js @@ -2,6 +2,11 @@ const express = require('express'); const path = require('path'); const crypto = require('crypto'); var app = express(); +// Trust the reverse proxy (for correct req.ip / X-Forwarded-For handling) only when +// explicitly enabled, so client IPs can't be spoofed in a direct-exposure deployment. +if (process.env.TRUST_PROXY === 'true') { + app.set('trust proxy', true); +} var http = require('http').Server(app); var io = require('socket.io')(http); diff --git a/src/network/chatHandler.js b/src/network/chatHandler.js index a58be5b..0564c5e 100644 --- a/src/network/chatHandler.js +++ b/src/network/chatHandler.js @@ -4,6 +4,7 @@ */ const ChatHistoryManager = require('./chatHistoryManager'); +const { clientIp, stableId } = require('./rateLimitContext'); class ChatHandler { constructor({ io, broadcastManager, debugManager, addressManager, paymentHandlers, queueManager, gameModeManager, rateLimiter, db }) { @@ -61,17 +62,19 @@ class ChatHandler { */ async handleChatMessage(socket, msg, additionalHandlers = {}) { try { - // Rate limiting for chat messages - const rateLimitResult = await this.rateLimiter.checkLimit(socket.id, 'chat:message'); - + // Rate limiting for chat messages — stable id + IP so reconnecting can't reset it. + const rlId = stableId(socket, this.gameModeManager?.sessionManager); + const rlIp = clientIp(socket); + const rateLimitResult = await this.rateLimiter.checkLimit(rlId, 'chat:message', rlIp); + if (!rateLimitResult.allowed) { - this.broadcastManager.sendStatusUpdate(socket.id, 'warning', + this.broadcastManager.sendStatusUpdate(socket.id, 'warning', `Please slow down! You can send ${rateLimitResult.remaining} more messages after ${Math.ceil(rateLimitResult.retryAfter / 1000)} seconds.`); return; } // Record the chat attempt - await this.rateLimiter.recordAttempt(socket.id, 'chat:message'); + await this.rateLimiter.recordAttempt(rlId, 'chat:message', rlIp); if (this.debugManager.CONSOLE_LOGGING) { console.log('Message received:', msg); @@ -147,16 +150,18 @@ class ChatHandler { } async _handleAddressDetection(socket, detected) { - // Rate limiting for address setting - const rateLimitResult = await this.rateLimiter.checkLimit(socket.id, 'address:set'); - + // Rate limiting for address setting — stable id + IP. + const rlId = stableId(socket, this.gameModeManager?.sessionManager); + const rlIp = clientIp(socket); + const rateLimitResult = await this.rateLimiter.checkLimit(rlId, 'address:set', rlIp); + if (!rateLimitResult.allowed) { - this.broadcastManager.sendStatusUpdate(socket.id, 'warning', + this.broadcastManager.sendStatusUpdate(socket.id, 'warning', `Address changes are rate limited. Try again in ${Math.ceil(rateLimitResult.retryAfter / 1000)} seconds.`); return; } - await this.rateLimiter.recordAttempt(socket.id, 'address:set'); + await this.rateLimiter.recordAttempt(rlId, 'address:set', rlIp); this.addressManager.handleDetection(socket.id, detected); } diff --git a/src/network/connectionHandler.js b/src/network/connectionHandler.js index 0d2a491..1f9cfe8 100644 --- a/src/network/connectionHandler.js +++ b/src/network/connectionHandler.js @@ -4,6 +4,7 @@ */ const user = require('../db/user'); +const { clientIp } = require('./rateLimitContext'); class ConnectionHandler { constructor({ io, broadcastManager, debugManager, sessionManager, rateLimiter }) { @@ -24,8 +25,9 @@ class ConnectionHandler { */ async handleConnection(socket) { try { - // Rate limiting for connections - const ip = socket.handshake.address; + // Rate limiting for connections — use the real client IP (honours a trusted + // reverse proxy via TRUST_PROXY) so per-IP connection limits work behind a proxy. + const ip = clientIp(socket) || socket.handshake.address; const rateLimitResult = await this.rateLimiter.checkLimit(ip, 'connection:new', ip); if (!rateLimitResult.allowed) { diff --git a/src/network/rateLimitContext.js b/src/network/rateLimitContext.js new file mode 100644 index 0000000..41c9b14 --- /dev/null +++ b/src/network/rateLimitContext.js @@ -0,0 +1,35 @@ +/** + * Rate-limit context resolver. + * + * Produces a STABLE identity and the real client IP for rate limiting, so limits cannot + * be bypassed by simply reconnecting (which mints a fresh socket.id). + * + * - stableId: prefers the session's stable user id (anon_token -> users.id); only when no + * session is resolvable does it fall back to the ephemeral socket id. A client that + * drops its token to mint a new identity is still caught by the IP limit. + * - clientIp: the socket handshake address, or the first X-Forwarded-For hop when the + * deployment is behind a trusted reverse proxy (TRUST_PROXY=true). XFF is ONLY trusted + * when TRUST_PROXY is set, since otherwise a client could spoof it. + */ + +function clientIp(socket) { + try { + if (process.env.TRUST_PROXY === 'true') { + const xff = socket.handshake?.headers?.['x-forwarded-for']; + if (xff) return String(xff).split(',')[0].trim(); + } + return socket.handshake?.address || null; + } catch (_) { + return null; + } +} + +function stableId(socket, sessionManager) { + try { + const u = sessionManager?.sessions?.get(socket.id); + if (u && u.id != null) return `u:${u.id}`; + } catch (_) {} + return `s:${socket.id}`; +} + +module.exports = { clientIp, stableId }; diff --git a/src/network/rateLimiter.js b/src/network/rateLimiter.js index b4167dc..567300d 100644 --- a/src/network/rateLimiter.js +++ b/src/network/rateLimiter.js @@ -5,12 +5,14 @@ class RateLimiter { constructor(options = {}) { - // Default rate limits - can be overridden via options + // Default rate limits. SocketHandlers passes a production `limits` override that + // takes precedence (merged below); these defaults are kept ALIGNED with it so a + // RateLimiter constructed without options (e.g. in tests) behaves the same. this.limits = { 'payment:create': { window: 60000, max: 3 }, // 3 payments per minute - 'game:start': { window: 60000, max: 10 }, // 10 game starts per minute + 'game:start': { window: 60000, max: 15 }, // 15 game starts per minute 'game:queue': { window: 30000, max: 5 }, // 5 queue attempts per 30s - 'chat:message': { window: 10000, max: 8 }, // 8 messages per 10s + 'chat:message': { window: 10000, max: 12 }, // 12 messages per 10s 'address:set': { window: 300000, max: 3 }, // 3 address changes per 5min 'move:player': { window: 1000, max: 50 }, // 50 moves per second (already handled separately) 'connection:new': { window: 60000, max: 10 }, // 10 connections per minute per IP @@ -81,7 +83,8 @@ class RateLimiter { 'connection:', // Connection attempts 'payment:', // Payment creation/operations 'game:', // Game start/queue operations - 'address:' // Address changes + 'address:', // Address changes + 'chat:' // Chat messages (reconnect-proof spam protection) ]; return ipLimitedPrefixes.some(prefix => action.startsWith(prefix)); } diff --git a/src/network/socketHandlers.js b/src/network/socketHandlers.js index 78ab4da..6c67ff0 100644 --- a/src/network/socketHandlers.js +++ b/src/network/socketHandlers.js @@ -11,6 +11,7 @@ const PaymentHandlers = require('./paymentHandlers'); const AddressManager = require('./addressManager'); const SessionManager = require('./sessionManager'); const RateLimiter = require('./rateLimiter'); +const { clientIp, stableId } = require('./rateLimitContext'); const ConnectionHandler = require('./connectionHandler'); const ChatHandler = require('./chatHandler'); const QueueHandler = require('./queueHandler'); @@ -501,8 +502,11 @@ class SocketHandlers { */ async handleAutoStart(socket) { try { - // Rate limiting for game starts - const rateLimitResult = await this.rateLimiter.checkLimit(socket.id, 'game:start'); + // Rate limiting for game starts — keyed on stable identity + IP so reconnecting + // (new socket.id) can't reset the limit. + const rlId = stableId(socket, this.sessionManager); + const rlIp = clientIp(socket); + const rateLimitResult = await this.rateLimiter.checkLimit(rlId, 'game:start', rlIp); if (!rateLimitResult.allowed) { this.broadcastManager.sendStatusUpdate(socket.id, 'warning', `Please wait ${Math.ceil(rateLimitResult.retryAfter / 1000)} seconds before starting another game. (${rateLimitResult.remaining} attempts remaining)`); @@ -559,8 +563,8 @@ class SocketHandlers { } } - // Record the game start attempt - await this.rateLimiter.recordAttempt(socket.id, 'game:start'); + // Record the game start attempt (same stable id + IP as the check above) + await this.rateLimiter.recordAttempt(rlId, 'game:start', rlIp); // Create game immediately const blockHeight = this.debugManager.getCurrentBlockHeight ? this.debugManager.getCurrentBlockHeight() : null; @@ -683,15 +687,17 @@ class SocketHandlers { return; } - const rateLimitResult = await this.rateLimiter.checkLimit(socket.id, 'address:set'); + const rlId = stableId(socket, this.sessionManager); + const rlIp = clientIp(socket); + const rateLimitResult = await this.rateLimiter.checkLimit(rlId, 'address:set', rlIp); if (!rateLimitResult.allowed) { - this.broadcastManager.sendStatusUpdate(socket.id, 'warning', + this.broadcastManager.sendStatusUpdate(socket.id, 'warning', `Address changes are rate limited. Try again in ${Math.ceil(rateLimitResult.retryAfter / 1000)} seconds.`); this.io.to(socket.id).emit('address_update_error', { message: 'Address changes are temporarily rate limited.' }); return; } - await this.rateLimiter.recordAttempt(socket.id, 'address:set'); + await this.rateLimiter.recordAttempt(rlId, 'address:set', rlIp); await this.addressManager.saveAddress(socket.id, address); } catch (err) { diff --git a/test/rateLimit.test.js b/test/rateLimit.test.js new file mode 100644 index 0000000..eab91ef --- /dev/null +++ b/test/rateLimit.test.js @@ -0,0 +1,61 @@ +/** + * Reconnect-proof rate limiting tests (Phase 2.2). + */ + +const RateLimiter = require('../src/network/rateLimiter'); +const { clientIp, stableId } = require('../src/network/rateLimitContext'); + +describe('rateLimitContext.stableId', () => { + test('prefers the session stable user id over the socket id', () => { + const sm = { sessions: new Map([['sockA', { id: 42 }]]) }; + expect(stableId({ id: 'sockA' }, sm)).toBe('u:42'); + }); + test('falls back to socket id when no session', () => { + const sm = { sessions: new Map() }; + expect(stableId({ id: 'sockZ' }, sm)).toBe('s:sockZ'); + expect(stableId({ id: 'sockZ' }, undefined)).toBe('s:sockZ'); + }); +}); + +describe('rateLimitContext.clientIp', () => { + afterEach(() => { delete process.env.TRUST_PROXY; }); + + test('uses handshake address by default (ignores XFF when proxy not trusted)', () => { + expect(clientIp({ handshake: { headers: { 'x-forwarded-for': '9.9.9.9' }, address: '10.0.0.1' } })).toBe('10.0.0.1'); + }); + test('uses first X-Forwarded-For hop when TRUST_PROXY=true', () => { + process.env.TRUST_PROXY = 'true'; + expect(clientIp({ handshake: { headers: { 'x-forwarded-for': '9.9.9.9, 10.0.0.1' }, address: '10.0.0.1' } })).toBe('9.9.9.9'); + }); +}); + +describe('reconnect cannot bypass the limit (stable-id keyed)', () => { + test('two sockets for the same user share the limit', async () => { + const rl = new RateLimiter({ limits: { 'game:start': { window: 60000, max: 2 } } }); + const sm = { sessions: new Map([['sockA', { id: 7 }], ['sockB', { id: 7 }]]) }; + const idA = stableId({ id: 'sockA' }, sm); // u:7 + const idB = stableId({ id: 'sockB' }, sm); // u:7 (reconnected, new socket) + expect(idA).toBe(idB); + + await rl.recordAttempt(idA, 'game:start', '1.2.3.4'); + await rl.recordAttempt(idA, 'game:start', '1.2.3.4'); // at max + const afterReconnect = await rl.checkLimit(idB, 'game:start', '1.2.3.4'); + expect(afterReconnect.allowed).toBe(false); // blocked despite the new socket id + rl.shutdown(); + }); + + test('chat:message is now IP-limited', () => { + const rl = new RateLimiter(); + expect(rl._shouldApplyIpLimit('chat:message')).toBe(true); + rl.shutdown(); + }); + + test('IP limit blocks a fresh anonymous identity from the same IP', async () => { + const rl = new RateLimiter({ limits: { 'game:start': { window: 60000, max: 1 } } }); + // Two different "users" (e.g. cleared token) but same IP. + await rl.recordAttempt('u:1', 'game:start', '5.5.5.5'); + const other = await rl.checkLimit('u:2', 'game:start', '5.5.5.5'); + expect(other.allowed).toBe(false); // IP bucket already exhausted + rl.shutdown(); + }); +});