diff --git a/package.json b/package.json index 58bbfeb6..0c16f08f 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,7 @@ "lint": "standard", "lint:fix": "standard --fix", "redis": "docker run -p 6379:6379 --rm redis", - "test": "standard && tap test/*.test.js && npm run typescript", + "test": "standard && tap \"test/**/*.test.js\" && npm run typescript", "typescript": "tsd" }, "repository": { diff --git a/store/LocalStore.js b/store/LocalStore.js index 810f84cf..bf912a58 100644 --- a/store/LocalStore.js +++ b/store/LocalStore.js @@ -3,37 +3,24 @@ const lru = require('tiny-lru') function LocalStore (timeWindow, cache, app, continueExceeding) { - this.lru = lru(cache || 5000) - this.interval = setInterval(beat.bind(this), timeWindow).unref() + this.lru = lru(cache || 5000, timeWindow) this.app = app this.timeWindow = timeWindow this.continueExceeding = continueExceeding - - app.addHook('onClose', (instance, done) => { - clearInterval(this.interval) - done() - }) - - function beat () { - this.lru.clear() - this.msLastBeat = null - } } LocalStore.prototype.incr = function (ip, cb) { - let current = this.lru.get(ip) || 0 - this.lru.set(ip, ++current) + const nowInMs = Date.now() + const current = this.lru.get(ip) || { count: 0, iterationStartMs: nowInMs } + + current.count++ + + this.lru.set(ip, current) if (this.continueExceeding) { - this.msLastBeat = Date.now() - cb(null, { current, ttl: this.timeWindow }) + cb(null, { current: current.count, ttl: this.timeWindow }) } else { - // start counting from the first request/increment - if (this.msLastBeat === undefined || this.msLastBeat === null) { - this.msLastBeat = Date.now() - } - - cb(null, { current, ttl: this.timeWindow - (Date.now() - this.msLastBeat) }) + cb(null, { current: current.count, ttl: this.timeWindow - (nowInMs - current.iterationStartMs) }) } } diff --git a/test/github-issues/issue-215.test.js b/test/github-issues/issue-215.test.js new file mode 100644 index 00000000..a2fda415 --- /dev/null +++ b/test/github-issues/issue-215.test.js @@ -0,0 +1,81 @@ +'use strict' + +const FakeTimers = require('@sinonjs/fake-timers') +const t = require('tap') +const test = t.test +const Fastify = require('fastify') +const rateLimit = require('../../index') + +t.beforeEach(t => { + t.context.clock = FakeTimers.install() +}) + +t.afterEach(t => { + t.context.clock.uninstall() +}) + +test('issue #215 - when using local store, 2nd user should not be rate limited when the time window is passed for the 1st user', async t => { + t.plan(5) + const fastify = Fastify() + + fastify.register(rateLimit, { + global: false + }) + + fastify.get('/', { + config: { + rateLimit: { + max: 1, + timeWindow: 5000, + continueExceeding: false + } + } + }, async () => 'hello!') + + const user1FirstRequest = await fastify.inject({ + url: '/', + method: 'GET', + remoteAddress: '1.1.1.1' + }) + + // Waiting for the time to pass to make the 2nd user start in a different start point + t.context.clock.tick(3000) + + const user2FirstRequest = await fastify.inject({ + url: '/', + method: 'GET', + remoteAddress: '2.2.2.2' + }) + + const user2SecondRequestAndShouldBeRateLimited = await fastify.inject({ + url: '/', + method: 'GET', + remoteAddress: '2.2.2.2' + }) + + // After this the total time passed for the 1st user is 6s and for the 2nd user only 3s + t.context.clock.tick(3000) + + const user2ThirdRequestAndShouldStillBeRateLimited = await fastify.inject({ + url: '/', + method: 'GET', + remoteAddress: '2.2.2.2' + }) + + // After this the total time passed for the 2nd user is 5.1s - he should not be rate limited + t.context.clock.tick(2100) + + const user2OkResponseAfterRateLimitCompleted = await fastify.inject({ + url: '/', + method: 'GET', + remoteAddress: '2.2.2.2' + }) + + t.equal(user1FirstRequest.statusCode, 200) + t.equal(user2FirstRequest.statusCode, 200) + + t.equal(user2SecondRequestAndShouldBeRateLimited.statusCode, 429) + t.equal(user2ThirdRequestAndShouldStillBeRateLimited.statusCode, 429) + + t.equal(user2OkResponseAfterRateLimitCompleted.statusCode, 200) +})