🛡️ Sentinel: dual rate-limiting security enhancement

.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-16 - Target Lockout DoS via Improper Dual Rate-Limiting Order
+**Vulnerability:** When implementing dual rate-limiting (checking both target-based keys like emails, and client IP), validating/incrementing the target-based bucket before the IP-based bucket allowed IP-blocked attackers to programmatically lock out arbitrary legitimate users from authentication endpoints (Account Lockout DoS) since target hits were still being registered before the IP restriction failed the request.
+**Learning:** Checking and mutating target limits prior to enforcing IP limits leaks rate-limit state increments to blocked IP addresses.
+**Prevention:** Always evaluate and enforce the IP-based rate limit first. Doing so blocks malicious IP addresses at the perimeter and prevents them from registering hits in or polluting any target-based buckets.

package.json

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

src/__tests__/setup.ts

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

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, { targetLimit: 5, ipLimit: 10, windowMs: 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, { targetLimit: 5, ipLimit: 10, windowMs: 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,148 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { rateLimit, rateLimitDual, _clearBuckets } from '../rate-limit';
+import { headers } from 'next/headers';
+
+vi.mock('next/headers', () => ({
+  headers: vi.fn(),
+}));
+
+describe('rate-limit.ts', () => {
+  beforeEach(() => {
+    vi.useFakeTimers();
+    _clearBuckets();
+    vi.clearAllMocks();
+  });
+
+  afterEach(() => {
+    vi.useRealTimers();
+  });
+
+  describe('rateLimit', () => {
+    it('allows hits up to the limit and then blocks them', () => {
+      // Limit 3, window 1000ms
+      expect(rateLimit('k1', 3, 1000).allowed).toBe(true);
+      expect(rateLimit('k1', 3, 1000).allowed).toBe(true);
+      expect(rateLimit('k1', 3, 1000).allowed).toBe(true);
+
+      const blocked = rateLimit('k1', 3, 1000);
+      expect(blocked.allowed).toBe(false);
+      expect(blocked.retryAfterSeconds).toBe(1);
+    });
+
+    it('sliding window lets hits decay', () => {
+      expect(rateLimit('k2', 2, 1000).allowed).toBe(true);
+      vi.advanceTimersByTime(600);
+      expect(rateLimit('k2', 2, 1000).allowed).toBe(true);
+
+      // Hit limit reached
+      expect(rateLimit('k2', 2, 1000).allowed).toBe(false);
+
+      // Advance past the first hit (at 0ms), oldest remaining is at 600ms
+      vi.advanceTimersByTime(401); // Current time is 1001ms. 0ms hit decayed.
+
+      const res = rateLimit('k2', 2, 1000);
+      expect(res.allowed).toBe(true);
+    });
+
+    it('denied hits do not extend lock out window', () => {
+      expect(rateLimit('k3', 1, 1000).allowed).toBe(true);
+      expect(rateLimit('k3', 1, 1000).allowed).toBe(false);
+
+      vi.advanceTimersByTime(1001);
+
+      expect(rateLimit('k3', 1, 1000).allowed).toBe(true);
+    });
+  });
+
+  describe('rateLimitDual', () => {
+    it('rate limits on IP first to block abusive actors and prevent target pollution', async () => {
+      // Mock headers to return IP '1.2.3.4'
+      (headers as any).mockResolvedValue({
+        get: (name: string) => {
+          if (name === 'x-forwarded-for') return '1.2.3.4';
+          return null;
+        },
+      });
+
+      // IP limit is 2, Target limit is 5
+      // Call first 2 times with the same IP but different target IDs
+      expect((await rateLimitDual('act1', 'targetA', { targetLimit: 5, ipLimit: 2, windowMs: 1000 })).allowed).toBe(true);
+      expect((await rateLimitDual('act1', 'targetB', { targetLimit: 5, ipLimit: 2, windowMs: 1000 })).allowed).toBe(true);
+
+      // The 3rd request from the same IP should be blocked due to IP rate-limiting, even with a fresh target
+      const blocked = await rateLimitDual('act1', 'targetC', { targetLimit: 5, ipLimit: 2, windowMs: 1000 });
+      expect(blocked.allowed).toBe(false);
+
+      // Because the IP was blocked first, targetC should NOT have had any hit registered in its bucket.
+      // Therefore, if a different/unblocked IP tries to access targetC, it should succeed!
+      (headers as any).mockResolvedValue({
+        get: (name: string) => {
+          if (name === 'x-forwarded-for') return '5.6.7.8'; // different IP
+          return null;
+        },
+      });
+      expect((await rateLimitDual('act1', 'targetC', { targetLimit: 5, ipLimit: 2, windowMs: 1000 })).allowed).toBe(true);
+    });
+
+    it('rate limits on target identifier second', async () => {
+      // Mock headers to return IP '1.2.3.4'
+      (headers as any).mockResolvedValue({
+        get: (name: string) => {
+          if (name === 'x-forwarded-for') return '1.2.3.4';
+          return null;
+        },
+      });
+
+      // Target limit is 1, but IP limit is 5 (different IPs)
+      // First request on IP 1.2.3.4 for targetX
+      expect((await rateLimitDual('act2', 'targetX', { targetLimit: 1, ipLimit: 5, windowMs: 1000 })).allowed).toBe(true);
+
+      // Second request from a DIFFERENT IP 5.6.7.8 for targetX should be blocked because targetX limit (1) was hit
+      (headers as any).mockResolvedValue({
+        get: (name: string) => {
+          if (name === 'x-forwarded-for') return '5.6.7.8';
+          return null;
+        },
+      });
+      const blocked = await rateLimitDual('act2', 'targetX', { targetLimit: 1, ipLimit: 5, windowMs: 1000 });
+      expect(blocked.allowed).toBe(false);
+    });
+
+    it('safely extracts IP using first element of x-forwarded-for', async () => {
+      (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 10, IP limit 1
+      expect((await rateLimitDual('act3', 'target1', { targetLimit: 10, ipLimit: 1, windowMs: 1000 })).allowed).toBe(true);
+
+      // Since IP was '1.2.3.4', hitting again on same IP should be blocked
+      const blocked = await rateLimitDual('act3', 'target2', { targetLimit: 10, ipLimit: 1, windowMs: 1000 });
+      expect(blocked.allowed).toBe(false);
+    });
+
+    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;
+        },
+      });
+
+      expect((await rateLimitDual('act4', 'target1', { targetLimit: 10, ipLimit: 1, windowMs: 1000 })).allowed).toBe(true);
+      const blocked = await rateLimitDual('act4', 'target2', { targetLimit: 10, ipLimit: 1, windowMs: 1000 });
+      expect(blocked.allowed).toBe(false);
+    });
+
+    it('falls back to unknown if headers throws or returns null', async () => {
+      (headers as any).mockRejectedValue(new Error('no headers in this environment'));
+
+      expect((await rateLimitDual('act5', 'target1', { targetLimit: 10, ipLimit: 1, windowMs: 1000 })).allowed).toBe(true);
+      const blocked = await rateLimitDual('act5', 'target2', { targetLimit: 10, ipLimit: 1, windowMs: 1000 });
+      expect(blocked.allowed).toBe(false);
+    });
+  });
+});

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,56 @@ export function rateLimit(key: string, limit = 5, windowMs = 60_000): RateLimitR
 
   return { allowed: true, retryAfterSeconds: 0 };
 }
+
+/**
+ * Perform dual rate-limiting on both client IP and a target-based identifier.
+ */
+export async function rateLimitDual(
+  actionName: string,
+  targetId: string,
+  options?: {
+    ipLimit?: number;
+    targetLimit?: number;
+    windowMs?: number;
+  }
+): Promise<RateLimitResult> {
+  const ipLimit = options?.ipLimit ?? 10;
+  const targetLimit = options?.targetLimit ?? 5;
+  const windowMs = options?.windowMs ?? 60_000;
+
+  // 1. Get client IP from headers safely (noUncheckedIndexedAccess enabled)
+  let ip = 'unknown';
+  try {
+    const heads = await headers();
+    const xff = heads.get('x-forwarded-for');
+    const xri = heads.get('x-real-ip');
+    ip = (xff && xff.split(',')[0]?.trim()) || xri || 'unknown';
+  } catch {
+    // Fail-safe default IP if headers cannot be read
+    ip = 'unknown';
+  }
+
+  // 2. Perform rate-limiting on client IP first to block abusive actors early
+  // and prevent an IP-blocked attacker from polluting target-based buckets.
+  const ipKey = `${actionName}:ip:${ip}`;
+  const ipRl = rateLimit(ipKey, ipLimit, windowMs);
+  if (!ipRl.allowed) {
+    return ipRl;
+  }
+
+  // 3. Perform rate-limiting on target ID (e.g., lowercase email)
+  const targetKey = `${actionName}:target:${targetId.toLowerCase()}`;
+  const targetRl = rateLimit(targetKey, targetLimit, windowMs);
+  if (!targetRl.allowed) {
+    return targetRl;
+  }
+
+  return { allowed: true, retryAfterSeconds: 0 };
+}
+
+/**
+ * Clean up helper for testing purposes.
+ */
+export function _clearBuckets(): void {
+  buckets.clear();
+}