diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 29abdfb..686a3fb 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-08-06 - Test Pollution from Persistent In-Memory Rate Limit Buckets +**Vulnerability:** When introducing asynchronous dual rate-limiting (`rateLimitDual`) to critical Server Actions, in-memory sliding-window buckets persisted across test executions. This caused sequential unit tests using identical email/identifier payloads to exceed rate limits and fail unexpectedly. +**Learning:** In Next.js Server Actions, in-memory rate limiters maintain static module-scope state. In a testing suite, successive actions on the same entity or test user will inadvertently trigger rate limit blocks. +**Prevention:** Always export a state reset helper (e.g., `resetRateLimits()`) to clear rate-limiting maps in `beforeEach` hooks, ensuring complete isolation between test runs and preventing test pollution. diff --git a/package.json b/package.json index e60c440..64c0178 100644 --- a/package.json +++ b/package.json @@ -72,5 +72,5 @@ "eslint --fix" ] }, - "packageManager": "pnpm@11.13.0+sha512.88d94724d8f2e6c186744a5584c6e59ecac869ec7ba15e9cb4cd628e8dc7066820b2481d8ee3b51ea8da323a7378068aa58c556a3720d32b7c20a051d088363a" + "packageManager": "pnpm@9.15.4" } diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index c01f7c7..00a6024 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -1,3 +1,6 @@ +packages: + - '.' + onlyBuiltDependencies: - "@prisma/client" - "@prisma/engines" 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..bf18992 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', () => ({ @@ -36,10 +37,12 @@ vi.mock('next/navigation', () => ({ })); import { db } from '@/lib/db'; +import { resetRateLimits } from '@/lib/rate-limit'; describe('auth actions', () => { beforeEach(() => { vi.resetAllMocks(); + resetRateLimits(); mockSignToken.mockResolvedValue('token'); }); diff --git a/src/app/actions/auth.ts b/src/app/actions/auth.ts index 2f10075..a27c357 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.toLowerCase()}`, 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.toLowerCase()}`, 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..1143708 --- /dev/null +++ b/src/lib/__tests__/rate-limit.test.ts @@ -0,0 +1,158 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +const mockHeadersGet = vi.fn<(key: string) => string | null>(() => null); + +vi.mock('next/headers', () => ({ + headers: () => Promise.resolve({ + get: (key: string) => mockHeadersGet(key), + }), +})); + +import { rateLimit, rateLimitDual } from '../rate-limit'; + +describe('rate-limit.ts', () => { + beforeEach(() => { + mockHeadersGet.mockReset(); + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + describe('rateLimit', () => { + it('allows hits under the limit and blocks exceeding hits within the window', () => { + const key = 'test:rateLimit'; + + // 5 allowed hits + 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 hit blocked + const blockedRes = rateLimit(key, 5, 60_000); + expect(blockedRes.allowed).toBe(false); + expect(blockedRes.retryAfterSeconds).toBe(60); + + // Advance time by 30 seconds, still blocked + vi.advanceTimersByTime(30_000); + const blockedRes2 = rateLimit(key, 5, 60_000); + expect(blockedRes2.allowed).toBe(false); + expect(blockedRes2.retryAfterSeconds).toBe(30); + + // Advance time by another 31 seconds, oldest hit falls out of window + vi.advanceTimersByTime(31_000); + const allowedRes = rateLimit(key, 5, 60_000); + expect(allowedRes.allowed).toBe(true); + }); + + it('does not record blocked hits so retryAfterSeconds does not get extended indefinitely', () => { + const key = 'test:blocked'; + + // Burn limit + for (let i = 0; i < 5; i++) { + rateLimit(key, 5, 60_000); + } + + // Advance 50 seconds + vi.advanceTimersByTime(50_000); + + // Blocked hit + const res1 = rateLimit(key, 5, 60_000); + expect(res1.allowed).toBe(false); + + // Advance 11 seconds (total 61s from start). The original hits expired, and blocked hits shouldn't count. + vi.advanceTimersByTime(11_000); + const res2 = rateLimit(key, 5, 60_000); + expect(res2.allowed).toBe(true); + }); + + it('performs opportunistic cleanup when buckets map size exceeds 10,000', () => { + // Fill buckets past 10,000 with expired entries + for (let i = 0; i < 10005; i++) { + rateLimit(`cleanup:${i}`, 5, 60_000); + } + // Advance time past windowMs + vi.advanceTimersByTime(65_000); + // Trigger cleanup + rateLimit('trigger-cleanup', 5, 60_000); + + // The cleanup should have run. Check that adding to cleanup:0 is fresh + const res = rateLimit('cleanup:0', 5, 60_000); + expect(res.allowed).toBe(true); + }); + }); + + describe('rateLimitDual', () => { + it('uses x-forwarded-for header if present (trimmed first item)', async () => { + mockHeadersGet.mockImplementation((key) => { + if (key === 'x-forwarded-for') return ' 1.2.3.4, 5.6.7.8 '; + return null; + }); + + const res = await rateLimitDual('target:1', 1, 60_000); + expect(res.allowed).toBe(true); + + // Check that IP bucket for 1.2.3.4 is populated and blocks + mockHeadersGet.mockImplementation((key) => { + if (key === 'x-forwarded-for') return '1.2.3.4'; + return null; + }); + const blockedRes = await rateLimitDual('target:other', 1, 60_000); + expect(blockedRes.allowed).toBe(false); // IP 1.2.3.4 blocked + }); + + it('falls back to x-real-ip if x-forwarded-for is absent', async () => { + mockHeadersGet.mockImplementation((key) => { + if (key === 'x-real-ip') return '8.8.8.8'; + return null; + }); + + const res = await rateLimitDual('target:2', 1, 60_000); + expect(res.allowed).toBe(true); + + // Blocked on real IP + const blockedRes = await rateLimitDual('target:other', 1, 60_000); + expect(blockedRes.allowed).toBe(false); + }); + + it('falls back to unknown if both headers are absent', async () => { + mockHeadersGet.mockReturnValue(null); + + const res = await rateLimitDual('target:3', 1, 60_000); + expect(res.allowed).toBe(true); + + // Blocked on unknown IP + const blockedRes = await rateLimitDual('target:other', 1, 60_000); + expect(blockedRes.allowed).toBe(false); + }); + + it('blocks IP first and does not pollute target bucket', async () => { + // Set IP + mockHeadersGet.mockImplementation((key) => { + if (key === 'x-forwarded-for') return '9.9.9.9'; + return null; + }); + + const target = 'target:pollute-test'; + + // Burn the IP rate limit with other targets + await rateLimitDual('dummy:1', 1, 60_000); + + // Now IP is blocked. A hit with the target should fail at the IP layer first. + const blockedIpRes = await rateLimitDual(target, 1, 60_000); + expect(blockedIpRes.allowed).toBe(false); + + // Now change the IP. The target limit shouldn't have been hit/polluted at all because the IP block prevented the target check. + mockHeadersGet.mockImplementation((key) => { + if (key === 'x-forwarded-for') return '10.10.10.10'; + return null; + }); + + const allowedRes = await rateLimitDual(target, 1, 60_000); + expect(allowedRes.allowed).toBe(true); // target still has its allowance because target check was bypassed when IP was blocked + }); + }); +}); diff --git a/src/lib/rate-limit.ts b/src/lib/rate-limit.ts index 5d0ef23..80df109 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,33 @@ export function rateLimit(key: string, limit = 5, windowMs = 60_000): RateLimitR return { allowed: true, retryAfterSeconds: 0 }; } + +/** + * Asynchronously dual rate-limit client IP first, then the target-based key. + * Prevents account lockout attacks by rejecting blocked IPs before updating target buckets. + */ +export async function rateLimitDual( + targetKey: string, + limit = 5, + windowMs = 60_000 +): Promise { + const heads = await headers(); + const xff = heads.get('x-forwarded-for'); + const ip = (xff ? xff.split(',')[0]?.trim() : null) ?? heads.get('x-real-ip') ?? 'unknown'; + + // Always check and record IP limit first. + const ipRl = rateLimit(`ip:${ip}`, limit, windowMs); + if (!ipRl.allowed) { + return ipRl; + } + + // Only check and record target limit if IP check passed. + return rateLimit(targetKey, limit, windowMs); +} + +/** + * Resets all rate limit buckets (primarily for testing isolation). + */ +export function resetRateLimits(): void { + buckets.clear(); +}