From 7912dc3da5e7c027b76186d63b1ba3e7d9b1bdca Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 11 Aug 2026 03:07:04 +0530 Subject: [PATCH] fix: key the rate-limit demos on the visitor, not the proxy 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. --- .agents/skills/webjs/references/built-ins.md | 4 +- .../features/rate-limit/ping/middleware.ts | 27 +++++++-- gallery/test/rate-limit/rate-limit.test.ts | 60 +++++++++++++++++++ packages/cli/lib/api-gallery.js | 19 +++++- .../cli/templates/scripts/clear-gallery.mjs | 9 ++- test/scaffolds/scaffold-gallery.test.js | 10 ++++ website/app/docs/rate-limiting/page.ts | 2 + 7 files changed, 122 insertions(+), 9 deletions(-) create mode 100644 gallery/test/rate-limit/rate-limit.test.ts diff --git a/.agents/skills/webjs/references/built-ins.md b/.agents/skills/webjs/references/built-ins.md index 16bb00f7d..a82cf44f9 100644 --- a/.agents/skills/webjs/references/built-ins.md +++ b/.agents/skills/webjs/references/built-ins.md @@ -109,7 +109,9 @@ import { rateLimit } from '@webjsdev/server'; export default rateLimit({ window: '1m', max: 60 }); ``` -Options: `window` (ms or a string like `'1m'`), `max`, `key` (a string prefix or a `(req) => string` function, defaults to the client IP), `message`, `store`, `trustProxy` (honour the forwarded-IP headers; inert while `WEBJS_NO_TRUST_PROXY=1` is set, which outranks it and keeps the limiter on the framework-stamped peer). Over-limit responds `429` with `Retry-After` and `X-RateLimit-*` headers; an allowed response carries the remaining-quota headers too. For multi-instance scaling, set the global store to Redis once at startup. +Options: `window` (ms or a string like `'1m'`), `max`, `key` (a string prefix or a `(req) => string` function, defaults to the framework-stamped socket peer), `message`, `store`, `trustProxy` (honour the forwarded-IP headers; inert while `WEBJS_NO_TRUST_PROXY=1` is set, which outranks it and keeps the limiter on the framework-stamped peer). Over-limit responds `429` with `Retry-After` and `X-RateLimit-*` headers; an allowed response carries the remaining-quota headers too. For multi-instance scaling, set the global store to Redis once at startup. + +**The default key is the socket PEER, which is the visitor only when the browser connects to you directly.** Deploy behind a CDN or a platform router and the peer is that proxy, so `trustProxy: true` is what a deployed limiter almost always wants. Get it wrong and nothing looks broken: a single shared proxy buckets every visitor together, and a proxy POOL (the common case) hands out one full allowance PER proxy, so the effective limit is multiplied by the pool size while `X-RateLimit-Remaining` still counts down convincingly inside each bucket. Diagnose it by sending the requests over ONE keep-alive connection, which pins them to one peer: counts that descend there but reset on a fresh connection mean you are bucketing proxies. `trustProxy: true` has one precondition, that the proxy in front strips an inbound `X-Forwarded-For` before adding its own (Cloudflare, Railway, Fly, Render, and Vercel do; nginx and Caddy only if configured), or a client can forge the header and choose its own bucket. ## Broadcast diff --git a/gallery/app/features/rate-limit/ping/middleware.ts b/gallery/app/features/rate-limit/ping/middleware.ts index 1b029c1a6..f858c621f 100644 --- a/gallery/app/features/rate-limit/ping/middleware.ts +++ b/gallery/app/features/rate-limit/ping/middleware.ts @@ -1,8 +1,27 @@ // Per-segment middleware. It sits in the ping/ folder, so it applies ONLY to // /features/rate-limit/ping (its route.ts), not to the demo page one level up. -// rateLimit() returns a standard webjs middleware: return a Response to -// short-circuit (the 429), or call next() to continue. Keyed by client IP by -// default; pass `key` to key by user id, API key, etc. +// rateLimit() returns a standard WebJs middleware: return a Response to +// short-circuit (the 429), or call next() to continue. Pass `key` to bucket by +// user id, API key, or anything else instead of by IP. +// +// `trustProxy: true` is the load-bearing option here, and it is why this demo +// works on the deployed site. WITHOUT it the bucket key is the socket peer, +// which is correct only when the visitor's browser is the thing connecting. +// Behind a CDN or a platform router the peer is that proxy, so every visitor +// sharing one proxy shares one bucket, and (worse for a limiter) a proxy POOL +// hands out one bucket per proxy, which multiplies the real limit by the pool +// size. WITH it the key comes from the forwarded client address instead. +// +// The tradeoff is real and worth knowing before copying this line: the proxy +// in front of you MUST strip an inbound X-Forwarded-For before adding its own, +// or a client can forge the header and pick its own bucket. WEBJS_NO_TRUST_PROXY=1 +// also outranks this option and puts the limiter back on the socket peer. +// /docs/rate-limiting has the full threat model. import { rateLimit } from '@webjsdev/server'; -export default rateLimit({ window: '10s', max: 5, message: 'Slow down: five requests per ten seconds.' }); +export default rateLimit({ + window: '10s', + max: 5, + trustProxy: true, + message: 'Slow down: five requests per ten seconds.', +}); diff --git a/gallery/test/rate-limit/rate-limit.test.ts b/gallery/test/rate-limit/rate-limit.test.ts new file mode 100644 index 000000000..2e50f30ad --- /dev/null +++ b/gallery/test/rate-limit/rate-limit.test.ts @@ -0,0 +1,60 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { fileURLToPath } from 'node:url'; +import { dirname, resolve } from 'node:path'; + +import { createRequestHandler } from '@webjsdev/server'; +import { testRequest } from '@webjsdev/server/testing'; + +const appDir = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..'); + +const PING = '/features/rate-limit/ping'; + +// The demo's own numbers, so a change to the middleware that these tests do not +// notice is a change that made them stale rather than one they tolerated. +const MAX = 5; + +// Each test picks its own visitor addresses. The limiter counts into the global +// in-memory cache store, which outlives a handler instance, so two tests sharing +// an address would share a bucket and the second would start already exhausted. +function ping(handle: (req: Request) => Promise, forwardedFor: string) { + return testRequest(handle, PING, { headers: { 'x-forwarded-for': forwardedFor } }); +} + +test('the demo limits one visitor to five requests per window', async () => { + const app = await createRequestHandler({ appDir, dev: true }); + const visitor = '203.0.113.10'; + + for (let i = 1; i <= MAX; i += 1) { + const res = await ping(app.handle, visitor); + assert.equal(res.status, 200, `request ${i} is inside the window`); + assert.equal(res.headers.get('x-ratelimit-remaining'), String(MAX - i)); + } + + const limited = await ping(app.handle, visitor); + assert.equal(limited.status, 429, 'the sixth request is refused'); + assert.equal(limited.headers.get('retry-after'), '10'); +}); + +// This is the assertion the deployed bug would have failed. Both visitors reach +// the app through the same proxy, so the socket peer is identical for both and a +// peer-keyed limiter would count them into ONE bucket: exhausting the first +// would refuse the second. Keying on the forwarded address keeps them apart. +// +// Counterfactual, proven at this commit: removing `trustProxy: true` from +// gallery/app/features/rate-limit/ping/middleware.ts fails this test on the last +// assertion (the second visitor gets a 429), while the single-visitor test above +// still passes. That asymmetry is the point, since the single-visitor test is +// what a peer-keyed limiter satisfies too. +test('one visitor exhausting the window does not refuse another behind the same proxy', async () => { + const app = await createRequestHandler({ appDir, dev: true }); + const noisy = '203.0.113.20'; + const bystander = '203.0.113.21'; + + for (let i = 0; i < MAX; i += 1) await ping(app.handle, noisy); + assert.equal((await ping(app.handle, noisy)).status, 429, 'the noisy visitor is limited'); + + const other = await ping(app.handle, bystander); + assert.equal(other.status, 200, 'a different visitor keeps their own window'); + assert.equal(other.headers.get('x-ratelimit-remaining'), String(MAX - 1)); +}); diff --git a/packages/cli/lib/api-gallery.js b/packages/cli/lib/api-gallery.js index 468de0b53..a78d90111 100644 --- a/packages/cli/lib/api-gallery.js +++ b/packages/cli/lib/api-gallery.js @@ -69,9 +69,26 @@ export async function writeApiGallery(appDir) { "// Per-segment middleware: it sits beside this route, so it rate-limits ONLY", "// /api/features/rate-limit. rateLimit() is backed by the pluggable cache store", "// (in-memory by default; point it at Redis to share the window across nodes).", + "//", + "// `trustProxy: true` decides WHAT gets counted. Without it the bucket key is", + "// the socket peer, which is the visitor only when the browser connects to you", + "// directly. Behind a CDN or a platform router the peer is that proxy, so a", + "// proxy POOL hands out one bucket per proxy and multiplies your real limit by", + "// the pool size. With it the key is the forwarded client address instead.", + "//", + "// It has a precondition: the proxy in front MUST strip an inbound", + "// X-Forwarded-For before adding its own, or a client can forge the header and", + "// pick its own bucket. Serving with nothing in front? Drop the option, since", + "// then the socket peer IS the visitor. WEBJS_NO_TRUST_PROXY=1 outranks it either", + "// way. https://webjs.dev/docs/rate-limiting has the full threat model.", "import { rateLimit } from '@webjsdev/server';", "", - "export default rateLimit({ window: '10s', max: 5, message: 'Slow down: five requests per ten seconds.' });", + "export default rateLimit({", + " window: '10s',", + " max: 5,", + " trustProxy: true,", + " message: 'Slow down: five requests per ten seconds.',", + "});", "", ].join('\n')); await writeFile(feat('rate-limit', 'route.ts'), [ diff --git a/packages/cli/templates/scripts/clear-gallery.mjs b/packages/cli/templates/scripts/clear-gallery.mjs index 89dbf81f9..4cada7026 100644 --- a/packages/cli/templates/scripts/clear-gallery.mjs +++ b/packages/cli/templates/scripts/clear-gallery.mjs @@ -53,10 +53,13 @@ if (!existsSync(join(root, 'app/features'))) { // 1) Gallery route trees + example metadata routes. `app/api/auth` is the auth // card's createAuth handler (it lives at the app root, not under app/features/, -// because createAuth hardcodes /api/auth/*), and `test/auth` is the auth card's -// request-pipeline test, so both are removed here alongside the card. +// because createAuth hardcodes /api/auth/*), and `test/auth` + `test/rate-limit` +// are card-owned request-pipeline tests, so they are removed alongside their +// cards. A card that ships a test under test/ MUST be listed here: the prune +// below only removes test/ once it is EMPTY, so a missed entry silently leaves +// the reset app with a test suite for a card it no longer has. const galleryPaths = [ - 'app/features', 'app/examples', 'app/sitemaps', 'app/api/auth', 'test/auth', + 'app/features', 'app/examples', 'app/sitemaps', 'app/api/auth', 'test/auth', 'test/rate-limit', 'app/icon.ts', 'app/apple-icon.ts', 'app/manifest.ts', 'app/opengraph-image.ts', 'app/twitter-image.ts', 'app/robots.ts', 'app/sitemap.ts', 'app/global-error.ts', 'app/global-not-found.ts', diff --git a/test/scaffolds/scaffold-gallery.test.js b/test/scaffolds/scaffold-gallery.test.js index d80bad5b3..f40f9114b 100644 --- a/test/scaffolds/scaffold-gallery.test.js +++ b/test/scaffolds/scaffold-gallery.test.js @@ -81,6 +81,12 @@ test('full-stack scaffold ships feature demos and one example app', async () => assert.ok(await exists(join(appDir, 'app', 'features', 'broadcast', 'feed', 'route.ts'))); assert.ok(await exists(join(appDir, 'app', 'features', 'rate-limit', 'ping', 'route.ts'))); assert.ok(await exists(join(appDir, 'app', 'features', 'rate-limit', 'ping', 'middleware.ts'))); + // The limiter must key on the FORWARDED client, not the socket peer, or the + // demo counts proxies instead of visitors the moment the app is deployed + // behind anything (#1387). This is an emitted file, so assert the generated + // bytes rather than trusting the source it was copied from. + const rateLimitMw = await readFile(join(appDir, 'app', 'features', 'rate-limit', 'ping', 'middleware.ts'), 'utf8'); + assert.match(rateLimitMw, /trustProxy:\s*true/, 'gallery rate-limit demo trusts the proxy'); assert.ok(await exists(join(appDir, 'app', 'features', 'file-storage', 'file', '[key]', 'route.ts'))); // Root-only boundaries + metadata image routes (the convention-file demos). for (const f of ['global-error.ts', 'global-not-found.ts', 'icon.ts', 'apple-icon.ts', 'opengraph-image.ts', 'twitter-image.ts']) { @@ -318,6 +324,10 @@ test('the api template ships the backend-features showcase, not the UI gallery', assert.ok(await exists(join(appDir, 'app', 'api', 'features', name, 'route.ts')), `api backend demo ${name}`); } assert.ok(await exists(join(appDir, 'app', 'api', 'features', 'rate-limit', 'middleware.ts')), 'rate-limit middleware'); + // Same requirement as the UI gallery's copy (#1387), and this one is emitted + // from a string template, so a quoting slip only shows in generated bytes. + const apiRateLimitMw = await readFile(join(appDir, 'app', 'api', 'features', 'rate-limit', 'middleware.ts'), 'utf8'); + assert.match(apiRateLimitMw, /trustProxy:\s*true/, 'api rate-limit demo trusts the proxy'); assert.ok(await exists(join(appDir, 'app', 'api', 'features', 'files', '[key]', 'route.ts')), 'file serve route'); assert.ok(await exists(join(appDir, 'modules', 'widgets', 'actions', 'create-widget.server.ts')), 'widgets action'); assert.ok(await exists(join(appDir, 'env.ts')), 'env-validation demo at the app root'); diff --git a/website/app/docs/rate-limiting/page.ts b/website/app/docs/rate-limiting/page.ts index 91ff701ea..226f20c1a 100644 --- a/website/app/docs/rate-limiting/page.ts +++ b/website/app/docs/rate-limiting/page.ts @@ -45,6 +45,8 @@ export default rateLimit({ window: '1m', max: 10 });

When you're fronted by a reverse proxy or CDN (Cloudflare, nginx, Caddy, Railway, Fly, Render, Vercel, Heroku), the socket IP is the proxy, not the user. Every request shares the same IP and the limiter buckets everyone together. Opt in to forwarded-header parsing:

+

A proxy POOL fails the other way, and it is the failure you are more likely to hit, because it does not look like a failure at all. Each proxy in the pool is a separate peer, so each gets its own full allowance and your effective limit is the configured one multiplied by the pool size. The headers stay plausible throughout: every response carries a X-RateLimit-Remaining that counts down correctly for its own bucket, so the limiter reads as working while no visitor is ever refused. The tell is that a fresh connection restarts the count while requests sharing one keep-alive connection do count down. This is what shipped in the feature gallery's rate-limit demo, which is why that demo now sets trustProxy: true.

+ // app/api/auth/middleware.ts import { rateLimit } from '@webjsdev/server';