From f18d9ae3d99191e27f6de5cf11d535f7b9e2fa24 Mon Sep 17 00:00:00 2001 From: Ralksta <16941050+ralksta@users.noreply.github.com> Date: Fri, 24 Apr 2026 22:12:42 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITICAL]?= =?UTF-8?q?=20Fix=20rate=20limit=20DoS=20vulnerability=20(#136)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> --- .jules/sentinel.md | 4 ++++ app/api/map/route.ts | 7 ++----- lib/__tests__/rate-limit.test.ts | 8 ++++---- lib/rate-limit.ts | 10 ++++++---- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index deafea1..1597b82 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -13,3 +13,7 @@ **Vulnerability:** The rate limiter implemented an LRU/FIFO map to track API request limits, evicting the oldest tracked IP when maximum capacity was reached. An attacker could flood the store with randomly spoofed IP headers, continuously evicting legitimate users' IP addresses. This allowed the attacker to effectively bypass their own rate limit and subject the upstream server to DoS. **Learning:** Evicting entries from a security control store (like a rate limit map) under pressure turns the limit into a revolving door if an attacker controls the keys (e.g., via IP spoofing headers). **Prevention:** Rather than evicting old entries when an in-memory rate limit store is full, the correct security posture is to fail closed (or block new keys) until entries naturally expire, preserving tracking on existing keys. +## 2026-04-23 - Prevent Global DoS in Rate Limiter via FIFO Eviction +**Vulnerability:** The in-memory rate limiter implemented a hard ceiling (`MAX_STORE_ENTRIES`) and rejected all new IPs when reached, creating a trivial Global Denial of Service (DoS) vulnerability where an attacker could spam spoofed IPs to fill the bounded Map and permanently block all legitimate traffic. +**Learning:** "Failing securely" by denying unknown requests during capacity limits is incorrect for rate limiters. Bounded Maps must actively evict older entries rather than rejecting new ones, otherwise the security mechanism itself becomes the vector for DoS. +**Prevention:** In-memory rate limiters using bounded Maps must implement a FIFO (First-In-First-Out) or LRU (Least Recently Used) eviction policy (e.g., deleting `store.keys().next().value`) to make room for new IPs when maximum capacity is reached. diff --git a/app/api/map/route.ts b/app/api/map/route.ts index feed4ac..20572e1 100644 --- a/app/api/map/route.ts +++ b/app/api/map/route.ts @@ -10,16 +10,13 @@ import { getConfig } from '@/lib/config'; import { imageUrl } from '@/lib/urls'; import { isAuthenticated } from '@/lib/auth'; import { getMapData } from '@/lib/mapService'; -import { checkRateLimit } from '@/lib/rate-limit'; +import { checkRateLimit, getClientIp } from '@/lib/rate-limit'; export const dynamic = 'force-dynamic'; export async function GET(request: NextRequest) { // Rate limit: 120 requests/minute per IP (map data is cached client-side; this guards against abuse) - const ip = - request.headers.get('x-forwarded-for')?.split(',')[0].trim() ?? - request.headers.get('x-real-ip') ?? - '127.0.0.1'; + const ip = getClientIp(request); const rl = checkRateLimit(ip, 120); if (!rl.success) { return NextResponse.json( diff --git a/lib/__tests__/rate-limit.test.ts b/lib/__tests__/rate-limit.test.ts index 5fe58ea..554a586 100644 --- a/lib/__tests__/rate-limit.test.ts +++ b/lib/__tests__/rate-limit.test.ts @@ -103,7 +103,7 @@ describe('checkRateLimit', () => { expect(result.resetAt).toBeLessThanOrEqual(before + 61_000); }); - it('does not crash when store is at capacity and prevents rate limit bypasses', () => { + it('does not crash when store is at capacity and allows new requests by evicting oldest entries', () => { // Fill the store beyond MAX_STORE_ENTRIES (10_000 in prod) // by hammering with unique IPs. const uniqueIps = Array.from({ length: 10005 }, (_, i) => `evict-test-ip-${i}-${Date.now()}`); @@ -111,10 +111,10 @@ describe('checkRateLimit', () => { checkRateLimit(ip, 5); } - // A brand-new IP must be blocked to prevent spoofing-based cache floods + // A brand-new IP must be allowed by evicting the oldest entry const freshIp = `evict-fresh-${Date.now()}`; const result = checkRateLimit(freshIp, 5); - expect(result.success).toBe(false); - expect(result.remaining).toBe(0); + expect(result.success).toBe(true); + expect(result.remaining).toBe(4); }); }); diff --git a/lib/rate-limit.ts b/lib/rate-limit.ts index cf1c9ea..2b43b14 100644 --- a/lib/rate-limit.ts +++ b/lib/rate-limit.ts @@ -67,10 +67,12 @@ export function checkRateLimit( // New window or expired if (!entry || now > entry.expiresAt) { if (store.size >= MAX_STORE_ENTRIES) { - // Store is full. Prevent adding new entries to mitigate cache flooding - // where an attacker spams spoofed IPs to evict legitimate users. - // Deny new unrecognized IPs to fail securely during an active flood. - return { success: false, remaining: 0, resetAt: now + windowMs }; + // Store is full. Evict the oldest entry to prevent cache flooding + // DOS attacks, which could otherwise block all legitimate users. + const oldestKey = store.keys().next().value; + if (oldestKey !== undefined) { + store.delete(oldestKey); + } } const resetAt = now + windowMs; store.set(key, { count: 1, expiresAt: resetAt });