diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 29abdfb..6728c9c 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -1,5 +1,10 @@ # Sentinel Journal — Critical Security Learnings +## 2026-07-20 - Dual Rate-Limiting Authentication Server Actions +**Vulnerability:** The application only rate-limited `signInAction` and `signUpAction` based on lowercase email keys. This allowed single-IP credential stuffing attacks targeting many different emails concurrently, bypassing email-specific rate limits and risking database/event-loop resource starvation. +**Learning:** Target-only rate-limiting leaves systems open to distributed credential stuffing and IP-wide brute-forcing. Dual rate-limiting (IP-based + target-based email keys) is essential to block high-frequency attacks from single IPs regardless of target email diversity. +**Prevention:** Always implement dual rate-limiting in sensitive user-facing entry points like login or register, utilizing `headers()` from `next/headers` to isolate requests by IP alongside target identifier keys. + ## 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. diff --git a/src/__tests__/setup.ts b/src/__tests__/setup.ts index 50336ab..ee27fc5 100644 --- a/src/__tests__/setup.ts +++ b/src/__tests__/setup.ts @@ -8,6 +8,7 @@ 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/__tests__/auth-actions.test.ts b/src/app/actions/__tests__/auth-actions.test.ts index 766ee9b..6014a94 100644 --- a/src/app/actions/__tests__/auth-actions.test.ts +++ b/src/app/actions/__tests__/auth-actions.test.ts @@ -29,6 +29,7 @@ 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/__tests__/progress-actions.test.ts b/src/app/actions/__tests__/progress-actions.test.ts index 460ba74..eb4cc14 100644 --- a/src/app/actions/__tests__/progress-actions.test.ts +++ b/src/app/actions/__tests__/progress-actions.test.ts @@ -25,6 +25,7 @@ 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/__tests__/tool-actions.test.ts b/src/app/actions/__tests__/tool-actions.test.ts index 5a60674..fa48e4d 100644 --- a/src/app/actions/__tests__/tool-actions.test.ts +++ b/src/app/actions/__tests__/tool-actions.test.ts @@ -35,6 +35,7 @@ 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..be52327 100644 --- a/src/app/actions/auth.ts +++ b/src/app/actions/auth.ts @@ -5,6 +5,7 @@ 'use server'; import { redirect } from 'next/navigation'; +import { headers } from 'next/headers'; import { db } from '@/lib/db'; import { hashPassword, @@ -32,9 +33,19 @@ import { // --------------------------------------------------------------------------- export const signUpAction = createSafeAction(signUpSchema, async (data) => { - const rl = rateLimit(`signup:${data.email.toLowerCase()}`, 5, 60_000); - if (!rl.allowed) { - throw new Error(`Too many attempts. Try again in ${rl.retryAfterSeconds}s.`); + const emailKey = `signup:${data.email.toLowerCase()}`; + const heads = await headers(); + const ip = heads.get('x-forwarded-for') || heads.get('x-real-ip') || '127.0.0.1'; + const ipKey = `signup:ip:${ip}`; + + const rlEmail = rateLimit(emailKey, 5, 60_000); + const rlIp = rateLimit(ipKey, 20, 60_000); + + if (!rlEmail.allowed) { + throw new Error(`Too many attempts. Try again in ${rlEmail.retryAfterSeconds}s.`); + } + if (!rlIp.allowed) { + throw new Error(`Too many attempts from this IP. Try again in ${rlIp.retryAfterSeconds}s.`); } const existing = await db.user.findUnique({ where: { email: data.email } }); @@ -125,9 +136,19 @@ 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); - if (!rl.allowed) { - throw new Error(`Too many attempts. Try again in ${rl.retryAfterSeconds}s.`); + const emailKey = `signin:${data.email.toLowerCase()}`; + const heads = await headers(); + const ip = heads.get('x-forwarded-for') || heads.get('x-real-ip') || '127.0.0.1'; + const ipKey = `signin:ip:${ip}`; + + const rlEmail = rateLimit(emailKey, 5, 60_000); + const rlIp = rateLimit(ipKey, 20, 60_000); + + if (!rlEmail.allowed) { + throw new Error(`Too many attempts. Try again in ${rlEmail.retryAfterSeconds}s.`); + } + if (!rlIp.allowed) { + throw new Error(`Too many attempts from this IP. Try again in ${rlIp.retryAfterSeconds}s.`); } const user = await db.user.findUnique({ where: { email: data.email } }); diff --git a/src/lib/__tests__/rate-limit.test.ts b/src/lib/__tests__/rate-limit.test.ts new file mode 100644 index 0000000..f5d4128 --- /dev/null +++ b/src/lib/__tests__/rate-limit.test.ts @@ -0,0 +1,101 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { rateLimit } from '../rate-limit'; + +vi.mock('server-only', () => ({})); + +describe('rate-limit.ts', () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date('2026-07-16T12:00:00Z')); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('allows requests within limit and denies requests exceeding limit', () => { + const key = 'user1'; + + // First 5 attempts should be allowed + for (let i = 0; i < 5; i++) { + const res = rateLimit(key, 5, 60_000); + expect(res.allowed).toBe(true); + expect(res.retryAfterSeconds).toBe(0); + } + + // 6th attempt should be denied + const res = rateLimit(key, 5, 60_000); + expect(res.allowed).toBe(false); + expect(res.retryAfterSeconds).toBe(60); // 12:00:00 to oldest hit (12:00:00) + 60s + }); + + it('correctly calculates retryAfterSeconds', () => { + const key = 'user2'; + + // Hit at T=0 + rateLimit(key, 1, 60_000); + + // Advance time by 15.5 seconds (15500 ms) + vi.advanceTimersByTime(15_500); + + // Next hit should be denied, retryAfterSeconds should be ceil((0 + 60s - 15.5s) / 1000) = ceil(44.5) = 45s + const res = rateLimit(key, 1, 60_000); + expect(res.allowed).toBe(false); + expect(res.retryAfterSeconds).toBe(45); + }); + + it('does not record denied hits', () => { + const key = 'user3'; + + // Limit is 1, window is 60s + rateLimit(key, 1, 60_000); // T=0 (Allowed) + + vi.advanceTimersByTime(30_000); // T=30s + rateLimit(key, 1, 60_000); // Denied, should not be recorded + + vi.advanceTimersByTime(31_000); // T=61s (which is > 60s from first hit) + + // If the denied hit at T=30s was recorded, this would be denied. But since it wasn't, this should be allowed! + const res = rateLimit(key, 1, 60_000); + expect(res.allowed).toBe(true); + }); + + it('allows requests again after window expires', () => { + const key = 'user4'; + + rateLimit(key, 2, 60_000); // Hit 1 at T=0 + vi.advanceTimersByTime(10_000); + rateLimit(key, 2, 60_000); // Hit 2 at T=10s + + // 3rd hit at T=10s should be denied + expect(rateLimit(key, 2, 60_000).allowed).toBe(false); + + // Advance past T=0 + 60s (to T=61s) + vi.advanceTimersByTime(51_000); // Now T=61s, the first hit is expired, but second hit (at T=10s) is still active. + + // We can make 1 more hit (since limit is 2 and only 1 active hit exists) + expect(rateLimit(key, 2, 60_000).allowed).toBe(true); + // Next hit is blocked + expect(rateLimit(key, 2, 60_000).allowed).toBe(false); + + // Advance past T=10s + 60s (to T=71s) + vi.advanceTimersByTime(10_000); // Now T=71s, both original hits are expired. + expect(rateLimit(key, 2, 60_000).allowed).toBe(true); + }); + + it('performs opportunistic cleanup when key count exceeds 10,000', () => { + // Generate 10,005 unique keys and call rateLimit on them + // All of them are called within the window, so no cleanup should delete active hits. + for (let i = 0; i < 10_005; i++) { + rateLimit(`cleanup_key_${i}`, 1, 60_000); + } + + // Now, advance the timer by 61 seconds so all of them expire + vi.advanceTimersByTime(61_000); + + // Calling rateLimit on one more key triggers the cleanup block (as buckets.size is > 10,000) + // and deletes all expired keys from the map. + const res = rateLimit('trigger_key', 1, 60_000); + expect(res.allowed).toBe(true); + }); +});