diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 29abdfb..03fd132 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-19 - Account Lockout Denial of Service via Target-Based Bucket Pollution +**Vulnerability:** Standard rate limiting on sensitive endpoints (like sign-in or sign-up) using only target-based identifiers (such as email) allows a malicious actor to continuously hit the rate limiter with a legitimate user's email, locking that user out of their own account (Account Lockout Denial of Service). +**Learning:** Single-key or incorrect-order multi-key rate-limiting can easily be weaponized to lock out legitimate users. If the target-based check is evaluated first or if there's no IP check, an attacker can pollute the target bucket. +**Prevention:** Always implement dual rate-limiting that combines client IP-based keys (safely extracted from headers like `x-forwarded-for` and `x-real-ip` using TS-safe array access fallbacks) and target-based keys (lowercase emails). Crucially, always execute the client IP rate-limiting check before checking or incrementing target-based buckets, ensuring blocked attackers cannot lock out legitimate users. diff --git a/package.json b/package.json index e60c440..dc510cb 100644 --- a/package.json +++ b/package.json @@ -72,5 +72,5 @@ "eslint --fix" ] }, - "packageManager": "pnpm@11.13.0+sha512.88d94724d8f2e6c186744a5584c6e59ecac869ec7ba15e9cb4cd628e8dc7066820b2481d8ee3b51ea8da323a7378068aa58c556a3720d32b7c20a051d088363a" + "packageManager": "pnpm@9.15.2" } diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index c01f7c7..5b748ec 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -1,3 +1,6 @@ +packages: + - "." + onlyBuiltDependencies: - "@prisma/client" - "@prisma/engines" diff --git a/src/app/actions/__tests__/auth-actions.test.ts b/src/app/actions/__tests__/auth-actions.test.ts index 766ee9b..e2632bb 100644 --- a/src/app/actions/__tests__/auth-actions.test.ts +++ b/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', () => ({ diff --git a/src/app/actions/auth.ts b/src/app/actions/auth.ts index 2f10075..597508d 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('signup', data.email, 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, 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..1df2840 --- /dev/null +++ b/src/lib/__tests__/rate-limit.test.ts @@ -0,0 +1,147 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { rateLimit, rateLimitDual, resetRateLimits } from '../rate-limit'; +import { headers } from 'next/headers'; + +vi.mock('next/headers', () => ({ + headers: vi.fn(), +})); + +describe('rate-limit.ts', () => { + beforeEach(() => { + resetRateLimits(); + vi.useFakeTimers(); + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + describe('rateLimit', () => { + it('allows requests up to the limit and blocks exceeding ones', () => { + const key = 'test-key'; + + // Limit of 3 + expect(rateLimit(key, 3, 60_000).allowed).toBe(true); + expect(rateLimit(key, 3, 60_000).allowed).toBe(true); + expect(rateLimit(key, 3, 60_000).allowed).toBe(true); + + // 4th request within the same window should be blocked + const blocked = rateLimit(key, 3, 60_000); + expect(blocked.allowed).toBe(false); + expect(blocked.retryAfterSeconds).toBeGreaterThan(0); + }); + + it('sliding window lets requests through after the window expires', () => { + const key = 'sliding-key'; + + rateLimit(key, 2, 10_000); + vi.advanceTimersByTime(4000); + rateLimit(key, 2, 10_000); + + // 3rd request is blocked + expect(rateLimit(key, 2, 10_000).allowed).toBe(false); + + // Advance by 6001 ms (total 10,001 ms from first hit) - first hit falls out + vi.advanceTimersByTime(6001); + + // Now we should be allowed again + expect(rateLimit(key, 2, 10_000).allowed).toBe(true); + }); + + it('performs cleanup if the buckets size exceeds 10,000', () => { + // Create over 10,000 stale entries + for (let i = 0; i < 10005; i++) { + rateLimit(`key-${i}`, 1, 10_000); + } + + // Advance time so they all expire + vi.advanceTimersByTime(11_000); + + // Run another rateLimit to trigger opportunistic cleanup + rateLimit('new-key', 5, 10_000); + + // Ensure behavior remains correct for the new key + expect(rateLimit('new-key', 5, 10_000).allowed).toBe(true); + }); + }); + + describe('rateLimitDual', () => { + it('extracts IP safely and limits requests', async () => { + const mockHeaders = { + get: (h: string) => { + if (h === 'x-forwarded-for') return '203.0.113.195, 198.51.100.1'; + return null; + } + }; + (headers as unknown as ReturnType).mockResolvedValue(mockHeaders); + + // Under standard limits (target = 2, ip = 4) + const res1 = await rateLimitDual('action', 'user@example.com', 2, 60_000, 4, 60_000); + expect(res1.allowed).toBe(true); + + const res2 = await rateLimitDual('action', 'user@example.com', 2, 60_000, 4, 60_000); + expect(res2.allowed).toBe(true); + + // 3rd target attempt should block + const res3 = await rateLimitDual('action', 'user@example.com', 2, 60_000, 4, 60_000); + expect(res3.allowed).toBe(false); + }); + + it('uses x-real-ip if x-forwarded-for is missing', async () => { + const mockHeaders = { + get: (h: string) => { + if (h === 'x-real-ip') return '198.51.100.5'; + return null; + } + }; + (headers as unknown as ReturnType).mockResolvedValue(mockHeaders); + + const res = await rateLimitDual('action', 'user2@example.com', 2, 60_000, 2, 60_000); + expect(res.allowed).toBe(true); + }); + + it('falls back to unknown-ip if all headers are missing', async () => { + const mockHeaders = { + get: () => null + }; + (headers as unknown as ReturnType).mockResolvedValue(mockHeaders); + + const res = await rateLimitDual('action', 'user3@example.com', 2, 60_000, 2, 60_000); + expect(res.allowed).toBe(true); + }); + + it('blocks by IP first, preventing target lockout (bucket pollution protection)', async () => { + const mockHeaders = { + get: (h: string) => { + if (h === 'x-forwarded-for') return '1.2.3.4'; + return null; + } + }; + (headers as unknown as ReturnType).mockResolvedValue(mockHeaders); + + // Target limit: 3, IP limit: 2 + // Execute 2 requests + await rateLimitDual('login', 'legit@example.com', 3, 60_000, 2, 60_000); + await rateLimitDual('login', 'legit@example.com', 3, 60_000, 2, 60_000); + + // 3rd request should hit the IP limit first + const resBlock = await rateLimitDual('login', 'legit@example.com', 3, 60_000, 2, 60_000); + expect(resBlock.allowed).toBe(false); + + // Change the IP to simulate a different client IP trying to log in as the same legitimate user + const mockHeadersNewIP = { + get: (h: string) => { + if (h === 'x-forwarded-for') return '5.6.7.8'; + return null; + } + }; + (headers as unknown as ReturnType).mockResolvedValue(mockHeadersNewIP); + + // The new IP should be able to attempt login for 'legit@example.com' since the target limit (3) wasn't breached + // and target bucket wasn't polluted by the IP-blocked requests + const resAllowed = await rateLimitDual('login', 'legit@example.com', 3, 60_000, 2, 60_000); + expect(resAllowed.allowed).toBe(true); + }); + }); +}); diff --git a/src/lib/rate-limit.ts b/src/lib/rate-limit.ts index 5d0ef23..c063f04 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,42 @@ export function rateLimit(key: string, limit = 5, windowMs = 60_000): RateLimitR return { allowed: true, retryAfterSeconds: 0 }; } + +/** + * Dual rate limiter: rate limits based on both the client IP address (retrieved safely + * from Next.js request headers) and a target identifier (such as email). + * + * Always runs the IP check BEFORE target-based check to prevent IP-blocked malicious actors + * from lockouting legitimate accounts (Account Lockout Denial of Service). + */ +export async function rateLimitDual( + action: string, + target: string, + targetLimit = 5, + targetWindowMs = 60_000, + ipLimit = 15, + ipWindowMs = 60_000 +): Promise { + const h = await headers(); + const xff = h.get('x-forwarded-for'); + const ip = (xff ? xff.split(',')[0]?.trim() : null) || h.get('x-real-ip') || 'unknown-ip'; + + const ipKey = `${action}:ip:${ip}`; + const targetKey = `${action}:target:${target.toLowerCase()}`; + + // 1. IP check first (avoids target-based bucket pollution by blocked IPs) + const ipResult = rateLimit(ipKey, ipLimit, ipWindowMs); + if (!ipResult.allowed) { + return ipResult; + } + + // 2. Target check second + return rateLimit(targetKey, targetLimit, targetWindowMs); +} + +/** + * Resets all rate-limiting buckets (useful for clearing test pollution/interference). + */ +export function resetRateLimits(): void { + buckets.clear(); +}