🛡️ Sentinel: implement dual rate-limiting on authentication server actions

.jules/sentinel.md

@@ -4,3 +4,8 @@
 **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.
 **Prevention:** Always use asynchronous password-hashing implementations (such as async `scrypt` wrapped in a Promise or bcrypt/argon2 async variants) inside Next.js/Node.js web entry points to delegate heavy hashing computations to the Node.js libuv thread pool, keeping the main event loop responsive.
+
+## 2026-07-17 - Dual Rate-Limiting Ordering Prevents Account Lockout DoS
+**Vulnerability:** Legacy rate-limiting checked only target-based identifiers (e.g. email) on authentication routes. This allowed attackers to perform distributed credential stuffing attacks by varying target emails. Furthermore, checking target buckets first meant malicious IP-blocked actors could still spam and fill target-based buckets, locking out or blocking legitimate users.
+**Learning:** Target-only rate-limiting leaves endpoints exposed to distributed attacks. When adding IP-based rate-limiting alongside target-based rate-limiting, the client IP check must be executed and enforced *before* the target-based check.
+**Prevention:** Implement `rateLimitDual` which performs the IP bucket verification first. If the client IP is blocked, return immediately without recording a hit in the target-based bucket, ensuring legitimate users' targets are not polluted or blocked by malicious IPs.

package.json

@@ -72,5 +72,5 @@
       "eslint --fix"
     ]
   },
-  "packageManager": "pnpm@11.13.0+sha512.88d94724d8f2e6c186744a5584c6e59ecac869ec7ba15e9cb4cd628e8dc7066820b2481d8ee3b51ea8da323a7378068aa58c556a3720d32b7c20a051d088363a"
+  "packageManager": "pnpm@11.14.0+sha512.66c1ac4c7d4762d6d7dde44c7f3e5a73591ed0a0806e751d4ed32d4f004f25b2285a906b1fd8a9e3e621df3b4e2858bf88e50e0cf626bedbe977fe434a5caf85"
 }

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,13 @@ import {
 // ---------------------------------------------------------------------------
 
 export const signUpAction = createSafeAction(signUpSchema, async (data) => {
-  const rl = rateLimit(`signup:${data.email.toLowerCase()}`, 5, 60_000);
+  // Use dual rate-limiting combining client IP check and target-based checks (email)
+  const rl = await rateLimitDual('signup-ip', `signup:${data.email.toLowerCase()}`, {
+    ipLimit: 10,
+    ipWindowMs: 60_000,
+    targetLimit: 5,
+    targetWindowMs: 60_000,
+  });
   if (!rl.allowed) {
     throw new Error(`Too many attempts. Try again in ${rl.retryAfterSeconds}s.`);
   }
@@ -123,9 +129,15 @@ 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);
+  // Use dual rate-limiting combining client IP check and target-based checks (email).
+  // Rate-limit BEFORE any DB or scrypt work — the async scrypt verify can still
+  // be targeted to cause severe load.
+  const rl = await rateLimitDual('signin-ip', `signin:${data.email.toLowerCase()}`, {
+    ipLimit: 10,
+    ipWindowMs: 60_000,
+    targetLimit: 5,
+    targetWindowMs: 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,216 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { headers } from 'next/headers';
+import { rateLimit, rateLimitDual, resetRateLimits } from '../rate-limit';
+
+vi.mock('next/headers', () => ({
+  headers: vi.fn(),
+}));
+
+describe('Rate Limiter', () => {
+  beforeEach(() => {
+    vi.useFakeTimers();
+    resetRateLimits();
+    vi.clearAllMocks();
+  });
+
+  afterEach(() => {
+    vi.useRealTimers();
+  });
+
+  describe('rateLimit', () => {
+    it('allows hits within limits and blocks when exceeded', () => {
+      // Limit 3 hits in 1 minute (60,000 ms)
+      expect(rateLimit('user1', 3, 60_000).allowed).toBe(true);
+      expect(rateLimit('user1', 3, 60_000).allowed).toBe(true);
+      expect(rateLimit('user1', 3, 60_000).allowed).toBe(true);
+
+      // 4th hit should be blocked
+      const blocked = rateLimit('user1', 3, 60_000);
+      expect(blocked.allowed).toBe(false);
+      expect(blocked.retryAfterSeconds).toBe(60); // 1 minute (60s)
+    });
+
+    it('sliding window lets hits expire', () => {
+      expect(rateLimit('user1', 2, 10_000).allowed).toBe(true);
+
+      // Advance time by 5s
+      vi.advanceTimersByTime(5000);
+      expect(rateLimit('user1', 2, 10_000).allowed).toBe(true);
+
+      // Third hit within 10s should block
+      expect(rateLimit('user1', 2, 10_000).allowed).toBe(false);
+
+      // Advance time by 6s (total 11s), first hit has expired, so we should be allowed again
+      vi.advanceTimersByTime(6000);
+      expect(rateLimit('user1', 2, 10_000).allowed).toBe(true);
+    });
+
+    it('cleans up obsolete buckets', () => {
+      // We need to insert > 10,000 buckets to trigger cleanup.
+      for (let i = 0; i < 10001; i++) {
+        rateLimit(`user_${i}`, 1, 10_000);
+      }
+      // Buckets are now full. Let's advance timers so they are obsolete
+      vi.advanceTimersByTime(11000);
+      // Trigger another rate limit to run cleanup
+      rateLimit('trigger', 1, 10_000);
+
+      // Since all other 10001 buckets were obsolete, they should be cleaned up.
+      // Let's verify that hitting an existing one starts a fresh limit instead of old one
+      const res = rateLimit('user_0', 1, 10_000);
+      expect(res.allowed).toBe(true);
+    });
+  });
+
+  describe('rateLimitDual', () => {
+    it('applies IP rate limit before target-based check', async () => {
+      // Mock headers with IP 1.2.3.4
+      (headers as any).mockResolvedValue({
+        get: (name: string) => {
+          if (name === 'x-forwarded-for') return '1.2.3.4, 5.6.7.8';
+          return null;
+        },
+      });
+
+      // Target limit is 2, IP limit is 3.
+      // 1st request
+      const r1 = await rateLimitDual('ip-test', 'target-test', {
+        ipLimit: 3,
+        targetLimit: 2,
+      });
+      expect(r1.allowed).toBe(true);
+
+      // 2nd request
+      const r2 = await rateLimitDual('ip-test', 'target-test', {
+        ipLimit: 3,
+        targetLimit: 2,
+      });
+      expect(r2.allowed).toBe(true);
+
+      // 3rd request should hit target limit (since targetLimit is 2)
+      const r3 = await rateLimitDual('ip-test', 'target-test', {
+        ipLimit: 3,
+        targetLimit: 2,
+      });
+      expect(r3.allowed).toBe(false);
+    });
+
+    it('blocks by IP limit before target limit, and does not record target-based hit on IP failure', async () => {
+      // Mock headers with IP 1.2.3.4
+      let currentIp = '1.2.3.4';
+      (headers as any).mockResolvedValue({
+        get: (name: string) => {
+          if (name === 'x-forwarded-for') return currentIp;
+          return null;
+        },
+      });
+
+      // Target limit is 5, IP limit is 2
+      const r1 = await rateLimitDual('ip-test', 'target-test', {
+        ipLimit: 2,
+        targetLimit: 5,
+      });
+      expect(r1.allowed).toBe(true);
+
+      const r2 = await rateLimitDual('ip-test', 'target-test', {
+        ipLimit: 2,
+        targetLimit: 5,
+      });
+      expect(r2.allowed).toBe(true);
+
+      // 3rd request hits IP limit first
+      const r3 = await rateLimitDual('ip-test', 'target-test', {
+        ipLimit: 2,
+        targetLimit: 5,
+      });
+      expect(r3.allowed).toBe(false);
+
+      // Crucial: since IP blocked the 3rd request, target-based bucket should NOT have been polluted
+      // i.e. target-based hits should still be 2. Let's change IP to 2.2.2.2 and try same target
+      currentIp = '2.2.2.2';
+
+      // Target hit count is still 2. We can hit it 3 more times with new IPs each time to avoid IP limit.
+      const r4 = await rateLimitDual('ip-test', 'target-test', {
+        ipLimit: 2,
+        targetLimit: 5,
+      });
+      expect(r4.allowed).toBe(true); // target hit 3
+
+      currentIp = '3.3.3.3';
+      const r5 = await rateLimitDual('ip-test', 'target-test', {
+        ipLimit: 2,
+        targetLimit: 5,
+      });
+      expect(r5.allowed).toBe(true); // target hit 4
+
+      currentIp = '4.4.4.4';
+      const r6 = await rateLimitDual('ip-test', 'target-test', {
+        ipLimit: 2,
+        targetLimit: 5,
+      });
+      expect(r6.allowed).toBe(true); // target hit 5
+
+      currentIp = '5.5.5.5';
+      const r7 = await rateLimitDual('ip-test', 'target-test', {
+        ipLimit: 2,
+        targetLimit: 5,
+      });
+      expect(r7.allowed).toBe(false); // target limit exceeded
+    });
+
+    it('falls back to x-real-ip if x-forwarded-for is missing', async () => {
+      (headers as any).mockResolvedValue({
+        get: (name: string) => {
+          if (name === 'x-real-ip') return '9.9.9.9';
+          return null;
+        },
+      });
+
+      const r1 = await rateLimitDual('ip-real', 'target-real', { ipLimit: 1 });
+      expect(r1.allowed).toBe(true);
+
+      // 2nd hit should block on IP
+      const r2 = await rateLimitDual('ip-real', 'target-real2', { ipLimit: 1 });
+      expect(r2.allowed).toBe(false);
+    });
+
+    it('skips IP check and uses target check if no IP headers exist', async () => {
+      (headers as any).mockResolvedValue({
+        get: () => null,
+      });
+
+      const r1 = await rateLimitDual('ip-none', 'target-only', { ipLimit: 1, targetLimit: 2 });
+      expect(r1.allowed).toBe(true);
+
+      const r2 = await rateLimitDual('ip-none', 'target-only', { ipLimit: 1, targetLimit: 2 });
+      expect(r2.allowed).toBe(true);
+
+      const r3 = await rateLimitDual('ip-none', 'target-only', { ipLimit: 1, targetLimit: 2 });
+      expect(r3.allowed).toBe(false);
+    });
+
+    it('falls back gracefully to target-only rate limiting if headers() throws', async () => {
+      (headers as any).mockRejectedValue(new Error('Outside of request context'));
+
+      const r1 = await rateLimitDual('ip-error', 'target-fallback', { ipLimit: 1, targetLimit: 2 });
+      expect(r1.allowed).toBe(true);
+
+      const r2 = await rateLimitDual('ip-error', 'target-fallback', { ipLimit: 1, targetLimit: 2 });
+      expect(r2.allowed).toBe(true);
+
+      const r3 = await rateLimitDual('ip-error', 'target-fallback', { ipLimit: 1, targetLimit: 2 });
+      expect(r3.allowed).toBe(false);
+    });
+  });
+
+  describe('resetRateLimits', () => {
+    it('clears rate limiting maps', () => {
+      expect(rateLimit('u', 1).allowed).toBe(true);
+      expect(rateLimit('u', 1).allowed).toBe(false);
+
+      resetRateLimits();
+
+      expect(rateLimit('u', 1).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,57 @@ export function rateLimit(key: string, limit = 5, windowMs = 60_000): RateLimitR
 
   return { allowed: true, retryAfterSeconds: 0 };
 }
+
+/**
+ * Dual rate limiter: combines client IP check and target-based checks (like email).
+ * Always executes the client IP check and rate limiting before checking the target-based
+ * identifier to prevent malicious IP-blocked actors from polluting target-based buckets
+ * and causing an Account Lockout Denial of Service (DoS) for legitimate users.
+ */
+export async function rateLimitDual(
+  ipKeyPrefix: string,
+  targetKey: string,
+  options: {
+    ipLimit?: number;
+    ipWindowMs?: number;
+    targetLimit?: number;
+    targetWindowMs?: number;
+  } = {}
+): Promise<RateLimitResult> {
+  const {
+    ipLimit = 10,
+    ipWindowMs = 60_000,
+    targetLimit = 5,
+    targetWindowMs = 60_000,
+  } = options;
+
+  let heads;
+  try {
+    heads = await headers();
+  } catch {
+    // Fallback gracefully if called outside of request context (e.g. testing)
+  }
+
+  if (heads) {
+    const xff = heads.get('x-forwarded-for');
+    // Safe access for split under noUncheckedIndexedAccess rule
+    const ip = xff ? xff.split(',')[0]?.trim() : (heads.get('x-real-ip') ?? null);
+
+    if (ip) {
+      const ipKey = `${ipKeyPrefix}:${ip}`;
+      const ipResult = rateLimit(ipKey, ipLimit, ipWindowMs);
+      if (!ipResult.allowed) {
+        return ipResult;
+      }
+    }
+  }
+
+  return rateLimit(targetKey, targetLimit, targetWindowMs);
+}
+
+/**
+ * Resets the in-memory rate-limiting buckets (primarily for testing purposes).
+ */
+export function resetRateLimits(): void {
+  buckets.clear();
+}