fix(rate-limit): namespace Redis buckets so limiters stop sharing a window

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);
+});

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'}`,

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