fix: let rateLimit name the header carrying the visitor - #1390
Merged
Conversation
`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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1389
#1388 set
trustProxy: trueon the gallery's rate-limit demo and the deployed demo still did not limit. This is the other half, found by measuring the live site instead of inferring from a local run.The default chain starts at the leftmost
X-Forwarded-Forentry, 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.4, 3, 2, 1, 0, then429429That split is the whole diagnosis. It also explains why #1388 looked like a fix: the single-connection path is the one a human clicks.
Supporting measurements from the same session: the app's socket peer is a Railway pool (
100.64.0.15/.19/.3/.4/.16/.11across 8 requests), the service runs ONE instance (20 rapid/__webjs/versionsamples, strictly monotonic uptime), and my egress IP was stable throughout, so neither replicas nor a moving client explain it.What changed
rateLimit({ clientIpHeader })andclientIp(req, { header })name the one forwarded header to trust. When set it is the only wire header read, falling back to the stamped peer then_anon_. A blank value falls through rather than becoming a bucket key every visitor shares, and a comma chain is split so a proxy that appends cannot mint a bucket per hop. It requirestrustProxy: true.The default is unchanged, deliberately. Preferring
CF-Connecting-IPglobally would be a spoofing regression: Cloudflare overwrites that header, so it is unforgeable behind Cloudflare and forgeable everywhere else, and on an nginx or bare-platform deploy a client could send it and outrank theX-Forwarded-Forthe real proxy set. Which header is trustworthy is a fact about the topology, so the app states it.The gallery names
cf-connecting-ip. The api template does not, since a generated app's topology is unknown; its comment explains when to add it.gallery/app/features/route-handler/data/route.tsnow reports the socket peer and the forwarded client side by side. The gap between those two was invisible from outside the app, which is exactly why this took two attempts.Test plan
packages/server/test/rate-limit/rate-limit.test.js, 5 new tests: the named header wins over XFF, case-insensitive matching and chain splitting, missing and blank both fall back to the peer, the option is inert withouttrustProxyand underWEBJS_NO_TRUST_PROXY=1, and one visitor arriving on two CDN egress addresses shares a bucket while a second visitor on the same egress does not. 32/32 pass.test/bun/rate-limit-client-ip.mjsplus its.test.mjswrapper. Both listener shells stamp the peer differently (node header vs Bun WeakMap, IMPORTANT: Bun listener per-request overhead (Request clone + zlib bridge) erodes the win #756) and the fallback rungs are where that shows, so it asserts both stamping forms. Verified green undernodeANDbun 1.3.14.gallery/test/rate-limit/rate-limit.test.ts, a third test: one visitor is limited across connections whatever CDN address they arrive on. Every request in this file now carries anX-Forwarded-Forthat DISAGREES withCF-Connecting-IP, so a test cannot pass by the two agreeing. 3/3 pass.4e583bdf: removingclientIpHeaderfrom the demo fails the new test and the two-visitor test, while the single-visitor test still passes. The survivor is the one whose requests all share a CDN address, which is precisely the blind spot that let fix: key the rate-limit demos on the visitor, not the proxy #1388 look complete.webjs checkclean ongalleryandwebsite.One local caveat, and it is a property of the checkout rather than the change: in a linked worktree a bare
@webjsdev/serverresolves to the PRIMARY checkout, so the new Bun parity test reds locally until the branch is installed. Both runtimes were verified by pointing resolution at this branch'spackages/server; CI builds from the branch and is unaffected.Docs
website/app/docs/rate-limiting/page.ts: a new "Behind a CDN, name the header" section with the symptom, the option, and why the framework will not guess the header, plus the option in the reference list..agents/skills/webjs/references/built-ins.md: same guidance for the agent-facing surface. The scaffold copy is generated from it atprepack.Verification still owed
Railway deploys from
main, so the live proof runs after merge: a429on the sixth request within ten seconds over SEPARATE connections, which is the check #1388 could not pass.