Skip to content

Commit 4e583bd

Browse files
committed
fix: let rateLimit name the header carrying the visitor
`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.
1 parent ec610c6 commit 4e583bd

11 files changed

Lines changed: 362 additions & 14 deletions

File tree

.agents/skills/webjs/references/built-ins.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,8 @@ Options: `window` (ms or a string like `'1m'`), `max`, `key` (a string prefix or
113113

114114
**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.
115115

116+
**Behind a CDN, `trustProxy: true` alone is usually still wrong, so name the header: `rateLimit({ trustProxy: true, clientIpHeader: 'cf-connecting-ip' })`.** The default chain starts at the leftmost `X-Forwarded-For` entry, which behind Cloudflare is Cloudflare's EGRESS address rather than the visitor. Cloudflare pins one egress IP per connection, so the symptom is a limiter that counts down correctly for a button that pings on one connection and never refuses anyone who opens a new one. When `clientIpHeader` is set it is the only wire header read (falling back to the peer, then `_anon_`), a blank value falls through rather than becoming a key every visitor shares, and a comma chain is split so an appending proxy cannot mint a bucket per hop. The framework will NOT prefer `CF-Connecting-IP` on its own, because Cloudflare overwrites it, which makes it unforgeable behind Cloudflare and forgeable anywhere else: on an nginx or bare-platform deploy a client could then send it and outrank the header the real proxy set. Name the header YOUR edge sets and overwrites.
117+
116118
## Broadcast
117119

118120
Send data to every WebSocket client connected to a route path, from inside that route's `WS` handler.

gallery/app/features/rate-limit/ping/middleware.ts

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,29 @@
1212
// hands out one bucket per proxy, which multiplies the real limit by the pool
1313
// size. WITH it the key comes from the forwarded client address instead.
1414
//
15-
// The tradeoff is real and worth knowing before copying this line: the proxy
16-
// in front of you MUST strip an inbound X-Forwarded-For before adding its own,
17-
// or a client can forge the header and pick its own bucket. WEBJS_NO_TRUST_PROXY=1
18-
// also outranks this option and puts the limiter back on the socket peer.
15+
// `clientIpHeader` then says WHICH forwarded header carries the visitor, and on
16+
// this deployment it is load-bearing too. Without it the default chain takes the
17+
// leftmost X-Forwarded-For entry, which behind Cloudflare is Cloudflare's EGRESS
18+
// address rather than yours. Cloudflare pins an egress IP per connection, so the
19+
// limiter hands out one bucket per connection: the count descends convincingly
20+
// while you hold one connection open and resets the moment a new one opens,
21+
// which is a limiter that limits nobody. Your address is in CF-Connecting-IP, so
22+
// that is the header this app names.
23+
//
24+
// Copying this into your own app? Name the header YOUR proxy sets, and only
25+
// after checking it cannot be forged past that proxy. Cloudflare overwrites
26+
// CF-Connecting-IP, which is what makes it safe HERE and unsafe on a deploy that
27+
// Cloudflare is not in front of. The same precondition applies to the default
28+
// chain: the proxy MUST strip an inbound X-Forwarded-For before adding its own.
29+
// Serving with nothing in front? Drop both options, since then the socket peer
30+
// IS the visitor. WEBJS_NO_TRUST_PROXY=1 outranks all of it.
1931
// /docs/rate-limiting has the full threat model.
2032
import { rateLimit } from '@webjsdev/server';
2133

2234
export default rateLimit({
2335
window: '10s',
2436
max: 5,
2537
trustProxy: true,
38+
clientIpHeader: 'cf-connecting-ip',
2639
message: 'Slow down: five requests per ten seconds.',
2740
});

gallery/app/features/route-handler/data/route.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,15 @@ export async function GET(req: Request) {
1616
return json({
1717
ok: true,
1818
at: new Date(), // a real Date; richFetch decodes it back to a Date, not a string
19+
// Two addresses, because behind a proxy they are NOT the same and the
20+
// difference is invisible until something depends on it (a rate limiter
21+
// did, and bucketed proxies instead of visitors). `ip` is the socket peer,
22+
// which is the visitor only when the browser connects to you directly.
23+
// `forwardedIp` is what the visitor's own CDN header says, which is what a
24+
// limiter or an audit log wants. Deployed behind Cloudflare and Railway,
25+
// `ip` is a rotating 100.64.x.x router address while `forwardedIp` is you.
1926
ip: clientIp(req),
27+
forwardedIp: clientIp(req, { trustProxy: true, header: 'cf-connecting-ip' }),
2028
requestId: requestId(),
2129
userAgent: headers().get('user-agent') ?? 'unknown',
2230
// cookies() reads the REQUEST cookies. Report how many are present (a

gallery/test/rate-limit/rate-limit.test.ts

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,15 @@ const MAX = 5;
1717
// Each test picks its own visitor addresses. The limiter counts into the global
1818
// in-memory cache store, which outlives a handler instance, so two tests sharing
1919
// an address would share a bucket and the second would start already exhausted.
20-
function ping(handle: (req: Request) => Promise<Response>, forwardedFor: string) {
21-
return testRequest(handle, PING, { headers: { 'x-forwarded-for': forwardedFor } });
20+
// The demo names CF-Connecting-IP, because that is the header carrying the
21+
// visitor on the deployment it runs on. Every request here also carries an
22+
// X-Forwarded-For that DISAGREES, standing in for the CDN egress address the
23+
// real deploy puts there, so a test that passes only because the two agree
24+
// cannot exist.
25+
function ping(handle: (req: Request) => Promise<Response>, visitor: string, cdnEgress = '172.68.1.9') {
26+
return testRequest(handle, PING, {
27+
headers: { 'cf-connecting-ip': visitor, 'x-forwarded-for': cdnEgress },
28+
});
2229
}
2330

2431
test('the demo limits one visitor to five requests per window', async () => {
@@ -58,3 +65,27 @@ test('one visitor exhausting the window does not refuse another behind the same
5865
assert.equal(other.status, 200, 'a different visitor keeps their own window');
5966
assert.equal(other.headers.get('x-ratelimit-remaining'), String(MAX - 1));
6067
});
68+
69+
// The half `trustProxy: true` alone did not deliver, and the one the live site
70+
// disproved (#1389). A CDN gives each connection a different egress address, so
71+
// one visitor opening several connections arrives with several X-Forwarded-For
72+
// values and ONE CF-Connecting-IP. Keyed on XFF that visitor gets a fresh bucket
73+
// per connection and is never refused, which is what shipped and read as working.
74+
//
75+
// Counterfactual, proven at this commit: removing `clientIpHeader` from the
76+
// middleware fails this test at the sixth request AND the two-visitor test
77+
// above, while the single-visitor test still passes. The one that survives is
78+
// the one whose requests all carry the same CDN address, which is exactly the
79+
// blind spot that let the first fix look complete on a real deployment.
80+
test('one visitor is limited across connections, whatever CDN address they arrive on', async () => {
81+
const app = await createRequestHandler({ appDir, dev: true });
82+
const visitor = '203.0.113.30';
83+
84+
for (let i = 1; i <= MAX; i += 1) {
85+
const res = await ping(app.handle, visitor, `172.68.9.${i}`);
86+
assert.equal(res.status, 200, `request ${i} arrives on its own CDN egress address`);
87+
}
88+
89+
const limited = await ping(app.handle, visitor, '172.68.9.99');
90+
assert.equal(limited.status, 429, 'a new CDN egress address does not buy a new window');
91+
});

packages/cli/lib/api-gallery.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,13 @@ export async function writeApiGallery(appDir) {
8181
"// pick its own bucket. Serving with nothing in front? Drop the option, since",
8282
"// then the socket peer IS the visitor. WEBJS_NO_TRUST_PROXY=1 outranks it either",
8383
"// way. https://webjs.dev/docs/rate-limiting has the full threat model.",
84+
"//",
85+
"// Behind a CDN, add `clientIpHeader` to name the header carrying the visitor,",
86+
"// e.g. `clientIpHeader: 'cf-connecting-ip'` behind Cloudflare. The default",
87+
"// chain reads the leftmost X-Forwarded-For entry, which behind a CDN is the",
88+
"// CDN's egress address; those are pinned per connection, so the limiter ends up",
89+
"// handing out a bucket per connection and refusing nobody. It is left unset",
90+
"// here because the right header depends on what you deploy behind.",
8491
"import { rateLimit } from '@webjsdev/server';",
8592
"",
8693
"export default rateLimit({",

packages/server/index.d.ts

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -635,6 +635,21 @@ export declare function rateLimit(opts?: {
635635
* that STRIPS inbound `X-Forwarded-For` before adding its own.
636636
*/
637637
trustProxy?: boolean;
638+
/**
639+
* Name the ONE forwarded header that carries the visitor, e.g.
640+
* `'cf-connecting-ip'` behind Cloudflare. Requires `trustProxy: true`, and
641+
* when set it is the only wire header consulted (falling back to the stamped
642+
* peer, then `_anon_`).
643+
*
644+
* Behind a CDN this is usually required for the limiter to work at all. The
645+
* default chain takes the leftmost `X-Forwarded-For` entry, which behind
646+
* Cloudflare is Cloudflare's EGRESS address, not the visitor, and since that
647+
* is pinned per connection the limiter hands out one bucket per connection.
648+
* The framework will not prefer `CF-Connecting-IP` on its own: Cloudflare
649+
* overwrites it, so it is unforgeable behind Cloudflare and forgeable
650+
* anywhere else, which makes the right choice a property of your topology.
651+
*/
652+
clientIpHeader?: string;
638653
}): Middleware;
639654
/** Parse a window string (`'1m'`, `'30s'`) to milliseconds. */
640655
export declare function parseWindow(w: number | string): number;
@@ -645,8 +660,17 @@ export declare function parseWindow(w: number | string): number;
645660
* `trustProxy: true` reads the forwarded-IP headers instead, UNLESS
646661
* `WEBJS_NO_TRUST_PROXY=1` is set, which overrides the option back to the
647662
* stamped peer and logs once per process.
663+
*
664+
* `header` names the one forwarded header to trust (`'cf-connecting-ip'`
665+
* behind Cloudflare) and, when given, is the only wire header read. Use it
666+
* whenever a CDN sits in front: the default chain's leftmost
667+
* `X-Forwarded-For` entry is then the CDN's egress address rather than the
668+
* visitor.
648669
*/
649-
export declare function clientIp(req: Request, opts?: { trustProxy?: boolean }): string;
670+
export declare function clientIp(
671+
req: Request,
672+
opts?: { trustProxy?: boolean; header?: string },
673+
): string;
650674
/** Stamp the socket remote address onto a request for `clientIp` to read. */
651675
export declare function stampRemoteIp(req: Request, remoteAddress: string): void;
652676

packages/server/src/rate-limit.js

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ let warnedProxyOverride = false;
4040
* message?: string,
4141
* store?: import('./cache.js').CacheStore,
4242
* trustProxy?: boolean,
43+
* clientIpHeader?: string,
4344
* }} opts
4445
* @returns {(req: Request, next: () => Promise<Response>) => Promise<Response>}
4546
*/
@@ -50,13 +51,17 @@ export function rateLimit(opts = {}) {
5051
const keyPrefix = typeof opts.key === 'string' ? opts.key : '';
5152
const message = opts.message ?? 'Too Many Requests';
5253
const trustProxy = opts.trustProxy === true;
54+
// The header carrying the visitor, when the app knows which one that is.
55+
// Inert without `trustProxy: true`, since naming a wire header to trust IS
56+
// the trust decision and must not be grantable by a second option.
57+
const header = typeof opts.clientIpHeader === 'string' ? opts.clientIpHeader : undefined;
5358
// Use the provided store, or fall back to the global cache store.
5459
// Whatever was set via `setStore()` at app startup (in-memory by default).
5560
const store = opts.store || null;
5661

5762
return async function rateLimitMiddleware(req, next) {
5863
const s = store || getStore();
59-
const raw = keyFn ? await keyFn(req) : clientIp(req, { trustProxy });
64+
const raw = keyFn ? await keyFn(req) : clientIp(req, { trustProxy, header });
6065
const key = `rl:${keyPrefix}${raw}`;
6166

6267
const count = await s.increment(key, windowMs);
@@ -185,10 +190,42 @@ export function propagateTrustedRemoteIp(src, dst) {
185190
* leaving them in disagreement buckets every visitor behind that proxy onto
186191
* one key.
187192
*
193+
* `header` names the ONE forwarded header to trust, and it is the option a
194+
* CDN deployment needs (#1389). The default chain reads the leftmost
195+
* `X-Forwarded-For` entry first, which behind Cloudflare is CLOUDFLARE'S
196+
* EGRESS address rather than the visitor: Cloudflare pins an egress IP per
197+
* connection, so a limiter keyed on it gives one bucket per connection, which
198+
* counts down convincingly and limits nobody. The visitor is in
199+
* `CF-Connecting-IP`, and naming it here is how the app says so.
200+
*
201+
* The framework does NOT reorder the default chain to prefer that header,
202+
* because which header is trustworthy is a property of the TOPOLOGY, not of
203+
* the framework. Cloudflare overwrites `CF-Connecting-IP`, so it is
204+
* unforgeable behind Cloudflare and forgeable everywhere else; preferring it
205+
* globally would let a client on an nginx or bare-Railway deploy outrank the
206+
* `X-Forwarded-For` the real proxy set. So the app names its header and owns
207+
* the claim. When `header` is set it is the only forwarded header consulted,
208+
* falling back to the stamped peer and then `_anon_`, and a blank value falls
209+
* through rather than becoming a shared literal key.
210+
*
188211
* @param {Request} req
189-
* @param {{ trustProxy?: boolean }} [opts]
212+
* @param {{ trustProxy?: boolean, header?: string }} [opts]
190213
* @returns {string}
191214
*/
215+
/**
216+
* First entry of a forwarded-IP header, trimmed, or `''` when there is nothing
217+
* usable. A blank value must FALL THROUGH rather than resolve: an empty string
218+
* as a bucket key is one key shared by every visitor whose proxy sent the
219+
* header empty, which is a limiter that throttles strangers together.
220+
*
221+
* @param {string | null | undefined} raw
222+
* @returns {string}
223+
*/
224+
function firstForwardedEntry(raw) {
225+
if (!raw) return '';
226+
return raw.split(',')[0].trim();
227+
}
228+
192229
export function clientIp(req, opts = {}) {
193230
if (opts.trustProxy === true && !proxyIsTrusted() && !warnedProxyOverride) {
194231
warnedProxyOverride = true;
@@ -201,10 +238,18 @@ export function clientIp(req, opts = {}) {
201238
);
202239
}
203240
if (opts.trustProxy === true && proxyIsTrusted()) {
241+
if (opts.header) {
242+
// One named header, and nothing else from the wire. A chain is still
243+
// split on the comma so a proxy that appends to the named header cannot
244+
// turn the key into a growing string, which would mint a fresh bucket per
245+
// hop and reproduce the very failure this option exists to fix.
246+
const named = req.headers.get(String(opts.header).toLowerCase());
247+
return firstForwardedEntry(named) || trustedRemoteIp(req) || '_anon_';
248+
}
204249
return (
205-
req.headers.get('x-forwarded-for')?.split(',')[0].trim() ||
206-
req.headers.get('cf-connecting-ip') ||
207-
req.headers.get('x-real-ip') ||
250+
firstForwardedEntry(req.headers.get('x-forwarded-for')) ||
251+
req.headers.get('cf-connecting-ip')?.trim() ||
252+
req.headers.get('x-real-ip')?.trim() ||
208253
trustedRemoteIp(req) ||
209254
'_anon_'
210255
);

packages/server/test/rate-limit/rate-limit.test.js

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,3 +385,86 @@ test('WEBJS_NO_TRUST_PROXY=1: the default path (no option) is unchanged (#1254)'
385385
assert.equal(clientIp(req), '9.9.9.9', 'the default path must still read only the stamped peer');
386386
});
387387
});
388+
389+
/* ------------------ clientIpHeader: naming the visitor's header ------------------ */
390+
391+
test('clientIpHeader reads ONLY the named header (#1389)', async () => {
392+
const { clientIp } = await import('../../src/rate-limit.js');
393+
// The shape a Cloudflare deploy actually receives: XFF's leftmost entry is
394+
// the CDN's own egress address, and the visitor is in CF-Connecting-IP. The
395+
// default chain takes the wrong one of the two, which is the whole bug.
396+
const req = new Request('http://x/', {
397+
headers: {
398+
'x-forwarded-for': '172.68.1.9, 100.64.0.3',
399+
'cf-connecting-ip': '203.0.113.44',
400+
'x-webjs-remote-ip': '100.64.0.3',
401+
},
402+
});
403+
assert.equal(clientIp(req, { trustProxy: true }), '172.68.1.9', 'default chain still prefers XFF leftmost');
404+
assert.equal(
405+
clientIp(req, { trustProxy: true, header: 'cf-connecting-ip' }),
406+
'203.0.113.44',
407+
'the named header wins over XFF',
408+
);
409+
});
410+
411+
test('clientIpHeader is matched case-insensitively and splits a chain', async () => {
412+
const { clientIp } = await import('../../src/rate-limit.js');
413+
const req = new Request('http://x/', {
414+
headers: { 'cf-connecting-ip': ' 203.0.113.44 , 10.0.0.1 ', 'x-webjs-remote-ip': '100.64.0.3' },
415+
});
416+
// A proxy that APPENDS to the named header must not mint a new bucket per
417+
// hop, which is the failure mode the option exists to end.
418+
assert.equal(clientIp(req, { trustProxy: true, header: 'CF-Connecting-IP' }), '203.0.113.44');
419+
});
420+
421+
test('a missing or blank named header falls back to the peer, never to a shared key', async () => {
422+
const { clientIp } = await import('../../src/rate-limit.js');
423+
const missing = new Request('http://x/', { headers: { 'x-webjs-remote-ip': '100.64.0.3' } });
424+
assert.equal(clientIp(missing, { trustProxy: true, header: 'cf-connecting-ip' }), '100.64.0.3');
425+
426+
// A blank value resolving to '' would be ONE key shared by every visitor
427+
// whose proxy sent the header empty, throttling strangers together.
428+
const blank = new Request('http://x/', {
429+
headers: { 'cf-connecting-ip': ' ', 'x-webjs-remote-ip': '100.64.0.3' },
430+
});
431+
assert.equal(clientIp(blank, { trustProxy: true, header: 'cf-connecting-ip' }), '100.64.0.3');
432+
433+
const nothing = new Request('http://x/', { headers: { 'cf-connecting-ip': '' } });
434+
assert.equal(clientIp(nothing, { trustProxy: true, header: 'cf-connecting-ip' }), '_anon_');
435+
});
436+
437+
test('clientIpHeader is inert without trustProxy, and under WEBJS_NO_TRUST_PROXY=1', async () => {
438+
const { clientIp } = await import('../../src/rate-limit.js');
439+
const req = new Request('http://x/', {
440+
headers: { 'cf-connecting-ip': '203.0.113.44', 'x-webjs-remote-ip': '100.64.0.3' },
441+
});
442+
// Naming a wire header to trust IS the trust decision, so it must not be
443+
// grantable by a second option that skips the first.
444+
assert.equal(clientIp(req, { header: 'cf-connecting-ip' }), '100.64.0.3');
445+
await withNoTrustProxy('1', () => {
446+
assert.equal(clientIp(req, { trustProxy: true, header: 'cf-connecting-ip' }), '100.64.0.3');
447+
});
448+
});
449+
450+
test('rateLimit buckets by the named header, not by the CDN egress address', async () => {
451+
const mw = rateLimit({ window: '1s', max: 1, trustProxy: true, clientIpHeader: 'cf-connecting-ip' });
452+
// Two requests from ONE visitor that arrive through DIFFERENT CDN egress
453+
// addresses, which is what a fresh connection produces. They must share a
454+
// bucket; keyed on XFF they would not, and the visitor would never be limited.
455+
const first = new Request('http://x/', {
456+
headers: { 'x-forwarded-for': '172.68.1.9', 'cf-connecting-ip': '203.0.113.44' },
457+
});
458+
const second = new Request('http://x/', {
459+
headers: { 'x-forwarded-for': '172.69.7.2', 'cf-connecting-ip': '203.0.113.44' },
460+
});
461+
assert.equal((await mw(first, async () => new Response('ok'))).status, 200);
462+
assert.equal((await mw(second, async () => new Response('ok'))).status, 429, 'same visitor, second egress');
463+
464+
// And a genuinely different visitor arriving through the SAME egress keeps
465+
// their own window, which is the other half of bucketing correctly.
466+
const other = new Request('http://x/', {
467+
headers: { 'x-forwarded-for': '172.68.1.9', 'cf-connecting-ip': '198.51.100.7' },
468+
});
469+
assert.equal((await mw(other, async () => new Response('ok'))).status, 200);
470+
});

0 commit comments

Comments
 (0)