fix(rate-limit): mitigate cache flooding vulnerability (#94)

Update in-memory rate limiter to fail secure (reject new requests) when maximum capacity is reached, rather than evicting the oldest tracking entry. This prevents an attacker from bypassing their rate limit by flooding the store with spoofed `x-forwarded-for` IPs to intentionally trigger cache eviction. Also cleaned up temporary files and updated tests to verify the behavior.

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>

.jules/sentinel.md

@@ -9,3 +9,7 @@
 **Vulnerability:** Comparing variable-length plaintext passwords using strict equality (`===`) or `crypto.timingSafeEqual` with unequal lengths, which leaks length information and enables timing attacks.
 **Learning:** When comparing variable-length secrets (like plaintext fallback passwords), both values must be hashed to a fixed-length algorithm (like SHA-256) first to ensure identical input lengths for the secure comparison, preventing side-channel leaks.
 **Prevention:** Always hash variable-length secrets to a fixed length before passing them to `crypto.timingSafeEqual`.
+## 2024-05-18 - Rate Limit Store Flooding and Cache Eviction Vulnerability
+**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.

lib/__tests__/rate-limit.test.ts

@@ -103,18 +103,18 @@ describe('checkRateLimit', () => {
     expect(result.resetAt).toBeLessThanOrEqual(before + 61_000);
   });
 
-  it('does not crash when store is at capacity and still accepts new IPs', () => {
+  it('does not crash when store is at capacity and prevents rate limit bypasses', () => {
     // Fill the store beyond MAX_STORE_ENTRIES (10_000 in prod)
-    // by hammering with unique IPs; evictIfFull must silently evict stale entries.
+    // by hammering with unique IPs.
     const uniqueIps = Array.from({ length: 10005 }, (_, i) => `evict-test-ip-${i}-${Date.now()}`);
     for (const ip of uniqueIps) {
       checkRateLimit(ip, 5);
     }
 
-    // A brand-new IP must still be accepted after potential eviction
+    // A brand-new IP must be blocked to prevent spoofing-based cache floods
     const freshIp = `evict-fresh-${Date.now()}`;
     const result = checkRateLimit(freshIp, 5);
-    expect(result.success).toBe(true);
+    expect(result.success).toBe(false);
-    expect(result.remaining).toBe(4);
+    expect(result.remaining).toBe(0);
   });
 });

lib/rate-limit.ts

@@ -67,12 +67,10 @@ export function checkRateLimit(
   // New window or expired
   if (!entry || now > entry.expiresAt) {
     if (store.size >= MAX_STORE_ENTRIES) {
-      // Store is full. Evict the oldest entry (FIFO) to prevent a global DoS
+      // Store is full. Prevent adding new entries to mitigate cache flooding
-      // where an attacker fills the store with spoofed IPs, locking out legitimate users.
+      // where an attacker spams spoofed IPs to evict legitimate users.
-      const oldestKey = store.keys().next().value;
+      // Deny new unrecognized IPs to fail securely during an active flood.
-      if (oldestKey !== undefined) {
+      return { success: false, remaining: 0, resetAt: now + windowMs };
-        store.delete(oldestKey);
-      }
     }
     const resetAt = now + windowMs;
     store.set(key, { count: 1, expiresAt: resetAt });