Skip to content

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

Description

@vivek7405

Problem

The gallery's rate-limit demo does not limit anything per visitor on the deployed site. https://gallery.webjs.dev/features/rate-limit tells the reader the /ping endpoint is "limited to five requests per ten seconds", and invites them to refresh past five times to see the 429. Refreshing never produces one.

The middleware IS running. The live response carries x-ratelimit-limit: 5 and x-ratelimit-remaining: 4, so the scoped middleware.ts is wired and executing. What is wrong is the bucket key.

Measured on 2026-08-11 against the live site:

probe result
8 requests, separate connections all 200, x-ratelimit-remaining stuck at 4 every time
8 requests over ONE keep-alive connection (curl --http1.1 -K urls.txt) 4, 4, 4, 3, 3, 2, 1, 4
8 requests, local webjs start (Node) 4, 3, 2, 1, 0, then 429
8 requests, local bun --bun run start 4, 3, 2, 1, 0, then 429

The framework limiter is correct on both runtimes. The keep-alive row is the diagnosis: counts DO increment, but across several distinct buckets, and a fresh connection usually lands on a new one.

gallery/app/features/rate-limit/ping/middleware.ts calls rateLimit({ window: '10s', max: 5 }) with no trustProxy, so the key comes from clientIp(req, { trustProxy: false }), which returns the framework-stamped socket peer. In production that peer is a Railway edge proxy, not the visitor. The proxy pool has several addresses, each gets its own five-per-ten-seconds allowance, so the effective limit is five times the pool size and no ordinary visitor reaches it.

This is a demo that teaches the wrong thing, and it ships. Since #1371 the scaffold gallery IS this app, so the file is copied into every generated full-stack app. The api template carries the same default independently at packages/cli/lib/api-gallery.js:74.

Design / approach

Set trustProxy: true on the two demo middlewares, and make the demo teach the topology question rather than hiding it.

trustProxy: true is exactly the switch this case exists for: clientIp then reads leftmost X-Forwarded-For, then CF-Connecting-IP, then X-Real-IP, then the stamped peer. Behind Cloudflare plus Railway the leftmost X-Forwarded-For entry is the visitor, so the demo starts limiting per visitor and the page's own prose becomes true.

The comment in the middleware currently says "Keyed by client IP by default", which is misleading in front of any CDN and is the sentence that produced this bug. It should say the default keys on the socket peer, that behind a proxy the socket peer is the proxy, and that trustProxy: true is what moves the key to the visitor and requires a proxy that strips inbound X-Forwarded-For. The docs already carry the full threat model at website/app/docs/rate-limiting/page.ts ("Behind a proxy"), so the demo comment should be short and link the reader there rather than restating it.

Deliberately NOT in scope 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 with its own argument, it affects every trustProxy: true app rather than the demo, and it should be decided in its own issue. This one makes the shipped demo honest.

Implementation notes (for the implementing agent)

Where to edit:

  • gallery/app/features/rate-limit/ping/middleware.ts L8, the single rateLimit({ window: '10s', max: 5, message: ... }) call. Add trustProxy: true. Rewrite the L1-L5 comment per the Design section.
  • packages/cli/lib/api-gallery.js L70-L74, the api template's string-emitted copy of the same middleware. Same change, same comment fix. This is an emitted STRING, so an escaping mistake only shows in a freshly generated app (see the AGENTS.md scaffold-sync rule).
  • gallery/app/features/rate-limit/page.ts L16-L26 holds the "five requests per ten seconds" prose and the "refresh past five times" invitation. It becomes true with the fix, so it needs no edit for correctness, but consider one sentence naming why the demo sets trustProxy: true, since a reader copying this file into their own app inherits the decision.
  • Do NOT change packages/server/src/rate-limit.js. The framework behaves correctly; the local Node and Bun runs above prove it.

Landmines:

  • The obvious verification (curl the deployed url N times) is the one that misled the original report into looking like "the middleware never runs". Verify with a KEEP-ALIVE connection (curl --http1.1 with repeated url = lines in a -K file) so the requests share one peer, and read x-ratelimit-remaining rather than only the status code.
  • Localhost cannot reproduce the bug at all: the peer is always 127.0.0.1, so the demo works locally both before and after. Any test asserting the FIX must synthesize forwarded headers rather than relying on a real socket.
  • WEBJS_NO_TRUST_PROXY=1 OUTRANKS trustProxy: true (Decide whether WEBJS_NO_TRUST_PROXY should override rateLimit trustProxy #1254) and would silently put the demo back on the socket peer. The gallery Railway service has 3 variables today (SESSION_SECRET, AUTH_SECRET, FILE_URL_SECRET) and does not set it. Do not add it. If it is ever set on that service this demo regresses with only a once-per-process warning to show for it.
  • Behind Cloudflare the leftmost X-Forwarded-For is client-appendable, so a visitor can dodge their own bucket. That is acceptable for a demo and is the reason the ordering question is split out, but do not write a comment claiming the demo is spoof-proof.
  • The gallery Railway service watch paths must include gallery/** for the fix to deploy. The blog service's watch paths deliberately exclude packages/*, so a framework-only change skips its deploy; confirm the gallery service actually redeploys on this commit rather than assuming it did (railway redeploy rebuilds the OLD commit).
  • Since feat(gallery): promote scaffold gallery to root gallery/ app #1371 the scaffold gallery is sourced from the repo-root gallery/ app and bundled into the cli tarball at prepack, so this fix reaches generated apps only through a cli release. Verify with npm pack --dry-run --workspace=@webjsdev/cli that the changed file is in the tarball.

Invariants to respect:

  • AGENTS.md scaffold + skill sync: a change to what a generated app should do means the generator, the gallery demo, and the agent skill all have to move together. Generate an app and run generate + boot + webjs check (the generators emit strings).
  • AGENTS.md doc sync: the docs site page for rate limiting is the canonical threat-model surface; keep the demo comment short and pointing at it rather than duplicating it.

Tests + docs surfaces:

  • Unit: packages/server/test/rate-limit/rate-limit.test.js already covers trustProxy key resolution. The new coverage this issue needs is a GALLERY-level assertion that the demo middleware resolves per forwarded client rather than per socket peer, with a counterfactual that fails when trustProxy: true is removed.
  • Bun parity: test/bun/forwarded-trust.mjs is the existing cross-runtime forwarded-trust assertion; extend it if the change touches how the key is resolved (it should not, but confirm).
  • Scaffold: test/scaffolds/** covers generated-app shape; the api template's emitted string needs an assertion that the generated middleware carries trustProxy: true.
  • Docs: website/app/docs/rate-limiting/page.ts "Behind a proxy" section, and check whether website/app/docs/deployment/page.ts should name the demo. packages/cli/templates/.agents/skills/webjs/references/built-ins.md if it shows a rateLimit example.

Acceptance criteria

  • gallery/app/features/rate-limit/ping/middleware.ts sets trustProxy: true, and its comment states that the default keys on the socket peer and what that means behind a CDN
  • packages/cli/lib/api-gallery.js emits the same, verified by generating an app rather than by reading the template string
  • Deployed https://gallery.webjs.dev/features/rate-limit/ping returns 429 on the sixth request within ten seconds from one client, verified over separate connections
  • A test proves the demo middleware buckets by forwarded client IP, with a counterfactual that fails when trustProxy: true is removed
  • Scaffold test asserts the generated api-template middleware carries the option
  • Docs surfaces updated per the list above

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