🛡️ Sentinel: dual rate-limiting to prevent IP/Account Lockout DoS - #113
🛡️ Sentinel: dual rate-limiting to prevent IP/Account Lockout DoS#113projectamazonph wants to merge 4 commits into
Conversation
…and Account Lockout DoS (STORY-058) - Added asynchronous dual rate-limiting utility rateLimitDual in src/lib/rate-limit.ts - Updated signUpAction and signInAction to use rateLimitDual - Updated next/headers mocks for vitest - Added comprehensive unit tests in rate-limit.test.ts, achieving 100% coverage - Logged critical learning in .jules/sentinel.md Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAuthentication signup and signin now use IP-first dual rate limiting. The rate-limit module adds request-header handling and a bucket reset helper. Tests cover enforcement, cleanup, IP selection, and test isolation. ChangesAuthentication rate limiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AuthAction
participant NextHeaders
participant RateLimit
AuthAction->>NextHeaders: Read client IP
NextHeaders-->>AuthAction: Return header value
AuthAction->>RateLimit: Check IP bucket
RateLimit-->>AuthAction: Allow or reject
AuthAction->>RateLimit: Check email bucket when allowed
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/__tests__/rate-limit.test.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winColocate this Vitest file with its implementation.
Move this file to
src/lib/rate-limit.test.ts. The coding guideline requiresfoo.test.tsnext tofoo.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/__tests__/rate-limit.test.ts` at line 1, Move the Vitest test file from the __tests__ directory to sit alongside the rate-limit implementation as src/lib/rate-limit.test.ts, preserving its existing test contents and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/__tests__/rate-limit.test.ts`:
- Around line 1-17: Import resetRateLimits alongside rateLimit and
rateLimitDual, then invoke resetRateLimits in the existing beforeEach setup so
every test starts with isolated rate-limit bucket state.
In `@src/lib/rate-limit.ts`:
- Around line 61-63: Update the IP identity handling in the rate-limit flow so
missing or untrusted client-supplied headers do not share the ip:unknown bucket
or select another user's bucket; apply the IP limit only to trusted
ingress-provided identities, or use the existing per-target-first fallback with
its first hit counted. Adjust the tests covering the “unknown” fallback to
assert the selected behavior, and verify the deployment ingress replaces rather
than forwards client-controlled x-forwarded-for values.
---
Nitpick comments:
In `@src/lib/__tests__/rate-limit.test.ts`:
- Line 1: Move the Vitest test file from the __tests__ directory to sit
alongside the rate-limit implementation as src/lib/rate-limit.test.ts,
preserving its existing test contents and behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19dfd4f0-a02a-4839-bcc5-cc3d7041cc01
📒 Files selected for processing (6)
.jules/sentinel.mdsrc/__tests__/setup.tssrc/app/actions/__tests__/auth-actions.test.tssrc/app/actions/auth.tssrc/lib/__tests__/rate-limit.test.tssrc/lib/rate-limit.ts
| 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(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reset rate-limit state before each test.
buckets persists across test cases. Import resetRateLimits and call it in beforeEach so each test starts with isolated limiter state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/__tests__/rate-limit.test.ts` around lines 1 - 17, Import
resetRateLimits alongside rateLimit and rateLimitDual, then invoke
resetRateLimits in the existing beforeEach setup so every test starts with
isolated rate-limit bucket state.
| 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'; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect repository deployment configuration and documentation for forwarding-header handling.
fd -HI -t f . | rg '(next\.config|vercel\.json|Dockerfile|docker-compose|nginx|caddy|traefik|kubernetes|helm|README|docs)' \
| while IFS= read -r file; do
rg -n -i -C 3 'x-forwarded-for|x-real-ip|trusted proxy|proxy_set_header|real_ip_header' "$file" || true
doneRepository: projectamazonph/amph-v2
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -HI -t f 'rate-limit|rate-limit\.test|vercel\.json|next\.config|Dockerfile|docker-compose|.*nginx.*|.*caddy.*|.*traefik.*|.*kubernetes.*|.*helm.*|README|docs/voice-guide' . \
| sed -n '1,120p'
echo "== src/lib/rate-limit.ts outline =="
ast-grep outline src/lib/rate-limit.ts 2>/dev/null || true
echo "== src/lib/rate-limit.ts relevant lines =="
cat -n src/lib/rate-limit.ts | sed -n '1,150p'
echo "== src/lib/__tests__/rate-limit.test.ts relevant lines =="
cat -n src/lib/__tests__/rate-limit.test.ts | sed -n '1,220p'
echo "== search for use of limitRequest/rate limiting helpers =="
rg -n "limitRequest|rateLimit|rate-limit|unknown|x-forwarded-for|x-real-ip" src package.json -SRepository: projectamazonph/amph-v2
Length of output: 31800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
fd -HI -t f 'rate-limit|rate-limit\.test|vercel\.json|next\.config|Dockerfile|docker-compose|.*nginx.*|.*caddy.*|.*traefik.*|.*kubernetes.*|.*helm.*|README|docs/voice-guide' . \
| sed -n '1,120p'
echo "== rate-limit source excerpt =="
cat -n src/lib/rate-limit.ts | sed -n '1,150p'
echo "== rate-limit tests excerpt =="
cat -n src/lib/__tests__/rate-limit.test.ts | sed -n '1,220p'
echo "== usages/excerpts =="
rg -n "limitRequest|rateLimit|rate-limit|unknown|x-forwarded-for|x-real-ip" src package.json -SRepository: projectamazonph/amph-v2
Length of output: 31357
Do not rate-limit missing-identity requests in a shared ip:unknown bucket.
When both x-forwarded-for and x-real-ip are absent, sign-in and sign-up requests use the same ip:unknown bucket. Five of those requests can block other authenticated sign-in/sign-up attempts to different targets for 60 seconds. Only apply the IP bucket for trusted ingress-supplied identities or add a separate per-target-first default path, with the first allowed hit counted; otherwise, skip the IP check for client-supplied/missing headers. Update the existing “falls back to unknown” tests to match the chosen default behavior.
Verify the deployment ingress replaces client-supplied x-forwarded-for; if it passes user-controlled values through, it can select the IP rate-limit bucket.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/rate-limit.ts` around lines 61 - 63, Update the IP identity handling
in the rate-limit flow so missing or untrusted client-supplied headers do not
share the ip:unknown bucket or select another user's bucket; apply the IP limit
only to trusted ingress-provided identities, or use the existing
per-target-first fallback with its first hit counted. Adjust the tests covering
the “unknown” fallback to assert the selected behavior, and verify the
deployment ingress replaces rather than forwards client-controlled
x-forwarded-for values.
…ix broken CI pnpm version (STORY-058) - Added asynchronous dual rate-limiting utility rateLimitDual in src/lib/rate-limit.ts - Updated signUpAction and signInAction to use rateLimitDual - Updated next/headers mocks for vitest - Added comprehensive unit tests in rate-limit.test.ts, achieving 100% coverage - Logged critical learning in .jules/sentinel.md - Changed packageManager in package.json to pnpm@11.12.0 to resolve broken pnpm v11.13.0 release in GitHub CI environment Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com>
…ix broken CI pnpm version (STORY-058) - Added asynchronous dual rate-limiting utility rateLimitDual in src/lib/rate-limit.ts - Updated signUpAction and signInAction to use rateLimitDual - Updated next/headers mocks for vitest - Added comprehensive unit tests in rate-limit.test.ts, achieving 100% coverage - Logged critical learning in .jules/sentinel.md - Changed packageManager in package.json to pnpm@9.15.4 to resolve broken pnpm v11.x release in GitHub CI environment Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com>
…ix broken CI pnpm version (STORY-058) - Added asynchronous dual rate-limiting utility rateLimitDual in src/lib/rate-limit.ts - Updated signUpAction and signInAction to use rateLimitDual - Updated next/headers mocks for vitest - Added comprehensive unit tests in rate-limit.test.ts, achieving 100% coverage - Logged critical learning in .jules/sentinel.md - Changed packageManager in package.json to pnpm@9.15.4 to resolve broken pnpm v11.x release in GitHub CI environment - Added packages field to pnpm-workspace.yaml to resolve pnpm workspace validation errors in CI cache and setup-node commands Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com>
🚨 Severity: MEDIUM
💡 Vulnerability:
Previously, rate-limiting on signup and signin actions was strictly limited by the target email address. This enabled malicious actors to trigger rate limits for target accounts, causing legitimate users to suffer from Account Lockout Denial of Service (DoS) attacks. It also made the event-loop vulnerable to distributed brute-force or credential stuffing.
🔧 Fix:
rateLimitDual) insrc/lib/rate-limit.tsthat constraints both the client IP and target email.signUpActionandsignInActioninsrc/app/actions/auth.tsto userateLimitDual.resetRateLimits()test reset function to prevent test pollution and mock state leak.✅ Verification:
src/lib/__tests__/rate-limit.test.tswith 100% statement, branch, and function coverage.pnpm typecheck) and successful production build (pnpm build)..jules/sentinel.md.PR created automatically by Jules for task 13655340484834439284 started by @projectamazonph
Summary by CodeRabbit
New Features
Bug Fixes
Tests