Skip to content

fix: key the rate-limit demos on the visitor, not the proxy - #1388

Merged
vivek7405 merged 1 commit into
mainfrom
fix/rate-limit-demo-proxy-key
Aug 10, 2026
Merged

fix: key the rate-limit demos on the visitor, not the proxy#1388
vivek7405 merged 1 commit into
mainfrom
fix/rate-limit-demo-proxy-key

Conversation

@vivek7405

Copy link
Copy Markdown
Collaborator

Closes #1387

The gallery's rate-limit card promises five requests per ten seconds and does not deliver one on the deployed site. Its middleware took the default bucket key, which is the socket peer, and behind Cloudflare plus Railway that peer is an edge proxy rather than the visitor. The pool has several addresses, each carrying its own full allowance, so the effective limit was five times the pool size and refreshing never produced a 429.

Nothing about it looked broken, which is why it survived. Every response still carried an X-RateLimit-Remaining that counted down correctly inside its own bucket. The tell only shows over one keep-alive connection, where the requests share a peer.

probe against the live site result
8 requests, separate connections all 200, remaining stuck at 4
8 requests, one keep-alive connection 4, 4, 4, 3, 3, 2, 1, 4
8 requests, local Node and local Bun 4, 3, 2, 1, 0, then 429

What changed

Both demos pass trustProxy: true, so the key is the forwarded client address. The comments now say what the default keys on and what a CDN does to it, because these files are copied into every generated app and the old keyed by client IP by default line is the sentence that made the bug easy to write.

The framework limiter needed no change. It is correct on both runtimes; the local runs above are against the same code.

  • gallery/app/features/rate-limit/ping/middleware.ts
  • packages/cli/lib/api-gallery.js, the api template's emitted copy
  • packages/cli/templates/scripts/clear-gallery.mjs, so gallery:clear sheds the new card-owned test with its card

Test plan

  • Unit N/A, no framework source changed.
  • Integration: gallery/test/rate-limit/rate-limit.test.ts, two tests. One asserts the window itself; the other asserts that a visitor exhausting their window does not refuse a different visitor behind the same proxy, which is the assertion the deployed bug fails.
  • Counterfactual, proven at 7912dc3d: removing trustProxy: true fails the second test on its last assertion while the first still passes. That asymmetry is the point, since a peer-keyed limiter satisfies the single-visitor test too.
  • Scaffold: test/scaffolds/scaffold-gallery.test.js asserts both emitted middlewares carry the option, read from generated bytes rather than from the template source. 13/13 pass.
  • Generated app: scaffolded an api app and confirmed the emitted file strips and parses, since that half is emitted from a string template where a quoting slip only shows downstream.
  • packages/server/test/rate-limit/* plus test/scaffolds/*: 99/99 pass.
  • webjs check clean in gallery and website.
  • Browser / e2e / Bun matrix N/A. No framework source, no client-shipped module, and no runtime-sensitive path is touched; the change is two config options, a script list, docs, and tests.

Docs

  • website/app/docs/rate-limiting/page.ts: the "Behind a proxy" section covered the shared-bucket direction but not the pool direction, which is the one that reads as working. Added it, with the keep-alive diagnostic.
  • .agents/skills/webjs/references/built-ins.md: the option list said the key "defaults to the client IP", which is the misleading phrasing. It now says socket peer and explains when that is not the visitor. The scaffold copy is generated from this file at prepack, so it follows automatically.

Deliberately not here

Whether clientIp should prefer CF-Connecting-IP over the client-appendable leftmost X-Forwarded-For when both are present. That is a framework security question affecting every trustProxy: true app rather than these demos, and it deserves its own issue rather than riding along with a demo fix.

The gallery's rate-limit card promises five requests per ten seconds and did
not deliver one on the deployed site. Its middleware took the default bucket
key, which is the socket peer, and behind Cloudflare plus Railway that peer is
an edge proxy rather than the visitor. The pool has several addresses, each
carrying its own full allowance, so the effective limit was five times the pool
size and refreshing never produced a 429.

Nothing about it looked broken, which is why it survived. Every response still
carried an X-RateLimit-Remaining that counted down correctly inside its own
bucket. The tell only shows over one keep-alive connection, where the requests
share a peer: the count descends there and resets on a fresh connection.

Both demos now pass trustProxy: true, so the key is the forwarded client
address. The comments say what the default keys on and what a CDN does to it,
since this file is copied into every generated app and the old comment's
"keyed by client IP by default" is the sentence that made the bug easy to
write.

The framework limiter needed no change. It behaves correctly on Node and on
Bun locally, where the peer really is the visitor.
@vivek7405 vivek7405 self-assigned this Aug 10, 2026
@vivek7405
vivek7405 merged commit ec610c6 into main Aug 10, 2026
10 checks passed
@vivek7405
vivek7405 deleted the fix/rate-limit-demo-proxy-key branch August 10, 2026 21:37
vivek7405 added a commit that referenced this pull request Aug 10, 2026
`trustProxy: true` was not enough behind a CDN, and the gallery demo proved it
on the live site after #1388 shipped. The default chain starts at the leftmost
X-Forwarded-For entry, which behind Cloudflare is Cloudflare's egress address,
not the visitor. Cloudflare pins an egress IP per connection, so the limiter
handed out one bucket per connection: the count descended correctly for the
page's probe button, which holds one connection, and reset for every fresh one,
so no visitor was ever refused.

rateLimit and clientIp now take a header name. When set it is the only wire
header read, falling back to the stamped peer and then _anon_. A blank value
falls through rather than becoming a key every visitor shares, and a comma
chain is split so a proxy that appends cannot mint a bucket per hop. It needs
trustProxy: true, because naming a header to trust is the trust decision.

The framework does not prefer CF-Connecting-IP on its own. Cloudflare
overwrites that header, which makes it unforgeable behind Cloudflare and
forgeable everywhere else, so preferring it globally would let a client on an
nginx or bare-platform deploy outrank the header the real proxy sets. Which
header is trustworthy is a fact about the topology, so the app states it.

The route-handler demo now reports the socket peer and the forwarded client
side by side. The gap between those two was invisible from outside the app,
which is what made this take two attempts to diagnose.
vivek7405 added a commit that referenced this pull request Aug 10, 2026
`trustProxy: true` was not enough behind a CDN, and the gallery demo proved it
on the live site after #1388 shipped. The default chain starts at the leftmost
X-Forwarded-For entry, which behind Cloudflare is Cloudflare's egress address,
not the visitor. Cloudflare pins an egress IP per connection, so the limiter
handed out one bucket per connection: the count descended correctly for the
page's probe button, which holds one connection, and reset for every fresh one,
so no visitor was ever refused.

rateLimit and clientIp now take a header name. When set it is the only wire
header read, falling back to the stamped peer and then _anon_. A blank value
falls through rather than becoming a key every visitor shares, and a comma
chain is split so a proxy that appends cannot mint a bucket per hop. It needs
trustProxy: true, because naming a header to trust is the trust decision.

The framework does not prefer CF-Connecting-IP on its own. Cloudflare
overwrites that header, which makes it unforgeable behind Cloudflare and
forgeable everywhere else, so preferring it globally would let a client on an
nginx or bare-platform deploy outrank the header the real proxy sets. Which
header is trustworthy is a fact about the topology, so the app states it.

The route-handler demo now reports the socket peer and the forwarded client
side by side. The gap between those two was invisible from outside the app,
which is what made this take two attempts to diagnose.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dogfood: gallery rate-limit demo buckets by proxy IP, so it never limits a visitor

1 participant