🛡️ Sentinel: Dual rate-limiting for auth endpoints - #91
Open
projectamazonph wants to merge 1 commit into
Open
Conversation
This commit introduces IP-based rate limiting alongside existing target-based (email) rate limiting inside `signUpAction` and `signInAction` Server Actions. By rate limiting on both client IP (safely extracted from `x-forwarded-for`/`x-real-ip` headers) and targeted email, the system is robustly protected against both single-target brute-forcing and multi-target distributed credential-stuffing attacks. Unit tests are added to verify rate limiter behavior and integration tests confirm the dual evaluation. Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com>
Contributor
|
👋 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. |
📝 WalkthroughWalkthroughAuthentication actions now enforce both normalized-email and client-IP rate limits. Tests add deterministic header mocks, verify both throttling paths, and cover rate-limit window behavior and cleanup. ChangesAuthentication rate limiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant signInAction
participant next_headers
participant rateLimit
Client->>signInAction: Submit credentials
signInAction->>next_headers: Read client IP headers
signInAction->>rateLimit: Check email key
rateLimit-->>signInAction: Email limit result
signInAction->>rateLimit: Check IP key
rateLimit-->>signInAction: IP limit result
signInAction-->>Client: Return authentication result or rate-limit error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 4
🤖 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 @.jules/sentinel.md:
- Around line 8-11: Update the “Dual Rate-Limiting” entry in sentinel.md so it
accurately states that signInAction uses signin:${email} while signUpAction uses
signup:${email}. Replace unexplained “distributed dictionary attack” wording
with plain language, and define credential stuffing as using stolen
username/password pairs against many accounts; retain the explanation that
IP-based and target-based limits are both required.
In `@src/app/actions/__tests__/auth-actions.test.ts`:
- Around line 150-204: Move the dual rate-limiting test suite from the nested
__tests__ location into src/app/actions/auth.test.ts, keeping its existing tests
and setup unchanged so the coverage remains adjacent to the auth action
implementation.
In `@src/app/actions/auth.ts`:
- Around line 36-47: Update the IP extraction used by the signup and signin
rate-limit flows around the x-forwarded-for handling to use a verified client IP
from the trusted hosting proxy boundary, rather than the leftmost forwarded
value. Ensure origin-direct requests cannot bypass the per-IP keys, and apply
the same change to both the signup and signin rate-limit logic.
In `@src/lib/__tests__/rate-limit.test.ts`:
- Around line 1-2: Move the rate-limit test suite from
src/lib/__tests__/rate-limit.test.ts to src/lib/rate-limit.test.ts, keeping its
imports and tests unchanged. Move the auth-actions test suite from
src/app/actions/__tests__/auth-actions.test.ts to src/app/actions/auth.test.ts,
preserving the existing tests and updating relative imports as needed.
🪄 Autofix (Beta)
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: 4bd0c52d-19bb-4a7c-9dd9-a2decaca6748
📒 Files selected for processing (6)
.jules/sentinel.mdsrc/__tests__/setup.tssrc/app/actions/__tests__/auth-actions.test.tssrc/app/actions/__tests__/tool-actions.test.tssrc/app/actions/auth.tssrc/lib/__tests__/rate-limit.test.ts
Comment on lines
+8
to
+11
| ## 2026-07-20 - Dual Rate-Limiting Protects Against Multi-Vector Brute-Force Attacks | ||
| **Vulnerability:** Authentication endpoints (`signInAction` and `signUpAction`) only rate-limited on a single target key (`signin:${email}`). This left the system vulnerable to distributed dictionary/credential stuffing attacks (same target from multiple IPs) and multi-target credential stuffing (multiple emails targeted from the same IP, bypassing target-based rate limits). | ||
| **Learning:** Single-key rate limiting is insufficient for modern high-value web applications. Attackers can distribute requests across thousands of accounts from a single IP to bypass target-locked throttling. | ||
| **Prevention:** Always implement dual rate-limiting combining both IP-based keys (retrieved safely via headers like `x-forwarded-for`/`x-real-ip`) and target-based keys (such as lowercase email addresses). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the old limiter description and define the attack terms.
Line 9 assigns the sign-in key to both actions, although sign-up used its own signup: key. Also define “credential stuffing” and avoid unexplained phrases such as “distributed dictionary attack.”
Proposed rewrite
-**Vulnerability:** Authentication endpoints (`signInAction` and `signUpAction`) only rate-limited on a single target key (`signin:${email}`). This left the system vulnerable to distributed dictionary/credential stuffing attacks (same target from multiple IPs) and multi-target credential stuffing (multiple emails targeted from the same IP, bypassing target-based rate limits).
-**Learning:** Single-key rate limiting is insufficient for modern high-value web applications. Attackers can distribute requests across thousands of accounts from a single IP to bypass target-locked throttling.
-**Prevention:** Always implement dual rate-limiting combining both IP-based keys (retrieved safely via headers like `x-forwarded-for`/`x-real-ip`) and target-based keys (such as lowercase email addresses).
+**Vulnerability:** Sign-in and sign-up limited repeated attempts for one email address only. An attacker could try one account from many IP addresses, or try many accounts from one IP address.
+**Learning:** Credential stuffing means trying leaked email and password pairs on many accounts. A limit for only one email address does not stop both attack paths.
+**Prevention:** Use two limits: one for the lowercase email address and one for a client IP address received from a trusted proxy.As per coding guidelines, “Use direct, plain-spoken language for the Filipino VA audience, define jargon, and avoid generic AI-slop phrases.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## 2026-07-20 - Dual Rate-Limiting Protects Against Multi-Vector Brute-Force Attacks | |
| **Vulnerability:** Authentication endpoints (`signInAction` and `signUpAction`) only rate-limited on a single target key (`signin:${email}`). This left the system vulnerable to distributed dictionary/credential stuffing attacks (same target from multiple IPs) and multi-target credential stuffing (multiple emails targeted from the same IP, bypassing target-based rate limits). | |
| **Learning:** Single-key rate limiting is insufficient for modern high-value web applications. Attackers can distribute requests across thousands of accounts from a single IP to bypass target-locked throttling. | |
| **Prevention:** Always implement dual rate-limiting combining both IP-based keys (retrieved safely via headers like `x-forwarded-for`/`x-real-ip`) and target-based keys (such as lowercase email addresses). | |
| ## 2026-07-20 - Dual Rate-Limiting Protects Against Multi-Vector Brute-Force Attacks | |
| **Vulnerability:** Sign-in and sign-up limited repeated attempts for one email address only. An attacker could try one account from many IP addresses, or try many accounts from one IP address. | |
| **Learning:** Credential stuffing means trying leaked email and password pairs on many accounts. A limit for only one email address does not stop both attack paths. | |
| **Prevention:** Use two limits: one for the lowercase email address and one for a client IP address received from a trusted proxy. |
🤖 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 @.jules/sentinel.md around lines 8 - 11, Update the “Dual Rate-Limiting”
entry in sentinel.md so it accurately states that signInAction uses
signin:${email} while signUpAction uses signup:${email}. Replace unexplained
“distributed dictionary attack” wording with plain language, and define
credential stuffing as using stolen username/password pairs against many
accounts; retain the explanation that IP-based and target-based limits are both
required.
Source: Coding guidelines
Comment on lines
+150
to
+204
| describe('dual rate-limiting', () => { | ||
| beforeEach(() => { | ||
| mockHeaders.get.mockImplementation((header: string) => { | ||
| if (header === 'x-forwarded-for') return '203.0.113.195'; | ||
| return null; | ||
| }); | ||
| }); | ||
|
|
||
| it('rate-limits by email on multiple sign-in attempts for same email', async () => { | ||
| (db.user.findUnique as unknown as ReturnType<typeof vi.fn>).mockResolvedValue(null); | ||
|
|
||
| // Trigger rate limit with 5 allowed requests | ||
| for (let i = 0; i < 5; i++) { | ||
| const res = await signInAction({ email: 'target@example.com', password: 'x' }); | ||
| expect(res.success).toBe(false); | ||
| if (!res.success) { | ||
| expect(res.error).toBe('Email or password is incorrect.'); | ||
| } | ||
| } | ||
|
|
||
| // 6th request triggers email rate limit | ||
| const blockedRes = await signInAction({ email: 'target@example.com', password: 'x' }); | ||
| expect(blockedRes.success).toBe(false); | ||
| if (!blockedRes.success) { | ||
| expect(blockedRes.error).toMatch(/Too many attempts\. Try again in \d+s\./); | ||
| } | ||
| }); | ||
|
|
||
| it('rate-limits by IP on multiple sign-in attempts from same IP but different emails', async () => { | ||
| (db.user.findUnique as unknown as ReturnType<typeof vi.fn>).mockResolvedValue(null); | ||
|
|
||
| // Use a distinct IP | ||
| mockHeaders.get.mockImplementation((header: string) => { | ||
| if (header === 'x-forwarded-for') return '198.51.100.42'; | ||
| return null; | ||
| }); | ||
|
|
||
| // 10 attempts on different emails from same IP are allowed to check credentials (but fail credential check) | ||
| for (let i = 0; i < 10; i++) { | ||
| const email = `user-${i}@example.com`; | ||
| const res = await signInAction({ email, password: 'x' }); | ||
| expect(res.success).toBe(false); | ||
| if (!res.success) { | ||
| expect(res.error).toBe('Email or password is incorrect.'); | ||
| } | ||
| } | ||
|
|
||
| // 11th request triggers IP rate limit | ||
| const blockedRes = await signInAction({ email: 'user-11@example.com', password: 'x' }); | ||
| expect(blockedRes.success).toBe(false); | ||
| if (!blockedRes.success) { | ||
| expect(blockedRes.error).toBe('Too many attempts from this IP. Try again in 60s.'); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep the auth action test beside auth.ts.
Move this suite to src/app/actions/auth.test.ts so the new throttling coverage is adjacent to its source.
As per coding guidelines, “Keep tests next to the code they test: foo.ts should have foo.test.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/app/actions/__tests__/auth-actions.test.ts` around lines 150 - 204, Move
the dual rate-limiting test suite from the nested __tests__ location into
src/app/actions/auth.test.ts, keeping its existing tests and setup unchanged so
the coverage remains adjacent to the auth action implementation.
Source: Coding guidelines
Comment on lines
+36
to
+47
| 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 rlEmail = rateLimit(`signup:email:${data.email.toLowerCase()}`, 5, 60_000); | ||
| if (!rlEmail.allowed) { | ||
| throw new Error(`Too many attempts. Try again in ${rlEmail.retryAfterSeconds}s.`); | ||
| } | ||
|
|
||
| const rlIp = rateLimit(`signup:ip:${ip}`, 10, 60_000); | ||
| if (!rlIp.allowed) { | ||
| throw new Error(`Too many attempts from this IP. Try again in ${rlIp.retryAfterSeconds}s.`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)auth\.ts$|rate(limit|limiter)?|src/app/actions' || true
echo "== auth outline =="
ast-grep outline src/app/actions/auth.ts || true
echo "== auth relevant lines =="
sed -n '1,190p' src/app/actions/auth.ts | cat -n
echo "== rate limiter references =="
rg -n "function rateLimit|const rateLimit|rateLimit|signup:email:|signup:ip:" -S . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: projectamazonph/amph-v2
Length of output: 11369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rate limit implementation and tests =="
sed -n '1,180p' src/lib/rate-limit.ts | cat -n
sed -n '1,140p' src/lib/__tests__/rate-limit.test.ts | cat -n
echo "== auth tests around signup/signin =="
rg -n "signUpAction|signInAction|signup|signin|Too many attempts from this IP|Too many attempts" src/app/actions/__tests__ src/app/actions/__tests__/auth-actions.test.ts --context 3 || true
echo "== deployment/reverse proxy/origin direct access references =="
rg -n "x-forwarded-for|Forwarded|Client-IP|x-real-ip|VERCEL|HOSTING|ORIGIN|direct|cloud|proxy|app-verification|origin" -S . --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo "== package/framework hints =="
for f in package.json next.config.{js,mjs,ts} app.html app.py; do
[ -f "$f" ] && { echo "-- $f"; sed -n '1,220p' "$f"; }
done
git ls-files | rg '(^|/)package\.json$|(^|/)next\.config\.(js|mjs|ts)$|(^|/)app\.html$|(^|/)app\.py$' || trueRepository: projectamazonph/amph-v2
Length of output: 50380
Do not trust the leftmost X-Forwarded-For value for rate limiting.
The auth actions use the first comma-separated value as signup:ip: / signin:ip: keys while an origin-reachable app can keep opening a fresh key each request, bypassing the per-IP 10-request rate limit. Use a verified client IP from the trusted hosting proxy boundary instead, or ensure origin requests cannot be reached directly.
Also applies to lines 136-149.
🤖 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/app/actions/auth.ts` around lines 36 - 47, Update the IP extraction used
by the signup and signin rate-limit flows around the x-forwarded-for handling to
use a verified client IP from the trusted hosting proxy boundary, rather than
the leftmost forwarded value. Ensure origin-direct requests cannot bypass the
per-IP keys, and apply the same change to both the signup and signin rate-limit
logic.
Comment on lines
+1
to
+2
| import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; | ||
| import { rateLimit } from '../rate-limit'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Colocate both new test suites with their sources.
src/lib/__tests__/rate-limit.test.ts#L1-L2: move tosrc/lib/rate-limit.test.ts.src/app/actions/__tests__/auth-actions.test.ts#L150-L204: move tosrc/app/actions/auth.test.ts.
As per coding guidelines, “Keep tests next to the code they test: foo.ts should have foo.test.ts.”
📍 Affects 2 files
src/lib/__tests__/rate-limit.test.ts#L1-L2(this comment)src/app/actions/__tests__/auth-actions.test.ts#L150-L204
🤖 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 - 2, Move the rate-limit
test suite from src/lib/__tests__/rate-limit.test.ts to
src/lib/rate-limit.test.ts, keeping its imports and tests unchanged. Move the
auth-actions test suite from src/app/actions/__tests__/auth-actions.test.ts to
src/app/actions/auth.test.ts, preserving the existing tests and updating
relative imports as needed.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛡️ Sentinel: Dual rate-limiting for auth endpoints. This PR implements a dual rate-limiting defense combining IP-based and target-based throttling. This prevents distributed dictionary attacks on individual accounts and multi-account credential stuffing from the same IP. Comprehensive tests are added to ensure correct behavior and avoid regressions.
PR created automatically by Jules for task 8885978203268662333 started by @projectamazonph
Summary by CodeRabbit
Bug Fixes
Tests