From ece1a7d7013a6e08c68969325cb0762b620eee41 Mon Sep 17 00:00:00 2001 From: oratis Date: Sun, 9 Aug 2026 23:03:16 +0800 Subject: [PATCH] fix(rate-limit): namespace Redis buckets so limiters stop sharing a window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #15 gave every in-process rateLimit() instance its own bucket namespace, but rate-limit.js transparently swaps to redis-rate-limit.js whenever REDIS_URL is set — and that module keyed its sorted set on `prefix + rawKey`, ignoring the `name` option outright. So the fix only ever covered single-replica deployments. With Redis configured, auth (10/min), discovery (5/min), export (10/min) and sendEmail (20/min) still drew from ONE window per IP: five CSV exports exhausted the discovery limiter for that IP, and the tightest max effectively governed all four. consume() had the same gap. On Redis both it and the middleware were un-namespaced, so batch-send's reservation landed in the right bucket by accident, while sharing that bucket with every other limiter. - redis-rate-limit.js: mirror rate-limit.js's namespacing — rateLimit() takes `name` (positional `rl` fallback), consume() takes `name` (default 'shared'), both key on `prefix + name|key` - warn once per process when a Redis limiter is built unnamed: positional names are safe in-process but only agree across replicas while limiter construction order is identical, and a disagreement silently splits one limiter into two buckets (doubling the effective cap) - index.js: name inviteLookupLimiter, the last unnamed limiter — all six are now explicit, so the warning never fires today - test stub: zadd only accepted (key, score, member), but consume() calls it variadically with n tickets. It was silently dropping every ticket past the first, which made a 50-recipient batch look like it cost one ticket Note: the key shape changes, so existing Redis buckets are orphaned on deploy. Harmless — worst case a few users get a fresh window. Verified by reverting namespaced() to a no-op: 4 of the 6 new tests fail against pre-fix behaviour, including the discovery-5/auth-10 scenario. npm test 662/662 green (was 656). node --check clean on all three files. Co-Authored-By: Claude Opus 5 --- server/__tests__/redis-rate-limit.test.js | 99 ++++++++++++++++++++++- server/index.js | 8 +- server/redis-rate-limit.js | 50 +++++++++++- 3 files changed, 149 insertions(+), 8 deletions(-) diff --git a/server/__tests__/redis-rate-limit.test.js b/server/__tests__/redis-rate-limit.test.js index 5b2c321..1e42d25 100644 --- a/server/__tests__/redis-rate-limit.test.js +++ b/server/__tests__/redis-rate-limit.test.js @@ -34,7 +34,13 @@ function loadWithStub({ failExec = false } = {}) { }; return m; }, - zadd: async (key, score, member) => { ensureSet(key).push([score, member]); }, + // Variadic, like real ioredis: consume() adds n tickets in one call as + // score,member,score,member,... A 3-arg-only stub silently dropped every + // ticket past the first, which would make a batch reservation look free. + zadd: async (key, ...args) => { + const arr = ensureSet(key); + for (let i = 0; i + 1 < args.length; i += 2) arr.push([args[i], args[i + 1]]); + }, zrange: async (key, start, stop, withscores) => { const arr = [...ensureSet(key)].sort((a, b) => a[0] - b[0]); const slice = arr.slice(start, stop + 1); @@ -56,6 +62,12 @@ function loadWithStub({ failExec = false } = {}) { return { mod: require('../redis-rate-limit'), sets }; } +// Most limiters here are built without a `name` on purpose — several tests +// exist precisely to pin what unnamed limiters do — and each fresh module +// instance warns once about positional bucket names. Keep that expected noise +// out of the runner output. +console.warn = () => {}; + function fakeRes() { const r = { headers: {}, @@ -126,3 +138,88 @@ test('rateLimit: fails open on Redis error (calls next, does not 429)', async () assert.equal(nextCalled, true); assert.equal(res.statusCode, 200); // never wrote 429 }); + +// --------------------------------------------------------------------------- +// Bucket isolation. +// +// This module keyed its sorted set on `prefix + rawKey` and ignored `name` +// outright, so every limiter built on the default IP keyFn shared ONE window +// per IP. rate-limit.js swaps to this module whenever REDIS_URL is set, which +// meant the isolation fix in #15 covered only the single-replica path — with +// Redis configured, production still had five discovery calls eating half the +// login budget. Mirrors server/__tests__/rate-limit-isolation.test.js. +// --------------------------------------------------------------------------- + +const WINDOW = 60 * 1000; + +async function run(limiter, ip) { + const res = fakeRes(); + let passed = false; + await limiter({ ip, headers: {} }, res, () => { passed = true; }); + return { passed, status: res.statusCode }; +} + +test('isolation: two limiters with the default keyFn do not share a window', async () => { + const { mod } = loadWithStub(); + const a = mod.rateLimit({ max: 3, windowMs: WINDOW, redisUrl: 'redis://stub' }); + const b = mod.rateLimit({ max: 3, windowMs: WINDOW, redisUrl: 'redis://stub' }); + const ip = '198.51.100.10'; + + for (let i = 0; i < 3; i++) assert.equal((await run(a, ip)).passed, true); + assert.equal((await run(a, ip)).passed, false, 'limiter A is exhausted'); + assert.equal((await run(b, ip)).passed, true, 'limiter B must be untouched'); +}); + +test('isolation: the tightest limiter no longer governs the others', async () => { + // Mirrors the real config: discovery is 5/min, auth is 10/min, same IP. + const { mod } = loadWithStub(); + const discovery = mod.rateLimit({ name: 'discovery', max: 5, windowMs: WINDOW, redisUrl: 'redis://stub' }); + const auth = mod.rateLimit({ name: 'auth', max: 10, windowMs: WINDOW, redisUrl: 'redis://stub' }); + const ip = '198.51.100.11'; + + for (let i = 0; i < 5; i++) await run(discovery, ip); + assert.equal((await run(discovery, ip)).passed, false, 'discovery budget spent'); + for (let i = 0; i < 10; i++) { + assert.equal((await run(auth, ip)).passed, true, `auth request ${i + 1} must pass`); + } + assert.equal((await run(auth, ip)).passed, false, 'auth has its own full budget, then stops'); +}); + +test('isolation: the bucket key carries the namespace', async () => { + const { mod, sets } = loadWithStub(); + const limiter = mod.rateLimit({ name: 'export', max: 2, windowMs: WINDOW, redisUrl: 'redis://stub' }); + await run(limiter, '198.51.100.14'); + const keys = [...sets.keys()]; + assert.deepEqual(keys, ['influencex:rl:export|198.51.100.14']); +}); + +test('isolation: same name = shared window (opt-in, used by batch-send)', async () => { + const { mod } = loadWithStub(); + const name = 'send-email-workspace'; + const limiter = mod.rateLimit({ name, max: 4, windowMs: WINDOW, redisUrl: 'redis://stub', keyFn: () => 'ws:w1' }); + await run(limiter, 'x'); + await run(limiter, 'x'); + + // consume() must land in the SAME sorted set as the middleware it shadows, + // otherwise a batch reservation is written where nothing ever checks it. + const r = await mod.consume({ name, key: 'ws:w1', n: 2, max: 4, windowMs: WINDOW, redisUrl: 'redis://stub' }); + assert.equal(r.allowed, true); + assert.equal((await run(limiter, 'x')).passed, false, 'the 2 consumed tickets count against the middleware'); +}); + +test('isolation: an unnamed consume() cannot silently drain a named limiter', async () => { + const { mod } = loadWithStub(); + const limiter = mod.rateLimit({ name: 'named-only', max: 2, windowMs: WINDOW, redisUrl: 'redis://stub', keyFn: () => 'k' }); + await mod.consume({ key: 'k', n: 5, max: 100, windowMs: WINDOW, redisUrl: 'redis://stub' }); // no name + assert.equal((await run(limiter, 'x')).passed, true, 'named limiter is unaffected by the anonymous bucket'); +}); + +test('isolation: consume() reserves every ticket, not just the first', async () => { + // Guards the variadic zadd contract: n tickets must all land in the window, + // or a batch of 50 would cost one ticket and sail past the cap. + const { mod } = loadWithStub(); + const opts = { name: 'batch', key: 'ws:w2', max: 5, windowMs: WINDOW, redisUrl: 'redis://stub' }; + assert.equal((await mod.consume({ ...opts, n: 3 })).allowed, true); + assert.equal((await mod.consume({ ...opts, n: 3 })).allowed, false, 'only 2 tickets left'); + assert.equal((await mod.consume({ ...opts, n: 2 })).allowed, true); +}); diff --git a/server/index.js b/server/index.js index 4c1510f..abef7ee 100644 --- a/server/index.js +++ b/server/index.js @@ -929,10 +929,12 @@ app.delete(`${BASE_PATH}/api/invite-codes/:id`, authMiddleware, requirePlatformA // thousand RPS. 20/min per IP puts a full sweep out of reach while leaving // plenty of headroom for someone re-typing a code off a chat message. // -// Its own key prefix (not the bare IP) keeps it out of the shared bucket -// authLimiter/exportLimiter draw from, so a signup attempt can't burn a -// login attempt and vice versa. +// Named like every other limiter so its bucket is explicit rather than +// positional: the Redis backend shares buckets across replicas, and a +// positional name only agrees between replicas while limiter construction +// order is identical. The keyFn prefix is belt-and-braces on top of that. const inviteLookupLimiter = rateLimit({ + name: 'invite-lookup', max: 20, windowMs: 60 * 1000, keyFn: (req) => `invite-lookup:${req.ip || req.headers['x-forwarded-for'] || 'anonymous'}`, diff --git a/server/redis-rate-limit.js b/server/redis-rate-limit.js index 5ee0eb9..470e98b 100644 --- a/server/redis-rate-limit.js +++ b/server/redis-rate-limit.js @@ -25,17 +25,55 @@ function getClient(redisUrl) { return _client; } -function rateLimit({ max, windowMs, keyFn, message, redisUrl = process.env.REDIS_URL, prefix = process.env.REDIS_RATELIMIT_PREFIX || 'influencex:rl:' } = {}) { +// Bucket namespacing, mirroring rate-limit.js. Without it every limiter built +// on the default IP keyFn — auth (10/min), discovery (5/min), export (10/min), +// sendEmail (20/min) — hashed to one sorted set per IP, so five discovery +// calls ate half the login budget and the tightest max governed them all. +// The in-process limiter fixed this in #15; because rate-limit.js swaps to +// this module whenever REDIS_URL is set, the bug survived in production until +// the same namespacing landed here. +// +// Naming must agree ACROSS REPLICAS, unlike the in-process case where the +// bucket is local: two replicas that disagree on a namespace silently split +// one logical limiter into two buckets (doubling the effective cap). The +// auto-generated `rl` fallback is a per-process construction counter, so +// it only agrees when every limiter is built unconditionally at module load +// in a fixed order. That holds today, but it is a silent failure mode — pass +// an explicit `name` for anything Redis-backed. autoName() warns when you +// don't. +let limiterSeq = 0; +let _warnedAnon = false; +function namespaced(name, key) { + return `${name}|${key}`; +} + +function autoName() { + const ns = `rl${++limiterSeq}`; + // Once per process: the message is identical every time, and one is enough + // to prompt the fix. + if (!_warnedAnon) { + _warnedAnon = true; + console.warn( + `[rate-limit] Redis limiter built without a name (using positional "${ns}"). ` + + 'Positional names only agree across replicas while limiter construction order is ' + + 'identical; pass { name } to make the bucket explicit.' + ); + } + return ns; +} + +function rateLimit({ max, windowMs, keyFn, message, name, redisUrl = process.env.REDIS_URL, prefix = process.env.REDIS_RATELIMIT_PREFIX || 'influencex:rl:' } = {}) { if (!redisUrl) { throw new Error('Redis rateLimit requires REDIS_URL (or pass redisUrl)'); } const client = getClient(redisUrl); const getKey = keyFn || ((req) => req.ip || req.headers['x-forwarded-for'] || 'anonymous'); const errorMsg = message || 'Too many requests, please slow down'; + const ns = name || autoName(); return async (req, res, next) => { const rawKey = getKey(req); - const key = prefix + rawKey; + const key = prefix + namespaced(ns, rawKey); const now = Date.now(); const cutoff = now - windowMs; @@ -79,13 +117,17 @@ function rateLimit({ max, windowMs, keyFn, message, redisUrl = process.env.REDIS * the same sorted-set window the middleware uses. Check-then-add (same * non-Lua tradeoff as the middleware); fail-open on Redis trouble so a cache * blip doesn't block real users. + * + * `name` must match the `name` the limiter it shadows was built with — + * otherwise the reservation lands in a bucket nothing ever checks and a batch + * sidesteps the cap entirely. Defaults to 'shared', same as rate-limit.js. */ -async function consume({ key: rawKey, n = 1, max, windowMs, redisUrl = process.env.REDIS_URL, prefix = process.env.REDIS_RATELIMIT_PREFIX || 'influencex:rl:' } = {}) { +async function consume({ key: rawKey, n = 1, max, windowMs, name, redisUrl = process.env.REDIS_URL, prefix = process.env.REDIS_RATELIMIT_PREFIX || 'influencex:rl:' } = {}) { if (!redisUrl) { throw new Error('Redis consume requires REDIS_URL (or pass redisUrl)'); } const client = getClient(redisUrl); - const key = prefix + rawKey; + const key = prefix + namespaced(name || 'shared', rawKey); const now = Date.now(); try { const m = client.multi();