🛡️ Sentinel: implement dual rate limiting for authentication actions

.jules/sentinel.md

@@ -1,5 +1,10 @@
 # Sentinel Journal — Critical Security Learnings
 
+## 2026-07-17 - Dual Rate Limiting Prevents Account Lockout DoS
+**Vulnerability:** Simple rate limiting on sensitive authentication actions (sign-in/sign-up) based purely on target-specific identifiers (e.g., username/email) enables malicious actors to trigger rate limits for legitimate users. An attacker could flood authentication requests for a specific user email, resulting in an Account Lockout Denial of Service (DoS) for the victim.
+**Learning:** Rate limiting must protect both system resources and user experience. Enforcing target-based rate limits before client IP rate limits allows malicious IPs to pollute and exhaust the target's limit bucket.
+**Prevention:** Implement a dual rate-limiting utility where client IP-based limits are verified and recorded *before* target-based limits. This ensures that malicious IP addresses are blocked first, preventing them from exhausting the target's quota and preserving account availability for the legitimate owner.
+
 ## 2026-07-16 - Synchronous Password Hashing Blocks Next.js Event Loop (DoS Risk)
 **Vulnerability:** The application used `scryptSync` (synchronous CPU-intensive password hashing) inside Next.js server action handlers for registration and login. Because Node.js runs on a single main event loop, a small number of concurrent authentication requests (or a distributed credential stuffing attack) completely blocks the event loop, starving all other concurrent requests and causing a full Denial of Service (DoS).
 **Learning:** Next.js Server Actions and Route Handlers run on Node's main thread by default. Using synchronous cryptography operations (such as `scryptSync` or `pbkdf2Sync`) prevents the server from processing other concurrent connections.

package.json

@@ -72,5 +72,5 @@
       "eslint --fix"
     ]
   },
-  "packageManager": "pnpm@11.13.0+sha512.88d94724d8f2e6c186744a5584c6e59ecac869ec7ba15e9cb4cd628e8dc7066820b2481d8ee3b51ea8da323a7378068aa58c556a3720d32b7c20a051d088363a"
+  "packageManager": "pnpm@11.14.0+sha512.ZsGsTH1HYtbX3eRMfz5ac1ke0KCAbnUdTtMtTwBPJbIoWpBrH9ip4+Yh3ztOKFi/iOUODPYmvtvpd/5DSlyvhQ=="
 }

src/app/actions/__tests__/auth-actions.test.ts

@@ -29,6 +29,9 @@ vi.mock('next/headers', () => ({
     set: vi.fn(),
     delete: vi.fn(),
   }),
+  headers: () => Promise.resolve({
+    get: () => null,
+  }),
 }));
 
 vi.mock('next/navigation', () => ({

src/app/actions/auth.ts

@@ -15,7 +15,7 @@ import {
   getSession,
 } from '@/lib/auth';
 import { logger } from '@/lib/logger';
-import { rateLimit } from '@/lib/rate-limit';
+import { rateLimitDual } from '@/lib/rate-limit';
 import {
   hashClaimToken,
   PLACEHOLDER_PASSWORD_PREFIX,
@@ -32,7 +32,7 @@ import {
 // ---------------------------------------------------------------------------
 
 export const signUpAction = createSafeAction(signUpSchema, async (data) => {
-  const rl = rateLimit(`signup:${data.email.toLowerCase()}`, 5, 60_000);
+  const rl = await rateLimitDual('signup', data.email, 10, 5, 60_000);
   if (!rl.allowed) {
     throw new Error(`Too many attempts. Try again in ${rl.retryAfterSeconds}s.`);
   }
@@ -125,7 +125,7 @@ export const signUpAction = createSafeAction(signUpSchema, async (data) => {
 export const signInAction = createSafeAction(signInSchema, async (data) => {
   // Rate-limit BEFORE any DB or scrypt work — the sync scrypt verify is
   // exactly what an attacker would use to burn the event loop.
-  const rl = rateLimit(`signin:${data.email.toLowerCase()}`, 5, 60_000);
+  const rl = await rateLimitDual('signin', data.email, 10, 5, 60_000);
   if (!rl.allowed) {
     throw new Error(`Too many attempts. Try again in ${rl.retryAfterSeconds}s.`);
   }

src/lib/__tests__/rate-limit.test.ts

@@ -0,0 +1,121 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { rateLimit, rateLimitDual, resetRateLimits } from '../rate-limit';
+
+// Mock 'next/headers' as requested
+const mockHeadersGet = vi.fn();
+vi.mock('next/headers', () => ({
+  headers: () => Promise.resolve({
+    get: mockHeadersGet,
+  }),
+}));
+
+describe('rate-limit.ts', () => {
+  beforeEach(() => {
+    resetRateLimits();
+    vi.useFakeTimers();
+    mockHeadersGet.mockReset();
+  });
+
+  afterEach(() => {
+    vi.useRealTimers();
+  });
+
+  describe('rateLimit', () => {
+    it('allows hits within the limit and blocks exceeding hits', () => {
+      // Limit of 3 hits in 60s
+      expect(rateLimit('key1', 3, 60_000).allowed).toBe(true);
+      expect(rateLimit('key1', 3, 60_000).allowed).toBe(true);
+      expect(rateLimit('key1', 3, 60_000).allowed).toBe(true);
+
+      const blocked = rateLimit('key1', 3, 60_000);
+      expect(blocked.allowed).toBe(false);
+      expect(blocked.retryAfterSeconds).toBe(60);
+    });
+
+    it('advances the window correctly over time', () => {
+      expect(rateLimit('key1', 2, 10_000).allowed).toBe(true);
+      vi.advanceTimersByTime(6000);
+      expect(rateLimit('key1', 2, 10_000).allowed).toBe(true);
+
+      // Exceeded
+      expect(rateLimit('key1', 2, 10_000).allowed).toBe(false);
+
+      // Advance so first hit falls out of the window
+      vi.advanceTimersByTime(5000); // Total 11s elapsed since 1st hit
+      expect(rateLimit('key1', 2, 10_000).allowed).toBe(true);
+    });
+
+    it('cleans up buckets opportunistically when map is too large', () => {
+      // Trigger opportunistic cleanup (buckets.size > 10_000)
+      for (let i = 0; i < 10005; i++) {
+        rateLimit(`key-${i}`, 1, 10);
+      }
+      // Advance time so everything is cutoff
+      vi.advanceTimersByTime(20);
+      // This call triggers cleanup
+      rateLimit('newkey', 1, 10);
+      // The old keys should have been cleaned up and removed.
+    });
+  });
+
+  describe('rateLimitDual', () => {
+    it('uses x-forwarded-for header for client IP rate limiting', async () => {
+      mockHeadersGet.mockImplementation((header: string) => {
+        if (header === 'x-forwarded-for') return '1.2.3.4, 5.6.7.8';
+        return null;
+      });
+
+      // Under IP limit of 2, target limit of 1
+      // First attempt: should succeed for both IP and target
+      const res1 = await rateLimitDual('login', 'user@example.com', 2, 1, 60_000);
+      expect(res1.allowed).toBe(true);
+
+      // Second attempt with same email: target limit exceeded (limit is 1)
+      const res2 = await rateLimitDual('login', 'user@example.com', 2, 1, 60_000);
+      expect(res2.allowed).toBe(false);
+    });
+
+    it('IP limit check runs BEFORE target check to prevent account lockout', async () => {
+      mockHeadersGet.mockImplementation((header: string) => {
+        if (header === 'x-forwarded-for') return '9.9.9.9';
+        return null;
+      });
+
+      // IP limit = 1, target limit = 5
+      // First attempt: allowed
+      const res1 = await rateLimitDual('login', 'legit@example.com', 1, 5, 60_000);
+      expect(res1.allowed).toBe(true);
+
+      // Second attempt: IP blocked (since IP limit is 1)
+      const res2 = await rateLimitDual('login', 'legit@example.com', 1, 5, 60_000);
+      expect(res2.allowed).toBe(false);
+
+      // Verify target-based key did not get polluted by checking another IP
+      mockHeadersGet.mockImplementation((header: string) => {
+        if (header === 'x-forwarded-for') return '8.8.8.8'; // safe IP
+        return null;
+      });
+
+      // Legit user on safe IP can still log in because their target bucket wasn't polluted by blocked IP
+      const res3 = await rateLimitDual('login', 'legit@example.com', 1, 5, 60_000);
+      expect(res3.allowed).toBe(true);
+    });
+
+    it('falls back to x-real-ip when x-forwarded-for is missing', async () => {
+      mockHeadersGet.mockImplementation((header: string) => {
+        if (header === 'x-real-ip') return '127.0.0.1';
+        return null;
+      });
+
+      const res = await rateLimitDual('login', 'user@example.com', 1, 1, 60_000);
+      expect(res.allowed).toBe(true);
+    });
+
+    it('falls back to unknown when both headers are missing', async () => {
+      mockHeadersGet.mockReturnValue(null);
+
+      const res = await rateLimitDual('login', 'user@example.com', 1, 1, 60_000);
+      expect(res.allowed).toBe(true);
+    });
+  });
+});

src/lib/rate-limit.ts

@@ -8,6 +8,7 @@
  */
 
 import 'server-only';
+import { headers } from 'next/headers';
 
 const buckets = new Map<string, number[]>();
 
@@ -47,3 +48,43 @@ export function rateLimit(key: string, limit = 5, windowMs = 60_000): RateLimitR
 
   return { allowed: true, retryAfterSeconds: 0 };
 }
+
+/**
+ * Perform dual rate limiting to prevent both brute-force attacks and target-based lockout DoS.
+ *
+ * Always runs the client IP check BEFORE checking target-based keys (such as email) so that
+ * blocked IPs cannot fill/exhaust a legitimate user's target rate-limiting bucket.
+ */
+export async function rateLimitDual(
+  action: string,
+  targetKey: string,
+  ipLimit = 10,
+  targetLimit = 5,
+  windowMs = 60_000,
+): Promise<RateLimitResult> {
+  const reqHeaders = await headers();
+  const xff = reqHeaders.get('x-forwarded-for');
+  const xri = reqHeaders.get('x-real-ip');
+
+  // Safely extract client IP from x-forwarded-for (handling noUncheckedIndexedAccess)
+  const clientIp = (xff ? xff.split(',')[0]?.trim() : null) || xri || 'unknown';
+
+  // 1. Check IP-based rate limiting first
+  const ipKey = `ip:${action}:${clientIp}`;
+  const ipResult = rateLimit(ipKey, ipLimit, windowMs);
+  if (!ipResult.allowed) {
+    return ipResult;
+  }
+
+  // 2. Check target-based rate limiting second (using lowercase keys for case insensitivity)
+  const targetKeyLower = targetKey.toLowerCase();
+  const targetKeyFormatted = `target:${action}:${targetKeyLower}`;
+  return rateLimit(targetKeyFormatted, targetLimit, windowMs);
+}
+
+/**
+ * Resets the in-memory rate-limiting maps to prevent test pollution.
+ */
+export function resetRateLimits(): void {
+  buckets.clear();
+}