diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 29abdfb..ac47bf3 100644 --- a/.jules/sentinel.md +++ b/.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 - Rate-Limit Bypass and Account Lockout via Target Key Pollution +**Vulnerability:** Sensitive authentication endpoints used standard, single-key rate-limiting bound only to user emails. A malicious actor could block legitimate users from signing in or signing up by flooding the rate limiter with target emails from a single IP, causing a widespread Account Lockout Denial of Service (DoS). +**Learning:** Checking or incrementing target-based rate limits before checking client IP rate limits allows IP-blocked attackers to pollute target-based rate limiting buckets and lockout innocent users. +**Prevention:** Always use a dual rate-limiting approach that checks and blocks malicious client IPs first before incrementing or checking target-based keys (such as email). Ensure the IP extraction safely fallback and handle array indexes, and that all calling utilities are asynchronous to support Next.js 15 async headers. diff --git a/package.json b/package.json index e60c440..6620aee 100644 --- a/package.json +++ b/package.json @@ -72,5 +72,5 @@ "eslint --fix" ] }, - "packageManager": "pnpm@11.13.0+sha512.88d94724d8f2e6c186744a5584c6e59ecac869ec7ba15e9cb4cd628e8dc7066820b2481d8ee3b51ea8da323a7378068aa58c556a3720d32b7c20a051d088363a" + "packageManager": "pnpm@11.15.0+sha512.266f8957a30d2be6e9468e5e66bcdedd35a794175f71b067ba8504d686cce1d0c0f429b33c323c3c569ad4891e667574a49ff71d1b89a22cc66f13c65818c578" } diff --git a/src/app/actions/__tests__/auth-actions.test.ts b/src/app/actions/__tests__/auth-actions.test.ts index 766ee9b..1b3c1be 100644 --- a/src/app/actions/__tests__/auth-actions.test.ts +++ b/src/app/actions/__tests__/auth-actions.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { signInAction, signUpAction, signOutAction } from '@/app/actions/auth'; import { PLACEHOLDER_PASSWORD_PREFIX } from '@/lib/claim-token'; +import { resetRateLimits } from '@/lib/rate-limit'; const mockSignToken = vi.fn(); const mockSetAuthCookie = vi.fn(); @@ -29,6 +30,7 @@ vi.mock('next/headers', () => ({ set: vi.fn(), delete: vi.fn(), }), + headers: () => Promise.resolve({ get: () => null }), })); vi.mock('next/navigation', () => ({ @@ -41,6 +43,7 @@ describe('auth actions', () => { beforeEach(() => { vi.resetAllMocks(); mockSignToken.mockResolvedValue('token'); + resetRateLimits(); }); it('signInAction rejects unknown email', async () => { diff --git a/src/app/actions/auth.ts b/src/app/actions/auth.ts index 2f10075..1af8a6c 100644 --- a/src/app/actions/auth.ts +++ b/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(data.email, 5, 60_000); if (!rl.allowed) { throw new Error(`Too many attempts. Try again in ${rl.retryAfterSeconds}s.`); } @@ -123,9 +123,9 @@ 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); + // Rate-limit BEFORE any DB or scrypt work to prevent event-loop abuse. + // Using dual rate limiting checks the client IP before target-based email. + const rl = await rateLimitDual(data.email, 5, 60_000); if (!rl.allowed) { throw new Error(`Too many attempts. Try again in ${rl.retryAfterSeconds}s.`); } diff --git a/src/lib/__tests__/rate-limit.test.ts b/src/lib/__tests__/rate-limit.test.ts new file mode 100644 index 0000000..ed4ae4f --- /dev/null +++ b/src/lib/__tests__/rate-limit.test.ts @@ -0,0 +1,152 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { rateLimit, rateLimitDual, resetRateLimits } from '../rate-limit'; + +const { mockHeaders } = vi.hoisted(() => ({ + mockHeaders: { + get: vi.fn(), + }, +})); + +vi.mock('next/headers', () => ({ + headers: vi.fn(() => Promise.resolve(mockHeaders)), +})); + +describe('rate-limit.ts', () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.clearAllMocks(); + resetRateLimits(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + describe('rateLimit', () => { + it('allows requests within limit and blocks when exceeded', () => { + // Limit = 3, window = 60s + expect(rateLimit('user-1', 3, 60_000).allowed).toBe(true); + expect(rateLimit('user-1', 3, 60_000).allowed).toBe(true); + expect(rateLimit('user-1', 3, 60_000).allowed).toBe(true); + + const block = rateLimit('user-1', 3, 60_000); + expect(block.allowed).toBe(false); + expect(block.retryAfterSeconds).toBe(60); + }); + + it('re-allows requests after the sliding window expires', () => { + expect(rateLimit('user-2', 2, 60_000).allowed).toBe(true); + + // Advance time by 30 seconds + vi.advanceTimersByTime(30_000); + expect(rateLimit('user-2', 2, 60_000).allowed).toBe(true); + expect(rateLimit('user-2', 2, 60_000).allowed).toBe(false); + + // Advance time by another 31 seconds (total 61s from first hit) + vi.advanceTimersByTime(31_000); + // The first hit has expired, so we should be allowed again + expect(rateLimit('user-2', 2, 60_000).allowed).toBe(true); + }); + + it('performs opportunistic cleanup of old buckets', () => { + // Populate 10,001 keys + for (let i = 0; i < 10005; i++) { + rateLimit(`key-${i}`, 5, 60_000); + } + // Advance past window to expire all hits + vi.advanceTimersByTime(61_000); + // The next hit should trigger cleanup + rateLimit('new-trigger', 5, 60_000); + }); + }); + + describe('rateLimitDual', () => { + it('allows requests when both IP and target are under limits', async () => { + mockHeaders.get.mockImplementation((name: string) => { + if (name === 'x-forwarded-for') return '1.2.3.4'; + return null; + }); + + const res = await rateLimitDual('test@example.com', 2, 60_000, 3); + expect(res.allowed).toBe(true); + }); + + it('correctly handles case-insensitivity of targets', async () => { + mockHeaders.get.mockImplementation((name: string) => { + if (name === 'x-forwarded-for') return '1.2.3.4'; + return null; + }); + + expect((await rateLimitDual('TEST@example.com', 2, 60_000, 5)).allowed).toBe(true); + expect((await rateLimitDual('test@EXAMPLE.com', 2, 60_000, 5)).allowed).toBe(true); + expect((await rateLimitDual('Test@Example.Com', 2, 60_000, 5)).allowed).toBe(false); + }); + + it('extracts client IP correctly from comma-separated x-forwarded-for', async () => { + mockHeaders.get.mockImplementation((name: string) => { + if (name === 'x-forwarded-for') return '2.3.4.5, 8.8.8.8'; + return null; + }); + + // We should hit IP limit for 2.3.4.5 after 2 hits + expect((await rateLimitDual('a@b.com', 5, 60_000, 2)).allowed).toBe(true); + expect((await rateLimitDual('b@b.com', 5, 60_000, 2)).allowed).toBe(true); + + const blocked = await rateLimitDual('c@b.com', 5, 60_000, 2); + expect(blocked.allowed).toBe(false); + }); + + it('falls back to x-real-ip if x-forwarded-for is missing', async () => { + mockHeaders.get.mockImplementation((name: string) => { + if (name === 'x-real-ip') return '3.4.5.6'; + return null; + }); + + expect((await rateLimitDual('a@b.com', 5, 60_000, 1)).allowed).toBe(true); + expect((await rateLimitDual('b@b.com', 5, 60_000, 1)).allowed).toBe(false); + }); + + it('falls back to 127.0.0.1 if both headers are missing', async () => { + mockHeaders.get.mockReturnValue(null); + + expect((await rateLimitDual('a@b.com', 5, 60_000, 1)).allowed).toBe(true); + expect((await rateLimitDual('b@b.com', 5, 60_000, 1)).allowed).toBe(false); + }); + + it('blocks IP first and prevents target bucket pollution (Account Lockout DoS protection)', async () => { + // Scenario: Attacker at IP 9.9.9.9 tries to brute force/block 'victim@example.com' + // The IP limit is 2, the target limit is 5. + mockHeaders.get.mockImplementation((name: string) => { + if (name === 'x-forwarded-for') return '9.9.9.9'; + return null; + }); + + // 1. IP is within limit (1st hit, allowed) + expect((await rateLimitDual('victim@example.com', 5, 60_000, 2)).allowed).toBe(true); + + // 2. IP is within limit (2nd hit, allowed) + expect((await rateLimitDual('victim@example.com', 5, 60_000, 2)).allowed).toBe(true); + + // 3. IP exceeds limit (3rd hit from 9.9.9.9, blocked on IP) + const res3 = await rateLimitDual('victim@example.com', 5, 60_000, 2); + expect(res3.allowed).toBe(false); + + // 4. Attacker tries a different target from same blocked IP 9.9.9.9 + const res4 = await rateLimitDual('other@example.com', 5, 60_000, 2); + expect(res4.allowed).toBe(false); // blocked because IP is blocked! + + // 5. The key security claim: has 'other@example.com' or 'victim@example.com' been polluted? + // Change IP to legitimate user's IP (e.g., 7.7.7.7) + mockHeaders.get.mockImplementation((name: string) => { + if (name === 'x-forwarded-for') return '7.7.7.7'; + return null; + }); + + // Legitimate user from 7.7.7.7 should still be able to request 'victim@example.com' + // because target 'victim@example.com' has only 2 actual hits (not blocked, limit is 5) + // and 'other@example.com' should have 0 hits (not polluted by attacker's attempt on step 4). + expect((await rateLimitDual('victim@example.com', 5, 60_000, 2)).allowed).toBe(true); + expect((await rateLimitDual('other@example.com', 5, 60_000, 2)).allowed).toBe(true); + }); + }); +}); diff --git a/src/lib/rate-limit.ts b/src/lib/rate-limit.ts index 5d0ef23..5028400 100644 --- a/src/lib/rate-limit.ts +++ b/src/lib/rate-limit.ts @@ -8,6 +8,7 @@ */ import 'server-only'; +import { headers } from 'next/headers'; const buckets = new Map(); @@ -47,3 +48,34 @@ export function rateLimit(key: string, limit = 5, windowMs = 60_000): RateLimitR return { allowed: true, retryAfterSeconds: 0 }; } + +/** + * Dual rate limiting that checks the client IP first before checking the target-based identifier. + * This prevents Account Lockout DoS attacks where an IP-blocked attacker pollutes the target bucket. + */ +export async function rateLimitDual( + targetKey: string, + limit = 5, + windowMs = 60_000, + ipLimit = 20, +): Promise { + const headersList = await headers(); + const xff = headersList.get('x-forwarded-for'); + const ip = (xff ? xff.split(',')[0]?.trim() : null) || headersList.get('x-real-ip') || '127.0.0.1'; + + // 1. IP check first to prevent Account Lockout DoS + const ipResult = rateLimit(`ip:${ip}`, ipLimit, windowMs); + if (!ipResult.allowed) { + return ipResult; + } + + // 2. Target check second + return rateLimit(`target:${targetKey.toLowerCase()}`, limit, windowMs); +} + +/** + * Helper to reset in-memory rate limits between tests. + */ +export function resetRateLimits(): void { + buckets.clear(); +}