🔒 Fix IP Spoofing vulnerability in rate limiter#304
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
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the API routes and their corresponding tests to utilize NextRequest and modifies the IP address extraction logic for rate limiting. The reviewer identified a security risk regarding IP spoofing due to manual header parsing and recommended using request.ip instead. Additionally, it was suggested to centralize the IP extraction logic into a utility function to maintain consistency and reduce code duplication.
| const fontUrl = `${allowedOrigin}/fonts/NotoSans-Regular.ttf`; | ||
|
|
||
| const ip = request.headers.get("x-forwarded-for") ?? "unknown"; | ||
| const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0] ?? "unknown"; |
There was a problem hiding this comment.
Using request.ip is the recommended and more secure way to retrieve the client's IP address in Next.js, especially when using the Edge runtime. Manually parsing the x-forwarded-for header by taking the first element (split(",")[0]) is susceptible to IP spoofing, as an attacker can easily prepend arbitrary IP addresses to this header. Furthermore, the current implementation lacks .trim(), which could lead to inconsistent rate limiting keys if the header contains whitespace.
Additionally, consider centralizing this IP extraction logic into a utility function to maintain consistency across different API routes and reduce duplication, as suggested by the general rules. Note that if you switch to request.ip, you may need to update your tests to mock the ip property on the NextRequest object (e.g., using Object.defineProperty).
| const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0] ?? "unknown"; | |
| const ip = request.ip ?? "unknown"; |
References
- Centralize session validation and authentication logic into utility functions to prevent code duplication and maintain consistency across multiple API endpoints.
|
|
||
| const forwarded = request.headers.get("x-forwarded-for"); | ||
| const ip = forwarded ? forwarded.split(",").at(-1)?.trim() ?? "unknown" : "unknown"; | ||
| const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0] ?? "unknown"; |
There was a problem hiding this comment.
Similar to the /api/card route, using request.ip is preferred over manual header parsing for security and reliability. The switch from taking the last element (at(-1)) in the previous implementation to the first element ([0]) actually increases the risk of IP spoofing if x-real-ip is not present, as the leftmost IP in x-forwarded-for is the one most easily controlled by the client. Additionally, centralizing this logic into a utility function would ensure consistency across the codebase.
| const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0] ?? "unknown"; | |
| const ip = request.ip ?? "unknown"; |
References
- Centralize session validation and authentication logic into utility functions to prevent code duplication and maintain consistency across multiple API endpoints.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const fontUrl = `${allowedOrigin}/fonts/NotoSans-Regular.ttf`; | ||
|
|
||
| const ip = request.headers.get("x-forwarded-for") ?? "unknown"; | ||
| const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0] ?? "unknown"; |
There was a problem hiding this comment.
x-forwarded-for フォールバックが依然としてなりすまし可能
x-real-ip が存在しない場合（ローカル開発・Vercel 以外のデプロイ環境など）、フォールバックとして x-forwarded-for の 最初（左端）の値 が使われますが、これはクライアントが任意に設定できるヘッダーです。Vercel の Edge Network はクライアントが付与した x-forwarded-for 値をそのまま連結して転送するため、攻撃者が X-Forwarded-For: spoofed-ip を送ると split(",")[0] は spoofed-ip を返し、レート制限を簡単に回避できます。フォールバックには左端（クライアント側）ではなく 右端（信頼できるプロキシが付加した） の IP を使うか、x-real-ip がなければ "unknown" に固定することを検討してください。同じ問題が src/app/api/og/[username]/route.tsx の 20 行目にも存在します。
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/card/[username]/route.ts
Line: 23
Comment:
**`x-forwarded-for` フォールバックが依然としてなりすまし可能**
`x-real-ip` が存在しない場合（ローカル開発・Vercel 以外のデプロイ環境など）、フォールバックとして `x-forwarded-for` の **最初（左端）の値** が使われますが、これはクライアントが任意に設定できるヘッダーです。Vercel の Edge Network はクライアントが付与した `x-forwarded-for` 値をそのまま連結して転送するため、攻撃者が `X-Forwarded-For: spoofed-ip` を送ると `split(",")[0]` は `spoofed-ip` を返し、レート制限を簡単に回避できます。フォールバックには左端（クライアント側）ではなく **右端（信頼できるプロキシが付加した）** の IP を使うか、`x-real-ip` がなければ `"unknown"` に固定することを検討してください。同じ問題が `src/app/api/og/[username]/route.tsx` の 20 行目にも存在します。
How can I resolve this? If you propose a fix, please make it concise.| const fontUrl = `${allowedOrigin}/fonts/NotoSans-Regular.ttf`; | ||
|
|
||
| const ip = request.headers.get("x-forwarded-for") ?? "unknown"; | ||
| const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0] ?? "unknown"; |
There was a problem hiding this comment.
x-forwarded-for の最初の IP に .trim() がない
"1.2.3.4, 5.6.7.8" のようにカンマ後にスペースが入るフォーマットでは最初の値に空白は付きませんが、プロキシによっては先頭にスペースを付与することがあります。旧 OG ルートは .trim() を呼び出していたため、同様に追加しておくと安全です。
| const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0] ?? "unknown"; | |
| const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ?? "unknown"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/card/[username]/route.ts
Line: 23
Comment:
**`x-forwarded-for` の最初の IP に `.trim()` がない**
`"1.2.3.4, 5.6.7.8"` のようにカンマ後にスペースが入るフォーマットでは最初の値に空白は付きませんが、プロキシによっては先頭にスペースを付与することがあります。旧 OG ルートは `.trim()` を呼び出していたため、同様に追加しておくと安全です。
```suggestion
const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ?? "unknown";
```
How can I resolve this? If you propose a fix, please make it concise.
|
Superseded by #296, which covers the same IP spoofing fix across the OG and card APIs with route tests. Closing this duplicate to keep the review queue focused. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
1 participant
🎯 What: Fixed an IP spoofing vulnerability in the
⚠️ Risk: The application previously read the raw
/api/card/[username]and/api/og/[username]API routes.x-forwarded-forheader for rate limiting. An attacker could easily spoof this header to bypass rate limits, potentially leading to denial of service, resource exhaustion, or abuse of the upstream GitHub API.🛡️ Solution: Updated the logic to securely obtain the client IP. For NextRequest objects in the Edge runtime, we use
request.headers.get("x-real-ip")or safely parse the first IP fromrequest.headers.get("x-forwarded-for"). Added/updated automated tests to verify the rate limiting logic properly reads the mocked ip fields.PR created automatically by Jules for task 163391641152232909 started by @is0692vs
Greptile Summary
このPRは
/api/card/[username]および/api/og/[username]のレートリミッターで発生していたIPスプーフィング脆弱性を修正します。従来はx-forwarded-forをそのまま参照していたため、ヘッダーを偽装してレート制限を回避できる状態でした。x-real-ipを優先し、存在しない場合はx-forwarded-forの最初のIPへフォールバックするよう変更。NextRequestベースに移行:Request→NextRequestへ切り替え、実際のEdgeランタイム環境に合わせたテスト構成に更新。.at(-1)（右端・プロキシが付加したIP）を使用していたが、新コードは[0]（左端・クライアント制御）へ変更されており、x-real-ip不在時のフォールバック挙動が旧来よりも脆弱になっています。Confidence Score: 3/5
レート制限のIPスプーフィング修正は
x-real-ipが存在する場合は正しく機能するが、フォールバックが旧来と同様に回避可能なため、修正が完全ではない。このPRはIPスプーフィングを修正する目的で作成されているにもかかわらず、
x-real-ipが不在の場合にx-forwarded-forの左端（クライアントが任意に設定できる）IPを使用するフォールバックが残っており、意図した保護が回避される可能性がある。旧OGルートが採用していた右端IP（プロキシが付加）のアプローチと比べ、フォールバック動作が後退している。route.tsとroute.tsxの両ファイルにおけるx-forwarded-forフォールバックのIP選択ロジックを要確認。Security Review
x-real-ipが存在しない環境ではx-forwarded-forの左端（クライアント制御）IPにフォールバックするため、レート制限の回避が依然として可能です（route.ts23行目、route.tsx20行目）。x-real-ipを信頼できるプロキシ（Vercel Edge）が確実に付与する環境では本番上の実害は限定的ですが、非Vercel環境やローカル開発環境ではバイパスが成立します。Important Files Changed
x-real-ip優先に変更。x-forwarded-forフォールバックが左端（クライアント制御）の IP を使用しており、x-real-ip不在時にレート制限をなりすまし回避できる。.trim()も欠落。x-real-ip優先に統一。旧コードは.at(-1)（右端・より信頼できる）を使っていたが、新コードは[0]（左端・スプーフィング可能）にフォールバックする。RequestをNextRequestに置き換え。レート制限テストはx-forwarded-forのみカバーし、新しいx-real-ipパスは未テスト。Comments Outside Diff (1)
src/app/api/card/[username]/route.test.ts, line 96-116 (link)x-real-ipパスのテストが欠如レート制限のテストは
x-forwarded-forヘッダーのみを使用しており、今回追加された主要な IP 取得パス（x-real-ip）がカバーされていません。x-real-ipを設定したリクエストでもレート制限が正しく機能することを確認するテストケースを追加することを推奨します。Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "🔒 Fix IP Spoofing vulnerability in rate..." | Re-trigger Greptile