Skip to content

Commit 2124ad9

Browse files
committed
fix: hold the report Worker's overall limit across Cloudflare locations
The rate-limiting binding counts separately in each location, so the "all" key allowed 30 reports a minute per location, not per Worker. The Worker-wide budget is now one Durable Object (the instance named "all") that every location asks. It's checked before the client is charged and charged only after the client and group budgets allow, so a report the Worker can't take no longer spends the client's budget, and one client hammering its own limit can't drain everyone else's. IPv4 clients now share a group budget by /24, as IPv6 clients already do by /48. The test "one per-minute budget across every network" drew its 30 clients from one /24, which the new group budget refuses at 21, so it now uses one client per /24; the behavior it checks is unchanged. The KV and rate-limit-binding fallbacks stay for a deployment without the counter. One commit per report is unchanged.
1 parent 1ba2233 commit 2124ad9

3 files changed

Lines changed: 241 additions & 26 deletions

File tree

‎worker/community-reports-worker.js‎

Lines changed: 100 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -174,9 +174,10 @@ export function sanitizeSmsReportFields(body) {
174174
// expiration TTL equal to the window so stale keys self-clean.
175175
const RATE_LIMIT_WINDOW_S = 60;
176176
const RATE_LIMIT_MAX_REQUESTS = 5;
177-
// A carrier hands each phone its own /64 out of a shared /48, so the /48 gets
178-
// a larger budget than one client, and the whole Worker gets one per minute so
179-
// a flood spread over many networks can't turn into a flood of GitHub commits.
177+
// A carrier hands each phone its own /64 out of a shared /48, so the /48 (an
178+
// IPv4 /24) gets a larger budget than one client, and the whole Worker gets one
179+
// per minute so a flood spread over many networks can't turn into a flood of
180+
// GitHub commits.
180181
const GROUP_RATE_LIMIT_MAX_REQUESTS = 20;
181182
const GLOBAL_RATE_LIMIT_MAX_REQUESTS = 30;
182183

@@ -347,16 +348,36 @@ export async function readBoundedText(request, limit = MAX_BODY_BYTES) {
347348
}
348349

349350
/**
350-
* Check the per-client, per-/48 and Worker-wide rate limits, in that order.
351-
* Returns { allowed: boolean, remaining: number, retryAfter: number } for the
352-
* first budget that refuses, or for the client budget when all allow.
351+
* The network a client's group budget keys on: an IPv6 /48 (a carrier hands
352+
* each phone a /64 out of a shared /48) or an IPv4 /24, so a handful of
353+
* addresses in one block can't each spend a full client budget.
354+
*/
355+
export function limiterGroupKey(ip) {
356+
const key = clientKey(ip, REPORTER_PREFIX_BITS);
357+
const octets = key.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.\d{1,3}$/);
358+
return octets ? `${octets[1]}.${octets[2]}.${octets[3]}.0/24` : key;
359+
}
360+
361+
/**
362+
* Check the Worker-wide, per-client and per-group rate limits. Returns
363+
* { allowed: boolean, remaining: number, retryAfter: number } for the first
364+
* budget that refuses, or for the client budget when all allow.
365+
*
366+
* The Worker-wide budget is the REPORT_GLOBAL_COUNTER Durable Object when it
367+
* is bound. A rate-limiting binding counts separately in every Cloudflare
368+
* location, so it can't hold one budget for the whole Worker; the single
369+
* object can. Its budget is checked before the client is charged, so a report
370+
* the Worker can't take doesn't spend the client's budget, and it is charged
371+
* only after the client and group budgets allow, so one client hammering its
372+
* own limit can't drain everyone else's.
353373
*
354374
* When the REPORT_LIMITER rate-limiting binding is bound it is authoritative:
355375
* the platform evaluates its counter atomically per key, so a burst of
356376
* concurrent requests cannot slip past the cap the way it can between the KV
357377
* counter's read and write (KV is eventually consistent and non-atomic).
358378
* The KV counter remains as a best-effort fallback for a deployment that has
359-
* not provisioned the limiter binding yet.
379+
* not provisioned the limiter binding yet. Without the Durable Object the
380+
* Worker-wide budget falls back the same way and is charged last.
360381
*
361382
* Missing bindings fail closed unless a local test explicitly sets
362383
* ALLOW_UNLIMITED_REPORTS=true. Production must never set that flag.
@@ -366,25 +387,90 @@ export async function checkRateLimit(ip, env) {
366387
return { allowed: false, remaining: 0, retryAfter: 0, identityError: true };
367388
}
368389
const client = clientKey(ip, LIMITER_PREFIX_BITS);
390+
const group = limiterGroupKey(ip);
369391
const budgets = [
370392
{ limiter: env?.REPORT_LIMITER, key: client, kvKey: `rl:${client}`, max: RATE_LIMIT_MAX_REQUESTS },
371-
{
372-
limiter: env?.REPORT_GROUP_LIMITER,
373-
key: clientKey(ip, REPORTER_PREFIX_BITS),
374-
kvKey: `rlg:${clientKey(ip, REPORTER_PREFIX_BITS)}`,
375-
max: GROUP_RATE_LIMIT_MAX_REQUESTS,
376-
},
377-
{ limiter: env?.REPORT_GLOBAL_LIMITER, key: "all", kvKey: "rlall", max: GLOBAL_RATE_LIMIT_MAX_REQUESTS },
393+
{ limiter: env?.REPORT_GROUP_LIMITER, key: group, kvKey: `rlg:${group}`, max: GROUP_RATE_LIMIT_MAX_REQUESTS },
378394
];
395+
const counter = env?.REPORT_GLOBAL_COUNTER;
396+
if (counter) {
397+
const open = await askGlobalCounter(counter, "peek");
398+
if (!open.allowed) return open;
399+
} else {
400+
budgets.push({ limiter: env?.REPORT_GLOBAL_LIMITER, key: "all", kvKey: "rlall", max: GLOBAL_RATE_LIMIT_MAX_REQUESTS });
401+
}
379402
let first;
380403
for (const budget of budgets) {
381404
const outcome = await consumeBudget(budget, env);
382405
if (!outcome.allowed) return outcome;
383406
first ??= outcome;
384407
}
408+
if (counter) {
409+
const taken = await askGlobalCounter(counter, "take");
410+
if (!taken.allowed) return taken;
411+
}
385412
return first;
386413
}
387414

415+
/** Peek at or take from the Worker-wide budget held by the one counter object. */
416+
async function askGlobalCounter(namespace, action) {
417+
let answer;
418+
try {
419+
const stub = namespace.get(namespace.idFromName(GLOBAL_COUNTER_NAME));
420+
const response = await stub.fetch(`https://global-counter/${action}`, { method: "POST" });
421+
answer = await response.json();
422+
} catch (error) {
423+
console.error("Unable to reach the Worker-wide report counter", error);
424+
return { allowed: false, remaining: 0, retryAfter: 0, stateError: true };
425+
}
426+
if (answer?.allowed === true) {
427+
return { allowed: true, remaining: answer.remaining, retryAfter: 0 };
428+
}
429+
if (answer?.allowed === false && Number.isFinite(answer.retryAfter)) {
430+
return { allowed: false, remaining: 0, retryAfter: Math.max(1, Math.ceil(answer.retryAfter)) };
431+
}
432+
console.error("Malformed answer from the Worker-wide report counter");
433+
return { allowed: false, remaining: 0, retryAfter: 0, stateError: true };
434+
}
435+
436+
const GLOBAL_COUNTER_NAME = "all";
437+
438+
/**
439+
* The Worker-wide report budget, one instance (named "all") for every
440+
* Cloudflare location. Each request runs to completion without awaiting, so
441+
* the check and the increment can't interleave. The count lives in memory:
442+
* the object stays loaded while reports arrive, and one evicted after a quiet
443+
* spell starts a fresh window, which an idle minute would have given anyway.
444+
*/
445+
export class ReportGlobalCounter {
446+
constructor(_state, _env, now = () => Date.now()) {
447+
this.now = now;
448+
this.windowStart = 0;
449+
this.count = 0;
450+
}
451+
452+
async fetch(request) {
453+
const action = new URL(request.url).pathname.slice(1);
454+
if (action !== "peek" && action !== "take") {
455+
return new Response("Not found", { status: 404 });
456+
}
457+
const now = this.now();
458+
if (now - this.windowStart >= RATE_LIMIT_WINDOW_S * 1000) {
459+
this.windowStart = now;
460+
this.count = 0;
461+
}
462+
const allowed = this.count < GLOBAL_RATE_LIMIT_MAX_REQUESTS;
463+
if (allowed && action === "take") this.count += 1;
464+
const retryAfter = allowed
465+
? 0
466+
: Math.max(1, Math.ceil((this.windowStart + RATE_LIMIT_WINDOW_S * 1000 - now) / 1000));
467+
return new Response(
468+
JSON.stringify({ allowed, remaining: GLOBAL_RATE_LIMIT_MAX_REQUESTS - this.count, retryAfter }),
469+
{ headers: { "Content-Type": "application/json" } },
470+
);
471+
}
472+
}
473+
388474
/** Take one request from a budget: the atomic limiter binding when bound, else a KV counter. */
389475
async function consumeBudget({ limiter, key: limiterKey, kvKey: key, max }, env) {
390476
if (limiter) {

‎worker/community-reports-worker.test.mjs‎

Lines changed: 125 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ import worker, {
1313
sanitizeSmsUrlIndicators,
1414
checkRateLimit,
1515
clientKey,
16+
limiterGroupKey,
17+
ReportGlobalCounter,
1618
getClientIp,
1719
checkDedup,
1820
deriveReporterBucket,
@@ -289,14 +291,136 @@ test("one IPv6 /48 has a budget of its own across its /64s", async () => {
289291

290292
test("the Worker has one per-minute budget across every network", async () => {
291293
const env = { RATE_LIMIT: createMockKV() };
294+
// One client per /24, so no group budget refuses first.
292295
for (let i = 1; i <= 30; i++) {
293-
assert.equal((await checkRateLimit(`198.51.100.${i}`, env)).allowed, true, `client ${i}`);
296+
assert.equal((await checkRateLimit(`198.51.${i}.1`, env)).allowed, true, `client ${i}`);
294297
}
295298
const refused = await checkRateLimit("203.0.113.200", env);
296299
assert.equal(refused.allowed, false);
297300
assert.ok(refused.retryAfter > 0);
298301
});
299302

303+
test("IPv4 clients share a group budget by /24", async () => {
304+
assert.equal(limiterGroupKey("192.0.2.77"), "192.0.2.0/24");
305+
assert.equal(limiterGroupKey("::ffff:192.0.2.77"), "192.0.2.0/24");
306+
assert.equal(limiterGroupKey("2001:db8:7:1::1"), clientKey("2001:db8:7:1::1", 48));
307+
308+
const env = { RATE_LIMIT: createMockKV() };
309+
for (let i = 1; i <= 20; i++) {
310+
assert.equal((await checkRateLimit(`192.0.2.${i}`, env)).allowed, true, `address ${i}`);
311+
}
312+
const refused = await checkRateLimit("192.0.2.200", env);
313+
assert.equal(refused.allowed, false);
314+
assert.ok(refused.retryAfter > 0);
315+
assert.equal((await checkRateLimit("192.0.3.1", env)).allowed, true);
316+
});
317+
318+
/** A Durable Object namespace that hands every location the same counter. */
319+
function counterNamespace(counter = new ReportGlobalCounter()) {
320+
const names = [];
321+
return {
322+
names,
323+
counter,
324+
idFromName(name) {
325+
names.push(name);
326+
return { name };
327+
},
328+
get() {
329+
return { fetch: (url, init) => counter.fetch(new Request(url, init)) };
330+
},
331+
};
332+
}
333+
334+
test("the Worker-wide counter takes 30 a minute, and a peek takes nothing", async () => {
335+
let now = 1_000_000;
336+
const counter = new ReportGlobalCounter(null, null, () => now);
337+
const ask = async (action) => (await counter.fetch(new Request(`https://global-counter/${action}`))).json();
338+
for (let i = 1; i <= 30; i++) {
339+
assert.equal((await ask("peek")).allowed, true, `peek ${i}`);
340+
assert.equal((await ask("take")).allowed, true, `take ${i}`);
341+
}
342+
const full = await ask("take");
343+
assert.equal(full.allowed, false);
344+
assert.equal(full.retryAfter, 60);
345+
assert.equal((await ask("peek")).allowed, false);
346+
now += 59_500;
347+
assert.equal((await ask("take")).retryAfter, 1);
348+
now += 500;
349+
assert.equal((await ask("take")).allowed, true);
350+
assert.equal((await counter.fetch(new Request("https://global-counter/reset"))).status, 404);
351+
});
352+
353+
test("every location asks the one Worker-wide counter", async () => {
354+
const namespace = counterNamespace();
355+
const env = { RATE_LIMIT: createMockKV(), REPORT_GLOBAL_COUNTER: namespace };
356+
for (let i = 1; i <= 30; i++) {
357+
assert.equal((await checkRateLimit(`198.51.${i}.1`, env)).allowed, true, `client ${i}`);
358+
}
359+
assert.equal((await checkRateLimit("203.0.113.200", env)).allowed, false);
360+
assert.deepEqual(new Set(namespace.names), new Set(["all"]));
361+
// The KV fallback for the Worker-wide budget isn't used while the counter is bound.
362+
assert.equal(env.RATE_LIMIT._store.has("rlall"), false);
363+
});
364+
365+
test("a full Worker-wide budget refuses before the client is charged", async () => {
366+
const namespace = counterNamespace();
367+
for (let i = 0; i < 30; i++) await namespace.counter.fetch(new Request("https://global-counter/take"));
368+
const charged = [];
369+
const limiter = { async limit({ key }) { charged.push(key); return { success: true }; } };
370+
const response = await worker.fetch(
371+
new Request("https://reports.example", {
372+
method: "POST",
373+
headers: { "content-type": "application/json", "cf-connecting-ip": "203.0.113.60" },
374+
body: JSON.stringify({ number: "+12122340101", type: "spam" }),
375+
}),
376+
{
377+
RATE_LIMIT: createMockKV(),
378+
REPORT_LIMITER: limiter,
379+
REPORT_GROUP_LIMITER: limiter,
380+
REPORT_GLOBAL_COUNTER: namespace,
381+
GITHUB_TOKEN: "test-token",
382+
REPORTER_BUCKET_SECRET: "s".repeat(32),
383+
},
384+
);
385+
assert.equal(response.status, 429);
386+
assert.ok(Number(response.headers.get("retry-after")) > 0);
387+
assert.deepEqual(charged, []);
388+
});
389+
390+
test("a report the client's own limit refuses doesn't spend the Worker-wide budget", async () => {
391+
const namespace = counterNamespace();
392+
const env = {
393+
RATE_LIMIT: createMockKV(),
394+
REPORT_LIMITER: { async limit() { return { success: false }; } },
395+
REPORT_GLOBAL_COUNTER: namespace,
396+
};
397+
for (let i = 0; i < 50; i++) {
398+
assert.equal((await checkRateLimit("203.0.113.70", env)).allowed, false);
399+
}
400+
const peek = await (await namespace.counter.fetch(new Request("https://global-counter/peek"))).json();
401+
assert.equal(peek.remaining, 30);
402+
});
403+
404+
test("an unreachable Worker-wide counter fails closed as a state error", async () => {
405+
const errors = [];
406+
const originalError = console.error;
407+
console.error = (...args) => errors.push(args.join(" "));
408+
try {
409+
const rl = await checkRateLimit("203.0.113.80", {
410+
RATE_LIMIT: createMockKV(),
411+
REPORT_GLOBAL_COUNTER: {
412+
idFromName: (name) => ({ name }),
413+
get: () => ({ fetch: async () => { throw new Error("object unavailable"); } }),
414+
},
415+
});
416+
assert.equal(rl.allowed, false);
417+
assert.equal(rl.stateError, true);
418+
} finally {
419+
console.error = originalError;
420+
}
421+
assert.equal(errors.length > 0, true);
422+
});
423+
300424
test("POST returns 429 when the /48 or the Worker-wide limiter refuses", async () => {
301425
for (const refusing of ["REPORT_GROUP_LIMITER", "REPORT_GLOBAL_LIMITER"]) {
302426
const keys = { REPORT_LIMITER: [], REPORT_GROUP_LIMITER: [], REPORT_GLOBAL_LIMITER: [] };

‎worker/wrangler.toml‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,9 @@ namespace_id = "5001"
3131
limit = 5
3232
period = 60
3333

34-
# A carrier gives each phone a /64 out of a shared /48, so the /48 gets a
35-
# larger budget of its own, and the whole Worker gets one budget per minute so
36-
# a flood spread across networks can't become a flood of GitHub commits. The
37-
# code falls back to KV counters when these bindings are missing.
34+
# A carrier gives each phone a /64 out of a shared /48, so the /48 (an IPv4
35+
# /24) gets a larger budget of its own. The code falls back to KV counters
36+
# when the limiter bindings are missing.
3837
[[ratelimits]]
3938
name = "REPORT_GROUP_LIMITER"
4039
namespace_id = "5002"
@@ -43,10 +42,16 @@ namespace_id = "5002"
4342
limit = 20
4443
period = 60
4544

46-
[[ratelimits]]
47-
name = "REPORT_GLOBAL_LIMITER"
48-
namespace_id = "5003"
49-
50-
[ratelimits.simple]
51-
limit = 30
52-
period = 60
45+
# The whole Worker gets one budget per minute so a flood spread across
46+
# networks can't become a flood of GitHub commits. A rate-limiting binding
47+
# counts separately in each Cloudflare location, so this budget is one
48+
# Durable Object (the instance named "all") that every location asks. It
49+
# keeps its count in memory and stores nothing; SQLite-backed so it runs on
50+
# the Workers Free plan.
51+
[[durable_objects.bindings]]
52+
name = "REPORT_GLOBAL_COUNTER"
53+
class_name = "ReportGlobalCounter"
54+
55+
[[migrations]]
56+
tag = "v1"
57+
new_sqlite_classes = ["ReportGlobalCounter"]

0 commit comments

Comments
 (0)