🔒 Fix Rate Limit Bypass via X-Forwarded-For Spoofing - #524
Conversation
Co-authored-by: is0692vs <135803462+is0692vs@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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its |
PR Summary by QodoFix rate-limit bypass by hardening X-Forwarded-For IP selection
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| if (ip && isValidIp(ip)) { | ||
| return ip; |
There was a problem hiding this comment.
プロキシが自身のプライベート IP を X-Forwarded-For の右端に追加する環境では、その IP が全クライアント共通のレート制限キーになります。そのため、1 クライアントが上限を消費すると、同じプロキシ配下の他クライアントにも 429 が返されます。
Knowledge Base Used: GitHub Client
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/rateLimit.ts
Line: 80-81
Comment:
**プライベートプロキシで共有キー化**
プロキシが自身のプライベート IP を `X-Forwarded-For` の右端に追加する環境では、その IP が全クライアント共通のレート制限キーになります。そのため、1 クライアントが上限を消費すると、同じプロキシ配下の他クライアントにも 429 が返されます。
**Knowledge Base Used:** [GitHub Client](https://app.greptile.com/hiroki-org/-/custom-context/knowledge-base/hiroki-org/github-user-summary/-/docs/github-client.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Code Review by Qodo
Context used✅ Compliance rules (platform):
30 rules 1. Proxy IP rate limiting
|
| // Iterate from right to left and return the first valid IP. | ||
| // We do not skip private IPs because doing so without a known proxy topology | ||
| // allows attackers to bypass rate limiting by appending a spoofed private IP. | ||
| for (let i = ips.length - 1; i >= 0; i--) { | ||
| const ip = ips[i]; | ||
| if (ip && isValidIp(ip) && !isTrustedProxy(ip)) { | ||
| if (ip && isValidIp(ip)) { | ||
| return ip; |
There was a problem hiding this comment.
1. Proxy ip rate limiting 🐞 Bug ≡ Correctness
getClientIp now returns the right-most syntactically valid X-Forwarded-For token even if it is a private proxy address, and the API routes use that value directly as the rate-limit key. In deployments where the right-most XFF entry is a shared internal proxy hop, this can group many users into one bucket and cause widespread false 429s.
Agent Prompt
## Issue description
`getClientIp()` now always returns the right-most valid `x-forwarded-for` token, including private IPs. When the right-most token represents a shared internal proxy hop, using it as the rate-limit key collapses many users into a single bucket and causes false throttling.
## Issue Context
- `/api/card` and `/api/og` both call `rateLimiter.check(getClientIp(request))`.
- The updated tests explicitly lock in returning a private right-most token (e.g., `10.0.0.1`).
- The security fix is valid for spoofing resistance, but the new behavior should be configurable so environments with known proxy topology can still rate-limit by the true client identity.
## Fix Focus Areas
- Add an explicit, opt-in trust model (e.g., trusted proxy hop count and/or trusted proxy CIDRs via env var) and derive the client IP accordingly.
- Keep the secure default behavior when no trust configuration is provided.
- Update/extend tests to cover both the secure default and the configured trusted-proxy mode.
### Code references
- src/lib/rateLimit.ts[69-86]
- src/app/api/card/[username]/route.ts[23-25]
- src/app/api/og/[username]/route.tsx[21-23]
- src/lib/__tests__/rateLimit.test.ts[99-108]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
🎯 What: Fixed an IP spoofing vulnerability in the rate limiter's
⚠️ Risk: The previous logic attempted to automatically skip "trusted" (private) IPs when reading the header from right to left. Attackers could exploit this by sending an
X-Forwarded-Forparsing logic that allowed attackers to bypass limits.X-Forwarded-Forheader containing a target public IP followed by a spoofed private IP (e.g.,X-Forwarded-For: <spoofed_ip>, 127.0.0.1). The parser would skip the private IP and incorrectly attribute the request to the attacker's chosen public IP, completely bypassing rate limits.🛡️ Solution: Removed the unsafe
isTrustedProxyskipping logic. When the proxy topology is unknown, blindly trusting private IPs is a security anti-pattern. The safest default behavior is to use the right-most valid IP in the chain without skipping. This ensures that even if an attacker attempts to spoof the header, they cannot easily inject a proxy layer to bypass the check. Associated tests were updated to reflect this secure behavior.PR created automatically by Jules for task 7941677267623678207 started by @is0692vs
Greptile Summary
この PR は、
X-Forwarded-Forの右端からプライベート IP を除外していた処理を削除し、右端の有効な IP をそのままレート制限キーに採用します。isTrustedProxyとプライベート/ループバック/リンクローカル判定を削除Confidence Score: 4/5
PR はマージ可能ですが、プライベート IP を追加するリバースプロキシ配下でレート制限が共有バケット化する非ブロッキングの構成上の懸念があります。
変更は説明された偽装経路を閉じていますが、getClientIp の結果を直接キーにする既存 API では、右端のプライベートプロキシ IP が全クライアント共通の識別子になります。
Files Needing Attention: src/lib/rateLimit.ts
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR C1[クライアント A] --> P[プライベート IP のリバースプロキシ] C2[クライアント B] --> P P -->|X-Forwarded-For の右端にプロキシ IP| G[getClientIp] G -->|同一のプロキシ IP| R[共有レート制限バケット] R -->|上限到達| E[両クライアントへ 429]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Fix rate limit IP spoofing vulnerability..." | Re-trigger Greptile
Context used: