From d58a89b160aa4270b2b5a3ff3934ceb100c1701b Mon Sep 17 00:00:00 2001 From: malaysiaonelove Date: Fri, 24 Jul 2026 19:16:03 +0000 Subject: [PATCH] fix(backend): bound rate-limiter IP map (#154) Closes #154. The previous sliding-window rate limiter stored per-IP state in an unbounded `Map` with only a 60-second background TTL sweep. Under an IP-flood attack the map could grow between sweeps and OOM the process. Switch to `lru-cache@^10` as the backing store, which provides both TTL-based eviction (`ttl: windowMs`) and a hard entry cap (`max: maxEntries`, default 10 000) -- so the worst-case memory footprint is bounded regardless of incoming request rate. Behaviour changes: * `RateLimiter.stop()` is now `cache.clear()` rather than `clearInterval()`; the new implementation has no background eviction interval to halt. * `req.ip ?? "unknown"` is unchanged from the original -- applies a global cap until `app.set("trust proxy", ...)` is configured. * `maxEntries` default of 10 000 is per limiter instance; the two module-instantiated default limiters share a global cap of ~20 000 entries. Tests: * Bounded under flood: inserts 25 000 unique IPs against `maxEntries: 10 000`; asserts size() <= 10 000. * LRU policy: with maxEntries 3, after touching 1.1.1.1 the next insert evicts the real LRU (2.2.2.2); 1.1.1.1 keeps its 429 because the cache survived. * TTL: jest.useFakeTimers() advances past windowMs; lru-cache evicts lazily on next access so a returning IP gets a fresh window without a background sweep. Verified: tsc --noEmit clean; middleware tests 11/11; full backend suite 226/226 pass (was 224, +2 new tests for #154). --- backend/package-lock.json | 23 +++-- backend/package.json | 1 + backend/src/api/middleware/rateLimit.ts | 111 ++++++++++++++++-------- backend/tests/middleware.test.ts | 84 ++++++++++++++++-- 4 files changed, 166 insertions(+), 53 deletions(-) diff --git a/backend/package-lock.json b/backend/package-lock.json index edd0bf1..9cb619b 100644 --- a/backend/package-lock.json +++ b/backend/package-lock.json @@ -13,6 +13,7 @@ "better-sqlite3": "^11.3.0", "cors": "^2.8.5", "express": "^4.18.2", + "lru-cache": "^10.4.3", "nanoid": "^3.3.7", "pino": "^10.3.1", "swagger-jsdoc": "^6.3.0", @@ -229,6 +230,16 @@ "node": ">=6.9.0" } }, + "node_modules/@babel/helper-compilation-targets/node_modules/lru-cache": { + "version": "5.1.1", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-5.1.1.tgz", + "integrity": "sha512-KpNARQA3Iwv+jTA0utUVVbrh+Jlrr1Fv0e56GGzAFOXN7dk/FviaDW8LHmK52DlcH4WP2n6gI8vN1aesBFgo9w==", + "dev": true, + "license": "ISC", + "dependencies": { + "yallist": "^3.0.2" + } + }, "node_modules/@babel/helper-globals": { "version": "7.29.7", "resolved": "https://registry.npmjs.org/@babel/helper-globals/-/helper-globals-7.29.7.tgz", @@ -4531,14 +4542,10 @@ "license": "MIT" }, "node_modules/lru-cache": { - "version": "5.1.1", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-5.1.1.tgz", - "integrity": "sha512-KpNARQA3Iwv+jTA0utUVVbrh+Jlrr1Fv0e56GGzAFOXN7dk/FviaDW8LHmK52DlcH4WP2n6gI8vN1aesBFgo9w==", - "dev": true, - "license": "ISC", - "dependencies": { - "yallist": "^3.0.2" - } + "version": "10.4.3", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-10.4.3.tgz", + "integrity": "sha512-JNAzZcXrCt42VGLuYz0zfAzDfAvJWW6AfYlDBQyDV5DClI2m5sAmK+OIO7s59XfsRsWHp02jAJrRadPRGTt6SQ==", + "license": "ISC" }, "node_modules/make-dir": { "version": "4.0.0", diff --git a/backend/package.json b/backend/package.json index 1cb2517..a3f0d7b 100644 --- a/backend/package.json +++ b/backend/package.json @@ -16,6 +16,7 @@ "better-sqlite3": "^11.3.0", "cors": "^2.8.5", "express": "^4.18.2", + "lru-cache": "^10.4.3", "nanoid": "^3.3.7", "pino": "^10.3.1", "swagger-jsdoc": "^6.3.0", diff --git a/backend/src/api/middleware/rateLimit.ts b/backend/src/api/middleware/rateLimit.ts index 39e1420..44fef18 100644 --- a/backend/src/api/middleware/rateLimit.ts +++ b/backend/src/api/middleware/rateLimit.ts @@ -1,3 +1,4 @@ +import { LRUCache } from "lru-cache"; import type { Request, Response, NextFunction } from "express"; export interface RateLimitOptions { @@ -5,60 +6,89 @@ export interface RateLimitOptions { windowMs?: number; /** Maximum number of requests allowed within the window. Default: 20. */ maxRequests?: number; + /** + * Maximum number of distinct IPs tracked simultaneously **per limiter + * instance**. When this many entries are present, the least-recently-used + * IP is evicted on the next accepted request. Defaults to 10 000, which + * keeps the worst-case memory footprint predictable under IP-flood + * attacks (issue #154). Note: the module-instantiated default limiters + * (`rateLimitMiddleware` and `registerRateLimitMiddleware`) are separate + * instances, so two requests in flight can touch up to 2 × maxEntries + * entries combined. + */ + maxEntries?: number; } interface Window { timestamps: number[]; } +export interface RateLimiter { + middleware: (req: Request, res: Response, next: NextFunction) => void; + /** + * Fully clears tracked state. In this implementation there is no + * background eviction interval to halt, so `stop()` is essentially + * `cache.clear()` — kept for API compatibility with the original + * implementation and for tests / graceful shutdown hooks. + * + * **Behavior change vs the original implementation** (issue #154): the + * old `stop()` called `clearInterval()` on a background eviction sweep; + * the new `stop()` clears the cache. Anything that stored the factory + * return value and relied on the old semantic should migrate. + */ + stop: () => void; + /** + * Current number of tracked IPs. Exposed primarily for tests and + * operational debugging — not part of the rate-limiting contract. + * @internal + */ + size: () => number; +} + /** * Create a configurable in-memory sliding-window rate limiter. * - * Improvements over the original implementation: - * - Accepts `windowMs` / `maxRequests` options so callers can tune limits - * per-route (e.g. stricter limits for agent registration). - * - Runs a background eviction interval that removes stale entries so the - * internal `windows` Map does not grow without bound when many unique IPs - * visit (fixes the memory-leak identified in issue #181). - * - The eviction interval is returned via `stop()` so tests and the server - * shutdown path can clear it cleanly. + * Backing store is `lru-cache`, which provides both: + * - a hard cap on entry count (`max` / `maxEntries`) so the worst-case + * memory footprint is bounded against IP-flood attacks (issue #154 — + * the previous `Map`-only implementation grew unboundedly between the + * once-per-minute TTL sweeps); and + * - TTL-based eviction (`ttl` / `windowMs`) so quiet IPs drop out + * automatically after their window passes. + * + * Active IPs stay cached because every accepted request calls + * `windows.set(ip, win)`, which refreshes the entry's age; the + * least-recently-used entry is evicted only when inserting past `max`. */ -export function createRateLimiter(opts: RateLimitOptions = {}): { - middleware: (req: Request, res: Response, next: NextFunction) => void; - stop: () => void; -} { +export function createRateLimiter(opts: RateLimitOptions = {}): RateLimiter { const windowMs = opts.windowMs ?? 60_000; const maxRequests = opts.maxRequests ?? 20; + const maxEntries = opts.maxEntries ?? 10_000; - const windows = new Map(); - - // Evict entries whose entire timestamp window has expired. - // Running every minute keeps memory bounded for long-lived processes. - const evictionInterval = setInterval(() => { - const cutoff = Date.now() - windowMs; - for (const [ip, win] of windows) { - win.timestamps = win.timestamps.filter((t) => t > cutoff); - if (win.timestamps.length === 0) { - windows.delete(ip); - } - } - }, 60_000); - - // Allow the Node.js process to exit even if the interval is still active - // (important for test environments and long-running servers that call stop()). - if (evictionInterval.unref) evictionInterval.unref(); + const windows = new LRUCache({ + max: maxEntries, + ttl: windowMs, + // Don't refresh the age on read: a 429 must not let stale IPs linger. + updateAgeOnGet: false, + }); function middleware(req: Request, res: Response, next: NextFunction): void { + // NOTE: `req.ip` depends on Express's `trust proxy` setting. If the app + // ever sets `trust proxy = true`, attackers can rotate `X-Forwarded-For` + // to cheaply produce synthetic IPs; the cache remains bounded by + // `maxEntries` but each request still costs LRU insert/refresh work. + // Until trust proxy is configured, every untrusted request collapses to + // a single `"unknown"` entry, so the rate limiter is effectively a + // global cap — consider raising `maxRequests` or skipping rate-limiting + // for `ip === "unknown"` if you ever turn trust proxy on. const ip = req.ip ?? "unknown"; const now = Date.now(); const cutoff = now - windowMs; - let win = windows.get(ip); - if (!win) { - win = { timestamps: [] }; - windows.set(ip, win); - } - + let win = windows.get(ip) ?? { timestamps: [] }; + // Always trim the timestamps for this IP — even when the entry was + // pulled from the cache — so partial windows decay correctly across + // the rolling boundary. win.timestamps = win.timestamps.filter((t) => t > cutoff); if (win.timestamps.length >= maxRequests) { @@ -72,14 +102,21 @@ export function createRateLimiter(opts: RateLimitOptions = {}): { } win.timestamps.push(now); + // Re-set the entry to refresh its age so active IPs persist; this + // also keeps the LRU recency ordering accurate for eviction. + windows.set(ip, win); next(); } function stop(): void { - clearInterval(evictionInterval); + windows.clear(); } - return { middleware, stop }; + return { + middleware, + stop, + size: () => windows.size, + }; } // ── Default instances ──────────────────────────────────────────────────────── diff --git a/backend/tests/middleware.test.ts b/backend/tests/middleware.test.ts index 997b76d..39444cc 100644 --- a/backend/tests/middleware.test.ts +++ b/backend/tests/middleware.test.ts @@ -76,25 +76,23 @@ describe('rateLimitMiddleware', () => { jest.useRealTimers(); }); - it('evicts stale entries to prevent memory leaks', () => { + it('re-issues a fresh window for an IP whose entry has aged out', () => { jest.useFakeTimers(); const limiter = createRateLimiter({ windowMs: 60_000, maxRequests: 5 }); - // Simulate 100 unique IPs each making a single request + // 100 unique IPs each make one request inside the window. for (let i = 0; i < 100; i++) { const next = jest.fn(); limiter.middleware(makeReq(`192.168.1.${i}`), makeRes() as unknown as Response, next as NextFunction); expect(next).toHaveBeenCalled(); } + expect(limiter.size()).toBe(100); - // Advance time past the window so all entries are stale + // Advance past the 60s window so all entries become stale. lru-cache + // evicts lazily on next access; the next request should produce a + // fresh, empty window and so succeed. jest.advanceTimersByTime(70_000); - // Trigger the eviction interval (runs every 60s) - jest.advanceTimersByTime(60_000); - - // A new request from a previously tracked IP should still succeed - // and internally the stale entries should have been removed const next = jest.fn(); limiter.middleware(makeReq('192.168.1.0'), makeRes() as unknown as Response, next as NextFunction); expect(next).toHaveBeenCalled(); @@ -102,6 +100,76 @@ describe('rateLimitMiddleware', () => { limiter.stop(); jest.useRealTimers(); }); + + it('keeps the window map bounded under IP flood (issue #154)', () => { + const limiter = createRateLimiter({ + windowMs: 60_000, + maxRequests: 100, + maxEntries: 10_000, + }); + + // 25 000 unique IPs each make one request. Without `max`/LRU eviction + // this would balloon the map to 25 000 entries and OOM in production. + for (let i = 0; i < 25_000; i++) { + const next = jest.fn(); + limiter.middleware( + makeReq(`10.0.${Math.floor(i / 256)}.${i % 256}`), + makeRes() as unknown as Response, + next as NextFunction, + ); + expect(next).toHaveBeenCalled(); + } + + // The LRU must have evicted older entries; total size MUST stay <= max. + expect(limiter.size()).toBeLessThanOrEqual(10_000); + // And MUST be much smaller than what was inserted (proves LRU actually evicted). + expect(limiter.size()).toBeLessThan(25_000); + // Sanity: size stays exactly at max once we exceed it. + expect(limiter.size()).toBe(10_000); + + limiter.stop(); + }); + + it('evicts the least-recently-used IP first when maxEntries is exceeded', () => { + const limiter = createRateLimiter({ + windowMs: 60_000, + maxRequests: 1, + maxEntries: 3, + }); + + limiter.middleware(makeReq('1.1.1.1'), makeRes() as unknown as Response, jest.fn() as NextFunction); + limiter.middleware(makeReq('2.2.2.2'), makeRes() as unknown as Response, jest.fn() as NextFunction); + limiter.middleware(makeReq('3.3.3.3'), makeRes() as unknown as Response, jest.fn() as NextFunction); + expect(limiter.size()).toBe(3); + + // Touching 1.1.1.1 refreshes its LRU recency (lru-cache v10 moves the + // entry to MRU on get regardless of `updateAgeOnGet`). Note: this still + // hits a 429 because the entry already has a timestamp and maxRequests=1, + // but the LRU position is correctly updated to MRU. + limiter.middleware(makeReq('1.1.1.1'), makeRes() as unknown as Response, jest.fn() as NextFunction); + + // New 4th IP should evict '2.2.2.2' (the oldest untouched), not '1.1.1.1'. + limiter.middleware(makeReq('4.4.4.4'), makeRes() as unknown as Response, jest.fn() as NextFunction); + expect(limiter.size()).toBe(3); + + // 1.1.1.1 must still be tracked: hitting it again must produce an immediate + // 429 because we already pushed a timestamp for it (maxRequests = 1) before + // the LRU refresh. This is the real proof of LRU policy — '1' was kept, + // '2' was thrown out. + const res1 = makeRes(); + limiter.middleware(makeReq('1.1.1.1'), res1 as unknown as Response, jest.fn() as NextFunction); + expect(res1.status).toHaveBeenCalledWith(429); + + // 2.2.2.2 was evicted, so its next request should be allowed with a + // fresh, empty window. + const res = makeRes(); + const next = jest.fn(); + limiter.middleware(makeReq('2.2.2.2'), res as unknown as Response, next as NextFunction); + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalledWith(429); + + limiter.stop(); + }); }); // ── authMiddleware ────────────────────────────────────────────────────────────