🛡️ Sentinel: dual rate-limiting security enhancement - #104
🛡️ Sentinel: dual rate-limiting security enhancement#104projectamazonph wants to merge 2 commits into
Conversation
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: 54 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. 📝 WalkthroughWalkthroughAuthentication signup and signin now use IP-first dual rate limiting. The limiter reads client IP headers, falls back to ChangesAuthentication rate limiting
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant AuthAction
participant rateLimitDual
participant Headers
participant RateLimitBuckets
AuthAction->>rateLimitDual: Submit target and limit options
rateLimitDual->>Headers: Read client IP headers
Headers-->>rateLimitDual: Return IP or unknown
rateLimitDual->>RateLimitBuckets: Check IP bucket
RateLimitBuckets-->>rateLimitDual: Allow or deny
rateLimitDual->>RateLimitBuckets: Check target bucket if allowed
RateLimitBuckets-->>rateLimitDual: Allow or deny
rateLimitDual-->>AuthAction: Return RateLimitResult
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 |
…and resolve CI pnpm version issue 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/lib/__tests__/rate-limit.test.ts (1)
60-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
(headers as any)casts withvi.mocked(headers).This file casts
headerstoanybefore calling.mockResolvedValue/.mockRejectedValueat lines 60, 78, 89, 101, 112, 128, and 141. The coding guidelines require strict TypeScript and forbidany. Sinceheadersis alreadyvi.fn(), usevi.mocked(headers)to keep full typing without the cast.As per coding guidelines, "Use strict TypeScript. Do not use any; define explicit types or use unknown with narrowing."
🔧 Proposed fix
- (headers as any).mockResolvedValue({ + vi.mocked(headers).mockResolvedValue({ get: (name: string) => { if (name === 'x-forwarded-for') return '1.2.3.4'; return null; }, });Apply the same replacement at each
(headers as any)occurrence in this file.🤖 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 60 - 146, Replace every `(headers as any)` cast in the rate-limit tests with `vi.mocked(headers)` before calling mockResolvedValue or mockRejectedValue. Update all occurrences in the existing test cases while preserving their mocked header behavior and strict typing.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 32-34: Update the auth action test setup to import _clearBuckets
from `@/lib/rate-limit` and invoke it in beforeEach alongside the existing mock
and mockSignToken resets, ensuring signin:ip:unknown state is cleared between
tests; optionally add a regression test covering the duplicate-action limit
state.
---
Nitpick comments:
In `@src/lib/__tests__/rate-limit.test.ts`:
- Around line 60-146: Replace every `(headers as any)` cast in the rate-limit
tests with `vi.mocked(headers)` before calling mockResolvedValue or
mockRejectedValue. Update all occurrences in the existing test cases while
preserving their mocked header behavior and strict typing.
🪄 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: 59e01813-1882-45de-bd6f-3736983d0d33
📒 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
| headers: () => Promise.resolve({ | ||
| get: () => null, | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether auth-actions.test.ts resets rate-limit bucket state between tests.
fd 'auth-actions.test.ts' src/app/actions/__tests__ --exec cat -n {}Repository: projectamazonph/amph-v2
Length of output: 7734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate rate-limit files =="
fd -a 'rate-limit\.ts$' . | sed 's#^\./##'
echo "== inspect src/lib/rate-limit.ts =="
if [ -f src/lib/rate-limit.ts ]; then
cat -n src/lib/rate-limit.ts
fi
echo "== references to _clearBuckets and rate limit helpers in tests =="
rg -n "_clearBuckets|buckets|rateLimit|rateLimitDual" src/app/actions/__tests__ src/app/actions || true
echo "== auth actions action files =="
fd -a 'auth\.(ts|tsx)$' src/app/actions | sed 's#^\./##'
for f in $(fd 'auth\.(ts|tsx)$' src/app/actions); do
echo "---- $f ----"
ast-grep outline "$f" --lang typescript --view expanded || true
rg -n "rateLimit|rateLimitDual|_clearBuckets|getIp|headers\\(" "$f" || true
doneRepository: projectamazonph/amph-v2
Length of output: 5345
Reset rate-limit buckets between auth action tests.
beforeEach only resets mocks and mockSignToken; it does not clear @/lib/rate-limit’s module-level buckets. Each signInAction call here uses the mocked unknown IP, so hits accumulate in signin:ip:unknown across test cases unless _clearBuckets() runs before each test. Add const { _clearBuckets } = await import('@/lib/rate-limit'); in the test file, clear the buckets in beforeEach, and optionally add a regression test for the duplicate-action limit 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/app/actions/__tests__/auth-actions.test.ts` around lines 32 - 34, Update
the auth action test setup to import _clearBuckets from `@/lib/rate-limit` and
invoke it in beforeEach alongside the existing mock and mockSignToken resets,
ensuring signin:ip:unknown state is cleared between tests; optionally add a
regression test covering the duplicate-action limit state.
There was a problem hiding this comment.
🟡 Not ready to approve
The ip = 'unknown' fallback can create a shared global IP bucket and unintentionally lock out legitimate users in environments where IP headers are unavailable, and the new tests introduce explicit as any casts that conflict with strict TypeScript conventions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds dual (IP + target identifier) rate limiting for auth server actions to reduce brute-force and credential-stuffing risk, with ordering intended to prevent target-bucket pollution from already-blocked IPs.
Changes:
- Introduce
rateLimitDual()that rate-limits by client IP first, then by target identifier. - Apply
rateLimitDual()tosignUpActionandsignInAction. - Add unit tests for
rateLimit/rateLimitDual, and extendnext/headersmocks for test environments.
File summaries
| File | Description |
|---|---|
| src/lib/rate-limit.ts | Adds dual rate-limiting helper that incorporates client IP extraction from request headers. |
| src/lib/tests/rate-limit.test.ts | Adds unit tests covering sliding window behavior and dual-limit ordering. |
| src/app/actions/auth.ts | Switches signup/signin actions to use dual rate limiting. |
| src/app/actions/tests/auth-actions.test.ts | Extends next/headers mock to support server-action code paths using headers. |
| src/tests/setup.ts | Adds a default headers() mock in the global Vitest setup. |
| .jules/sentinel.md | Documents the dual-rate-limit ordering vulnerability and the prevention guidance. |
Review details
Suppressed comments (6)
src/lib/tests/rate-limit.test.ts:78
- Avoid
as anycasts in tests; use the repo-standardas unknown as ReturnType<typeof vi.fn>cast instead so we keep strict typing consistent.
(headers as any).mockResolvedValue({
src/lib/tests/rate-limit.test.ts:89
- Avoid
as anycasts in tests; use the repo-standardas unknown as ReturnType<typeof vi.fn>cast instead so we keep strict typing consistent.
(headers as any).mockResolvedValue({
src/lib/tests/rate-limit.test.ts:101
- Avoid
as anycasts in tests; use the repo-standardas unknown as ReturnType<typeof vi.fn>cast instead so we keep strict typing consistent.
(headers as any).mockResolvedValue({
src/lib/tests/rate-limit.test.ts:112
- Avoid
as anycasts in tests; use the repo-standardas unknown as ReturnType<typeof vi.fn>cast instead so we keep strict typing consistent.
(headers as any).mockResolvedValue({
src/lib/tests/rate-limit.test.ts:128
- Avoid
as anycasts in tests; use the repo-standardas unknown as ReturnType<typeof vi.fn>cast instead so we keep strict typing consistent.
(headers as any).mockResolvedValue({
src/lib/tests/rate-limit.test.ts:141
- Avoid
as anycasts in tests; use the repo-standardas unknown as ReturnType<typeof vi.fn>cast instead so we keep strict typing consistent.
(headers as any).mockRejectedValue(new Error('no headers in this environment'));
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| 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({ |
| // 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; | ||
| } |
This security enhancement implements dual rate-limiting (checking both client IP and target-based identifiers like emails) on signup and signin server actions. It resolves potential credential-stuffing and brute-force vulnerabilities, while ensuring IP rate limiting is executed first to prevent target-based bucket pollution and account lockout Denial of Service (DoS).
PR created automatically by Jules for task 360127238735165060 started by @projectamazonph
Summary by CodeRabbit
Security
Tests