🛡️ Sentinel: implement dual rate-limiting for auth actions - #97
🛡️ Sentinel: implement dual rate-limiting for auth actions#97projectamazonph wants to merge 1 commit into
Conversation
- Added rateLimitDual helper in rate-limit.ts to limit requests by both client IP and target email. - Integrated rateLimitDual into signUpAction and signInAction. - Created robust unit test coverage in rate-limit.test.ts. 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. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches📝 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: 1
🧹 Nitpick comments (1)
src/app/actions/auth.ts (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace or remove duplicated restatement comments.
Both comments repeat the adjacent
rateLimitDualcall and its parameters. Remove them, or explain the non-obvious reason for the check, such as protecting database and password-hashing work.
src/app/actions/auth.ts#L35-L35: remove the duplicated call description or explain why rate limiting precedes sign-up work.src/app/actions/auth.ts#L129-L129: remove the duplicated call description because lines 127-128 already explain the reason.As per coding guidelines, comments should explain why rather than restate code.
🤖 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` at line 35, Remove the duplicated rateLimitDual description at src/app/actions/auth.ts lines 35-35, or replace it with a concise explanation that the check protects sign-up database and password-hashing work. Remove the duplicated comment at src/app/actions/auth.ts lines 129-129 because the preceding lines already explain its purpose.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/lib/rate-limit.ts`:
- Around line 38-39: Update the IP-key derivation used by signUpAction to avoid
trusting x-forwarded-for or x-real-ip; use the deployment’s trusted
proxy/platform client-IP signal instead, while preserving the unknown-IP
fallback. Add a regression test proving spoofed forwarded headers cannot create
separate rate-limit buckets or bypass the 10-request limit before hashPassword.
---
Nitpick comments:
In `@src/app/actions/auth.ts`:
- Line 35: Remove the duplicated rateLimitDual description at
src/app/actions/auth.ts lines 35-35, or replace it with a concise explanation
that the check protects sign-up database and password-hashing work. Remove the
duplicated comment at src/app/actions/auth.ts lines 129-129 because the
preceding lines already explain its purpose.
🪄 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: 3c749446-6c97-43eb-a9ee-1e90210db311
📒 Files selected for processing (4)
src/app/actions/__tests__/auth-actions.test.tssrc/app/actions/auth.tssrc/lib/__tests__/rate-limit.test.tssrc/lib/rate-limit.ts
| const xff = heads.get('x-forwarded-for'); | ||
| const ip = (xff ? xff.split(',')[0]?.trim() : null) ?? heads.get('x-real-ip') ?? 'unknown-ip'; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)rate-limit\.ts$|docs/voice-guide\.md|src/lib/logger\.ts|next\.config|middleware|vercel|fly|docker|nginx|traefik|caddy|hono|aws|render' || true
echo
echo "rate-limit outline and contents:"
if [ -f src/lib/rate-limit.ts ]; then
wc -l src/lib/rate-limit.ts
cat -n src/lib/rate-limit.ts
fi
echo
echo "Search for rate limiter usage / sign-up action:"
rg -n "rateLimitDual|rate-limit|rateLimit|isRateLimited|signUpAction|createAccount|password" src || true
echo
echo "Deployment/config header clues:"
rg -n "x-forwarded-for|x-real-ip|x-forwarded|headers|i?forward|trusted|proxy|CF-Connecting-IP|X-Real-IP|fastly-client-ip|forwarded" . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' | head -200 || trueRepository: projectamazonph/amph-v2
Length of output: 27794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "middleware.ts:"
cat -n src/middleware.ts
echo
echo "rate-limit tests:"
cat -n src/lib/__tests__/rate-limit.test.ts | sed -n '70,155p'
echo
echo "auth action and tests around signUp:"
cat -n src/app/actions/auth.ts | sed -n '1,110p'
cat -n src/app/actions/__tests__/auth-actions.test.ts | sed -n '60,135p'
echo
echo "read-only verifier: rateLimitDual keyed by spoofed first x-forwarded-for value"
node - <<'JS'
let buckets = new Map();
function rateLimit(key, limit = 5, windowMs = 60000) {
const now = Date.now();
const cutoff = now - windowMs;
const hits = (buckets.get(key) ?? []).filter((t) => t > cutoff);
if (hits.length >= limit) {
buckets.set(key, hits);
return { allowed: false, retryAfterSeconds: Math.max(1, Math.ceil((hits[0] + windowMs - now) / 1000)) };
}
hits.push(now);
buckets.set(key, hits);
return { allowed: true, retryAfterSeconds: 0 };
}
function ipFromHeaders(headers) {
const xff = headers.get('x-forwarded-for');
return (xff ? xff.split(',')[0]?.trim() : null) ?? headers.get('x-real-ip') ?? 'unknown-ip';
}
function rateLimitDual(headers, email, ipLimit = 10) {
const ip = ipFromHeaders(headers);
const ipRes = rateLimit(`ip:${ip}`, ipLimit, 60000);
if (!ipRes.allowed) return { allowed: false, key: `ip:${ip}`, emailRejected: false };
return { allowed: true, key: `ip:${ip}` };
}
const email = 'target@example.com';
let allowed = 0;
for (const spoofedIp of ['0.0.0.0', '10.20.30.40', '192.168.255.255', '8.8.8.8']) {
const result = rateLimitDual(
{ get: (name) => name === 'x-forwarded-for' ? spoofedIp : null },
email,
1
);
if (result.allowed) allowed++;
}
console.log(JSON.stringify({
allowedAcrossSpoofedXffValues: allowed,
firstIp: ipFromHeaders({ get: () => '8.8.8.8, 7.7.7.7, 1.2.3.4' }),
firstIpAfterSpace: ipFromHeaders({ get: () => ' 1.2.3.4 , 8.8.8.8' }),
fallbackToRealIp: ipFromHeaders({ get: () => null }),
}));
JSRepository: projectamazonph/amph-v2
Length of output: 17651
Do not use client-controlled forwarded headers as the IP key.
In this deployment path, x-forwarded-for and x-real-ip are not rewritten by middleware, so signUpAction maps each spoofed first x-forwarded-for value to a different ip:* bucket and can bypass the 10-request IP limit before hashPassword runs. Derive the key from a trusted proxy/platform signal and add a spoofed-header regression 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/rate-limit.ts` around lines 38 - 39, Update the IP-key derivation
used by signUpAction to avoid trusting x-forwarded-for or x-real-ip; use the
deployment’s trusted proxy/platform client-IP signal instead, while preserving
the unknown-IP fallback. Add a regression test proving spoofed forwarded headers
cannot create separate rate-limit buckets or bypass the 10-request limit before
hashPassword.
🛡️ Sentinel Security Improvement
rateLimitDualto enforce sliding window limits on both client IP (usingx-forwarded-for/x-real-ip) and target email addresses.src/lib/__tests__/rate-limit.test.tssatisfying 100% statements, branches, and lines coverage. All 219 system tests pass successfully.PR created automatically by Jules for task 2331528734594301326 started by @projectamazonph
Summary by CodeRabbit
Security Improvements
Tests