🛡️ Sentinel: dual rate-limiting#77
Conversation
Add client IP extraction and IP-based rate limiting as a defense-in-depth layer on top of email-based (target-based) rate limiting for both signUpAction and signInAction. 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 sign-in and sign-up actions now enforce email- and client-IP-based rate limits. Header and rate-limit mocks support the updated tests, which cover rejection messages, retry intervals, rate-limit keys, sliding windows, and bucket cleanup. ChangesAuthentication rate limiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant signInAction
participant headers
participant rateLimit
Client->>signInAction: Submit sign-in request
signInAction->>headers: Read forwarded IP headers
headers-->>signInAction: Return client IP
signInAction->>rateLimit: Check email and IP limits
rateLimit-->>signInAction: Return allowed or retry interval
signInAction-->>Client: Return authentication result or rate-limit error
Possibly related PRs
🚥 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 |
Add client IP extraction and IP-based rate limiting as a defense-in-depth layer on top of email-based (target-based) rate limiting for both signUpAction and signInAction. Also add unit tests to ensure 100% test coverage of the rate-limit utility and meet repository quality gates. Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/actions/auth.ts (1)
36-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate dual-rate-limit logic between
signUpActionandsignInAction.The IP derivation plus target/IP rate-limit-and-throw pattern is copy-pasted verbatim across both actions. Extracting a shared helper (e.g.,
getClientIp(heads)and/orenforceRateLimits(prefix, email, ip)) would centralize the fix location for the parsing/trust issue above and reduce future drift between the two call sites.Also applies to: 139-153
🤖 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 - 50, Extract the duplicated client-IP derivation and target/IP rate-limit enforcement from signUpAction and signInAction into shared helpers such as getClientIp and enforceRateLimits. Update both actions to call the helpers while preserving their existing prefixes, limits, retry messages, and headers input.
🤖 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/auth.ts`:
- Around line 36-37: Update the IP extraction in both sign-up and sign-in paths
around the headers() calls to normalize x-forwarded-for by parsing its first
comma-separated IP before using it as the rate-limit key. Preserve the x-real-ip
fallback and 127.0.0.1 default when x-forwarded-for is absent.
---
Nitpick comments:
In `@src/app/actions/auth.ts`:
- Around line 36-50: Extract the duplicated client-IP derivation and target/IP
rate-limit enforcement from signUpAction and signInAction into shared helpers
such as getClientIp and enforceRateLimits. Update both actions to call the
helpers while preserving their existing prefixes, limits, retry messages, and
headers input.
🪄 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: 59efa99b-629b-4f54-b09d-f99de5aa01cd
📒 Files selected for processing (3)
src/__tests__/setup.tssrc/app/actions/__tests__/auth-actions.test.tssrc/app/actions/auth.ts
| const heads = await headers(); | ||
| const ip = heads.get('x-forwarded-for') ?? heads.get('x-real-ip') ?? '127.0.0.1'; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auth.ts ==\n'
sed -n '1,220p' src/app/actions/auth.ts
printf '\n== search for x-forwarded-for / x-real-ip / trusted proxy ==\n'
rg -n "x-forwarded-for|x-real-ip|trusted proxy|forwarded-for|rate-limit|rate limit|rate limiting|headers\(\)" src . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'
printf '\n== deployment-related files ==\n'
git ls-files | rg '(^|/)(vercel|netlify|docker|nginx|traefik|caddy|proxy|middleware|next\.config|README|docs|\.env\.example|\.github/workflows).*'Repository: projectamazonph/amph-v2
Length of output: 15157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact auth action implementation with line numbers.
cat -n src/app/actions/auth.ts | sed -n '1,220p'
# Look for deployment or middleware code that normalizes client IPs.
rg -n "x-forwarded-for|x-real-ip|headers\(\)|NextRequest|middleware|proxy|rateLimit|rate limit|rate-limit" src .github docs README.md next.config.* -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'Repository: projectamazonph/amph-v2
Length of output: 17004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== deployment docs mentioning Vercel / proxy / headers ==\n'
rg -n "Vercel|x-forwarded-for|x-real-ip|proxy|CDN|edge|headers" README.md docs/LAUNCH-DEPLOY.md docs/runbooks/production-deploy.md docs/SECURITY.md docs/ARCHITECTURE.md docs/security/code-audit-2026-07-15.md next.config.ts src/middleware.ts src/lib/admin-audit.ts
printf '\n== next.config.ts ==\n'
cat -n next.config.ts | sed -n '1,220p'
printf '\n== production deploy runbook ==\n'
sed -n '1,240p' docs/runbooks/production-deploy.md
printf '\n== launch deploy doc ==\n'
sed -n '1,220p' docs/LAUNCH-DEPLOY.mdRepository: projectamazonph/amph-v2
Length of output: 24340
Normalize x-forwarded-for before using it as the IP rate-limit key. The header can contain multiple hops, so the same client can land in different buckets depending on proxy chain. Parse the first IP in both sign-up and sign-in paths.
🤖 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 - 37, Update the IP extraction in
both sign-up and sign-in paths around the headers() calls to normalize
x-forwarded-for by parsing its first comma-separated IP before using it as the
rate-limit key. Preserve the x-real-ip fallback and 127.0.0.1 default when
x-forwarded-for is absent.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-2: Move the rate-limit unit test from the __tests__ directory to
sit beside rate-limit.ts at the src/lib level, preserving its Vitest imports and
test contents unchanged.
- Around line 49-63: Update the rate-limit test around rateLimit so it directly
verifies opportunistic cleanup removed the expired bucket, rather than inferring
cleanup from a subsequent allowed request. Expose cleanup through a separately
testable helper or a test-only state inspection hook in the rate-limit
implementation, and assert that expired-key is absent after inserting more than
10,000 entries.
🪄 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: 990ac93f-36f0-4987-b456-dcda38a7b1c0
📒 Files selected for processing (1)
src/lib/__tests__/rate-limit.test.ts
| 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
Keep this test next to rate-limit.ts.
Move src/lib/__tests__/rate-limit.test.ts to src/lib/rate-limit.test.ts. This file is a unit test for src/lib/rate-limit.ts, so it should live beside the module it tests.
As per coding guidelines, tests must use Vitest and live next to the code they test.
🤖 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
unit test from the __tests__ directory to sit beside rate-limit.ts at the
src/lib level, preserving its Vitest imports and test contents unchanged.
Source: Coding guidelines
| it('triggers opportunistic cleanup when buckets map exceeds 10,000 entries', () => { | ||
| // First, let's add some expired hits to the map | ||
| rateLimit('expired-key', 1, 1000); | ||
| vi.advanceTimersByTime(2000); // Expiry passed | ||
|
|
||
| // Now, insert 10,001 entries to force buckets.size > 10,000 | ||
| for (let i = 0; i < 10005; i++) { | ||
| rateLimit(`key-${i}`, 1, 1000); | ||
| } | ||
|
|
||
| // Since 'expired-key' is past its expiry, it should have been cleaned up by the opportunistic cleanup. | ||
| // Let's verify that hitting it again behaves as a fresh hit | ||
| const res = rateLimit('expired-key', 1, 1000); | ||
| expect(res.allowed).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the cleanup assertion observable.
In src/lib/rate-limit.ts:25-49, expired hits are filtered on every request. Therefore, lines 61-62 pass even if the opportunistic cleanup loop is removed. Extract cleanup into a separately testable helper or add a test-only state inspection hook so this test proves that expired buckets are deleted.
🤖 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 49 - 63, Update the
rate-limit test around rateLimit so it directly verifies opportunistic cleanup
removed the expired bucket, rather than inferring cleanup from a subsequent
allowed request. Expose cleanup through a separately testable helper or a
test-only state inspection hook in the rate-limit implementation, and assert
that expired-key is absent after inserting more than 10,000 entries.
Implemented dual rate-limiting (combining target-based/email rate limiting and IP-based rate limiting) in auth actions. Updated Next.js mocks for testing next/headers
headers()function correctly, and wrote comprehensive unit tests ensuring robustness.PR created automatically by Jules for task 13284815474013834007 started by @projectamazonph
Summary by CodeRabbit
New Features
Tests