From e5f53d038ab0d3681e548133a2ff63401c16d8ef Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Sat, 1 Aug 2026 12:56:42 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20add=20dual?= =?UTF-8?q?=20rate=20limiting=20to=20authentication=20server=20actions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com> --- .jules/sentinel.md | 5 + src/__tests__/setup.ts | 3 + src/app/actions/auth.ts | 6 +- src/lib/__tests__/rate-limit.test.ts | 180 +++++++++++++++++++++++++++ src/lib/rate-limit.ts | 38 ++++++ 5 files changed, 229 insertions(+), 3 deletions(-) create mode 100644 src/lib/__tests__/rate-limit.test.ts diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 29abdfb..84bb594 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-17 - Dual-Layer Rate Limiting for Signup and Signin Server Actions +**Vulnerability:** The signup and signin Server Actions used only target-based (email) rate limiting. This left the application vulnerable to distributed credential stuffing attacks, where attackers query different target accounts from a single client IP (or a small set of IPs) without hitting target-based lockout thresholds. +**Learning:** Target-only rate limiting can be bypassed by distributing requests across a wide variety of target keys (e.g., trying a common password against many different usernames/emails). +**Prevention:** Always combine target-based rate limiting with client IP-based rate limiting (dual-layer rate limiting) using the `x-forwarded-for` and `x-real-ip` headers on sensitive authentication actions, allowing graceful degradation if headers are not present. diff --git a/src/__tests__/setup.ts b/src/__tests__/setup.ts index 50336ab..138985f 100644 --- a/src/__tests__/setup.ts +++ b/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', () => ({ diff --git a/src/app/actions/auth.ts b/src/app/actions/auth.ts index 2f10075..ceb0455 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); 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); 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..c6baf8d --- /dev/null +++ b/src/lib/__tests__/rate-limit.test.ts @@ -0,0 +1,180 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +vi.mock('next/headers', () => ({ + headers: vi.fn(), +})); + +import { rateLimit, rateLimitDual } from '../rate-limit'; +import { headers } from 'next/headers'; + +describe('rate-limit.ts', () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + describe('rateLimit (sliding window)', () => { + it('allows hits within limits', () => { + const key = 'user1'; + for (let i = 0; i < 5; i++) { + const res = rateLimit(key, 5, 60_000); + expect(res.allowed).toBe(true); + expect(res.retryAfterSeconds).toBe(0); + } + }); + + it('denies hits when limit is exceeded', () => { + const key = 'user2'; + for (let i = 0; i < 5; i++) { + rateLimit(key, 5, 60_000); + } + + const res = rateLimit(key, 5, 60_000); + expect(res.allowed).toBe(false); + expect(res.retryAfterSeconds).toBe(60); // 60 seconds remaining + }); + + it('denies hits and calculates remaining lockout seconds correctly', () => { + const key = 'user3'; + // T = 0ms: hit 1 + rateLimit(key, 2, 60_000); + + // Advance by 15.5 seconds (T = 15500ms) + vi.advanceTimersByTime(15_500); + + // T = 15.5s: hit 2 (limit reached) + rateLimit(key, 2, 60_000); + + // Hit 3 at T = 15.5s should be denied + const res = rateLimit(key, 2, 60_000); + expect(res.allowed).toBe(false); + // Hit 1 was at T=0. It falls out at T=60000ms. + // So retryAfterSeconds should be ceil((0 + 60000 - 15500) / 1000) = ceil(44.5) = 45 seconds. + expect(res.retryAfterSeconds).toBe(45); + }); + + it('allows hits again after window has elapsed', () => { + const key = 'user4'; + rateLimit(key, 1, 60_000); + + // Denied + expect(rateLimit(key, 1, 60_000).allowed).toBe(false); + + // Advance by 60.1 seconds + vi.advanceTimersByTime(60_100); + + // Allowed again + expect(rateLimit(key, 1, 60_000).allowed).toBe(true); + }); + + it('performs opportunistic cleanup of buckets Map when size exceeds threshold', () => { + // Create over 10,000 keys with a past timestamp so they are stale and cleaned up. + // Let's set time to T=100000 + vi.setSystemTime(new Date(100_000)); + + for (let i = 0; i < 10005; i++) { + rateLimit(`key-${i}`, 5, 10); // small window of 10ms + } + + // Advance time so all of them are now past cutoff (10ms) + vi.advanceTimersByTime(50); + + // Calling rateLimit again with a new key will trigger size > 10_000 check and cleanup + const res = rateLimit('trigger-cleanup-key', 5, 10); + expect(res.allowed).toBe(true); + }); + }); + + describe('rateLimitDual', () => { + it('allows when under limits and headers are available', async () => { + (headers as unknown as ReturnType).mockResolvedValue({ + get: (h: string) => { + if (h === 'x-forwarded-for') return '192.168.1.1'; + return null; + }, + }); + + const res = await rateLimitDual('login', 'test@example.com', { limit: 3 }); + expect(res.allowed).toBe(true); + expect(res.retryAfterSeconds).toBe(0); + }); + + it('applies lowercase to targetKey', async () => { + (headers as unknown as ReturnType).mockResolvedValue({ + get: () => null, + }); + + // Email with uppercase chars + await rateLimitDual('test-case', 'User@Example.Com', { limit: 1 }); + + // Hit with same email, lowercase + const res = await rateLimitDual('test-case', 'user@example.com', { limit: 1 }); + expect(res.allowed).toBe(false); + }); + + it('extracts first IP from x-forwarded-for list correctly', async () => { + (headers as unknown as ReturnType).mockResolvedValue({ + get: (h: string) => { + if (h === 'x-forwarded-for') return '1.2.3.4, 5.6.7.8, 9.10.11.12'; + return null; + }, + }); + + // IP limit is limit * 2 = 2. + // 1st hit from IP 1.2.3.4 (target different) + await rateLimitDual('ip-test', 'email1@example.com', { limit: 1 }); + // 2nd hit from same IP (target different) + await rateLimitDual('ip-test', 'email2@example.com', { limit: 1 }); + // 3rd hit from same IP (exceeds IP limit of 2) + const res = await rateLimitDual('ip-test', 'email3@example.com', { limit: 1 }); + expect(res.allowed).toBe(false); + }); + + it('falls back to x-real-ip if x-forwarded-for is missing', async () => { + (headers as unknown as ReturnType).mockResolvedValue({ + get: (h: string) => { + if (h === 'x-real-ip') return '9.9.9.9'; + return null; + }, + }); + + // limit * 2 = 2 hits allowed for IP 9.9.9.9. + await rateLimitDual('real-ip-test', 'email1@example.com', { limit: 1 }); + await rateLimitDual('real-ip-test', 'email2@example.com', { limit: 1 }); + + const res = await rateLimitDual('real-ip-test', 'email3@example.com', { limit: 1 }); + expect(res.allowed).toBe(false); + }); + + it('falls back to unknown if no IP headers are present', async () => { + (headers as unknown as ReturnType).mockResolvedValue({ + get: () => null, + }); + + await rateLimitDual('unknown-ip-test', 'email1@example.com', { limit: 1 }); + await rateLimitDual('unknown-ip-test', 'email2@example.com', { limit: 1 }); + + const res = await rateLimitDual('unknown-ip-test', 'email3@example.com', { limit: 1 }); + expect(res.allowed).toBe(false); + }); + + it('degrades gracefully if headers() throws an error', async () => { + (headers as unknown as ReturnType).mockRejectedValue(new Error('Headers unavailable')); + + // IP-based limit won't apply because headers throws, but target limit still applies. + // Limit = 2. + const res1 = await rateLimitDual('throw-test', 'email1@example.com', { limit: 2 }); + expect(res1.allowed).toBe(true); + + const res2 = await rateLimitDual('throw-test', 'email1@example.com', { limit: 2 }); + expect(res2.allowed).toBe(true); + + const res3 = await rateLimitDual('throw-test', 'email1@example.com', { limit: 2 }); + expect(res3.allowed).toBe(false); + }); + }); +}); diff --git a/src/lib/rate-limit.ts b/src/lib/rate-limit.ts index 5d0ef23..c5e7eaa 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,40 @@ export function rateLimit(key: string, limit = 5, windowMs = 60_000): RateLimitR return { allowed: true, retryAfterSeconds: 0 }; } + +/** + * Dual rate limiter: combines target-based rate limiting with client IP-based rate limiting. + * Protects against credential stuffing and distributed brute-force attacks. + */ +export async function rateLimitDual( + actionType: string, + targetKey: string, + options: { limit?: number; windowMs?: number } = {} +): Promise { + const limit = options.limit ?? 5; + const windowMs = options.windowMs ?? 60_000; + + // 1. Target-based rate limiting (e.g., lowercase email) + const targetKeyLower = targetKey.toLowerCase(); + const targetKeyPrefixed = `${actionType}:target:${targetKeyLower}`; + const targetResult = rateLimit(targetKeyPrefixed, limit, windowMs); + if (!targetResult.allowed) { + return targetResult; + } + + // 2. IP-based rate limiting (double the limit of target-based to allow NAT/shared IPs) + try { + 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'; + const ipPrefixed = `${actionType}:ip:${ip}`; + const ipResult = rateLimit(ipPrefixed, limit * 2, windowMs); + if (!ipResult.allowed) { + return ipResult; + } + } catch { + // Graceful degradation if headers() throws or is unavailable + } + + return { allowed: true, retryAfterSeconds: 0 }; +}