Skip to content

dogfood: rateLimit cannot name the header carrying the visitor, so it buckets the CDN #1389

Description

@vivek7405

Problem

rateLimit({ trustProxy: true }) cannot express which forwarded header actually carries the visitor, so behind Cloudflare it buckets Cloudflare's egress addresses instead of visitors. The gallery's rate-limit demo is the live proof (#1387 set trustProxy: true and the demo still does not limit across connections).

clientIp resolves, in order, leftmost X-Forwarded-For, then CF-Connecting-IP, then X-Real-IP, then the stamped peer (packages/server/src/rate-limit.js, the trustProxy === true && proxyIsTrusted() branch). Behind Cloudflare plus Railway that first entry is Cloudflare's egress address, not the visitor, so CF-Connecting-IP never gets consulted even though it is the header carrying the right value.

Measured against https://gallery.webjs.dev/features/rate-limit/ping after #1388 shipped:

how the requests arrive result
one connection (the page's probe button, a tab refresh) 4, 3, 2, 1, 0, then 429
separate connections scattered remaining, no 429

That split is the signature. Cloudflare pins an egress IP per connection, so one connection is one bucket and a new connection is a new bucket. Two consequences, both wrong: two visitors sharing a Cloudflare egress share a bucket, and one visitor on several connections gets several buckets.

Supporting measurements, same session: the app's socket peer is a Railway pool (100.64.0.15/.19/.3/.4/.16/.11 across 8 requests, via the gallery's own /features/route-handler/data), the service runs ONE instance (20 rapid /__webjs/version samples, strictly monotonic uptime), and the client egress IP was stable throughout, so neither replicas nor a moving client explain it.

Design / approach

Add a clientIpHeader option that names the ONE header to trust, defaulting to today's behaviour.

Do NOT globally reorder the preference to put CF-Connecting-IP first. That header is only trustworthy when Cloudflare is the immediate proxy, because Cloudflare overwrites it. On an app behind nginx, or on bare Railway, a client can send CF-Connecting-IP: anything and it would then outrank the X-Forwarded-For the real proxy set, which is a spoofing regression for every such app. Which header is trustworthy is a property of the deployment topology, so it has to be stated by the app rather than guessed by the framework.

// behind Cloudflare
export default rateLimit({ window: '10s', max: 5, trustProxy: true, clientIpHeader: 'cf-connecting-ip' });

When clientIpHeader is set it is the ONLY forwarded header read, falling back to the stamped peer then _anon_ when absent. Absent the option, resolution is byte-identical to today. WEBJS_NO_TRUST_PROXY=1 keeps outranking everything (#1254), since it can only subtract trust.

The gallery demo then names cf-connecting-ip, which is correct for where it is deployed and is the case a reader is most likely to be in.

Implementation notes (for the implementing agent)

Where to edit:

  • packages/server/src/rate-limit.js: clientIp(req, opts)'s trustProxy branch (the x-forwarded-for / cf-connecting-ip / x-real-ip chain), and rateLimit(opts) where it reads opts.trustProxy and calls clientIp, so the option threads through to the key.
  • packages/server/index.d.ts around L637 and L649: the RateLimitOptions trustProxy field and the exported clientIp(req, opts?) signature.
  • gallery/app/features/rate-limit/ping/middleware.ts: add clientIpHeader: 'cf-connecting-ip', and say in the comment why the deployment picks that header.
  • packages/cli/lib/api-gallery.js (the emitted api middleware): keep the default resolution, mention the option in the comment. A generated app's topology is unknown, so naming a Cloudflare header there would be wrong.
  • gallery/app/features/route-handler/data/route.ts: report the socket peer AND the resolved forwarded client side by side. That is a good demo in its own right and it is the instrument whose absence made this bug opaque from outside.

Landmines:

  • A header name arrives from app config, so normalize case before reading (Headers.get is case-insensitive, but do not assume the caller lowercased it).
  • An empty or whitespace-only header value must fall through to the peer rather than becoming a literal bucket key that every visitor shares.
  • Do NOT read true-client-ip as a second default. Cloudflare only overwrites it on Enterprise plans and passes a client-supplied one through otherwise, so defaulting to it reintroduces the spoof this option exists to avoid.
  • The single-connection case already looks fixed, which is exactly how this hid. Any test or manual check MUST use separate connections, or it proves nothing.
  • Verifying on localhost cannot reproduce any of it: the peer is the visitor there, so the demo passes before and after.

Invariants to respect:

Tests + docs surfaces:

  • packages/server/test/rate-limit/rate-limit.test.js: the named header wins, an absent or blank one falls back to the peer, the option is inert under WEBJS_NO_TRUST_PROXY=1, and default resolution is unchanged.
  • test/bun/ : a cross-runtime assertion that the named header resolves identically on both shells.
  • gallery/test/rate-limit/rate-limit.test.ts: the demo's own key follows CF-Connecting-IP now, with a counterfactual.
  • Docs: website/app/docs/rate-limiting/page.ts, .agents/skills/webjs/references/built-ins.md.

Acceptance criteria

  • rateLimit({ trustProxy: true, clientIpHeader: 'cf-connecting-ip' }) buckets by that header
  • Omitting the option leaves resolution byte-identical to today
  • WEBJS_NO_TRUST_PROXY=1 still outranks it
  • Cross-runtime assertion covers both listener shells
  • https://gallery.webjs.dev/features/rate-limit/ping returns a 429 on the sixth request within ten seconds over separate connections, not only over one
  • Docs and the skill reference state which header to name and why the framework will not guess

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions