🛡️ Sentinel: [security improvement] Dual Rate-Limiting - #120
🛡️ Sentinel: [security improvement] Dual Rate-Limiting#120projectamazonph 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: 52 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 dual IP- and target-based rate limiting. The implementation detects client IPs from request headers, checks IP limits before target limits, and adds tests for enforcement, state isolation, and forwarded-IP parsing. ChangesDual authentication rate limiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AuthAction
participant RequestHeaders
participant RateLimiter
AuthAction->>RequestHeaders: Read client IP headers
RequestHeaders-->>AuthAction: Return first forwarded IP or fallback
AuthAction->>RateLimiter: Check IP limit
RateLimiter-->>AuthAction: Deny or continue
AuthAction->>RateLimiter: Check lowercased target limit
RateLimiter-->>AuthAction: Return rate-limit result
🚥 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 |
![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 (3)
src/app/actions/__tests__/auth-actions.test.ts (1)
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the rate-limit buckets between tests in this file.
The mock returns
nullfor every header, so all calls resolve to the'127.0.0.1'fallback bucket. The buckets are module-level state that persists across tests in the file. The current test count stays underipLimit = 10per action, so the suite passes. When someone adds moresignInActionorsignUpActioncases, the limiter blocks them and the failures look unrelated to the new test.Call
resetRateLimits()inbeforeEachto remove this hidden coupling.♻️ Proposed test isolation
+import { resetRateLimits } from '`@/lib/rate-limit`'; + +beforeEach(() => { + resetRateLimits(); +});🤖 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 test setup in auth-actions.test.ts to call resetRateLimits() from a beforeEach hook, clearing the module-level rate-limit buckets before every test while preserving the existing header mock and test behavior.src/lib/__tests__/rate-limit.test.ts (2)
140-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the
x-real-ipand no-header paths.The suite covers
x-forwarded-foronly. Two branches of the IP resolution insrc/lib/rate-limit.tsLine 80 stay untested: thex-real-ipfallback and the'127.0.0.1'default when no header is present. Both affect which bucket a request lands in, so a regression there is silent.💚 Proposed additional cases
+ it('falls back to x-real-ip when x-forwarded-for is absent', async () => { + mockHeadersMap.set('x-real-ip', '9.9.9.9'); + expect((await rateLimitDual('login', 'user@example.com', { ipLimit: 1 })).allowed).toBe(true); + expect((await rateLimitDual('login', 'user@example.com', { ipLimit: 1 })).allowed).toBe(false); + }); + + it('uses a single fallback bucket when no IP header is present', async () => { + expect((await rateLimitDual('login', 'a@example.com', { ipLimit: 1 })).allowed).toBe(true); + expect((await rateLimitDual('login', 'b@example.com', { ipLimit: 1 })).allowed).toBe(false); + });🤖 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 140 - 152, Add tests in the rate-limit suite for IP resolution when only x-real-ip is set and when neither IP header is present, exercising rateLimitDual and verifying repeated requests use the same bucket and enforce the configured limit. Clear the mock headers between cases so each test validates the x-real-ip fallback and the 127.0.0.1 default independently.
58-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the self-dialogue from the comments.
Lines 65-69 contain working notes ("Wait! Let's double check this logic", "This is correct!"). Line 65 also contradicts lines 58-62: line 61 says the request is allowed, line 65 says it is blocked. The assertion on Line 74 expects
false, which matches Line 65. Keep one accurate statement.♻️ Proposed comment cleanup
- // Now, a legitimate user from a different IP (5.6.7.8) tries to log in. - // Since the blocked 3rd request from IP 1.2.3.4 did not pollute the target bucket, - // the target bucket for user@example.com should only have 2 active hits (from res1 and res2). - // Let's verify that IP 5.6.7.8 can still make their first allowed request for user@example.com, - // but a subsequent one is blocked by targetLimit (since total hits for target is now 3). + // A different IP (5.6.7.8) now tries the same target. Its IP bucket is + // empty, so only the target bucket can block it. mockHeadersMap.set('x-forwarded-for', '5.6.7.8'); - // First request from 5.6.7.8 is blocked because targetLimit is 2, and we have 2 hits already. - // Wait! Let's double check this logic: - // res1 and res2 were allowed, so target bucket has 2 hits. - // So the target-level limit (2) is reached. - // This is correct! + // The target bucket holds 2 hits from res1 and res2, which reaches + // targetLimit. The blocked res3 added nothing.🤖 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 58 - 69, Clean up the comments in the rate-limit test around the mockHeadersMap update by removing the self-dialogue and contradictory explanation. Keep one accurate statement consistent with the assertion that the first request from 5.6.7.8 is blocked because the target bucket has already reached targetLimit.
🤖 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 126-137: Update the later rateLimitDual calls for r7 and r8 in the
test so their ipLimit is higher than the accumulated requests from 5.6.7.8,
while keeping targetLimit at 5. This isolates the target-limit behavior and
ensures r8 is blocked specifically by targetLimit.
In `@src/lib/rate-limit.ts`:
- Around line 78-80: Harden IP resolution in the rate-limit flow around
headers() by using only a trusted platform IP header or a configured
trusted-proxy offset from x-forwarded-for; do not trust the first
client-controlled entry by default. Normalize trimmed empty values to null,
allow fallback to x-real-ip, and remove the shared 127.0.0.1 fallback by failing
closed or applying the established no-IP handling/logging path when no address
is available.
---
Nitpick comments:
In `@src/app/actions/__tests__/auth-actions.test.ts`:
- Around line 32-34: Update the test setup in auth-actions.test.ts to call
resetRateLimits() from a beforeEach hook, clearing the module-level rate-limit
buckets before every test while preserving the existing header mock and test
behavior.
In `@src/lib/__tests__/rate-limit.test.ts`:
- Around line 140-152: Add tests in the rate-limit suite for IP resolution when
only x-real-ip is set and when neither IP header is present, exercising
rateLimitDual and verifying repeated requests use the same bucket and enforce
the configured limit. Clear the mock headers between cases so each test
validates the x-real-ip fallback and the 127.0.0.1 default independently.
- Around line 58-69: Clean up the comments in the rate-limit test around the
mockHeadersMap update by removing the self-dialogue and contradictory
explanation. Keep one accurate statement consistent with the assertion that the
first request from 5.6.7.8 is blocked because the target bucket has already
reached targetLimit.
🪄 Autofix
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: dd0d621e-7a75-4251-9eb3-71a9cf30be08
📒 Files selected for processing (5)
.jules/sentinel.mdsrc/app/actions/__tests__/auth-actions.test.tssrc/app/actions/auth.tssrc/lib/__tests__/rate-limit.test.tssrc/lib/rate-limit.ts
| const r7 = await rateLimitDual('login', 'user@example.com', { | ||
| ipLimit: 5, | ||
| targetLimit: 5, | ||
| }); | ||
| expect(r7.allowed).toBe(true); // target hit 5 | ||
|
|
||
| // 6th target hit overall: should be blocked by targetLimit | ||
| const r8 = await rateLimitDual('login', 'user@example.com', { | ||
| ipLimit: 5, | ||
| targetLimit: 5, | ||
| }); | ||
| expect(r8.allowed).toBe(false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
r8 is blocked by the IP limit, not the target limit.
The test sets ipLimit: 5 for requests from 5.6.7.8. Requests r4 through r7 consume 4 IP hits, and r8 consumes the fifth check against a bucket that already holds 4... The count reaches the IP threshold at the same request that reaches the target threshold. The assertion passes, but it does not prove that targetLimit caused the block. The comment on Line 132 claims a target-limit block.
Raise ipLimit for the later requests so only the target limit can fire.
💚 Proposed fix to isolate the target limit
// 6th target hit overall: should be blocked by targetLimit
const r8 = await rateLimitDual('login', 'user@example.com', {
- ipLimit: 5,
+ ipLimit: 50, // high enough that only targetLimit can block
targetLimit: 5,
});
expect(r8.allowed).toBe(false);📝 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.
| const r7 = await rateLimitDual('login', 'user@example.com', { | |
| ipLimit: 5, | |
| targetLimit: 5, | |
| }); | |
| expect(r7.allowed).toBe(true); // target hit 5 | |
| // 6th target hit overall: should be blocked by targetLimit | |
| const r8 = await rateLimitDual('login', 'user@example.com', { | |
| ipLimit: 5, | |
| targetLimit: 5, | |
| }); | |
| expect(r8.allowed).toBe(false); | |
| const r7 = await rateLimitDual('login', 'user@example.com', { | |
| ipLimit: 5, | |
| targetLimit: 5, | |
| }); | |
| expect(r7.allowed).toBe(true); // target hit 5 | |
| // 6th target hit overall: should be blocked by targetLimit | |
| const r8 = await rateLimitDual('login', 'user@example.com', { | |
| ipLimit: 50, // high enough that only targetLimit can block | |
| targetLimit: 5, | |
| }); | |
| expect(r8.allowed).toBe(false); |
🤖 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 126 - 137, Update the
later rateLimitDual calls for r7 and r8 in the test so their ipLimit is higher
than the accumulated requests from 5.6.7.8, while keeping targetLimit at 5. This
isolates the target-limit behavior and ensures r8 is blocked specifically by
targetLimit.
| const heads = await headers(); | ||
| const xff = heads.get('x-forwarded-for'); | ||
| const ip = (xff ? xff.split(',')[0]?.trim() : null) ?? heads.get('x-real-ip') ?? '127.0.0.1'; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Harden the client IP resolution.
Three concerns in the IP derivation on Line 80:
x-forwarded-foris client controllable. If the deployment does not sit behind a proxy that overwrites this header, an attacker sets a new value on each request and bypasses the IP bucket completely. Read the IP from a trusted source (platform-provided IP header, or take the entry at a known trusted-proxy offset from the right).- An input like
','produces an empty string.''is not nullish, so the??chain does not fall through tox-real-ip, and every such request shares the bucket keyip:<action>:. - The
'127.0.0.1'fallback also merges all header-less requests into one bucket. WithipLimit = 10per minute, a deployment that loses these headers rate-limits all users together.
Normalize to null on empty values, and consider failing closed or logging when no IP can be resolved.
🔒 Proposed fix for parsing and fallback
- const heads = await headers();
- const xff = heads.get('x-forwarded-for');
- const ip = (xff ? xff.split(',')[0]?.trim() : null) ?? heads.get('x-real-ip') ?? '127.0.0.1';
+ const heads = await headers();
+ const forwardedFor = heads.get('x-forwarded-for')?.split(',')[0]?.trim();
+ const realIp = heads.get('x-real-ip')?.trim();
+ // Empty strings must not win the fallback chain, otherwise unrelated
+ // requests collapse into a single shared bucket.
+ const ip = forwardedFor || realIp || 'unknown';🤖 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 78 - 80, Harden IP resolution in the
rate-limit flow around headers() by using only a trusted platform IP header or a
configured trusted-proxy offset from x-forwarded-for; do not trust the first
client-controlled entry by default. Normalize trimmed empty values to null,
allow fallback to x-real-ip, and remove the shared 127.0.0.1 fallback by failing
closed or applying the established no-IP handling/logging path when no address
is available.
…oken pnpm release pin in CI Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com>
🛡️ Sentinel: [security improvement] Dual Rate-Limiting for Auth Actions
🚨 Severity: MEDIUM / SECURITY ENHANCEMENT
💡 Vulnerability:
Critical authentication actions (
signUpActionandsignInAction) previously used target-only (email) rate-limiting buckets. This opened the system up to:🎯 Impact:
An attacker could cause widespread account lockout for target users, or exhaust system resources by stuffing credentials across multiple accounts without IP-level restrictions.
🔧 Fix:
rateLimitDualinsidesrc/lib/rate-limit.tsusing Next.js 15 asynchronousheaders().resetRateLimits()helper to clear rate-limiting buckets and prevent inter-test pollution in Vitest.signUpActionandsignInActioninsidesrc/app/actions/auth.tsto callrateLimitDualasynchronously.src/lib/__tests__/rate-limit.test.tsto assert that:✅ Verification:
pnpm typecheck) completed successfully with 0 errors.pnpm lint) completed successfully with 0 errors.PR created automatically by Jules for task 3799331359200161410 started by @projectamazonph
Summary by CodeRabbit
Security Improvements
Tests