🛡️ Sentinel: implement dual rate limiting (IP + Email) for auth actions#73
🛡️ Sentinel: implement dual rate limiting (IP + Email) for auth actions#73projectamazonph wants to merge 1 commit into
Conversation
- Added client IP-based rate limiting to both `signInAction` and `signUpAction` alongside existing email-based rate limiting. - Prevents distributed brute force and credential stuffing attacks. - Mocked `headers()` from `next/headers` in setup.ts and test suite. - Added comprehensive unit/integration tests to verify both limiters. 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. |
📝 WalkthroughWalkthroughAuthentication actions now enforce IP-based rate limits before existing email-based limits. Test mocks support request headers, and sign-in tests cover both email and IP thresholds. ChangesAuthentication rate limiting
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant signInAction
participant headers
participant rateLimit
signInAction->>headers: Read forwarded IP headers
headers-->>signInAction: Return caller IP
signInAction->>rateLimit: Check IP limit
rateLimit-->>signInAction: Allow or block attempt
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. 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/app/actions/auth.ts (1)
141-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine “credential stuffing” in plain language.
Use wording such as “repeated login attempts using stolen email-password pairs.” As per coding guidelines, define jargon and use direct, plain-spoken language for the Filipino VA audience.
🤖 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` at line 141, Update the comment above the IP-based rate-limiting logic to define “credential stuffing” in plain language, describing it as repeated login attempts using stolen email-password pairs, while preserving the comment’s security context.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/app/actions/__tests__/auth-actions.test.ts`:
- Around line 172-192: Add a regression test for signUpAction mirroring the
existing signInAction IP-rate-limit test: use one fresh IP, submit 10 sign-ups
with distinct emails, then verify the 11th returns the “Too many attempts from
this IP.” error while preserving the existing failed-attempt assertions.
In `@src/app/actions/auth.ts`:
- Around line 36-42: Update both signup and signin rate-limiting paths in
src/app/actions/auth.ts at lines 36-42 and 138-144 to use the same canonical,
trusted client-IP value normalized at ingress instead of directly trusting
x-forwarded-for or x-real-ip. Ensure the limiter uses shared/distributed storage
rather than the per-instance in-memory implementation in src/lib/rate-limit.ts,
preserving consistent keys across both flows.
---
Nitpick comments:
In `@src/app/actions/auth.ts`:
- Line 141: Update the comment above the IP-based rate-limiting logic to define
“credential stuffing” in plain language, describing it as repeated login
attempts using stolen email-password pairs, while preserving the comment’s
security context.
🪄 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: e9a85162-3d96-4632-a7e8-bddcdbcacb25
📒 Files selected for processing (3)
src/__tests__/setup.tssrc/app/actions/__tests__/auth-actions.test.tssrc/app/actions/auth.ts
| it('signInAction rate limits by IP after 10 attempts with different emails', async () => { | ||
| currentTestIp = '192.168.1.50'; // Use a fresh IP address to avoid prior test interference | ||
| mockVerifyPassword.mockReturnValue(false); | ||
| (db.user.findUnique as unknown as ReturnType<typeof vi.fn>).mockResolvedValue({ | ||
| id: 'u1', email: 'irrelevant@example.com', passwordHash: 'x', status: 'ACTIVE', role: 'STUDENT', name: 'A', emailVerified: new Date(), lastActiveAt: null, createdAt: new Date(), updatedAt: new Date(), deletedAt: null, | ||
| }); | ||
|
|
||
| for (let i = 0; i < 10; i++) { | ||
| const result = await signInAction({ email: `ip-test-${i}@example.com`, password: 'wrong' }); | ||
| expect(result.success).toBe(false); | ||
| if (!result.success) { | ||
| expect(result.error).toBe('Email or password is incorrect.'); | ||
| } | ||
| } | ||
|
|
||
| const result = await signInAction({ email: 'ip-test-blocked@example.com', password: 'wrong' }); | ||
| expect(result.success).toBe(false); | ||
| if (!result.success) { | ||
| expect(result.error).toContain('Too many attempts from this IP.'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a sign-up IP-limit regression test.
These tests cover only signInAction, but this PR adds the same IP gate to signUpAction. Add a test with 10 distinct emails from one IP, then assert the 11th sign-up returns the IP-limit error. As per coding guidelines, new features must include tests.
🤖 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 172 - 192, Add a
regression test for signUpAction mirroring the existing signInAction
IP-rate-limit test: use one fresh IP, submit 10 sign-ups with distinct emails,
then verify the 11th returns the “Too many attempts from this IP.” error while
preserving the existing failed-attempt assertions.
Source: Coding guidelines
| const heads = await headers(); | ||
| const ip = heads.get('x-forwarded-for') ?? heads.get('x-real-ip') ?? 'unknown'; | ||
|
|
||
| // IP-based rate limiting to protect against general registration floods / spamming | ||
| const ipRl = rateLimit(`signup:ip:${ip}`, 10, 60_000); | ||
| if (!ipRl.allowed) { | ||
| throw new Error(`Too many attempts from this IP. Try again in ${ipRl.retryAfterSeconds}s.`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the auth actions and any IP / headers handling helpers.
git ls-files 'src/app/actions/auth.ts' 'src/lib/**' 'src/**' | sed -n '1,200p'
echo
echo '--- auth.ts outline ---'
ast-grep outline src/app/actions/auth.ts --view expanded || true
echo
echo '--- search for rateLimit and headers usage ---'
rg -n "rateLimit\\(|headers\\(|x-forwarded-for|x-real-ip|client ip|trusted.*ip|forwarded" src -SRepository: projectamazonph/amph-v2
Length of output: 9028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant sections of auth.ts with line numbers.
wc -l src/app/actions/auth.ts
cat -n src/app/actions/auth.ts | sed -n '1,240p'Repository: projectamazonph/amph-v2
Length of output: 9777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'auth.ts line count: '
wc -l < src/app/actions/auth.ts
echo
echo '--- auth.ts excerpt ---'
sed -n '1,220p' src/app/actions/auth.ts | cat -n
echo
echo '--- trusted IP / proxy related matches ---'
rg -n "x-forwarded-for|x-real-ip|request-ip|clientIp|trusted.*proxy|proxy.*trust|forwarded.*ip|real-ip" src -SRepository: projectamazonph/amph-v2
Length of output: 9749
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/lib/rate-limit.ts ---'
cat -n src/lib/rate-limit.ts
echo
echo '--- src/middleware.ts ---'
cat -n src/middleware.ts
echo
echo '--- src/lib/middleware-context.ts ---'
cat -n src/lib/middleware-context.ts
echo
echo '--- any x-forwarded-for normalization helpers ---'
rg -n "x-forwarded-for|x-real-ip|middleware-context|clientIp|ipAddress|forwarded" src/lib src/middleware.ts src/app -SRepository: projectamazonph/amph-v2
Length of output: 9467
Use a trusted client-IP source for these rate-limit keys, or move the limiter to shared storage. x-forwarded-for and x-real-ip can be spoofed or missing here, so the bucket can be rotated or collapsed into unknown. src/lib/rate-limit.ts is also in-memory per warm instance, so this protection will not hold across multiple servers. Normalize the IP at ingress and reuse one canonical value in both signup and signin, or replace this with a distributed rate limiter.
📍 Affects 1 file
src/app/actions/auth.ts#L36-L42(this comment)src/app/actions/auth.ts#L138-L144
🤖 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 - 42, Update both signup and signin
rate-limiting paths in src/app/actions/auth.ts at lines 36-42 and 138-144 to use
the same canonical, trusted client-IP value normalized at ingress instead of
directly trusting x-forwarded-for or x-real-ip. Ensure the limiter uses
shared/distributed storage rather than the per-instance in-memory implementation
in src/lib/rate-limit.ts, preserving consistent keys across both flows.
🛡️ Sentinel Security Improvement: Dual Rate Limiting (IP + Email)
🚨 Severity: MEDIUM (Security Enhancement)
💡 Vulnerability / Gaps:
Previously, the authentication server actions (
signInActionandsignUpAction) only rate limited by the client's email address. An attacker performing a distributed credential stuffing or brute force attack using multiple different emails would completely bypass the email-based rate limiting, potentially allowing them to brute force passwords across multiple accounts or cause a DoS by exhausting system/DB connections with heavy cryptographic computations.🎯 Impact:
With the old single rate limiter, credential stuffing and dictionary attacks were possible if performed across many different emails from the same IP.
🔧 Fix:
signInActionandsignUpAction.X-Forwarded-ForandX-Real-IPrequest headers.headers()to prevent test runtime crashes when Server Actions request headers.✅ Verification:
pnpm test.pnpm typecheck.pnpm lint.PR created automatically by Jules for task 6841729069551316806 started by @projectamazonph
Summary by CodeRabbit
Security Improvements
Tests