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

agent-security-engineer · 2026-10-30T01:07:23-04:00

2026-10-30 01:07:23 -04:00

Summary

The rate-limit key and the bug-report abuse quota were derived from the leftmost x-forwarded-for hop, with x-real-ip read unconditionally. In our topology (client → Cloudflare → same-host origin proxy → app) both proxies append, so the leftmost hop is whatever the sender typed: one scripted loop rotating X-Forwarded-For: <random> minted a fresh rate-limit bucket per request and unlimited bug-report quota.

 resolveTrustedClientIp(event)            # new, src/server/trusted-client-ip.ts
-  x-forwarded-for.split(",")[0]          # leftmost = sender-chosen
-  x-real-ip                              # client-settable, nothing overwrites it
+  1. non-loopback socket peer            # the client itself; headers ignored
+  2. cf-connecting-ip                    # Cloudflare overwrites it at the edge
+  3. x-forwarded-for rightmost-minus-one # discount the one trusted proxy hop
+  4. "unknown"                           # callers key stricter limits on it — fails closed

Addresses are never truncated (a /24-truncated key pools attackers with the innocents behind them).

Share-vs-copy, stated deliberately: the parsing primitives are a port of the ones FRE-366 reviewed in request-client.ts, not an import. resolveClientIp carries a standing rule (1d9287a) that it never feeds a security decision, it only exposes truncated prefixes while rate limiting needs the full address, and it ships on an analytics branch still under review. If the two drift, trusted-client-ip.ts is the one security paths follow. Deploy-side precondition (origin reachable only via Cloudflare + the same-host proxy) is documented in the file header.

Evidence

In-run gate, branch freno-dev/FRE-371-trusted-client-ip-clean @ 2fd2a4a:

bun test src/server/trusted-client-ip.test.ts src/server/security/rate-limit.test.ts src/routes/api/the-nook/report.test.ts
 56 pass, 0 fail (127 expect() calls)

New pin: a rotating leftmost hop with a fixed cf-connecting-ip resolves to the same client on every request — the spoof case the old read failed.

Merge Danger

Door: two-way (revert restores the old read; no data migration).

Blast Radius: every caller of getClientIP — login/password-reset/registration rate limits, audit-log IPs, bug-report quota. Behind the pinned proxy chain the resolved address becomes more accurate; a deployment that breaks the topology precondition (app directly internet-reachable) would degrade to the stricter "unknown" shared limits, not a looser one.

## Summary The rate-limit key and the bug-report abuse quota were derived from the **leftmost** `x-forwarded-for` hop, with `x-real-ip` read unconditionally. In our topology (client → Cloudflare → same-host origin proxy → app) both proxies **append**, so the leftmost hop is whatever the sender typed: one scripted loop rotating `X-Forwarded-For: <random>` minted a fresh rate-limit bucket per request and unlimited bug-report quota. ```diff resolveTrustedClientIp(event) # new, src/server/trusted-client-ip.ts - x-forwarded-for.split(",")[0] # leftmost = sender-chosen - x-real-ip # client-settable, nothing overwrites it + 1. non-loopback socket peer # the client itself; headers ignored + 2. cf-connecting-ip # Cloudflare overwrites it at the edge + 3. x-forwarded-for rightmost-minus-one # discount the one trusted proxy hop + 4. "unknown" # callers key stricter limits on it — fails closed ``` Addresses are never truncated (a /24-truncated key pools attackers with the innocents behind them). **Share-vs-copy, stated deliberately:** the parsing primitives are a port of the ones FRE-366 reviewed in `request-client.ts`, not an import. `resolveClientIp` carries a standing rule (1d9287a) that it never feeds a security decision, it only exposes truncated prefixes while rate limiting needs the full address, and it ships on an analytics branch still under review. If the two drift, `trusted-client-ip.ts` is the one security paths follow. Deploy-side precondition (origin reachable only via Cloudflare + the same-host proxy) is documented in the file header. ## Evidence In-run gate, branch `freno-dev/FRE-371-trusted-client-ip-clean` @ `2fd2a4a`: ``` bun test src/server/trusted-client-ip.test.ts src/server/security/rate-limit.test.ts src/routes/api/the-nook/report.test.ts 56 pass, 0 fail (127 expect() calls) ``` New pin: a rotating leftmost hop with a fixed `cf-connecting-ip` resolves to the same client on every request — the spoof case the old read failed. ## Merge Danger **Door:** two-way (revert restores the old read; no data migration). **Blast Radius:** every caller of `getClientIP` — login/password-reset/registration rate limits, audit-log IPs, bug-report quota. Behind the pinned proxy chain the resolved address becomes *more* accurate; a deployment that breaks the topology precondition (app directly internet-reachable) would degrade to the stricter "unknown" shared limits, not a looser one.

agent-security-engineer added 1 commit 2026-10-30 01:07:23 -04:00

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

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.

Mike merged commit a0e6ff1044 into master

2026-11-04 10:58:26 -05:00

Mike referenced this issue from a commit

2026-11-04 10:58:26 -05:00

Merge pull request 'security: stop trusting the leftmost x-forwarded-for for rate limits and abuse quotas (FRE-371)' (#1) from agent-security-engineer/freno-dev:freno-dev/FRE-371-trusted-client-ip-clean into master

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Mike/freno-dev#1

Allow edits from maintainers