feat: add ratelimiting #1874
feat: add ratelimiting #1874
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a security feature set: new rate limiting utilities, fraud-prevention helpers, and a security index re-export. Integrates IP-based rate limits into auth sign-in and billing endpoints. Enhances IP parsing. Extends Stripe webhook to handle payment failures and trigger fraud-prevention actions. Updates the upgrade page to surface 429 rate-limit errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser
participant NextAuth API as NextAuth API [/api/auth/[...nextauth]]
participant RateLimit as RateLimit (auth)
participant Logger
User->>Browser: Submit sign-in
Browser->>NextAuth API: NextAuth request
NextAuth API->>RateLimit: checkRateLimit(ip)
alt Over limit
RateLimit-->>NextAuth API: { success: false, remaining }
NextAuth API->>Logger: log rate-limit violation
NextAuth API-->>Browser: Deny sign-in
else Allowed / fail-open
RateLimit-->>NextAuth API: { success: true, remaining }
NextAuth API-->>Browser: Continue normal sign-in flow
end
sequenceDiagram
autonumber
actor User
participant Browser
participant Billing API as Billing API [/api/teams/:id/billing/(manage|upgrade)]
participant RateLimit as RateLimit (billing)
User->>Browser: Click manage/upgrade
Browser->>Billing API: POST request
Billing API->>RateLimit: checkRateLimit(ip)
alt Over limit
RateLimit-->>Billing API: { success: false, remaining }
Billing API-->>Browser: 429 { error, remaining }
Browser->>User: Show toast "Rate limit exceeded"
else Allowed / fail-open
RateLimit-->>Billing API: { success: true, remaining }
Billing API-->>Browser: Proceed with existing billing flow
end
sequenceDiagram
autonumber
participant Stripe
participant Webhook as Webhook [/api/stripe/webhook]
participant Security as Security (fraud-prevention)
participant Radar as Stripe Radar
participant EdgeCfg as Vercel Edge Config
participant Logger
Stripe-->>Webhook: payment_intent.payment_failed
Webhook->>Security: processPaymentFailure(event)
Security->>Security: extract receipt_email, decline_code
alt Decline code indicates fraud and email present
par Add to Radar
Security->>Radar: add email to value list
Radar-->>Security: result
and Update Edge Config
Security->>EdgeCfg: fetch+PATCH emails list
EdgeCfg-->>Security: result
end
Security->>Logger: record outcomes
else Not fraudulent / missing data
Security->>Logger: skip processing
end
Webhook-->>Stripe: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pages/api/teams/[teamId]/billing/manage.ts (1)
112-114: Guard priceId/minQuantity to avoid runtime errors when not upgrading seats.getQuantityFromPriceId(priceId) is called unconditionally even when upgradePlan/addSeat is false, and priceId may be absent. Compute it only when needed.
Minimal fix:
- const minQuantity = getQuantityFromPriceId(priceId); + const minQuantity = + priceId && (type === "manage" && (upgradePlan || addSeat)) + ? getQuantityFromPriceId(priceId) + : undefined; @@ - quantity: isOldAccount(team.plan) - ? 1 - : (quantity ?? minQuantity), + quantity: isOldAccount(team.plan) + ? 1 + : (quantity ?? (minQuantity ?? 1)),Alternatively, move both minQuantity calculation and the whole flow_data block behind a single boolean like shouldUpdate = type === "manage" && (upgradePlan || addSeat) && subscriptionItemId.
Also applies to: 118-144
pages/api/teams/[teamId]/billing/upgrade.ts (1)
96-118: Add Stripe idempotency keys to prevent duplicate Checkout sessions on retries.External-call hazard: network retries or client re-submissions can create multiple sessions.
@@ - const stripe = stripeInstance(oldAccount); + const stripe = stripeInstance(oldAccount); + const idempotencyKey = + (req.headers["x-request-id"] as string | undefined) ?? + `checkout:${teamId}:${priceId}:${userId}`; @@ - stripeSession = await stripe.checkout.sessions.create({ + stripeSession = await stripe.checkout.sessions.create( + { customer: team.stripeId, customer_update: { name: "auto" }, billing_address_collection: "required", success_url: `${process.env.NEXTAUTH_URL}/settings/billing?success=true`, cancel_url: `${process.env.NEXTAUTH_URL}/settings/billing?cancel=true`, line_items: [lineItem], automatic_tax: { enabled: true, }, tax_id_collection: { enabled: true, }, mode: "subscription", allow_promotion_codes: true, client_reference_id: teamId, - }); + }, + { idempotencyKey }, + ); @@ - stripeSession = await stripe.checkout.sessions.create({ + stripeSession = await stripe.checkout.sessions.create( + { customer_email: userEmail ?? undefined, billing_address_collection: "required", success_url: `${process.env.NEXTAUTH_URL}/settings/billing?success=true`, cancel_url: `${process.env.NEXTAUTH_URL}/settings/billing?cancel=true`, line_items: [lineItem], automatic_tax: { enabled: true, }, tax_id_collection: { enabled: true, }, mode: "subscription", client_reference_id: teamId, ...(dubDiscount ?? { allow_promotion_codes: true }), metadata: { dubCustomerId: userId, }, - }); + }, + { idempotencyKey }, + );Optionally, let clients pass a dedicated X-Idempotency-Key header and prefer it over X-Request-Id.
Also applies to: 120-139
🧹 Nitpick comments (15)
lib/utils/ip.ts (1)
26-27: Fallback may collapse distinct users in some environments.Returning 127.0.0.1 for unknown IPs will group rate limits for all such requests. Consider returning a stable opaque identifier (e.g., “unknown:”) at call sites or log/metric only, and fail closed on rate‑limit for auth paths if IP is unavailable.
ee/features/security/lib/ratelimit.ts (2)
9-24: Comment–config mismatch: update to reflect actual limits.Comments say “3 per hour” (auth) and “5 per hour” (billing) but code is 3/20m and 3/30m.
Apply this diff to align comments:
- // 3 auth attempts per hour per IP + // 3 auth attempts per 20 minutes per IP ... - // 5 billing operations per hour per IP + // 3 billing operations per 30 minutes per IP
8-24: Enable Upstash ephemeralCache for hot rate limiters.Add a top-level in-memory cache and pass it as
ephemeralCacheto each Ratelimit to reduce Upstash round‑trips. Example:// top-level
const ephemeralCache = new Map();auth: new Ratelimit({
redis,
limiter: Ratelimit.slidingWindow(3, "20 m"),
ephemeralCache,
prefix: "rl:auth",
analytics: true,
}),billing: new Ratelimit({
redis,
limiter: Ratelimit.slidingWindow(3, "30 m"),
ephemeralCache,
prefix: "rl:billing",
analytics: true,
}),File: ee/features/security/lib/ratelimit.ts (around lines 8–24).
ee/features/security/lib/fraud-prevention.ts (6)
1-1: Remove unused import.NextApiResponse isn’t used.
-import { NextApiResponse } from "next";
23-31: Guard required env before calling Stripe Radar.Missing STRIPE_LIST_ID will throw at runtime. Fail fast with a clear log.
export async function addEmailToStripeRadar(email: string): Promise<boolean> { try { + if (!process.env.STRIPE_LIST_ID) { + log({ message: "STRIPE_LIST_ID not configured; cannot add to Radar", type: "error" }); + return false; + } const stripeClient = stripeInstance(); await stripeClient.radar.valueListItems.create({ value_list: process.env.STRIPE_LIST_ID!, value: email, });
48-66: Type and dedupe Edge Config emails.Be explicit about types and ensure no duplicates even with races between reads.
- const currentEmails = (await get("emails")) || []; + const currentEmails = (await get("emails")) as string[] | undefined; @@ - const updatedEmails = Array.isArray(currentEmails) - ? [...currentEmails, email] - : [email]; + const updatedEmails = Array.from( + new Set([...(Array.isArray(currentEmails) ? currentEmails : []), email]), + );
88-91: Propagate error details for observability.Include response text to aid debugging.
- if (!response.ok) { - throw new Error(`Vercel API error: ${response.status}`); - } + if (!response.ok) { + const body = await response.text().catch(() => ""); + throw new Error(`Vercel API error: ${response.status} ${body}`); + }
120-138: Avoid logging full emails; mask or hash PII.Slack logs may be retained; mask emails to reduce PII exposure.
- message: `Fraud indicator detected: ${declineCode} for email: ${email}`, + message: `Fraud indicator detected: ${declineCode} for email: ${maskEmail(email)}`, @@ - message: `Successfully added ${email} to Stripe Radar`, + message: `Successfully added ${maskEmail(email)} to Stripe Radar`, @@ - message: `Successfully added ${email} to Edge Config`, + message: `Successfully added ${maskEmail(email)} to Edge Config`,Add helper (outside this hunk):
const maskEmail = (e: string) => { const [u, d] = e.split("@"); if (!d) return "redacted"; return `${u[0] ?? ""}***@${d}`; };Also applies to: 146-156
127-131: Include rejection reasons from Promise.allSettled.When rejected, you lose the underlying error.
- if (stripeResult.status === "fulfilled" && stripeResult.value) { + if (stripeResult.status === "fulfilled" && stripeResult.value) { log({ message: `Successfully added ${email} to Stripe Radar`, type: "info", }); } else { log({ - message: `Failed to add ${email} to Stripe Radar:`, + message: `Failed to add ${email} to Stripe Radar: ${ + stripeResult.status === "rejected" ? String(stripeResult.reason) : "returned false" + }`, type: "error", }); } @@ - } else { + } else { log({ - message: `Failed to add ${email} to Edge Config:`, + message: `Failed to add ${email} to Edge Config: ${ + edgeConfigResult.status === "rejected" ? String(edgeConfigResult.reason) : "returned false" + }`, type: "error", }); }Also applies to: 133-156
pages/api/teams/[teamId]/billing/manage.ts (1)
29-41: Add response headers and async logging on 429 (observability + client UX).Return X-RateLimit-Remaining and log the violation without blocking the request.
Apply:
@@ - if (!rateLimitResult.success) { - return res.status(429).json({ + if (!rateLimitResult.success) { + // async, do not block response + waitUntil( + log({ + message: `Billing manage rate limit exceeded for IP ${clientIP}`, + type: "error", + }), + ); + res.setHeader( + "X-RateLimit-Remaining", + String(rateLimitResult.remaining ?? 0), + ); + return res.status(429).json({ error: "Too many billing requests. Please try again later.", remaining: rateLimitResult.remaining, }); }And add at top:
@@ import { identifyUser, trackAnalytics } from "@/lib/analytics"; import { errorhandler } from "@/lib/errorHandler"; import prisma from "@/lib/prisma"; import { CustomUser } from "@/lib/types"; +import { log } from "@/lib/utils"; import { getIpAddress } from "@/lib/utils/ip";pages/api/stripe/webhook.ts (1)
71-73: Avoid delaying webhook ack; run fraud processing via waitUntil.Stripe retries on slow handlers. Offload processPaymentFailure to background to reduce latency.
+import { waitUntil } from "@vercel/functions"; @@ - case "payment_intent.payment_failed": - await processPaymentFailure(event); - break; + case "payment_intent.payment_failed": + waitUntil(processPaymentFailure(event)); + break;pages/api/teams/[teamId]/billing/upgrade.ts (1)
27-40: Add rate-limit headers and async logging for parity with auth and better client handling.Expose remaining quota and log violations without blocking.
@@ - if (!rateLimitResult.success) { - return res.status(429).json({ + if (!rateLimitResult.success) { + waitUntil( + log({ + message: `Billing upgrade rate limit exceeded for IP ${clientIP}`, + type: "error", + }), + ); + res.setHeader( + "X-RateLimit-Remaining", + String(rateLimitResult.remaining ?? 0), + ); + return res.status(429).json({ error: "Too many billing requests. Please try again later.", remaining: rateLimitResult.remaining, }); }Also import:
@@ import { identifyUser, trackAnalytics } from "@/lib/analytics"; import { getDubDiscountForExternalUserId } from "@/lib/dub"; import prisma from "@/lib/prisma"; import { CustomUser } from "@/lib/types"; +import { log } from "@/lib/utils"; import { getIpAddress } from "@/lib/utils/ip";pages/api/auth/[...nextauth].ts (3)
221-227: Return a specific redirect for rate-limited sign-ins to improve UX.Returning false shows a generic AccessDenied. Redirect with a descriptive error.
- return false; // Block the signin + return "/login?error=RateLimited";
229-230: Don’t swallow errors silently in rate-limit check.At least log once to aid debugging; continue to fail-open if desired.
- } catch (error) {} + } catch (error) { + await log({ + message: `Signin rate-limit check failed open: ${String(error)}`, + type: "error", + }); + }
215-220: Optional: include remaining quota in analytics/log for better triage.If available, log remaining tokens to spot abuse patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ee/features/security/index.ts(1 hunks)ee/features/security/lib/fraud-prevention.ts(1 hunks)ee/features/security/lib/ratelimit.ts(1 hunks)lib/utils/ip.ts(1 hunks)pages/api/auth/[...nextauth].ts(4 hunks)pages/api/stripe/webhook.ts(3 hunks)pages/api/teams/[teamId]/billing/manage.ts(3 hunks)pages/api/teams/[teamId]/billing/upgrade.ts(3 hunks)pages/settings/upgrade.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
ee/features/security/lib/fraud-prevention.ts (2)
ee/stripe/index.ts (1)
stripeInstance(29-31)lib/utils.ts (1)
log(64-124)
pages/api/teams/[teamId]/billing/manage.ts (2)
lib/utils/ip.ts (1)
getIpAddress(1-28)ee/features/security/lib/ratelimit.ts (2)
checkRateLimit(29-44)rateLimiters(8-24)
pages/api/stripe/webhook.ts (1)
ee/features/security/lib/fraud-prevention.ts (1)
processPaymentFailure(109-158)
pages/api/teams/[teamId]/billing/upgrade.ts (2)
lib/utils/ip.ts (1)
getIpAddress(1-28)ee/features/security/lib/ratelimit.ts (2)
checkRateLimit(29-44)rateLimiters(8-24)
pages/api/auth/[...nextauth].ts (4)
lib/edge-config/blacklist.ts (1)
isBlacklistedEmail(3-21)lib/utils/ip.ts (1)
getIpAddress(1-28)ee/features/security/lib/ratelimit.ts (2)
checkRateLimit(29-44)rateLimiters(8-24)lib/utils.ts (1)
log(64-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
pages/settings/upgrade.tsx (1)
202-208: Good UX: graceful 429 handling with reset and toast.Early‑return on 429 and clearing selectedPlan prevents stuck buttons and double actions.
Consider adding a small helper to DRY the repeated block:
const handleRateLimit = (res: Response) => { if (res.status === 429) { toast.error("Rate limit exceeded. Please try again later."); setSelectedPlan(null); return true; } return false; };Then:
.then(async (res) => { if (handleRateLimit(res)) return; // ... })Also applies to: 238-244
ee/features/security/index.ts (1)
1-2: Aggregator looks good.Re-exports are straightforward and help centralize the security API surface.
ee/features/security/lib/fraud-prevention.ts (2)
12-18: Keep FRAUD_DECLINE_CODES privateDefinition at ee/features/security/lib/fraud-prevention.ts:12 and only referenced in the same file (includes check at line 121), so do not export.
67-91: Replace "update" with "upsert" — there is no "set" and "update" 4xxs if the key is missing.Vercel Edge Config supports create, update, upsert, delete; use "upsert" to create-or-replace the "emails" key when it may not exist.
File: ee/features/security/lib/fraud-prevention.ts (lines 67–91)
Likely an incorrect or invalid review comment.
pages/api/teams/[teamId]/billing/manage.ts (1)
31-34: Confirm desired limiter scope (shared bucket across billing endpoints).Using only client IP with a single billing limiter means manage and upgrade share the same bucket. If that’s intentional, ignore. If not, consider namespacing the identifier (e.g.,
${clientIP}:manage).pages/api/stripe/webhook.ts (1)
30-35: Event wired correctly.Adding payment_intent.payment_failed to the allow-list is correct and matches the new handler.
pages/api/teams/[teamId]/billing/upgrade.ts (1)
30-33: Confirm limiter scope.Same shared “billing” limiter per IP across manage and upgrade; if that’s intentional, fine. Otherwise namespace the identifier (e.g.,
${clientIP}:upgrade).
| export async function checkRateLimit( | ||
| limiter: Ratelimit, | ||
| identifier: string, | ||
| ): Promise<{ success: boolean; remaining?: number; error?: string }> { | ||
| try { | ||
| const result = await limiter.limit(identifier); | ||
| return { | ||
| success: result.success, | ||
| remaining: result.remaining, | ||
| }; | ||
| } catch (error) { | ||
| console.error("Rate limiting error:", error); | ||
| // Fail open - allow request if rate limiting fails | ||
| return { success: true, error: "Rate limiting unavailable" }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fail‑open on limiter errors weakens auth/billing protections.
Returning success: true on errors makes brute force/billing abuse possible during Redis/Upstash outages. Prefer fail‑closed for auth (and maybe billing) or make behavior configurable per call.
Proposed API (non‑breaking default can stay fail‑open, but allow opting into strict mode):
-export async function checkRateLimit(
- limiter: Ratelimit,
- identifier: string,
-): Promise<{ success: boolean; remaining?: number; error?: string }> {
+export async function checkRateLimit(
+ limiter: Ratelimit,
+ identifier: string,
+ opts?: { failOpen?: boolean },
+): Promise<{ success: boolean; remaining?: number; error?: string }> {
try {
const result = await limiter.limit(identifier);
return {
success: result.success,
remaining: result.remaining,
};
} catch (error) {
console.error("Rate limiting error:", error);
- // Fail open - allow request if rate limiting fails
- return { success: true, error: "Rate limiting unavailable" };
+ const failOpen = opts?.failOpen ?? false;
+ return { success: failOpen, error: "Rate limiting unavailable" };
}
}Then call with { failOpen: false } for auth and { failOpen: true } where availability trumps security.
🏁 Script executed:
#!/bin/bash
# Find all call sites to gauge where strict mode should be used.
rg -nP --type=ts '\bcheckRateLimit\s*\(' -C3Length of output: 2037
Make rate-limiter configurable; fail-closed for auth & billing
Fail‑open on limiter errors weakens auth/billing protections. Add an opt‑in failOpen flag and enable strict (fail‑closed) behavior for sensitive endpoints.
-export async function checkRateLimit(
- limiter: Ratelimit,
- identifier: string,
-): Promise<{ success: boolean; remaining?: number; error?: string }> {
+export async function checkRateLimit(
+ limiter: Ratelimit,
+ identifier: string,
+ opts?: { failOpen?: boolean },
+): Promise<{ success: boolean; remaining?: number; error?: string }> {
try {
const result = await limiter.limit(identifier);
return {
success: result.success,
remaining: result.remaining,
};
} catch (error) {
console.error("Rate limiting error:", error);
- // Fail open - allow request if rate limiting fails
- return { success: true, error: "Rate limiting unavailable" };
+ // Default to non‑breaking fail‑open; callers may pass { failOpen: false } for strict behavior
+ const failOpen = opts?.failOpen ?? true;
+ return { success: failOpen, error: "Rate limiting unavailable" };
}
}Update call sites to opt into strict mode:
- pages/api/auth/[...nextauth].ts — call at ~line 216: pass { failOpen: false }.
- pages/api/teams/[teamId]/billing/upgrade.ts — call at ~line 29: pass { failOpen: false }.
- pages/api/teams/[teamId]/billing/manage.ts — call at ~line 31: pass { failOpen: false }.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function checkRateLimit( | |
| limiter: Ratelimit, | |
| identifier: string, | |
| ): Promise<{ success: boolean; remaining?: number; error?: string }> { | |
| try { | |
| const result = await limiter.limit(identifier); | |
| return { | |
| success: result.success, | |
| remaining: result.remaining, | |
| }; | |
| } catch (error) { | |
| console.error("Rate limiting error:", error); | |
| // Fail open - allow request if rate limiting fails | |
| return { success: true, error: "Rate limiting unavailable" }; | |
| } | |
| } | |
| export async function checkRateLimit( | |
| limiter: Ratelimit, | |
| identifier: string, | |
| opts?: { failOpen?: boolean }, | |
| ): Promise<{ success: boolean; remaining?: number; error?: string }> { | |
| try { | |
| const result = await limiter.limit(identifier); | |
| return { | |
| success: result.success, | |
| remaining: result.remaining, | |
| }; | |
| } catch (error) { | |
| console.error("Rate limiting error:", error); | |
| // Default to non‑breaking fail‑open; callers may pass { failOpen: false } for strict behavior | |
| const failOpen = opts?.failOpen ?? true; | |
| return { success: failOpen, error: "Rate limiting unavailable" }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In ee/features/security/lib/ratelimit.ts around lines 29 to 44, the
checkRateLimit function currently fails-open on limiter errors; change its
signature to accept an optional options object (e.g., { failOpen?: boolean })
with failOpen defaulting to true to preserve current behavior, and update the
catch block to return success: true when failOpen is true but to return success:
false (with an error message) when failOpen is false (fail-closed). Then update
the three call sites to opt into strict mode by passing { failOpen: false }:
pages/api/auth/[...nextauth].ts at ~line 216,
pages/api/teams/[teamId]/billing/upgrade.ts at ~line 29, and
pages/api/teams/[teamId]/billing/manage.ts at ~line 31.
| // Check x-forwarded-for header (most common for proxied requests) | ||
| const forwardedFor = headers["x-forwarded-for"]; | ||
| if (typeof forwardedFor === "string") { | ||
| const ip = forwardedFor.split(",")[0]?.trim(); | ||
| if (ip) return ip; | ||
| } | ||
| if (Array.isArray(forwardedFor) && forwardedFor.length > 0) { | ||
| const ip = forwardedFor[0].split(",")[0]?.trim(); | ||
| if (ip) return ip; | ||
| } | ||
|
|
||
| // Check x-real-ip header (nginx proxy) | ||
| const realIp = headers["x-real-ip"]; | ||
| if (typeof realIp === "string") { | ||
| const ip = realIp.trim(); | ||
| if (ip) return ip; | ||
| } | ||
| if (Array.isArray(realIp) && realIp.length > 0) { | ||
| const ip = realIp[0].trim(); | ||
| if (ip) return ip; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Don’t trust client‑controlled IP headers without a trust boundary.
If a client can set x-forwarded-for/x-real-ip, IP‑based rate limits are trivially bypassed. Ensure these headers are only used behind a trusted proxy (e.g., Vercel/NGINX that overwrites them), or gate parsing behind an explicit trust flag/env. At minimum, document the assumption at call sites.
Apply validation and make acceptance of these headers explicit:
export function getIpAddress(headers: {
[key: string]: string | string[] | undefined;
}): string {
- // Check x-forwarded-for header (most common for proxied requests)
+ // Check x-forwarded-for header (only trusted behind a proxy)
const forwardedFor = headers["x-forwarded-for"];
if (typeof forwardedFor === "string") {
- const ip = forwardedFor.split(",")[0]?.trim();
- if (ip) return ip;
+ const ip = forwardedFor.split(",")[0]?.trim();
+ if (ip && isIP(ip)) return ip;
}
if (Array.isArray(forwardedFor) && forwardedFor.length > 0) {
- const ip = forwardedFor[0].split(",")[0]?.trim();
- if (ip) return ip;
+ const ip = forwardedFor[0].split(",")[0]?.trim();
+ if (ip && isIP(ip)) return ip;
}
- // Check x-real-ip header (nginx proxy)
+ // Check x-real-ip header (nginx proxy; trusted only behind proxy)
const realIp = headers["x-real-ip"];
if (typeof realIp === "string") {
- const ip = realIp.trim();
- if (ip) return ip;
+ const ip = realIp.trim();
+ if (ip && isIP(ip)) return ip;
}
if (Array.isArray(realIp) && realIp.length > 0) {
- const ip = realIp[0].trim();
- if (ip) return ip;
+ const ip = realIp[0].trim();
+ if (ip && isIP(ip)) return ip;
}Add at top of file:
import { isIP } from "node:net";Optionally, add a boolean parameter or env (e.g., PPMK_TRUST_PROXY) to require explicit opt‑in before using these headers.
Run to confirm usage is only in server routes behind your proxy:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts 'getIpAddress\s*\(' -C2Length of output: 3650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== lib/utils/ip.ts (full) ==="
if [ -f lib/utils/ip.ts ]; then
sed -n '1,240p' lib/utils/ip.ts || true
else
echo "lib/utils/ip.ts not found"
fi
echo
echo "=== getIpAddress call sites (rg results) ==="
rg -nP --type=ts 'getIpAddress\s*\(' -C2 || true
echo
echo "=== Show top of each file that calls getIpAddress (first 160 lines) ==="
files=$(rg -nP --type=ts 'getIpAddress\s*\(' -l || true)
if [ -n "$files" ]; then
for f in $files; do
echo
echo "---- $f ----"
sed -n '1,160p' "$f" || true
done
else
echo "No files found"
fi
echo
echo "=== Search for explicit trust / proxy env flags or isIP usage ==="
rg -n --hidden --no-ignore 'PPMK_TRUST_PROXY|TRUST_PROXY|TRUSTED_PROXY|TRUST_PROXY|TRUST_PROXY' || true
rg -n --hidden --no-ignore 'process.env\.(PPMK_TRUST_PROXY|TRUST_PROXY|TRUSTED_PROXY|TRUST_PROXY)' || true
rg -n --hidden --no-ignore '\bisIP\b|from \"node:net\"' || true
echo
echo "=== Check for next.config.js / vercel.json presence ==="
[ -f next.config.js ] && sed -n '1,240p' next.config.js || echo "next.config.js not found"
[ -f vercel.json ] && sed -n '1,240p' vercel.json || echo "vercel.json not found"
echo
echo "=== Search repo for other x-forwarded-for / x-real-ip handling ==="
rg -n --hidden --no-ignore 'x-forwarded-for|x-real-ip|cf-connecting-ip|true-client-ip' -S || trueLength of output: 51406
Don't trust client-controlled IP headers — validate & require an explicit trust boundary
getIpAddress reads x-forwarded-for / x-real-ip without validation and is used in security‑sensitive flows; these headers can be forged and allow rate‑limit/session bypass.
-
Critical call sites that MUST be fixed:
- Rate limiting / auth: pages/api/teams/[teamId]/billing/upgrade.ts, pages/api/teams/[teamId]/billing/manage.ts, pages/api/auth/[...nextauth].ts (used for checkRateLimit).
- Session verification: lib/auth/dataroom-auth.ts (verifyDataroomSessionInPagesRouter compares stored ipAddress to getIpAddress(req.headers)).
- IP logging/watermarking: pages/api/links/download/* (index.ts, dataroom-folder.ts, dataroom-document.ts, bulk.ts), pages/api/record_video_view.ts.
- Other raw header usages: pages/api/unsubscribe/*, lib/middleware/posthog.ts, pages/api/assistants/chat.ts.
-
Required fixes (apply to lib/utils/ip.ts and call sites):
- Gate header parsing behind an explicit opt‑in (env var, e.g., TRUST_PROXY or PPMK_TRUST_PROXY = "1"). Default: do NOT trust forwarded headers.
- Validate any extracted value with node:net's isIP(ip) before using/returning it.
- If not trusted, use a server‑side source (req.socket.remoteAddress or platform API such as @vercel/functions ipAddress(request)) instead of headers.
- For session/auth flows, never accept header‑sourced IP for authentication/session binding unless TRUST_PROXY=true and the deployment/proxy is guaranteed to overwrite these headers.
- Document the trust assumption at each call site and add tests to assert behavior with TRUST_PROXY on/off.
-
Minimal implementation hints: import { isIP } from "node:net" and only return header IPs when isIP(ip) && process.env.TRUST_PROXY === "1".
Address these before merging — current usage exposes immediate rate‑limit and session security issues.
Summary by CodeRabbit
New Features
Bug Fixes