🛡️ Sentinel: Fix brute force vulnerability on authentication actions#79
Open
projectamazonph wants to merge 1 commit into
Open
🛡️ Sentinel: Fix brute force vulnerability on authentication actions#79projectamazonph wants to merge 1 commit into
projectamazonph wants to merge 1 commit into
Conversation
- Retrieve client IP address using 'x-forwarded-for' and 'x-real-ip' headers. - Rate-limit unauthenticated entry points (signInAction, signUpAction) using both IP-based and target-based keys. - Mock headers in test suites to prevent failures and added integration tests verifying dual rate-limiting behaviors. Co-authored-by: projectamazonph <286085559+projectamazonph@users.noreply.github.com>
Contributor
|
👋 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 apply IP-based rate limiting alongside email-target limiting. Tests add controllable request-header mocks and verify both blocking paths. ChangesAuthentication rate limiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant signInAction
participant RequestHeaders
participant RateLimiters
signInAction->>RequestHeaders: Read client IP headers
RequestHeaders-->>signInAction: Return IP or fallback
signInAction->>RateLimiters: Check IP limit
RateLimiters-->>signInAction: Allow or reject
signInAction->>RateLimiters: Check email-target limit
RateLimiters-->>signInAction: Allow or reject
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 |
Analysis CompleteGenerated ECC bundle from 1 commits | Confidence: 55% View Pull Request #80Repository Profile
Changed Files (6)
Top hotspots
Top directories
Analysis Depth Readiness (commit-history, 7%)ECC Tools uses this to decide whether recommendations should stay at commit-history/setup guidance or expand into CI, security, harness, reference-set, AI-routing, and team backlog work.
Reference Set Readiness (0/7, 0%)
Likely Future Issues (2)
Suggested Follow-up Work (2)
Copy-ready bodies test: add auth coverage for src/app/actions/auth.ts ## Summary
- Add auth, session, or permission regression coverage for the recently changed security-sensitive surface.
## Why
- Backfill auth or permission regression coverage before another access-control change lands on the touched surface.
## Touched paths
- `src/app/actions/auth.ts`
## Validation
- Add or extend integration / e2e coverage for the changed auth, session, middleware, or permission surface.
- Exercise allowed and denied flows, invalid or expired credentials, or equivalent access-control boundary cases.security: add scanner evidence for src/app/actions/auth.ts ## Summary
- Add security scanner or code-scanning evidence for the recently changed security-sensitive surface.
## Why
- Backfill explicit scanner or code-scanning evidence before another security-sensitive change lands on the touched surface.
## Touched paths
- `src/app/actions/auth.ts`
## Validation
- Run or add the relevant security scanner, code scanning, secret scanning, or dependency/security review check for the touched surface.
- Attach the scanner output, SARIF/code-scanning result, or focused security regression test to the follow-up PR.
- Confirm the changed auth, billing, webhook, secret-handling, agent, or CI surface has an explicit pass/fail gate.Review Activity (1 reviews, 0 inline comments, 0 unresolved threads)
Latest reviewer states
Review Follow-up Signals (1)
Recommended next actions
Generated Instincts (20)
After merging, import with: Files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.jules/sentinel.md:
- Around line 8-11: Rewrite the “Dual-Layer Rate Limiting on Authentication
Actions” section in plain language for non-security readers: replace or define
credential stuffing, brute force, target-based key, and “singly rate-limiting,”
and explain the purpose of both account-based and IP-based limits without
changing the documented prevention guidance.
In `@src/app/actions/__tests__/auth-actions.test.ts`:
- Around line 151-209: Add equivalent dual rate-limiting regression tests for
signUpAction alongside the existing signInAction cases: verify repeated attempts
for the same email across different IPs are blocked at the target limit, and
different emails from one IP are blocked at the IP limit. Reuse the existing
mockHeadersGet and database setup patterns, and assert the expected non-blocked
and blocked error messages.
In `@src/app/actions/auth.ts`:
- Around line 36-48: Replace the raw x-forwarded-for/x-real-ip lookups used for
IP-based rate limiting in src/app/actions/auth.ts lines 36-48 and 139-151 with a
trusted canonical client-IP source or proxy-boundary helper, and use that value
for both signup and signin rate-limit keys. Do not allow callers to control the
limiter identity by supplying forwarding headers.
🪄 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: bae08b36-2e08-4c12-b94f-53469f607152
📒 Files selected for processing (6)
.jules/sentinel.mdsrc/__tests__/setup.tssrc/app/actions/__tests__/auth-actions.test.tssrc/app/actions/__tests__/progress-actions.test.tssrc/app/actions/__tests__/tool-actions.test.tssrc/app/actions/auth.ts
Comment on lines
+8
to
+11
| ## 2026-07-24 - Dual-Layer Rate Limiting on Authentication Actions | ||
| **Vulnerability:** Singly rate-limiting Next.js Server Actions by target email allows a distributed credential stuffing/brute force attack where attackers change targets or IPs to bypass simple limits. | ||
| **Learning:** Target-based limits alone do not prevent brute-forcing different accounts from a single IP, and IP-based limits alone can easily block innocent users behind shared proxies/NATs if they hit limits too early. | ||
| **Prevention:** Implement dual rate-limiting on unauthenticated entry points (like sign-in and sign-up). Combine a target-based key (e.g. `signup:${email.toLowerCase()}`) with an IP-based key retrieved via request headers (`x-forwarded-for` / `x-real-ip`) to secure authentication actions robustly. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Define the security terms in plain language.
“Credential stuffing,” “brute force,” and “target-based key” are not defined, and “singly rate-limiting” is unclear. Rewrite this for readers without security background.
🤖 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 @.jules/sentinel.md around lines 8 - 11, Rewrite the “Dual-Layer Rate
Limiting on Authentication Actions” section in plain language for non-security
readers: replace or define credential stuffing, brute force, target-based key,
and “singly rate-limiting,” and explain the purpose of both account-based and
IP-based limits without changing the documented prevention guidance.
Source: Coding guidelines
Comment on lines
+151
to
+209
| describe('Dual Rate-Limiting', () => { | ||
| it('target-based rate limiting blocks the same email after 5 attempts even with different IPs', async () => { | ||
| const email = 'target-limit@rate.com'; | ||
| (db.user.findUnique as unknown as ReturnType<typeof vi.fn>).mockResolvedValue(null); | ||
|
|
||
| // Make 5 successful non-blocked attempts with different IPs | ||
| for (let i = 1; i <= 5; i++) { | ||
| mockHeadersGet.mockImplementation((key: string) => { | ||
| if (key === 'x-forwarded-for') return `9.9.1.${i}`; | ||
| return null; | ||
| }); | ||
| const result = await signInAction({ email, password: 'wrong' }); | ||
| expect(result.success).toBe(false); | ||
| // It fails because the user doesn't exist/wrong password, but it is NOT blocked yet | ||
| if (!result.success) { | ||
| expect(result.error).toBe('Email or password is incorrect.'); | ||
| } | ||
| } | ||
|
|
||
| // The 6th attempt with a new IP should be blocked by the target-based rate limiter | ||
| mockHeadersGet.mockImplementation((key: string) => { | ||
| if (key === 'x-forwarded-for') return '9.9.1.6'; | ||
| return null; | ||
| }); | ||
| const result = await signInAction({ email, password: 'wrong' }); | ||
| expect(result.success).toBe(false); | ||
| if (!result.success) { | ||
| expect(result.error).toContain('Too many attempts.'); | ||
| expect(result.error).not.toContain('from this IP'); | ||
| } | ||
| }); | ||
|
|
||
| it('IP-based rate limiting blocks different emails from the same IP after 10 attempts', async () => { | ||
| // Use a static IP for all requests | ||
| mockHeadersGet.mockImplementation((key: string) => { | ||
| if (key === 'x-forwarded-for') return '9.9.9.9'; | ||
| return null; | ||
| }); | ||
| (db.user.findUnique as unknown as ReturnType<typeof vi.fn>).mockResolvedValue(null); | ||
|
|
||
| // Make 10 attempts targeting different emails from the same IP | ||
| for (let i = 1; i <= 10; i++) { | ||
| const email = `ip-limit-${i}@rate.com`; | ||
| const result = await signInAction({ email, password: 'wrong' }); | ||
| expect(result.success).toBe(false); | ||
| if (!result.success) { | ||
| expect(result.error).toBe('Email or password is incorrect.'); | ||
| } | ||
| } | ||
|
|
||
| // The 11th attempt targeting a fresh email from the same IP should be blocked by the IP-based rate limiter | ||
| const email = 'ip-limit-11@rate.com'; | ||
| const result = await signInAction({ email, 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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add rate-limit regression tests for signUpAction.
This suite verifies only signInAction, while signUpAction gained the same IP and email limit logic. Add target-limit and IP-limit cases for sign-up so its security path cannot silently regress.
As per coding guidelines, “New features must include tests; admin and business-layer features require 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 151 - 209, Add
equivalent dual rate-limiting regression tests for signUpAction alongside the
existing signInAction cases: verify repeated attempts for the same email across
different IPs are blocked at the target limit, and different emails from one IP
are blocked at the IP limit. Reuse the existing mockHeadersGet and database
setup patterns, and assert the expected non-blocked and blocked error messages.
Source: Coding guidelines
Comment on lines
+36
to
+48
| const heads = await headers(); | ||
| const ip = heads.get('x-forwarded-for') ?? heads.get('x-real-ip') ?? '127.0.0.1'; | ||
|
|
||
| // 1. IP-based rate limiting to prevent distributed brute force / event-loop starvation | ||
| 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.`); | ||
| } | ||
|
|
||
| // 2. Target-based rate limiting to protect specific accounts | ||
| const targetRl = rateLimit(`signup:${data.email.toLowerCase()}`, 5, 60_000); | ||
| if (!targetRl.allowed) { | ||
| throw new Error(`Too many attempts. Try again in ${targetRl.retryAfterSeconds}s.`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## auth.ts\n'
sed -n '1,240p' src/app/actions/auth.ts
printf '\n## rateLimit references\n'
rg -n "function rateLimit|const rateLimit|rateLimit\(" src -g '*.ts' -g '*.tsx'
printf '\n## rateLimit implementation files\n'
fd -a "rateLimit" src
printf '\n## headers() usage in auth-related files\n'
rg -n "headers\(\)|x-forwarded-for|x-real-ip" src/app src/lib src/components -g '*.ts' -g '*.tsx'Repository: projectamazonph/amph-v2
Length of output: 9985
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## src/lib/rate-limit.ts\n'
sed -n '1,220p' src/lib/rate-limit.ts
printf '\n## src/lib/admin-audit.ts\n'
sed -n '1,120p' src/lib/admin-audit.ts
printf '\n## tests touching x-forwarded-for\n'
sed -n '1,260p' src/app/actions/__tests__/auth-actions.test.tsRepository: projectamazonph/amph-v2
Length of output: 11932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## search for proxy/IP handling\n'
rg -n "trusted proxy|canonical IP|client IP|x-forwarded-for|x-real-ip|proxy" src docs . -g '*.ts' -g '*.tsx' -g '*.md' -g '*.mdx' -g '*.json'
printf '\n## middleware / edge / request handling files\n'
fd -a "middleware|proxy|ip|headers" src docsRepository: projectamazonph/amph-v2
Length of output: 3613
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## src/middleware.ts\n'
sed -n '1,220p' src/middleware.ts
printf '\n## src/lib/middleware-context.ts\n'
sed -n '1,220p' src/lib/middleware-context.ts
printf '\n## any request-ip helpers\n'
rg -n "ipAddress|clientIp|canonical.*ip|forwarded.*for|real-ip|headers\(\).*get\('x-forwarded-for'" src/lib src/app -g '*.ts' -g '*.tsx'Repository: projectamazonph/amph-v2
Length of output: 6739
Do not use raw forwarding headers for rate-limit identity.
x-forwarded-for can be client-supplied and multi-hop, and x-real-ip is only trustworthy if a proxy sets it. With no canonical client-IP helper or trusted proxy boundary here, callers can vary the header value and sidestep the per-IP limiter.
src/app/actions/auth.ts#L36-L48: sign-up throttling is easy to evade.src/app/actions/auth.ts#L139-L151: same issue on sign-in.
📍 Affects 1 file
src/app/actions/auth.ts#L36-L48(this comment)src/app/actions/auth.ts#L139-L151
🤖 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 - 48, Replace the raw
x-forwarded-for/x-real-ip lookups used for IP-based rate limiting in
src/app/actions/auth.ts lines 36-48 and 139-151 with a trusted canonical
client-IP source or proxy-boundary helper, and use that value for both signup
and signin rate-limit keys. Do not allow callers to control the limiter identity
by supplying forwarding headers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛡️ Sentinel: [HIGH] Fix brute force vulnerability on authentication actions
🚨 Severity
HIGH
💡 Vulnerability
Singly rate-limiting Server Actions by target email address allowed distributed credential stuffing or IP cycling to bypass rate limiting.
🎯 Impact
Malicious users could brute force credentials or run credential-stuffing campaigns against users.
🔧 Fix
Implemented dual rate-limiting (IP-based + target/email-based) on unauthenticated entry points
signUpActionandsignInAction.✅ Verification
Added Vitest integration tests to verify that:
All 214 tests pass successfully. Linter check runs clean with zero errors. Production build is fully verified and successful.
PR created automatically by Jules for task 5824063639222886420 started by @projectamazonph
Summary by CodeRabbit
Security Improvements
Bug Fixes