🛡️ Sentinel: add dual rate limiting to authentication server actions - #101
🛡️ Sentinel: add dual rate limiting to authentication server actions#101projectamazonph wants to merge 1 commit 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. |
📝 WalkthroughWalkthroughChangesAuthentication rate limiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AuthActions
participant rateLimitDual
participant headers
participant RateLimitStore
AuthActions->>rateLimitDual: submit signup or signin action and email
rateLimitDual->>RateLimitStore: enforce normalized target limit
rateLimitDual->>headers: read client IP headers
headers-->>rateLimitDual: return IP or unavailable
rateLimitDual->>RateLimitStore: enforce doubled IP limit
rateLimitDual-->>AuthActions: return allowance or denial
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 |
There was a problem hiding this comment.
Pull request overview
This PR strengthens authentication throttling by introducing dual-layer rate limiting (per target email and per client IP) and wiring it into the sign-up and sign-in server actions to better mitigate credential stuffing and brute-force attempts.
Changes:
- Added
rateLimitDual()insrc/lib/rate-limit.ts, combining target-key and IP-based sliding-window limits. - Updated
signUpActionandsignInActionto userateLimitDual()instead of target-only limiting. - Added a unit test suite for the rate limiter and updated Vitest setup mocks; documented the security learning in
.jules/sentinel.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/rate-limit.ts | Adds dual-layer rate limiting using request headers for IP extraction. |
| src/lib/tests/rate-limit.test.ts | Introduces unit tests covering the sliding-window limiter and dual limiter behavior. |
| src/app/actions/auth.ts | Switches auth server actions to use the new dual limiter. |
| src/tests/setup.ts | Extends the next/headers mock to include headers() for tests. |
| .jules/sentinel.md | Documents the security finding and the applied mitigation approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 ipPrefixed = `${actionType}:ip:${ip}`; | ||
| const ipResult = rateLimit(ipPrefixed, limit * 2, windowMs); | ||
| if (!ipResult.allowed) { | ||
| return ipResult; | ||
| } |
| it('falls back to unknown if no IP headers are present', async () => { | ||
| (headers as unknown as ReturnType<typeof vi.fn>).mockResolvedValue({ | ||
| get: () => null, | ||
| }); | ||
|
|
||
| await rateLimitDual('unknown-ip-test', 'email1@example.com', { limit: 1 }); | ||
| await rateLimitDual('unknown-ip-test', 'email2@example.com', { limit: 1 }); | ||
|
|
||
| const res = await rateLimitDual('unknown-ip-test', 'email3@example.com', { limit: 1 }); | ||
| expect(res.allowed).toBe(false); | ||
| }); |
| } from '@/lib/auth'; | ||
| import { logger } from '@/lib/logger'; | ||
| import { rateLimit } from '@/lib/rate-limit'; | ||
| import { rateLimitDual } from '@/lib/rate-limit'; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/rate-limit.ts (1)
82-84: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winLog the header-access failure instead of silently swallowing it.
The
catchblock discards the error without any logging. Ifheaders()fails repeatedly in production, the IP-based layer ofrateLimitDualis silently disabled and only the target-based layer remains, with no visibility into the degradation.As per coding guidelines, use the structured logger from
src/lib/logger.tsinstead of leaving the error unlogged.🛠️ Proposed fix
} catch { - // Graceful degradation if headers() throws or is unavailable + // Graceful degradation if headers() throws or is unavailable. + logger.warn({ actionType }, 'rateLimitDual: headers() unavailable, IP-based limit skipped'); }🤖 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 82 - 84, Update the catch block in rateLimitDual to log the headers() access failure using the structured logger from logger.ts, including the caught error and clear context, while preserving the existing graceful-degradation 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.
Nitpick comments:
In `@src/lib/rate-limit.ts`:
- Around line 82-84: Update the catch block in rateLimitDual to log the
headers() access failure using the structured logger from logger.ts, including
the caught error and clear context, while preserving the existing
graceful-degradation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5fcb1d6-d48d-4f7f-bf94-e61ed659bf20
📒 Files selected for processing (5)
.jules/sentinel.mdsrc/__tests__/setup.tssrc/app/actions/auth.tssrc/lib/__tests__/rate-limit.test.tssrc/lib/rate-limit.ts
🛡️ Sentinel: [security improvement]
🚨 Severity
MEDIUM
💡 Vulnerability
Prior to this change, the sign-up and sign-in Server Actions only rate-limited incoming requests by the targeted lowercase email. This left the application vulnerable to distributed credential-stuffing or brute-force attacks where many different emails are targeted from a single client IP (or small set of client IPs) without hitting the single-email rate limits.
🎯 Impact
Attackers could execute high-volume password-spraying or credentials-stuffing attacks, potentially compromising user accounts while remaining undetected by the target-based rate limiter.
🔧 Fix
rateLimitDualinsidesrc/lib/rate-limit.tswhich usesnext/headersto safely extract client IP (supportingx-forwarded-forandx-real-ipwith safe indexing and error-handling fallbacks) and performs sliding-window rate limiting on both the target email and the client IP address.signUpActionandsignInActioninsrc/app/actions/auth.tsto userateLimitDual.src/lib/__tests__/rate-limit.test.tsto test all scenarios including header fallbacks, multi-IP parsing, Map cleanup, and timing lockout calculations, achieving 100% line coverage for the module..jules/sentinel.md.✅ Verification
pnpm testexecutes and passes all 223 unit/integration tests successfully.pnpm test:coveragereports 100% line, statement, and function coverage onsrc/lib/rate-limit.ts.pnpm typecheckandpnpm lintyields zero errors.PR created automatically by Jules for task 16242830136162380155 started by @projectamazonph
Summary by CodeRabbit