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();