Skip to content

🔒 Fix IP Spoofing vulnerability in rate limiter #304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
is0692vs wants to merge 1 commit into from
+10 −9
Closed

🔒 Fix IP Spoofing vulnerability in rate limiter #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/app/api/card/[username]/route.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { NextRequest } from "next/server";
import { describe, expect, it, vi } from "vitest";

vi.mock("@/lib/cardDataFetcher", () => ({
Expand All @@ -23,7 +24,7 @@ describe("GET /api/card/[username] cache headers", () => {
});

const { GET } = await import("./route");
const req = new Request("http://localhost/api/card/alice");
const req = new NextRequest("http://localhost/api/card/alice");
const response = await GET(req, { params: Promise.resolve({ username: "alice" }) });

expect(response.headers.get("Cache-Control")).toBe("public, s-maxage=1800, stale-while-revalidate=3600");
Expand All @@ -34,7 +35,7 @@ describe("GET /api/card/[username] cache headers", () => {
vi.mocked(fetchCardData).mockResolvedValueOnce(null);

const { GET } = await import("./route");
const req = new Request("http://localhost/api/card/ghost");
const req = new NextRequest("http://localhost/api/card/ghost");
const response = await GET(req, { params: Promise.resolve({ username: "ghost" }) });

expect(response.status).toBe(404);
Expand All @@ -46,7 +47,7 @@ describe("GET /api/card/[username] cache headers", () => {
vi.mocked(fetchCardData).mockRejectedValueOnce(new Error("API Error"));

const { GET } = await import("./route");
const req = new Request("http://localhost/api/card/erroruser");
const req = new NextRequest("http://localhost/api/card/erroruser");
const response = await GET(req, { params: Promise.resolve({ username: "erroruser" }) });

expect(response.status).toBe(503);
Expand All @@ -66,7 +67,7 @@ describe("GET /api/card/[username] error responses", () => {
}

const { GET } = await import("./route");
const req = new Request(`http://localhost/api/card/${username}`);
const req = new NextRequest(`http://localhost/api/card/${username}`);
await GET(req, { params: Promise.resolve({ username }) });

expect(renderErrorCardResponse).toHaveBeenCalledWith(expect.objectContaining({
Expand All @@ -92,7 +93,7 @@ describe("GET /api/card/[username] rate limiting", () => {
const { fetchCardData } = await import("@/lib/cardDataFetcher");
const { renderErrorCardResponse } = await import("@/lib/cardRenderer");

const req1 = new Request("http://localhost/api/card/testuser", {
const req1 = new NextRequest("http://localhost/api/card/testuser", {
headers: {
"x-forwarded-for": "127.0.0.1",
},
Expand Down
5 changes: 3 additions & 2 deletions src/app/api/card/[username]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { NextRequest } from "next/server";
import { RateLimiter } from "@/lib/rateLimit";
import { fetchCardData } from "@/lib/cardDataFetcher";
import { parseCardQueryParams, renderCardResponse, renderErrorCardResponse } from "@/lib/cardRenderer";
Expand All @@ -10,7 +11,7 @@ const SUCCESS_CACHE = "public, s-maxage=1800, stale-while-revalidate=3600";
const ERROR_CACHE = "public, s-maxage=60, stale-while-revalidate=120";

export async function GET(
request: Request,
request: NextRequest,
{ params }: { params: Promise<{ username: string }> }
): Promise<Response> {
const { username } = await params;
Expand All @@ -19,7 +20,7 @@ export async function GET(
const allowedOrigin = process.env.APP_URL || "http://localhost:3000";
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";
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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).

Suggested change
const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0] ?? "unknown";
const ip = request.ip ?? "unknown";
References
  1. Centralize session validation and authentication logic into utility functions to prevent code duplication and maintain consistency across multiple API endpoints.

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security 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.

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 x-forwarded-for の最初の IP に .trim() がない

"1.2.3.4, 5.6.7.8" のようにカンマ後にスペースが入るフォーマットでは最初の値に空白は付きませんが、プロキシによっては先頭にスペースを付与することがあります。旧 OG ルートは .trim() を呼び出していたため、同様に追加しておくと安全です。

Suggested change
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.

const rateLimitResult = rateLimiter.check(ip);

if (!rateLimitResult.success) {
Expand Down
3 changes: 1 addition & 2 deletions src/app/api/og/[username]/route.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
) {
const { username } = await params;

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";
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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.

Suggested change
const ip = request.headers.get("x-real-ip") ?? request.headers.get("x-forwarded-for")?.split(",")[0] ?? "unknown";
const ip = request.ip ?? "unknown";
References
  1. Centralize session validation and authentication logic into utility functions to prevent code duplication and maintain consistency across multiple API endpoints.

const rateLimitResult = rateLimiter.check(ip);

if (!rateLimitResult.success) {
Expand Down Expand Up @@ -84,7 +83,7 @@
}}
>
{avatarUrl && (
<img

Check warning on line 86 in src/app/api/og/[username]/route.tsx

View workflow job for this annotation

GitHub Actions / Lint 

Using `<img>` could result in slower LCP and higher bandwidth. Consider using `<Image />` from `next/image` or a custom image loader to automatically optimize images. This may incur additional usage or cost from your provider. See: https://nextjs.org/docs/messages/no-img-element
src={sanitizeUrl(avatarUrl)}
alt=""
width={120}
Expand Down
Loading