From 6d99f6dfae385f5a1fe782c485b351f802df143a Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Wed, 28 Feb 2024 11:20:47 +0000 Subject: [PATCH] Merge pull request #17201 from overleaf/dp-ip-rate-limit Add subnet rate limiter for login rate limit GitOrigin-RevId: c9f68829887dbc1778eff3b465dbde40bc2073d8 --- .../web/app/src/infrastructure/RateLimiter.js | 46 +++++++++++++- .../src/infrastructure/RateLimiterTests.js | 63 +++++++++++++++++++ 2 files changed, 106 insertions(+), 3 deletions(-) create mode 100644 services/web/test/unit/src/infrastructure/RateLimiterTests.js diff --git a/services/web/app/src/infrastructure/RateLimiter.js b/services/web/app/src/infrastructure/RateLimiter.js index 825aca1ed5..1989b09b10 100644 --- a/services/web/app/src/infrastructure/RateLimiter.js +++ b/services/web/app/src/infrastructure/RateLimiter.js @@ -3,6 +3,7 @@ const Metrics = require('@overleaf/metrics') const logger = require('@overleaf/logger') const RedisWrapper = require('./RedisWrapper') const RateLimiterFlexible = require('rate-limiter-flexible') +const OError = require('@overleaf/o-error') const rclient = RedisWrapper.client('ratelimiter') @@ -21,6 +22,9 @@ class RateLimiter { * * points - number of points that can be consumed over the given duration * (default: 4) + * subnetPoints - number of points that can be consumed over the given + * duration accross a sub-network. This should only be used + * ip-based rate limits. * duration - duration of the fixed window in seconds (default: 1) * blockDuration - additional seconds to block after all points are consumed * (default: 0) @@ -32,6 +36,14 @@ class RateLimiter { keyPrefix: `rate-limit:${name}`, storeClient: rclient, }) + if (opts.subnetPoints) { + this._subnetRateLimiter = new RateLimiterFlexible.RateLimiterRedis({ + ...opts, + points: opts.subnetPoints, + keyPrefix: `rate-limit:${name}`, + storeClient: rclient, + }) + } } async consume(key, points = 1, options = { method: 'unknown' }) { @@ -44,8 +56,24 @@ class RateLimiter { isFirstInDuration: false, } } + + await this.consumeForRateLimiter(this._rateLimiter, key, options, points) + + if (options.method === 'ip' && this._subnetRateLimiter) { + const subnetKey = this.getSubnetKeyFromIp(key) + await this.consumeForRateLimiter( + this._subnetRateLimiter, + subnetKey, + options, + points, + 'ip-subnet' + ) + } + } + + async consumeForRateLimiter(rateLimiter, key, options, points, method) { try { - const res = await this._rateLimiter.consume(key, points, options) + const res = await rateLimiter.consume(key, points, options) return res } catch (err) { if (err instanceof Error) { @@ -54,18 +82,29 @@ class RateLimiter { // Only log the first time we exceed the rate limit for a given key and // duration. This happens when the previous amount of consumed points // was below the threshold. - if (err.consumedPoints - points <= this._rateLimiter.points) { + if (err.consumedPoints - points <= rateLimiter.points) { logger.warn({ path: this.name, key }, 'rate limit exceeded') } Metrics.inc('rate-limit-hit', 1, { path: this.name, - method: options.method, + method: method || options.method, }) throw err } } } + getSubnetKeyFromIp(ip) { + if (!/^(?:[0-9]{1,3}\.){3}[0-9]{1,3}$/.test(ip)) { + throw new OError( + 'Cannot generate subnet key as the ip address is not of the expected format.', + { ip } + ) + } + + return ip.split('.').slice(0, 3).join('.') + } + async delete(key) { return await this._rateLimiter.delete(key) } @@ -83,6 +122,7 @@ const openProjectRateLimiter = new RateLimiter('open-project', { // Keep in sync with the can-skip-captcha options. const overleafLoginRateLimiter = new RateLimiter('overleaf-login', { points: 20, + subnetPoints: 200, duration: 60, }) diff --git a/services/web/test/unit/src/infrastructure/RateLimiterTests.js b/services/web/test/unit/src/infrastructure/RateLimiterTests.js new file mode 100644 index 0000000000..ca125c0455 --- /dev/null +++ b/services/web/test/unit/src/infrastructure/RateLimiterTests.js @@ -0,0 +1,63 @@ +const SandboxedModule = require('sandboxed-module') +const sinon = require('sinon') +const { expect } = require('chai') +const modulePath = require('path').join( + __dirname, + '../../../../app/src/infrastructure/RateLimiter' +) + +describe('RateLimiter', function () { + beforeEach(function () { + this.rclient = {} + this.RedisWrapper = { + client: sinon.stub().returns(this.rclient), + } + this.RateLimiter = { + RateLimiter: sinon.stub().withArgs('login').returns(this.rateLimiter), + } + + this.RateLimiterFlexible = { + RateLimiterRedis: sinon.stub(), + } + + this.RateLimiter = SandboxedModule.require(modulePath, { + requires: { + './RedisWrapper': this.RedisWrapper, + 'rate-limiter-flexible': this.RateLimiterFlexible, + }, + }) + }) + + describe('getSubnetKeyFromIp', function () { + beforeEach(function () { + this.rateLimiter = new this.RateLimiter.RateLimiter('some-limit', { + points: 20, + subnetPoints: 200, + duration: 60, + }) + }) + + it('should correctly extract a subnet key from an ip address', function () { + expect(this.rateLimiter.getSubnetKeyFromIp('255.255.255.255')).to.equal( + '255.255.255' + ) + expect(this.rateLimiter.getSubnetKeyFromIp('127.0.0.1')).to.equal( + '127.0.0' + ) + expect(this.rateLimiter.getSubnetKeyFromIp('243.12.3.126')).to.equal( + '243.12.3' + ) + }) + + it('should throw an error when given an incorrectly formatted ip', function () { + expect(() => this.rateLimiter.getSubnetKeyFromIp(1)).to.throw() + expect(() => this.rateLimiter.getSubnetKeyFromIp('Not an ip')).to.throw() + expect(() => + this.rateLimiter.getSubnetKeyFromIp('255.255.255.255.255.255') + ).to.throw() + expect(() => + this.rateLimiter.getSubnetKeyFromIp('255.255.255') + ).to.throw() + }) + }) +})