security: stop trusting the leftmost x-forwarded-for for rate limits and abuse quotas (FRE-371)

getClientIP (the rate-limit and audit key) took the LEFTMOST x-forwarded-for
hop and read x-real-ip unconditionally. In our topology — client → Cloudflare
→ same-host origin proxy → app — both proxies APPEND, so the leftmost hop is
whatever the sender typed: a rotating X-Forwarded-For minted a fresh rate-limit
bucket per request, and unlimited bug-report quota via the same pattern in
routes/api/the-nook/report.ts. x-real-ip is client-settable too; nothing in
the chain overwrites it. It is no longer read at all.

New trust order (src/server/trusted-client-ip.ts, resolveTrustedClientIp):

  1. a non-loopback socket peer IS the client — its headers are ignored;
  2. cf-connecting-ip (Cloudflare overwrites it at the edge; parsed as an
     address or rejected);
  3. x-forwarded-for counted from the RIGHT, discounting one hop for the
     Cloudflare edge address the origin proxy appended (the topology has
     exactly one trusted proxy below Cloudflare);
  4. "unknown" — callers key stricter shared limits on it, so resolution
     fails closed toward a tighter bucket, never a looser one.

Addresses are never truncated: a /24-truncated key would pool attackers with
the innocents behind them.

Share-vs-copy, stated deliberately: this is a port of the parsing primitives
 FRE-366 reviewed in request-client.ts, NOT an import. resolveClientIp carries
a standing rule (1d9287a) that it never feeds a security decision; lifting
that rule for these callers would couple an access-control change to an
analytics branch under review, and the analytics helper only exposes
truncated prefixes while rate limiting needs the full address. If the two
drift, trusted-client-ip.ts is the one security paths follow. The deploy-side
precondition (origin reachable only via Cloudflare + the same-host proxy) is
documented in the file header; if the app is ever directly internet-reachable
no in-app read can recover the true client.

Tests updated deliberately, not to go green: rate-limit.test.ts now pins the
rightmost-minus-one order, cf-connecting-ip precedence, peer-over-headers,
x-real-ip rejection, and the spoof case (a rotating leftmost hop does not
change the resolved client); report.test.ts pins the quota key stability
under a rotating header. New trusted-client-ip.test.ts covers the helper's
own edges: APIEvent wrapper, loopback forms, unparseable hops failing closed.
This commit is contained in:
2026-10-30 05:06:18 +00:00
committed by Paperclip Agent
parent 134f41eac4
commit 2fd2a4a71a
7 changed files with 577 additions and 47 deletions

View File

@@ -3,6 +3,7 @@ import { z } from "zod";
import sendEmail from "~/server/email";
import { CONTACT_RECIPIENT_EMAIL } from "~/lib/contact-config";
import { takeBugReportToken } from "~/server/bug-report-rate-limit";
import { resolveTrustedClientIp } from "~/server/trusted-client-ip";
import { json, error } from "./_lib";
/**
@@ -103,10 +104,9 @@ export async function POST(event: APIEvent): Promise<Response> {
)
.trim()
.toLowerCase();
const forwarded = event.request.headers.get("x-forwarded-for");
const rateKey =
fingerprint ||
(forwarded ? forwarded.split(",")[0]!.trim() : "unknown");
// FRE-371: the IP fallback must be the spoof-resistant read — a rotating
// leftmost x-forwarded-for used to mint unlimited report quota.
const rateKey = fingerprint || resolveTrustedClientIp(event);
const token = takeBugReportToken(rateKey);
if (!token.allowed) {
return new Response(JSON.stringify({ error: "Too many reports" }), {