Skip to content

Commit 76dc0e0

Browse files
authored
Merge pull request #9
FIX: issue with passing the distributed key when creating ratelimiter in middlewares
2 parents bc6403f + fc97e74 commit 76dc0e0

File tree

7 files changed

+745
-16
lines changed

7 files changed

+745
-16
lines changed

README.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -285,11 +285,11 @@ function customRateLimiter(options = {}) {
285285
defaultKey,
286286
options.maxTokens || 100,
287287
typeof options.window === 'string'
288-
? parseTimeString(options.window)
288+
? parseDuration(options.window)
289289
: (options.window || 60000),
290290
options.sliding !== false,
291291
typeof options.block === 'string'
292-
? parseTimeString(options.block)
292+
? parseDuration(options.block)
293293
: (options.block || 0),
294294
options.maxPenalty || 0
295295
);
@@ -314,7 +314,7 @@ function customRateLimiter(options = {}) {
314314
}
315315

316316
// Helper to parse time strings (e.g., '1m', '30s')
317-
function parseTimeString(timeStr) {
317+
function parseDuration(timeStr) {
318318
const units = { ms: 1, s: 1000, m: 60000, h: 3600000, d: 86400000 };
319319
const match = timeStr.match(/^(\d+)([a-z]+)$/i);
320320
if (!match) return parseInt(timeStr, 10);

packages/express/index.js

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,9 @@ function rateLimit(options = {}) {
2929
// Convert time strings to milliseconds for static config
3030
const windowMs = parseDuration(window);
3131
const blockMs = block ? parseDuration(block) : 0;
32-
limiter.createLimiter(key, maxTokens, windowMs, sliding, blockMs, maxPenalty);
32+
// Use distributed key if distributed storage is configured
33+
const distributedKey = (redis || nats) ? `${key}:distributed` : '';
34+
limiter.createLimiter(key, maxTokens, windowMs, sliding, blockMs, maxPenalty, distributedKey);
3335
}
3436

3537
// Cache for resolved configs to avoid excessive calls to configResolver
@@ -118,13 +120,16 @@ function rateLimit(options = {}) {
118120
const resolvedMaxPenalty = effectiveConfig?.maxPenalty !== undefined ? effectiveConfig.maxPenalty : maxPenalty;
119121

120122
// Create/update the limiter (createLimiter updates if it exists)
123+
// Use distributed key if distributed storage is configured
124+
const distributedKey = (redis || nats) ? `${limiterKey}:distributed` : '';
121125
limiter.createLimiter(
122126
limiterKey,
123127
resolvedMaxTokens,
124128
resolvedWindowMs,
125129
resolvedSliding,
126130
resolvedBlockMs,
127-
resolvedMaxPenalty
131+
resolvedMaxPenalty,
132+
distributedKey
128133
);
129134
}
130135
}

packages/fastify/index.js

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,9 @@ function createMiddleware(options) {
4747
// Convert time strings to milliseconds for static config
4848
const windowMs = parseDuration(window);
4949
const blockMs = block ? parseDuration(block) : 0;
50-
limiter.createLimiter(key, maxTokens, windowMs, sliding, blockMs, maxPenalty);
50+
// Use distributed key if distributed storage is configured
51+
const distributedKey = (redis || nats) ? `${key}:distributed` : '';
52+
limiter.createLimiter(key, maxTokens, windowMs, sliding, blockMs, maxPenalty, distributedKey);
5153
}
5254

5355
// Cache for resolved configs to avoid excessive calls to configResolver
@@ -137,13 +139,16 @@ function createMiddleware(options) {
137139
const resolvedMaxPenalty = effectiveConfig?.maxPenalty !== undefined ? effectiveConfig.maxPenalty : maxPenalty;
138140

139141
// Create/update the limiter (createLimiter updates if it exists)
142+
// Use distributed key if distributed storage is configured
143+
const distributedKey = (redis || nats) ? `${limiterKey}:distributed` : '';
140144
limiter.createLimiter(
141145
limiterKey,
142146
resolvedMaxTokens,
143147
resolvedWindowMs,
144148
resolvedSliding,
145149
resolvedBlockMs,
146-
resolvedMaxPenalty
150+
resolvedMaxPenalty,
151+
distributedKey
147152
);
148153
}
149154
}

packages/hyperexpress/index.js

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,9 @@ function rateLimit(options = {}) {
2929
// Convert time strings to milliseconds for static config
3030
const windowMs = parseDuration(window);
3131
const blockMs = block ? parseDuration(block) : 0;
32-
limiter.createLimiter(key, maxTokens, windowMs, sliding, blockMs, maxPenalty);
32+
// Use distributed key if distributed storage is configured
33+
const distributedKey = (redis || nats) ? `${key}:distributed` : '';
34+
limiter.createLimiter(key, maxTokens, windowMs, sliding, blockMs, maxPenalty, distributedKey);
3335
}
3436

3537
// Cache for resolved configs to avoid excessive calls to configResolver
@@ -121,13 +123,16 @@ function rateLimit(options = {}) {
121123
const resolvedMaxPenalty = effectiveConfig?.maxPenalty !== undefined ? effectiveConfig.maxPenalty : maxPenalty;
122124

123125
// Create/update the limiter (createLimiter updates if it exists)
126+
// Use distributed key if distributed storage is configured
127+
const distributedKey = (redis || nats) ? `${limiterKey}:distributed` : '';
124128
limiter.createLimiter(
125129
limiterKey,
126130
resolvedMaxTokens,
127131
resolvedWindowMs,
128132
resolvedSliding,
129133
resolvedBlockMs,
130-
resolvedMaxPenalty
134+
resolvedMaxPenalty,
135+
distributedKey
131136
);
132137
}
133138
}

test/express.test.js

Lines changed: 226 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,9 @@ describe('Express Middleware', () => {
141141
// Skip if NATS server not available
142142
let natsLimiter;
143143
try {
144+
const uniqueKey = 'nats-test-' + Date.now();
144145
app.get('/nats-test', rateLimit({
145-
key: 'nats-test',
146+
key: uniqueKey,
146147
maxTokens: 2,
147148
window: '10s',
148149
nats: {
@@ -280,4 +281,228 @@ describe('Express Middleware', () => {
280281
assert.strictEqual(configResolverCalls, 1);
281282
});
282283
});
284+
285+
describe('Distributed Rate Limiting', () => {
286+
it('should share rate limits across multiple Express instances with NATS', async function() {
287+
// Skip if NATS server not available
288+
let app1, app2;
289+
let server1, server2;
290+
291+
try {
292+
// Create two Express apps with same NATS configuration
293+
app1 = express();
294+
app2 = express();
295+
296+
const natsConfig = {
297+
servers: 'nats://localhost:4222',
298+
bucket: 'test-express-distributed',
299+
prefix: 'exp_dist_'
300+
};
301+
302+
// Use same key and configuration for both apps
303+
const uniqueKey = 'distributed-test-' + Date.now();
304+
const rateLimitConfig = {
305+
key: uniqueKey,
306+
maxTokens: 10,
307+
window: '10s',
308+
nats: natsConfig
309+
};
310+
311+
app1.get('/distributed', rateLimit(rateLimitConfig), (req, res) => {
312+
res.json({ message: 'success from app1' });
313+
});
314+
315+
app2.get('/distributed', rateLimit(rateLimitConfig), (req, res) => {
316+
res.json({ message: 'success from app2' });
317+
});
318+
319+
server1 = app1.listen(0);
320+
server2 = app2.listen(0);
321+
322+
} catch (err) {
323+
if (err.message.includes('NATS connection failed')) {
324+
console.log(' ⚠️ NATS server not available, skipping distributed middleware test');
325+
this.skip();
326+
return;
327+
}
328+
throw err;
329+
}
330+
331+
const port1 = server1.address().port;
332+
const port2 = server2.address().port;
333+
334+
// Make 6 requests to app1
335+
let app1Allowed = 0;
336+
for (let i = 0; i < 6; i++) {
337+
const res = await request(app1).get('/distributed');
338+
if (res.status === 200) app1Allowed++;
339+
}
340+
assert.strictEqual(app1Allowed, 6);
341+
342+
// Make 6 requests to app2 - should only allow 4 more
343+
let app2Allowed = 0;
344+
for (let i = 0; i < 6; i++) {
345+
const res = await request(app2).get('/distributed');
346+
if (res.status === 200) app2Allowed++;
347+
}
348+
assert.strictEqual(app2Allowed, 4);
349+
350+
// Total should not exceed the limit
351+
assert.strictEqual(app1Allowed + app2Allowed, 10);
352+
353+
// Clean up
354+
server1.close();
355+
server2.close();
356+
});
357+
358+
it('should handle concurrent distributed requests correctly', async function() {
359+
// Skip if NATS server not available
360+
let app1, app2;
361+
let server1, server2;
362+
363+
try {
364+
// Create two Express apps
365+
app1 = express();
366+
app2 = express();
367+
368+
const natsConfig = {
369+
servers: 'nats://localhost:4222',
370+
bucket: 'test-express-concurrent',
371+
prefix: 'exp_conc_'
372+
};
373+
374+
const uniqueKey = 'concurrent-test-' + Date.now();
375+
const rateLimitConfig = {
376+
key: uniqueKey,
377+
maxTokens: 50,
378+
window: '10s',
379+
nats: natsConfig
380+
};
381+
382+
app1.get('/concurrent', rateLimit(rateLimitConfig), (req, res) => {
383+
res.json({ message: 'success' });
384+
});
385+
386+
app2.get('/concurrent', rateLimit(rateLimitConfig), (req, res) => {
387+
res.json({ message: 'success' });
388+
});
389+
390+
server1 = app1.listen(0);
391+
server2 = app2.listen(0);
392+
393+
} catch (err) {
394+
if (err.message.includes('NATS connection failed')) {
395+
console.log(' ⚠️ NATS server not available, skipping concurrent distributed test');
396+
this.skip();
397+
return;
398+
}
399+
throw err;
400+
}
401+
402+
// Make concurrent requests from both servers
403+
const promises = [];
404+
405+
// 40 requests to app1
406+
for (let i = 0; i < 40; i++) {
407+
promises.push(
408+
request(app1).get('/concurrent')
409+
.then(res => res.status === 200)
410+
);
411+
}
412+
413+
// 40 requests to app2
414+
for (let i = 0; i < 40; i++) {
415+
promises.push(
416+
request(app2).get('/concurrent')
417+
.then(res => res.status === 200)
418+
);
419+
}
420+
421+
const results = await Promise.all(promises);
422+
const totalAllowed = results.filter(r => r).length;
423+
424+
// Should respect the limit with some tolerance for race conditions
425+
assert(totalAllowed >= 48 && totalAllowed <= 52,
426+
`Expected ~50 allowed requests, got ${totalAllowed}`);
427+
428+
// Clean up
429+
server1.close();
430+
server2.close();
431+
});
432+
433+
it('should use distributed storage with configResolver', async function() {
434+
// Skip if NATS server not available
435+
let app1, app2;
436+
let server1, server2;
437+
438+
try {
439+
app1 = express();
440+
app2 = express();
441+
442+
const natsConfig = {
443+
servers: 'nats://localhost:4222',
444+
bucket: 'test-express-resolver',
445+
prefix: 'exp_res_'
446+
};
447+
448+
const rateLimitConfig = {
449+
keyGenerator: (req) => req.headers['x-api-key'] || 'anonymous',
450+
configResolver: (apiKey) => {
451+
if (apiKey && apiKey.startsWith('test-key-')) {
452+
return {
453+
maxTokens: 5,
454+
window: '10s'
455+
};
456+
}
457+
return null;
458+
},
459+
nats: natsConfig
460+
};
461+
462+
app1.get('/resolver', rateLimit(rateLimitConfig), (req, res) => {
463+
res.json({ message: 'success from app1' });
464+
});
465+
466+
app2.get('/resolver', rateLimit(rateLimitConfig), (req, res) => {
467+
res.json({ message: 'success from app2' });
468+
});
469+
470+
server1 = app1.listen(0);
471+
server2 = app2.listen(0);
472+
473+
} catch (err) {
474+
if (err.message.includes('NATS connection failed')) {
475+
console.log(' ⚠️ NATS server not available, skipping resolver distributed test');
476+
this.skip();
477+
return;
478+
}
479+
throw err;
480+
}
481+
482+
// Use unique test key to avoid conflicts
483+
const testApiKey = 'test-key-' + Date.now();
484+
485+
// Make 3 requests to app1 with test-key
486+
for (let i = 0; i < 3; i++) {
487+
const res = await request(app1)
488+
.get('/resolver')
489+
.set('x-api-key', testApiKey);
490+
assert.strictEqual(res.status, 200);
491+
}
492+
493+
// Make 3 more requests to app2 - should only allow 2
494+
let app2Allowed = 0;
495+
for (let i = 0; i < 3; i++) {
496+
const res = await request(app2)
497+
.get('/resolver')
498+
.set('x-api-key', testApiKey);
499+
if (res.status === 200) app2Allowed++;
500+
}
501+
assert.strictEqual(app2Allowed, 2);
502+
503+
// Clean up
504+
server1.close();
505+
server2.close();
506+
});
507+
});
283508
});

0 commit comments

Comments
 (0)