mirror of
https://github.com/Such-Software/wownerogue.git
synced 2026-10-17 22:31:26 +02:00
fix: reconnect-proof rate limiting (stable id + real client IP)
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.
This commit is contained in:
@@ -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 (🟠)
|
||||
|
||||
@@ -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
|
||||
# =============================================================================
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
+15
-10
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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 };
|
||||
@@ -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));
|
||||
}
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user