fix: harden IP and bound in-memory rate limiters (#1435)

e2e/dashboard-widgets.spec.js

@@ -214,7 +214,7 @@ test("goal form posts a new goal", async ({ page }) => {
   await page.getByLabel("Goal title").fill("Ship one PR");
   await page.getByLabel("Target").fill("1");
   await page.getByLabel("Unit").selectOption("prs");
-  await page.getByRole("button", { name: "Add goal" }).click();
+  await page.getByRole("button", { name: "Create goal" }).click();
 
   await expect.poll(() => goalPosts, { timeout: 15000 }).toHaveLength(1);
   expect(goalPosts[0]).toMatchObject({

src/app/api/public/[username]/route.ts

@@ -2,80 +2,47 @@
 import { NextRequest, NextResponse } from "next/server";
 import { fetchPublicProfile } from "@/lib/public-profile-data";
 import { getUpstashConfig, upstashRateLimitFixedWindow } from "@/lib/upstash-rest";
+import { createMemoryFixedWindowRateLimiter, getClientIp } from "@/lib/rate-limit";
 
 export const dynamic = "force-dynamic";
 
-/**
- * In-memory rate limiter for IP addresses.
- * Maps IP -> { count: number, resetAt: number }
- * This resets on server restart. For production, use Redis.
- */
-const ipRateLimits = new Map<
-  string,
-  { count: number; resetAt: number }
->();
-
 const RATE_LIMIT_REQUESTS = 30;
 const RATE_LIMIT_WINDOW_MS = 60 * 1000; // 1 minute
-
-function cleanOldEntries(map: Map<string, { count: number; resetAt: number }>) {
-  const now = Date.now();
-  for (const [key, val] of map.entries()) {
-    if (val.resetAt <= now) map.delete(key);
-  }
-}
-
-function getRateLimitKey(req: NextRequest): string {
-  // req.headers.get("x-forwarded-for") is populated by the Next.js / Vercel runtime from the verified
-  // network-layer source address and cannot be spoofed by the caller.
-  //
-  // x-forwarded-for is intentionally excluded here: it is a plain request
-  // header that any client can set to an arbitrary value. Trusting it as the
-  // primary key allows an attacker to rotate the header on every request,
-  // bypass the per-IP limit entirely, and exhaust the shared GITHUB_TOKEN
-  // quota (5 000 req/hr), making the endpoint unavailable for all users.
-  return req.headers.get("x-forwarded-for")?.split(",")[0]?.trim() || req.headers.get("x-real-ip") || "unknown";
-}
-
-function checkRateLimit(ip: string): { allowed: boolean; retryAfter?: number } {
-  const now = Date.now();
-  for (const [key, record] of ipRateLimits) {
-    if (now > record.resetAt) ipRateLimits.delete(key);
-  }
-  const record = ipRateLimits.get(ip);
-
-  if (!record || now > record.resetAt) {
-    // New window or expired
-    ipRateLimits.set(ip, { count: 1, resetAt: now + RATE_LIMIT_WINDOW_MS });
-    return { allowed: true };
-  }
-
-  if (record.count < RATE_LIMIT_REQUESTS) {
-    record.count++;
-    return { allowed: true };
-  }
-
-  // Rate limit exceeded
-  const retryAfter = Math.ceil((record.resetAt - now) / 1000);
-  return { allowed: false, retryAfter };
-}
+const MEMORY_MAX_ENTRIES = Number(process.env.MEMORY_RATE_LIMIT_MAX_ENTRIES ?? 10_000);
+const memoryLimiter = createMemoryFixedWindowRateLimiter({
+  windowMs: RATE_LIMIT_WINDOW_MS,
+  pruneIntervalMs: 5 * 60 * 1000,
+  maxEntries: Number.isFinite(MEMORY_MAX_ENTRIES) ? MEMORY_MAX_ENTRIES : 10_000,
+});
 
 export async function GET(
   req: NextRequest,
   { params }: { params: Promise<{ username: string }> }
 ): Promise<NextResponse> {
-  cleanOldEntries(ipRateLimits);
-  const resolvedParams = await params;
-  const username = resolvedParams.username;
+  const { username } = await params;
   // Rate limiting
-  const ip = getRateLimitKey(req);
+  const ip = getClientIp(req);
   const rateLimit = getUpstashConfig()
     ? await upstashRateLimitFixedWindow({
         key: `public-profile-rate-limit:${ip}`,
         limit: RATE_LIMIT_REQUESTS,
         windowSeconds: Math.ceil(RATE_LIMIT_WINDOW_MS / 1000),
       })
-    : checkRateLimit(ip);
+    : (() => {
+        const local = memoryLimiter.check(
+          `public-profile-rate-limit:${ip}`,
+          RATE_LIMIT_REQUESTS
+        );
+        return local.allowed
+          ? { allowed: true }
+          : {
+              allowed: false,
+              retryAfter: Math.max(
+                local.reset - Math.floor(Date.now() / 1000),
+                1
+              ),
+            };
+      })();
 
   if (!rateLimit.allowed) {
     return NextResponse.json(

src/app/dashboard/settings/page.tsx

@@ -148,6 +148,7 @@ function SettingsPageContent() {
   const [showConfirmModal, setShowConfirmModal] = useState(false);
   const [pendingPath, setPendingPath] = useState<string | null>(null);
 
+
   // Spotlight Repos States
   const [userRepos, setUserRepos] = useState<string[]>([]);
   const [loadingRepos, setLoadingRepos] = useState(false);

src/app/u/[username]/page.tsx

@@ -198,11 +198,14 @@ export default async function PublicProfilePage({
       </div>
 
       {/* Custom Spotlight Repositories */}
-      {profile.spotlightRepos && profile.spotlightRepos.length > 0 && (
+      {profile.spotlightRepos?.length ? (
         <div className="mt-6">
-          <PinnedReposWidget initialRepos={profile.spotlightRepos} isPublic={true} />
+          <PinnedReposWidget
+            initialRepos={profile.spotlightRepos}
+            isPublic={true}
+          />
         </div>
-      )}
+      ) : null}
 
       {/* Row 2: Top repos */}
       <div className="mt-6">

src/lib/badge-rate-limit.ts

@@ -1,50 +1,36 @@
 import { NextRequest } from "next/server";
+import { createMemoryFixedWindowRateLimiter, getClientIp } from "@/lib/rate-limit";
 
 const WINDOW_MS = 60 * 1000;
 const BADGE_LIMIT = 20;
 
 // NOTE: This rate limiter is separate from the API middleware rate limiting.
 // It applies per-IP limiting specifically for badge generation endpoints.
 
-const buckets = new Map<string, number[]>();
+const MEMORY_MAX_ENTRIES = Number(process.env.MEMORY_RATE_LIMIT_MAX_ENTRIES ?? 10_000);
+const memoryLimiter = createMemoryFixedWindowRateLimiter({
+  windowMs: WINDOW_MS,
+  pruneIntervalMs: 5 * 60 * 1000,
+  maxEntries: Number.isFinite(MEMORY_MAX_ENTRIES) ? MEMORY_MAX_ENTRIES : 10_000,
+});
 
 export type BadgeRateLimitResult = {
   allowed: boolean;
   remaining: number;
   reset: number;
 };
 
-function pruneBuckets(now: number) {
-  if (buckets.size < 500) return;
-  const cutoff = now - WINDOW_MS;
-  for (const [key, timestamps] of Array.from(buckets.entries())) {
-    if (timestamps.every((t) => t <= cutoff)) buckets.delete(key);
-  }
-}
-
 export function checkBadgeRateLimit(ip: string): BadgeRateLimitResult {
   const now = Date.now();
-  pruneBuckets(now);
-
   const key = `badge:${ip}`;
-  const cutoff = now - WINDOW_MS;
-  const active = (buckets.get(key) ?? []).filter((t) => t > cutoff);
-  const reset = Math.ceil(((active[0] ?? now) + WINDOW_MS) / 1000);
-
-  if (active.length >= BADGE_LIMIT) {
-    buckets.set(key, active);
-    return { allowed: false, remaining: 0, reset };
-  }
-
-  active.push(now);
-  buckets.set(key, active);
-  return { allowed: true, remaining: BADGE_LIMIT - active.length, reset };
+  const result = memoryLimiter.check(key, BADGE_LIMIT, now);
+  return {
+    allowed: result.allowed,
+    remaining: result.remaining,
+    reset: result.reset,
+  };
 }
 
 export function getBadgeClientIp(req: NextRequest): string {
-  return (
-    req.headers.get("x-forwarded-for")?.split(",")[0]?.trim() ??
-    req.headers.get("x-real-ip") ??
-    "unknown"
-  );
+  return getClientIp(req);
 }

src/lib/rate-limit.test.ts

@@ -0,0 +1,72 @@
+import { describe, it, expect } from "vitest";
+import { createMemoryFixedWindowRateLimiter, getClientIp } from "./rate-limit";
+
+describe("getClientIp", () => {
+  it("prefers req.ip when present", () => {
+    const req = {
+      ip: "203.0.113.1",
+      headers: new Headers({ "x-forwarded-for": "1.1.1.1" }),
+    };
+    expect(getClientIp(req as any)).toBe("203.0.113.1");
+  });
+
+  it("prefers cf-connecting-ip over x-real-ip and x-forwarded-for", () => {
+    const req = {
+      ip: undefined,
+      headers: new Headers({
+        "cf-connecting-ip": "203.0.113.2",
+        "x-real-ip": "203.0.113.3",
+        "x-forwarded-for": "203.0.113.4",
+      }),
+    };
+    expect(getClientIp(req as any)).toBe("203.0.113.2");
+  });
+
+  it("falls back to x-real-ip and then first x-forwarded-for hop", () => {
+    const req = {
+      ip: undefined,
+      headers: new Headers({
+        "x-forwarded-for": "1.1.1.1, 2.2.2.2",
+        "x-real-ip": "203.0.113.5",
+      }),
+    };
+    expect(getClientIp(req as any)).toBe("203.0.113.5");
+
+    const req2 = {
+      ip: undefined,
+      headers: new Headers({ "x-forwarded-for": "1.1.1.1, 2.2.2.2" }),
+    };
+    expect(getClientIp(req2 as any)).toBe("1.1.1.1");
+  });
+});
+
+describe("createMemoryFixedWindowRateLimiter", () => {
+  it("evicts expired entries during prune", () => {
+    const limiter = createMemoryFixedWindowRateLimiter({
+      windowMs: 1000,
+      pruneIntervalMs: 0,
+      maxEntries: 100,
+    });
+
+    limiter.check("k1", 5, 0);
+    expect(limiter._unsafeBuckets.has("k1")).toBe(true);
+
+    limiter.check("k2", 5, 2000);
+    expect(limiter._unsafeBuckets.has("k1")).toBe(false);
+  });
+
+  it("caps bucket growth with maxEntries", () => {
+    const limiter = createMemoryFixedWindowRateLimiter({
+      windowMs: 60_000,
+      pruneIntervalMs: 0,
+      maxEntries: 2,
+    });
+
+    limiter.check("a", 1, 0);
+    limiter.check("b", 1, 0);
+    limiter.check("c", 1, 0);
+
+    expect(limiter._unsafeBuckets.size).toBeLessThanOrEqual(2);
+  });
+});
+

src/lib/rate-limit.ts

@@ -0,0 +1,93 @@
+import type { NextRequest } from "next/server";
+
+export type MemoryRateLimitResult = {
+  allowed: boolean;
+  remaining: number;
+  reset: number; // unix seconds
+};
+
+function normalizeIp(value: string | null | undefined): string | null {
+  const ip = typeof value === "string" ? value.trim() : "";
+  return ip.length > 0 ? ip : null;
+}
+
+export function getClientIp(
+  req: Pick<NextRequest, "ip" | "headers">
+): string {
+  return (
+    normalizeIp(req.ip) ??
+    normalizeIp(req.headers.get("cf-connecting-ip")) ??
+    normalizeIp(req.headers.get("x-real-ip")) ??
+    normalizeIp(req.headers.get("x-forwarded-for")?.split(",")[0]) ??
+    "unknown"
+  );
+}
+
+type Bucket = { count: number; resetAt: number };
+
+export function createMemoryFixedWindowRateLimiter(options: {
+  windowMs: number;
+  pruneIntervalMs?: number;
+  maxEntries?: number;
+}) {
+  const windowMs = options.windowMs;
+  const pruneIntervalMs = options.pruneIntervalMs ?? windowMs;
+  const maxEntries = options.maxEntries ?? 10_000;
+
+  const buckets = new Map<string, Bucket>();
+  let lastPruneAt = 0;
+
+  function prune(now: number) {
+    if (now - lastPruneAt < pruneIntervalMs) return;
+    lastPruneAt = now;
+
+    for (const [key, bucket] of buckets.entries()) {
+      if (bucket.resetAt <= now) buckets.delete(key);
+    }
+
+    if (buckets.size <= maxEntries) return;
+
+    const overflow = buckets.size - maxEntries;
+    let removed = 0;
+    for (const key of buckets.keys()) {
+      buckets.delete(key);
+      removed += 1;
+      if (removed >= overflow) break;
+    }
+  }
+
+  function check(
+    key: string,
+    limit: number,
+    now = Date.now()
+  ): MemoryRateLimitResult {
+    prune(now);
+
+    const existing = buckets.get(key);
+    if (!existing || existing.resetAt <= now) {
+      buckets.set(key, { count: 1, resetAt: now + windowMs });
+      return {
+        allowed: true,
+        remaining: Math.max(limit - 1, 0),
+        reset: Math.ceil((now + windowMs) / 1000),
+      };
+    }
+
+    if (existing.count >= limit) {
+      return {
+        allowed: false,
+        remaining: 0,
+        reset: Math.ceil(existing.resetAt / 1000),
+      };
+    }
+
+    existing.count += 1;
+    return {
+      allowed: true,
+      remaining: Math.max(limit - existing.count, 0),
+      reset: Math.ceil(existing.resetAt / 1000),
+    };
+  }
+
+  return { check, _unsafeBuckets: buckets };
+}