From b401813d822ab9cf35ae0fa9830997482e14780d Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 3 Aug 2026 12:47:27 +0000 Subject: [PATCH 1/2] fix(auth): implement secure dual rate-limiting for signup and signin Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com> --- .jules/sentinel.md | 5 + src/__tests__/setup.ts | 3 + .../actions/__tests__/auth-actions.test.ts | 3 + src/app/actions/auth.ts | 6 +- src/lib/__tests__/rate-limit.test.ts | 148 ++++++++++++++++++ src/lib/rate-limit.ts | 54 +++++++ 6 files changed, 216 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..3a3b65f 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 - Target Lockout DoS via Improper Dual Rate-Limiting Order +**Vulnerability:** When implementing dual rate-limiting (checking both target-based keys like emails, and client IP), validating/incrementing the target-based bucket before the IP-based bucket allowed IP-blocked attackers to programmatically lock out arbitrary legitimate users from authentication endpoints (Account Lockout DoS) since target hits were still being registered before the IP restriction failed the request. +**Learning:** Checking and mutating target limits prior to enforcing IP limits leaks rate-limit state increments to blocked IP addresses. +**Prevention:** Always evaluate and enforce the IP-based rate limit first. Doing so blocks malicious IP addresses at the perimeter and prevents them from registering hits in or polluting any target-based buckets. 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/__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..aace029 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, { targetLimit: 5, ipLimit: 10, windowMs: 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, { targetLimit: 5, ipLimit: 10, windowMs: 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..f995e27 --- /dev/null +++ b/src/lib/__tests__/rate-limit.test.ts @@ -0,0 +1,148 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { rateLimit, rateLimitDual, _clearBuckets } from '../rate-limit'; +import { headers } from 'next/headers'; + +vi.mock('next/headers', () => ({ + headers: vi.fn(), +})); + +describe('rate-limit.ts', () => { + beforeEach(() => { + vi.useFakeTimers(); + _clearBuckets(); + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + describe('rateLimit', () => { + it('allows hits up to the limit and then blocks them', () => { + // Limit 3, window 1000ms + expect(rateLimit('k1', 3, 1000).allowed).toBe(true); + expect(rateLimit('k1', 3, 1000).allowed).toBe(true); + expect(rateLimit('k1', 3, 1000).allowed).toBe(true); + + const blocked = rateLimit('k1', 3, 1000); + expect(blocked.allowed).toBe(false); + expect(blocked.retryAfterSeconds).toBe(1); + }); + + it('sliding window lets hits decay', () => { + expect(rateLimit('k2', 2, 1000).allowed).toBe(true); + vi.advanceTimersByTime(600); + expect(rateLimit('k2', 2, 1000).allowed).toBe(true); + + // Hit limit reached + expect(rateLimit('k2', 2, 1000).allowed).toBe(false); + + // Advance past the first hit (at 0ms), oldest remaining is at 600ms + vi.advanceTimersByTime(401); // Current time is 1001ms. 0ms hit decayed. + + const res = rateLimit('k2', 2, 1000); + expect(res.allowed).toBe(true); + }); + + it('denied hits do not extend lock out window', () => { + expect(rateLimit('k3', 1, 1000).allowed).toBe(true); + expect(rateLimit('k3', 1, 1000).allowed).toBe(false); + + vi.advanceTimersByTime(1001); + + expect(rateLimit('k3', 1, 1000).allowed).toBe(true); + }); + }); + + describe('rateLimitDual', () => { + it('rate limits on IP first to block abusive actors and prevent target pollution', async () => { + // Mock headers to return IP '1.2.3.4' + (headers as any).mockResolvedValue({ + get: (name: string) => { + if (name === 'x-forwarded-for') return '1.2.3.4'; + return null; + }, + }); + + // IP limit is 2, Target limit is 5 + // Call first 2 times with the same IP but different target IDs + expect((await rateLimitDual('act1', 'targetA', { targetLimit: 5, ipLimit: 2, windowMs: 1000 })).allowed).toBe(true); + expect((await rateLimitDual('act1', 'targetB', { targetLimit: 5, ipLimit: 2, windowMs: 1000 })).allowed).toBe(true); + + // The 3rd request from the same IP should be blocked due to IP rate-limiting, even with a fresh target + const blocked = await rateLimitDual('act1', 'targetC', { targetLimit: 5, ipLimit: 2, windowMs: 1000 }); + expect(blocked.allowed).toBe(false); + + // Because the IP was blocked first, targetC should NOT have had any hit registered in its bucket. + // Therefore, if a different/unblocked IP tries to access targetC, it should succeed! + (headers as any).mockResolvedValue({ + get: (name: string) => { + if (name === 'x-forwarded-for') return '5.6.7.8'; // different IP + return null; + }, + }); + expect((await rateLimitDual('act1', 'targetC', { targetLimit: 5, ipLimit: 2, windowMs: 1000 })).allowed).toBe(true); + }); + + it('rate limits on target identifier second', async () => { + // Mock headers to return IP '1.2.3.4' + (headers as any).mockResolvedValue({ + get: (name: string) => { + if (name === 'x-forwarded-for') return '1.2.3.4'; + return null; + }, + }); + + // Target limit is 1, but IP limit is 5 (different IPs) + // First request on IP 1.2.3.4 for targetX + expect((await rateLimitDual('act2', 'targetX', { targetLimit: 1, ipLimit: 5, windowMs: 1000 })).allowed).toBe(true); + + // Second request from a DIFFERENT IP 5.6.7.8 for targetX should be blocked because targetX limit (1) was hit + (headers as any).mockResolvedValue({ + get: (name: string) => { + if (name === 'x-forwarded-for') return '5.6.7.8'; + return null; + }, + }); + const blocked = await rateLimitDual('act2', 'targetX', { targetLimit: 1, ipLimit: 5, windowMs: 1000 }); + expect(blocked.allowed).toBe(false); + }); + + it('safely extracts IP using first element of x-forwarded-for', async () => { + (headers as any).mockResolvedValue({ + get: (name: string) => { + if (name === 'x-forwarded-for') return '1.2.3.4, 5.6.7.8'; + return null; + }, + }); + + // Target limit 10, IP limit 1 + expect((await rateLimitDual('act3', 'target1', { targetLimit: 10, ipLimit: 1, windowMs: 1000 })).allowed).toBe(true); + + // Since IP was '1.2.3.4', hitting again on same IP should be blocked + const blocked = await rateLimitDual('act3', 'target2', { targetLimit: 10, ipLimit: 1, windowMs: 1000 }); + expect(blocked.allowed).toBe(false); + }); + + it('falls back to x-real-ip if x-forwarded-for is missing', async () => { + (headers as any).mockResolvedValue({ + get: (name: string) => { + if (name === 'x-real-ip') return '9.9.9.9'; + return null; + }, + }); + + expect((await rateLimitDual('act4', 'target1', { targetLimit: 10, ipLimit: 1, windowMs: 1000 })).allowed).toBe(true); + const blocked = await rateLimitDual('act4', 'target2', { targetLimit: 10, ipLimit: 1, windowMs: 1000 }); + expect(blocked.allowed).toBe(false); + }); + + it('falls back to unknown if headers throws or returns null', async () => { + (headers as any).mockRejectedValue(new Error('no headers in this environment')); + + expect((await rateLimitDual('act5', 'target1', { targetLimit: 10, ipLimit: 1, windowMs: 1000 })).allowed).toBe(true); + const blocked = await rateLimitDual('act5', 'target2', { targetLimit: 10, ipLimit: 1, windowMs: 1000 }); + expect(blocked.allowed).toBe(false); + }); + }); +}); diff --git a/src/lib/rate-limit.ts b/src/lib/rate-limit.ts index 5d0ef23..c5ee6dc 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,56 @@ export function rateLimit(key: string, limit = 5, windowMs = 60_000): RateLimitR return { allowed: true, retryAfterSeconds: 0 }; } + +/** + * Perform dual rate-limiting on both client IP and a target-based identifier. + */ +export async function rateLimitDual( + actionName: string, + targetId: string, + options?: { + ipLimit?: number; + targetLimit?: number; + windowMs?: number; + } +): Promise { + const ipLimit = options?.ipLimit ?? 10; + const targetLimit = options?.targetLimit ?? 5; + const windowMs = options?.windowMs ?? 60_000; + + // 1. Get client IP from headers safely (noUncheckedIndexedAccess enabled) + let ip = 'unknown'; + try { + const heads = await headers(); + const xff = heads.get('x-forwarded-for'); + const xri = heads.get('x-real-ip'); + ip = (xff && xff.split(',')[0]?.trim()) || xri || 'unknown'; + } catch { + // Fail-safe default IP if headers cannot be read + ip = 'unknown'; + } + + // 2. Perform rate-limiting on client IP first to block abusive actors early + // and prevent an IP-blocked attacker from polluting target-based buckets. + const ipKey = `${actionName}:ip:${ip}`; + const ipRl = rateLimit(ipKey, ipLimit, windowMs); + if (!ipRl.allowed) { + return ipRl; + } + + // 3. Perform rate-limiting on target ID (e.g., lowercase email) + const targetKey = `${actionName}:target:${targetId.toLowerCase()}`; + const targetRl = rateLimit(targetKey, targetLimit, windowMs); + if (!targetRl.allowed) { + return targetRl; + } + + return { allowed: true, retryAfterSeconds: 0 }; +} + +/** + * Clean up helper for testing purposes. + */ +export function _clearBuckets(): void { + buckets.clear(); +} From 3ebf019867f65fcbec7ee184803e9977bb3a0e4c Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 3 Aug 2026 12:51:52 +0000 Subject: [PATCH 2/2] fix(auth): implement secure dual rate-limiting for signup and signin and resolve CI pnpm version issue Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com> --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e60c440..bc7cc2f 100644 --- a/package.json +++ b/package.json @@ -72,5 +72,5 @@ "eslint --fix" ] }, - "packageManager": "pnpm@11.13.0+sha512.88d94724d8f2e6c186744a5584c6e59ecac869ec7ba15e9cb4cd628e8dc7066820b2481d8ee3b51ea8da323a7378068aa58c556a3720d32b7c20a051d088363a" + "packageManager": "pnpm@11.14.0+sha512.66c1ac4c7d4762d6d7dde44c7f3e5a73591ed0a0806e751d4ed32d4f004f25b2285a906b1fd8a9e3e621df3b4e2858bf88e50e0cf626bedbe977fe434a5caf85" }