diff --git a/package-lock.json b/package-lock.json index 82f4e498ab..6ecad6d443 100644 --- a/package-lock.json +++ b/package-lock.json @@ -22740,24 +22740,6 @@ "node": ">=8.6" } }, - "node_modules/microtime": { - "version": "3.0.0", - "resolved": "https://registry.npmjs.org/microtime/-/microtime-3.0.0.tgz", - "integrity": "sha512-SirJr7ZL4ow2iWcb54bekS4aWyBQNVcEDBiwAz9D/sTgY59A+uE8UJU15cp5wyZmPBwg/3zf8lyCJ5NUe1nVlQ==", - "hasInstallScript": true, - "dependencies": { - "node-addon-api": "^1.2.0", - "node-gyp-build": "^3.8.0" - }, - "engines": { - "node": ">= 4.0.0" - } - }, - "node_modules/microtime/node_modules/node-addon-api": { - "version": "1.7.2", - "resolved": "https://registry.npmjs.org/node-addon-api/-/node-addon-api-1.7.2.tgz", - "integrity": "sha512-ibPK3iA+vaY1eEjESkQkM0BbCqFOaZMiXRTtdB0u7b4djtY6JnsjvPdUHVMg6xQt3B8fpTTWHI9A+ADjM9frzg==" - }, "node_modules/mime": { "version": "2.6.0", "resolved": "https://registry.npmjs.org/mime/-/mime-2.6.0.tgz", @@ -23674,16 +23656,6 @@ "node": ">= 6.13.0" } }, - "node_modules/node-gyp-build": { - "version": "3.9.0", - "resolved": "https://registry.npmjs.org/node-gyp-build/-/node-gyp-build-3.9.0.tgz", - "integrity": "sha512-zLcTg6P4AbcHPq465ZMFNXx7XpKKJh+7kkN699NiQWisR2uWYOWNWqRHAmbnmKiL4e9aLSlmy5U7rEMUXV59+A==", - "bin": { - "node-gyp-build": "bin.js", - "node-gyp-build-optional": "optional.js", - "node-gyp-build-test": "build-test.js" - } - }, "node_modules/node-int64": { "version": "0.4.0", "resolved": "https://registry.npmjs.org/node-int64/-/node-int64-0.4.0.tgz", @@ -27579,26 +27551,6 @@ "resolved": "https://registry.npmjs.org/rndm/-/rndm-1.2.0.tgz", "integrity": "sha1-8z/pz7Urv9UgqhgyO8ZdsRCht2w=" }, - "node_modules/rolling-rate-limiter": { - "version": "0.2.13", - "resolved": "https://registry.npmjs.org/rolling-rate-limiter/-/rolling-rate-limiter-0.2.13.tgz", - "integrity": "sha512-fF5XeJn7t22cC9LIh77BDfs7Mg0jon03qezMfPige0vehzhmNyQ6U+eSYbaL1l80sX9z+22+6x4pQ8xHT0LrVw==", - "dependencies": { - "microtime": "^3.0.0", - "uuid": "^8.3.0" - }, - "engines": { - "node": ">= 4.0.0" - } - }, - "node_modules/rolling-rate-limiter/node_modules/uuid": { - "version": "8.3.2", - "resolved": "https://registry.npmjs.org/uuid/-/uuid-8.3.2.tgz", - "integrity": "sha512-+NYs2QeMWy+GWFOEm9xnn6HCDp0l7QBD7ml8zLUmJ+93Q5NF0NocErnwkTkXVFNiX3/fpC6afS8Dhb/gz7R7eg==", - "bin": { - "uuid": "dist/bin/uuid" - } - }, "node_modules/route-recognizer": { "version": "0.3.4", "resolved": "https://registry.npmjs.org/route-recognizer/-/route-recognizer-0.3.4.tgz", @@ -34505,7 +34457,6 @@ "request": "^2.88.2", "requestretry": "^7.1.0", "rimraf": "2.2.6", - "rolling-rate-limiter": "^0.2.10", "sanitize-html": "^1.27.1", "scroll-into-view-if-needed": "^2.2.25", "tsscmp": "^1.0.6", @@ -43718,7 +43669,6 @@ "requestretry": "^7.1.0", "requirejs": "^2.3.6", "rimraf": "2.2.6", - "rolling-rate-limiter": "^0.2.10", "samlp": "^7.0.2", "sandboxed-module": "https://github.com/overleaf/node-sandboxed-module/archive/cafa2d60f17ce75cc023e6f296eb8de79d92d35d.tar.gz", "sanitize-html": "^1.27.1", @@ -57775,22 +57725,6 @@ "picomatch": "^2.2.3" } }, - "microtime": { - "version": "3.0.0", - "resolved": "https://registry.npmjs.org/microtime/-/microtime-3.0.0.tgz", - "integrity": "sha512-SirJr7ZL4ow2iWcb54bekS4aWyBQNVcEDBiwAz9D/sTgY59A+uE8UJU15cp5wyZmPBwg/3zf8lyCJ5NUe1nVlQ==", - "requires": { - "node-addon-api": "^1.2.0", - "node-gyp-build": "^3.8.0" - }, - "dependencies": { - "node-addon-api": { - "version": "1.7.2", - "resolved": "https://registry.npmjs.org/node-addon-api/-/node-addon-api-1.7.2.tgz", - "integrity": "sha512-ibPK3iA+vaY1eEjESkQkM0BbCqFOaZMiXRTtdB0u7b4djtY6JnsjvPdUHVMg6xQt3B8fpTTWHI9A+ADjM9frzg==" - } - } - }, "mime": { "version": "2.6.0", "resolved": "https://registry.npmjs.org/mime/-/mime-2.6.0.tgz", @@ -58532,11 +58466,6 @@ "resolved": "https://registry.npmjs.org/node-forge/-/node-forge-1.3.1.tgz", "integrity": "sha512-dPEtOeMvF9VMcYV/1Wb8CPoVAXtp6MKMlcbAt4ddqmGqUJ6fQZFXkNZNkNlfevtNkGtaSoXf/vNNNSvgrdXwtA==" }, - "node-gyp-build": { - "version": "3.9.0", - "resolved": "https://registry.npmjs.org/node-gyp-build/-/node-gyp-build-3.9.0.tgz", - "integrity": "sha512-zLcTg6P4AbcHPq465ZMFNXx7XpKKJh+7kkN699NiQWisR2uWYOWNWqRHAmbnmKiL4e9aLSlmy5U7rEMUXV59+A==" - }, "node-int64": { "version": "0.4.0", "resolved": "https://registry.npmjs.org/node-int64/-/node-int64-0.4.0.tgz", @@ -61733,22 +61662,6 @@ "resolved": "https://registry.npmjs.org/rndm/-/rndm-1.2.0.tgz", "integrity": "sha1-8z/pz7Urv9UgqhgyO8ZdsRCht2w=" }, - "rolling-rate-limiter": { - "version": "0.2.13", - "resolved": "https://registry.npmjs.org/rolling-rate-limiter/-/rolling-rate-limiter-0.2.13.tgz", - "integrity": "sha512-fF5XeJn7t22cC9LIh77BDfs7Mg0jon03qezMfPige0vehzhmNyQ6U+eSYbaL1l80sX9z+22+6x4pQ8xHT0LrVw==", - "requires": { - "microtime": "^3.0.0", - "uuid": "^8.3.0" - }, - "dependencies": { - "uuid": { - "version": "8.3.2", - "resolved": "https://registry.npmjs.org/uuid/-/uuid-8.3.2.tgz", - "integrity": "sha512-+NYs2QeMWy+GWFOEm9xnn6HCDp0l7QBD7ml8zLUmJ+93Q5NF0NocErnwkTkXVFNiX3/fpC6afS8Dhb/gz7R7eg==" - } - } - }, "route-recognizer": { "version": "0.3.4", "resolved": "https://registry.npmjs.org/route-recognizer/-/route-recognizer-0.3.4.tgz", diff --git a/services/web/app/src/Features/Analytics/AnalyticsRouter.js b/services/web/app/src/Features/Analytics/AnalyticsRouter.js index 3027d5961f..6f37839230 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsRouter.js +++ b/services/web/app/src/Features/Analytics/AnalyticsRouter.js @@ -1,28 +1,37 @@ const AuthenticationController = require('./../Authentication/AuthenticationController') const AnalyticsController = require('./AnalyticsController') const AnalyticsProxy = require('./AnalyticsProxy') -const RateLimiterMiddleware = require('./../Security/RateLimiterMiddleware') +const { RateLimiter } = require('../../infrastructure/RateLimiter') +const RateLimiterMiddleware = require('../Security/RateLimiterMiddleware') const { expressify } = require('../../util/promises') +const rateLimiters = { + recordEvent: new RateLimiter('analytics-record-event', { + points: 200, + duration: 60, + }), + updateEditingSession: new RateLimiter('analytics-update-editing-session', { + points: 20, + duration: 60, + }), + uniExternalCollabProxy: new RateLimiter( + 'analytics-uni-external-collab-proxy', + { points: 20, duration: 60 } + ), +} + module.exports = { apply(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/event/:event([a-z0-9-_]+)', - RateLimiterMiddleware.rateLimit({ - endpointName: 'analytics-record-event', - maxRequests: 200, - timeInterval: 60, - }), + RateLimiterMiddleware.rateLimit(rateLimiters.recordEvent), AnalyticsController.recordEvent ) webRouter.put( '/editingSession/:projectId', - RateLimiterMiddleware.rateLimit({ - endpointName: 'analytics-update-editing-session', + RateLimiterMiddleware.rateLimit(rateLimiters.updateEditingSession, { params: ['projectId'], - maxRequests: 20, - timeInterval: 60, }), expressify(AnalyticsController.updateEditingSession) ) @@ -30,11 +39,7 @@ module.exports = { publicApiRouter.use( '/analytics/uniExternalCollaboration', AuthenticationController.requirePrivateApiAuth(), - RateLimiterMiddleware.rateLimit({ - endpointName: 'analytics-uni-external-collab-proxy', - maxRequests: 20, - timeInterval: 60, - }), + RateLimiterMiddleware.rateLimit(rateLimiters.uniExternalCollabProxy), AnalyticsProxy.call('/uniExternalCollaboration') ) }, diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js b/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js index e87ea0cb62..a3ddb3b264 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js @@ -1,17 +1,3 @@ -/* eslint-disable - camelcase, - n/handle-callback-err, - max-len, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ let CollaboratorsInviteController const OError = require('@overleaf/o-error') const ProjectGetter = require('../Project/ProjectGetter') @@ -25,73 +11,84 @@ const EmailHelper = require('../Helpers/EmailHelper') const EditorRealTimeController = require('../Editor/EditorRealTimeController') const AnalyticsManager = require('../Analytics/AnalyticsManager') const SessionManager = require('../Authentication/SessionManager') -const rateLimiter = require('../../infrastructure/RateLimiter') +const { RateLimiter } = require('../../infrastructure/RateLimiter') + +// This rate limiter allows a different number of requests depending on the +// number of callaborators a user is allowed. This is implemented by providing +// a number of points (P) and consuming c = floor(P / maxRequests) on each +// request. We'd like (maxRequests + 1) requests to trigger the rate limit, so +// one constrait that we have is that c * (maxRequests + 1) > P. This is +// achieved if P = M^2 where M is the largest value possible for maxRequests. +// +// In the present case, we allow 10 requests per collaborator per 30 minutes, +// with a maximum of 200 requests, so P = 200^2 = 40000. +const RATE_LIMIT_POINTS = 40000 +const rateLimiter = new RateLimiter('invite-to-project-by-user-id', { + points: RATE_LIMIT_POINTS, + duration: 60 * 30, +}) module.exports = CollaboratorsInviteController = { getAllInvites(req, res, next) { const projectId = req.params.Project_id logger.debug({ projectId }, 'getting all active invites for project') - return CollaboratorsInviteHandler.getAllInvites( + CollaboratorsInviteHandler.getAllInvites( projectId, function (err, invites) { - if (err != null) { + if (err) { OError.tag(err, 'error getting invites for project', { projectId, }) return next(err) } - return res.json({ invites }) + res.json({ invites }) } ) }, _checkShouldInviteEmail(email, callback) { - if (callback == null) { - callback = function () {} - } if (Settings.restrictInvitesToExistingAccounts === true) { logger.debug({ email }, 'checking if user exists with this email') - return UserGetter.getUserByAnyEmail( - email, - { _id: 1 }, - function (err, user) { - if (err != null) { - return callback(err) - } - const userExists = - user != null && (user != null ? user._id : undefined) != null - return callback(null, userExists) + UserGetter.getUserByAnyEmail(email, { _id: 1 }, function (err, user) { + if (err) { + return callback(err) } - ) + const userExists = user?._id != null + callback(null, userExists) + }) } else { - return callback(null, true) + callback(null, true) } }, - _checkRateLimit(user_id, callback) { - if (callback == null) { - callback = function () {} - } - return LimitationsManager.allowedNumberOfCollaboratorsForUser( - user_id, - function (err, collabLimit) { - if (collabLimit == null) { - collabLimit = 1 - } - if (err != null) { + _checkRateLimit(userId, callback) { + LimitationsManager.allowedNumberOfCollaboratorsForUser( + userId, + (err, collabLimit) => { + if (err) { return callback(err) } - if (collabLimit === -1) { + if (collabLimit == null || collabLimit === 0) { + collabLimit = 1 + } else if (collabLimit < 0 || collabLimit > 20) { collabLimit = 20 } - collabLimit = collabLimit * 10 - const opts = { - endpointName: 'invite-to-project-by-user-id', - timeInterval: 60 * 30, - subjectName: user_id, - throttle: collabLimit, - } - return rateLimiter.addCount(opts, callback) + + // Consume enough points to hit the rate limit at 10 * collabLimit + const maxRequests = 10 * collabLimit + const points = Math.floor(RATE_LIMIT_POINTS / maxRequests) + rateLimiter + .consume(userId, points) + .then(() => { + callback(null, true) + }) + .catch(err => { + if (err instanceof Error) { + callback(err) + } else { + callback(null, false) + } + }) } ) }, @@ -109,107 +106,103 @@ module.exports = CollaboratorsInviteController = { return res.json({ invite: null, error: 'cannot_invite_self' }) } logger.debug({ projectId, email, sendingUserId }, 'inviting to project') - return LimitationsManager.canAddXCollaborators( - projectId, - 1, - (error, allowed) => { - let privileges - if (error != null) { - return next(error) - } - if (!allowed) { - logger.debug( - { projectId, email, sendingUserId }, - 'not allowed to invite more users to project' - ) - return res.json({ invite: null }) - } - ;({ email, privileges } = req.body) - email = EmailHelper.parseEmail(email, true) - if (email == null || email === '') { - logger.debug( - { projectId, email, sendingUserId }, - 'invalid email address' - ) - return res.status(400).json({ errorReason: 'invalid_email' }) - } - return CollaboratorsInviteController._checkRateLimit( - sendingUserId, - function (error, underRateLimit) { - if (error != null) { - return next(error) - } - if (!underRateLimit) { - return res.sendStatus(429) - } - return CollaboratorsInviteController._checkShouldInviteEmail( - email, - function (err, shouldAllowInvite) { - if (err != null) { - OError.tag( - err, - 'error checking if we can invite this email address', - { - email, - projectId, - sendingUserId, - } - ) - return next(err) - } - if (!shouldAllowInvite) { - logger.debug( - { email, projectId, sendingUserId }, - 'not allowed to send an invite to this email address' - ) - return res.json({ - invite: null, - error: 'cannot_invite_non_user', - }) - } - return CollaboratorsInviteHandler.inviteToProject( - projectId, - sendingUser, - email, - privileges, - function (err, invite) { - if (err != null) { - OError.tag(err, 'error creating project invite', { - projectId, - email, - sendingUserId, - }) - return next(err) - } - logger.debug( - { projectId, email, sendingUserId }, - 'invite created' - ) - EditorRealTimeController.emitToRoom( - projectId, - 'project:membership:changed', - { invites: true } - ) - return res.json({ invite }) + LimitationsManager.canAddXCollaborators(projectId, 1, (error, allowed) => { + let privileges + if (error) { + return next(error) + } + if (!allowed) { + logger.debug( + { projectId, email, sendingUserId }, + 'not allowed to invite more users to project' + ) + return res.json({ invite: null }) + } + ;({ email, privileges } = req.body) + email = EmailHelper.parseEmail(email, true) + if (email == null || email === '') { + logger.debug( + { projectId, email, sendingUserId }, + 'invalid email address' + ) + return res.status(400).json({ errorReason: 'invalid_email' }) + } + CollaboratorsInviteController._checkRateLimit( + sendingUserId, + function (error, underRateLimit) { + if (error) { + return next(error) + } + if (!underRateLimit) { + return res.sendStatus(429) + } + CollaboratorsInviteController._checkShouldInviteEmail( + email, + function (err, shouldAllowInvite) { + if (err) { + OError.tag( + err, + 'error checking if we can invite this email address', + { + email, + projectId, + sendingUserId, } ) + return next(err) } - ) - } - ) - } - ) + if (!shouldAllowInvite) { + logger.debug( + { email, projectId, sendingUserId }, + 'not allowed to send an invite to this email address' + ) + return res.json({ + invite: null, + error: 'cannot_invite_non_user', + }) + } + CollaboratorsInviteHandler.inviteToProject( + projectId, + sendingUser, + email, + privileges, + function (err, invite) { + if (err) { + OError.tag(err, 'error creating project invite', { + projectId, + email, + sendingUserId, + }) + return next(err) + } + logger.debug( + { projectId, email, sendingUserId }, + 'invite created' + ) + EditorRealTimeController.emitToRoom( + projectId, + 'project:membership:changed', + { invites: true } + ) + res.json({ invite }) + } + ) + } + ) + } + ) + }) }, revokeInvite(req, res, next) { const projectId = req.params.Project_id const inviteId = req.params.invite_id logger.debug({ projectId, inviteId }, 'revoking invite') - return CollaboratorsInviteHandler.revokeInvite( + CollaboratorsInviteHandler.revokeInvite( projectId, inviteId, function (err) { - if (err != null) { + if (err) { OError.tag(err, 'error revoking invite', { projectId, inviteId, @@ -221,7 +214,7 @@ module.exports = CollaboratorsInviteController = { 'project:membership:changed', { invites: true } ) - return res.sendStatus(201) + res.sendStatus(201) } ) }, @@ -231,28 +224,28 @@ module.exports = CollaboratorsInviteController = { const inviteId = req.params.invite_id logger.debug({ projectId, inviteId }, 'resending invite') const sendingUser = SessionManager.getSessionUser(req.session) - return CollaboratorsInviteController._checkRateLimit( + CollaboratorsInviteController._checkRateLimit( sendingUser._id, function (error, underRateLimit) { - if (error != null) { + if (error) { return next(error) } if (!underRateLimit) { return res.sendStatus(429) } - return CollaboratorsInviteHandler.resendInvite( + CollaboratorsInviteHandler.resendInvite( projectId, sendingUser, inviteId, function (err) { - if (err != null) { + if (err) { OError.tag(err, 'error resending invite', { projectId, inviteId, }) return next(err) } - return res.sendStatus(201) + res.sendStatus(201) } ) } @@ -264,15 +257,15 @@ module.exports = CollaboratorsInviteController = { const { token } = req.params const _renderInvalidPage = function () { logger.debug({ projectId }, 'invite not valid, rendering not-valid page') - return res.render('project/invite/not-valid', { title: 'Invalid Invite' }) + res.render('project/invite/not-valid', { title: 'Invalid Invite' }) } // check if the user is already a member of the project const currentUser = SessionManager.getSessionUser(req.session) - return CollaboratorsGetter.isUserInvitedMemberOfProject( + CollaboratorsGetter.isUserInvitedMemberOfProject( currentUser._id, projectId, function (err, isMember) { - if (err != null) { + if (err) { OError.tag(err, 'error checking if user is member of project', { projectId, }) @@ -286,11 +279,11 @@ module.exports = CollaboratorsInviteController = { return res.redirect(`/project/${projectId}`) } // get the invite - return CollaboratorsInviteHandler.getInviteByToken( + CollaboratorsInviteHandler.getInviteByToken( projectId, token, function (err, invite) { - if (err != null) { + if (err) { OError.tag(err, 'error getting invite by token', { projectId, }) @@ -302,11 +295,11 @@ module.exports = CollaboratorsInviteController = { return _renderInvalidPage() } // check the user who sent the invite exists - return UserGetter.getUser( + UserGetter.getUser( { _id: invite.sendingUserId }, { email: 1, first_name: 1, last_name: 1 }, function (err, owner) { - if (err != null) { + if (err) { OError.tag(err, 'error getting project owner', { projectId, }) @@ -317,11 +310,11 @@ module.exports = CollaboratorsInviteController = { return _renderInvalidPage() } // fetch the project name - return ProjectGetter.getProject( + ProjectGetter.getProject( projectId, {}, function (err, project) { - if (err != null) { + if (err) { OError.tag(err, 'error getting project', { projectId, }) @@ -332,7 +325,7 @@ module.exports = CollaboratorsInviteController = { return _renderInvalidPage() } // finally render the invite - return res.render('project/invite/show', { + res.render('project/invite/show', { invite, project, owner, @@ -356,12 +349,12 @@ module.exports = CollaboratorsInviteController = { { projectId, userId: currentUser._id }, 'got request to accept invite' ) - return CollaboratorsInviteHandler.acceptInvite( + CollaboratorsInviteHandler.acceptInvite( projectId, token, currentUser, function (err) { - if (err != null) { + if (err) { OError.tag(err, 'error accepting invite by token', { projectId, token, @@ -381,9 +374,9 @@ module.exports = CollaboratorsInviteController = { } ) if (req.xhr) { - return res.sendStatus(204) // Done async via project page notification + res.sendStatus(204) // Done async via project page notification } else { - return res.redirect(`/project/${projectId}`) + res.redirect(`/project/${projectId}`) } } ) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js index 663f7fe3d9..e21545295f 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js @@ -3,11 +3,27 @@ const AuthenticationController = require('../Authentication/AuthenticationContro const AuthorizationMiddleware = require('../Authorization/AuthorizationMiddleware') const PrivilegeLevels = require('../Authorization/PrivilegeLevels') const CollaboratorsInviteController = require('./CollaboratorsInviteController') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const RateLimiterMiddleware = require('../Security/RateLimiterMiddleware') const CaptchaMiddleware = require('../Captcha/CaptchaMiddleware') const AnalyticsRegistrationSourceMiddleware = require('../Analytics/AnalyticsRegistrationSourceMiddleware') const { Joi, validate } = require('../../infrastructure/Validation') +const rateLimiters = { + inviteToProjectByProjectId: new RateLimiter( + 'invite-to-project-by-project-id', + { points: 100, duration: 60 * 10 } + ), + inviteToProjectByIp: new RateLimiter('invite-to-project-by-ip', { + points: 100, + duration: 60 * 10, + }), + resendInvite: new RateLimiter('resend-invite', { + points: 200, + duration: 60 * 10, + }), +} + module.exports = { apply(webRouter) { webRouter.post( @@ -66,17 +82,11 @@ module.exports = { // invites webRouter.post( '/project/:Project_id/invite', - RateLimiterMiddleware.rateLimit({ - endpointName: 'invite-to-project-by-project-id', + RateLimiterMiddleware.rateLimit(rateLimiters.inviteToProjectByProjectId, { params: ['Project_id'], - maxRequests: 100, - timeInterval: 60 * 10, }), - RateLimiterMiddleware.rateLimit({ - endpointName: 'invite-to-project-by-ip', + RateLimiterMiddleware.rateLimit(rateLimiters.inviteToProjectByIp, { ipOnly: true, - maxRequests: 100, - timeInterval: 60 * 10, }), CaptchaMiddleware.validateCaptcha('invite'), AuthenticationController.requireLogin(), @@ -100,11 +110,8 @@ module.exports = { webRouter.post( '/project/:Project_id/invite/:invite_id/resend', - RateLimiterMiddleware.rateLimit({ - endpointName: 'resend-invite', + RateLimiterMiddleware.rateLimit(rateLimiters.resendInvite, { params: ['Project_id'], - maxRequests: 200, - timeInterval: 60 * 10, }), AuthenticationController.requireLogin(), AuthorizationMiddleware.ensureUserCanAdminProject, diff --git a/services/web/app/src/Features/Compile/CompileController.js b/services/web/app/src/Features/Compile/CompileController.js index 32daff9554..82518097ac 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -9,7 +9,7 @@ const logger = require('@overleaf/logger') const request = require('request') const Settings = require('@overleaf/settings') const SessionManager = require('../Authentication/SessionManager') -const RateLimiter = require('../../infrastructure/RateLimiter') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const ClsiCookieManager = require('./ClsiCookieManager')( Settings.apis.clsi?.backendGroupName ) @@ -20,6 +20,11 @@ const { callbackify } = require('../../util/promises') const COMPILE_TIMEOUT_MS = 10 * 60 * 1000 +const pdfDownloadRateLimiter = new RateLimiter('full-pdf-download', { + points: 1000, + duration: 60 * 60, +}) + function getImageNameForProject(projectId, callback) { ProjectGetter.getProject(projectId, { imageName: 1 }, (err, project) => { if (err) return callback(err) @@ -297,13 +302,18 @@ module.exports = CompileController = { if (isPdfjsPartialDownload) { callback(null, true) } else { - const rateLimitOpts = { - endpointName: 'full-pdf-download', - throttle: 1000, - subjectName: req.ip, - timeInterval: 60 * 60, - } - RateLimiter.addCount(rateLimitOpts, callback) + pdfDownloadRateLimiter + .consume(req.ip) + .then(() => { + callback(null, true) + }) + .catch(err => { + if (err instanceof Error) { + callback(err) + } else { + callback(null, false) + } + }) } } diff --git a/services/web/app/src/Features/Compile/CompileManager.js b/services/web/app/src/Features/Compile/CompileManager.js index 5a41716a3e..7b569d0a32 100644 --- a/services/web/app/src/Features/Compile/CompileManager.js +++ b/services/web/app/src/Features/Compile/CompileManager.js @@ -7,7 +7,7 @@ const ProjectRootDocManager = require('../Project/ProjectRootDocManager') const UserGetter = require('../User/UserGetter') const ClsiManager = require('./ClsiManager') const Metrics = require('@overleaf/metrics') -const rateLimiter = require('../../infrastructure/RateLimiter') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const SplitTestHandler = require('../SplitTests/SplitTestHandler') const { getAnalyticsIdFromMongoUser } = require('../Analytics/AnalyticsHelper') @@ -228,23 +228,19 @@ module.exports = CompileManager = { if (!isAutoCompile) { return callback(null, true) } - const bucket = Math.random() > 0.5 ? 'b-one' : 'b-two' - Metrics.inc(`auto-compile-${compileGroup}`, 1, { method: bucket }) - const opts = { - endpointName: 'auto_compile', - timeInterval: 20, - subjectName: `${compileGroup}-${bucket}`, - throttle: (Settings.rateLimit.autoCompile[compileGroup] || 25) / 2, - } - rateLimiter.addCount(opts, function (err, canCompile) { - if (err) { - canCompile = false - } - if (!canCompile) { + Metrics.inc(`auto-compile-${compileGroup}`) + const rateLimiter = getAutoCompileRateLimiter(compileGroup) + rateLimiter + .consume('global') + .then(() => { + callback(null, true) + }) + .catch(() => { + // Don't differentiate between errors and rate limits. Silently trigger + // the rate limit if there's an error consuming the points. Metrics.inc(`auto-compile-${compileGroup}-limited`) - } - callback(null, canCompile) - }) + callback(null, false) + }) }, _getCompileBackendClassDetails(owner, compileGroup, callback) { @@ -287,3 +283,16 @@ module.exports = CompileManager = { }) }, } + +const autoCompileRateLimiters = new Map() +function getAutoCompileRateLimiter(compileGroup) { + let rateLimiter = autoCompileRateLimiters.get(compileGroup) + if (rateLimiter == null) { + rateLimiter = new RateLimiter(`auto-compile:${compileGroup}`, { + points: Settings.rateLimit.autoCompile[compileGroup] || 25, + duration: 20, + }) + autoCompileRateLimiters.set(compileGroup, rateLimiter) + } + return rateLimiter +} diff --git a/services/web/app/src/Features/Editor/EditorRouter.js b/services/web/app/src/Features/Editor/EditorRouter.js index 0cd9bbee9c..6539e9e524 100644 --- a/services/web/app/src/Features/Editor/EditorRouter.js +++ b/services/web/app/src/Features/Editor/EditorRouter.js @@ -1,29 +1,36 @@ const EditorHttpController = require('./EditorHttpController') const AuthenticationController = require('../Authentication/AuthenticationController') const AuthorizationMiddleware = require('../Authorization/AuthorizationMiddleware') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const RateLimiterMiddleware = require('../Security/RateLimiterMiddleware') +const rateLimiters = { + addDocToProject: new RateLimiter('add-doc-to-project', { + points: 30, + duration: 60, + }), + addFolderToProject: new RateLimiter('add-folder-to-project', { + points: 60, + duration: 60, + }), + joinProject: new RateLimiter('join-project', { points: 45, duration: 60 }), +} + module.exports = { apply(webRouter, privateApiRouter) { webRouter.post( '/project/:Project_id/doc', AuthorizationMiddleware.ensureUserCanWriteProjectContent, - RateLimiterMiddleware.rateLimit({ - endpointName: 'add-doc-to-project', + RateLimiterMiddleware.rateLimit(rateLimiters.addDocToProject, { params: ['Project_id'], - maxRequests: 30, - timeInterval: 60, }), EditorHttpController.addDoc ) webRouter.post( '/project/:Project_id/folder', AuthorizationMiddleware.ensureUserCanWriteProjectContent, - RateLimiterMiddleware.rateLimit({ - endpointName: 'add-folder-to-project', + RateLimiterMiddleware.rateLimit(rateLimiters.addFolderToProject, { params: ['Project_id'], - maxRequests: 60, - timeInterval: 60, }), EditorHttpController.addFolder ) @@ -61,11 +68,8 @@ module.exports = { privateApiRouter.post( '/project/:Project_id/join', AuthenticationController.requirePrivateApiAuth(), - RateLimiterMiddleware.rateLimit({ - endpointName: 'join-project', + RateLimiterMiddleware.rateLimit(rateLimiters.joinProject, { params: ['Project_id'], - maxRequests: 45, - timeInterval: 60, }), EditorHttpController.joinProject ) diff --git a/services/web/app/src/Features/Email/EmailSender.js b/services/web/app/src/Features/Email/EmailSender.js index 93ee7a5724..41cee94c25 100644 --- a/services/web/app/src/Features/Email/EmailSender.js +++ b/services/web/app/src/Features/Email/EmailSender.js @@ -6,7 +6,7 @@ const nodemailer = require('nodemailer') const sesTransport = require('nodemailer-ses-transport') const mandrillTransport = require('nodemailer-mandrill-transport') const OError = require('@overleaf/o-error') -const RateLimiter = require('../../infrastructure/RateLimiter') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const _ = require('lodash') const EMAIL_SETTINGS = Settings.email || {} @@ -20,6 +20,11 @@ module.exports = { const client = getClient() +const rateLimiter = new RateLimiter('send_email', { + points: 100, + duration: 3 * 60 * 60, +}) + function getClient() { let client if (EMAIL_SETTINGS.parameters) { @@ -106,12 +111,14 @@ async function checkCanSendEmail(options) { // email not sent from user, not rate limited return true } - const opts = { - endpointName: 'send_email', - timeInterval: 60 * 60 * 3, - subjectName: options.sendingUser_id, - throttle: 100, + try { + await rateLimiter.consume(options.sendingUser_id) + } catch (err) { + if (err instanceof Error) { + throw err + } else { + return false + } } - const allowed = await RateLimiter.promises.addCount(opts) - return allowed + return true } diff --git a/services/web/app/src/Features/LinkedFiles/LinkedFilesRouter.js b/services/web/app/src/Features/LinkedFiles/LinkedFilesRouter.js index d66825be61..d9a5bde7d3 100644 --- a/services/web/app/src/Features/LinkedFiles/LinkedFilesRouter.js +++ b/services/web/app/src/Features/LinkedFiles/LinkedFilesRouter.js @@ -1,42 +1,38 @@ -/* eslint-disable - max-len, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const AuthorizationMiddleware = require('../Authorization/AuthorizationMiddleware') const AuthenticationController = require('../Authentication/AuthenticationController') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const RateLimiterMiddleware = require('../Security/RateLimiterMiddleware') const LinkedFilesController = require('./LinkedFilesController') +const rateLimiters = { + createLinkedFile: new RateLimiter('create-linked-file', { + points: 100, + duration: 60, + }), + refreshLinkedFile: new RateLimiter('refresh-linked-file', { + points: 100, + duration: 60, + }), +} + module.exports = { apply(webRouter) { webRouter.post( '/project/:project_id/linked_file', AuthenticationController.requireLogin(), AuthorizationMiddleware.ensureUserCanWriteProjectContent, - RateLimiterMiddleware.rateLimit({ - endpointName: 'create-linked-file', + RateLimiterMiddleware.rateLimit(rateLimiters.createLinkedFile, { params: ['project_id'], - maxRequests: 100, - timeInterval: 60, }), LinkedFilesController.createLinkedFile ) - return webRouter.post( + webRouter.post( '/project/:project_id/linked_file/:file_id/refresh', AuthenticationController.requireLogin(), AuthorizationMiddleware.ensureUserCanWriteProjectContent, - RateLimiterMiddleware.rateLimit({ - endpointName: 'refresh-linked-file', + RateLimiterMiddleware.rateLimit(rateLimiters.refreshLinkedFile, { params: ['project_id'], - maxRequests: 100, - timeInterval: 60, }), LinkedFilesController.refreshLinkedFile ) diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js b/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js index c7b251fa9a..da7b4f751e 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetRouter.js @@ -1,16 +1,19 @@ const PasswordResetController = require('./PasswordResetController') const AuthenticationController = require('../Authentication/AuthenticationController') const CaptchaMiddleware = require('../../Features/Captcha/CaptchaMiddleware') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const RateLimiterMiddleware = require('../Security/RateLimiterMiddleware') const { Joi, validate } = require('../../infrastructure/Validation') +const rateLimiter = new RateLimiter('password_reset_rate_limit', { + points: 6, + duration: 60, +}) + module.exports = { apply(webRouter) { - const rateLimit = RateLimiterMiddleware.rateLimit({ - endpointName: 'password_reset_rate_limit', + const rateLimit = RateLimiterMiddleware.rateLimit(rateLimiter, { ipOnly: true, - maxRequests: 6, - timeInterval: 60, }) webRouter.get( @@ -69,4 +72,6 @@ module.exports = { PasswordResetController.requestReset ) }, + + rateLimiter, } diff --git a/services/web/app/src/Features/Security/LoginRateLimiter.js b/services/web/app/src/Features/Security/LoginRateLimiter.js index 21b2efd5e9..c9bb30fba2 100644 --- a/services/web/app/src/Features/Security/LoginRateLimiter.js +++ b/services/web/app/src/Features/Security/LoginRateLimiter.js @@ -1,24 +1,35 @@ -const RateLimiter = require('../../infrastructure/RateLimiter') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const { promisifyAll } = require('../../util/promises') -const ONE_MIN = 60 -const ATTEMPT_LIMIT = 10 +const rateLimiter = new RateLimiter('login', { + points: 10, + duration: 120, +}) function processLoginRequest(email, callback) { - const opts = { - endpointName: 'login', - throttle: ATTEMPT_LIMIT, - timeInterval: ONE_MIN * 2, - subjectName: email, - } - RateLimiter.addCount(opts, (err, shouldAllow) => callback(err, shouldAllow)) + rateLimiter + .consume(email) + .then(() => { + callback(null, true) + }) + .catch(err => { + if (err instanceof Error) { + callback(err) + } else { + callback(null, false) + } + }) } function recordSuccessfulLogin(email, callback) { - if (callback == null) { - callback = function () {} - } - RateLimiter.clearRateLimit('login', email, callback) + rateLimiter + .delete(email) + .then(() => { + callback() + }) + .catch(err => { + callback(err) + }) } const LoginRateLimiter = { diff --git a/services/web/app/src/Features/Security/RateLimiterMiddleware.js b/services/web/app/src/Features/Security/RateLimiterMiddleware.js index 7095b96857..85a066aa38 100644 --- a/services/web/app/src/Features/Security/RateLimiterMiddleware.js +++ b/services/web/app/src/Features/Security/RateLimiterMiddleware.js @@ -1,65 +1,23 @@ -const RateLimiter = require('../../infrastructure/RateLimiter') const logger = require('@overleaf/logger') const SessionManager = require('../Authentication/SessionManager') const LoginRateLimiter = require('./LoginRateLimiter') const settings = require('@overleaf/settings') -/* - Do not allow more than opts.maxRequests from a single client in - opts.timeInterval. Pass an array of opts.params to segment this based on - parameters in the request URL, e.g.: - - app.get "/project/:project_id", RateLimiterMiddleware.rateLimit(endpointName: "open-editor", params: ["project_id"]) - - will rate limit each project_id separately. - - Unique clients are identified by user_id if logged in, and IP address if not. -*/ -function rateLimit(opts) { - const getUserId = - opts.getUserId || (req => SessionManager.getLoggedInUserId(req.session)) - return function (req, res, next) { - const userId = getUserId(req) || req.ip - if ( - settings.smokeTest && - settings.smokeTest.userId && - settings.smokeTest.userId.toString() === userId.toString() - ) { - // ignore smoke test user - return next() - } - const params = (opts.params || []).map(p => req.params[p]) - params.push(userId) - let subjectName = params.join(':') - if (opts.ipOnly) { - subjectName = req.ip - } - if (opts.endpointName == null) { - throw new Error('no endpointName provided') - } - const options = { - endpointName: opts.endpointName, - timeInterval: opts.timeInterval || 60, - subjectName, - throttle: opts.maxRequests || 6, - } - return RateLimiter.addCount(options, function (error, canContinue) { - if (error != null) { - return next(error) - } - if (canContinue) { - return next() - } else { - logger.warn(options, 'rate limit exceeded') - res.status(429) // Too many requests - res.write('Rate limit reached, please try again later') - return res.end() - } - }) - } -} - -function rateLimitV2(rateLimiter, opts = {}) { +/** + * Return a rate limiting middleware + * + * Pass an array of opts.params to segment this based on parameters in the + * request URL, e.g.: + * + * app.get "/project/:project_id", RateLimiterMiddleware.rateLimit( + * rateLimiter, params: ["project_id"] + * ) + * + * will rate limit each project_id separately. + * + * Unique clients are identified by user_id if logged in, and IP address if not. + */ +function rateLimit(rateLimiter, opts = {}) { const getUserId = opts.getUserId || (req => SessionManager.getLoggedInUserId(req.session)) return function (req, res, next) { @@ -102,24 +60,23 @@ function loginRateLimit(req, res, next) { if (!email) { return next() } - LoginRateLimiter.processLoginRequest(email, function (err, isAllowed) { + LoginRateLimiter.processLoginRequest(email, (err, isAllowed) => { if (err) { return next(err) } if (isAllowed) { - return next() + next() } else { logger.warn({ email }, 'rate limit exceeded') res.status(429) // Too many requests res.write('Rate limit reached, please try again later') - return res.end() + res.end() } }) } const RateLimiterMiddleware = { rateLimit, - rateLimitV2, loginRateLimit, } diff --git a/services/web/app/src/Features/Subscription/SubscriptionRouter.js b/services/web/app/src/Features/Subscription/SubscriptionRouter.js index c3e62cd4e2..a27e1507b6 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionRouter.js +++ b/services/web/app/src/Features/Subscription/SubscriptionRouter.js @@ -2,9 +2,15 @@ const AuthenticationController = require('../Authentication/AuthenticationContro const SubscriptionController = require('./SubscriptionController') const SubscriptionGroupController = require('./SubscriptionGroupController') const TeamInvitesController = require('./TeamInvitesController') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const RateLimiterMiddleware = require('../Security/RateLimiterMiddleware') const Settings = require('@overleaf/settings') +const teamInviteRateLimiter = new RateLimiter('team-invite', { + points: 10, + duration: 60, +}) + module.exports = { apply(webRouter, privateApiRouter, publicApiRouter) { if (!Settings.enableSubscriptions) { @@ -64,11 +70,7 @@ module.exports = { webRouter.put( '/subscription/invites/:token/', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimit({ - endpointName: 'team-invite', - maxRequests: 10, - timeInterval: 60, - }), + RateLimiterMiddleware.rateLimit(teamInviteRateLimiter), TeamInvitesController.acceptInvite ) diff --git a/services/web/app/src/Features/Templates/TemplatesRouter.js b/services/web/app/src/Features/Templates/TemplatesRouter.js index 3d6b28d103..207bd7ddec 100644 --- a/services/web/app/src/Features/Templates/TemplatesRouter.js +++ b/services/web/app/src/Features/Templates/TemplatesRouter.js @@ -1,9 +1,15 @@ const AuthenticationController = require('../Authentication/AuthenticationController') const TemplatesController = require('./TemplatesController') const TemplatesMiddleware = require('./TemplatesMiddleware') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const RateLimiterMiddleware = require('../Security/RateLimiterMiddleware') const AnalyticsRegistrationSourceMiddleware = require('../Analytics/AnalyticsRegistrationSourceMiddleware') +const rateLimiter = new RateLimiter('create-project-from-template', { + points: 20, + duration: 60, +}) + module.exports = { apply(app) { app.get( @@ -22,11 +28,7 @@ module.exports = { app.post( '/project/new/template', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimit({ - endpointName: 'create-project-from-template', - maxRequests: 20, - timeInterval: 60, - }), + RateLimiterMiddleware.rateLimit(rateLimiter), TemplatesController.createProjectFromV1Template ) }, diff --git a/services/web/app/src/Features/Uploads/UploadsRouter.js b/services/web/app/src/Features/Uploads/UploadsRouter.js index 5ab3602916..c7c5073768 100644 --- a/services/web/app/src/Features/Uploads/UploadsRouter.js +++ b/services/web/app/src/Features/Uploads/UploadsRouter.js @@ -1,30 +1,38 @@ const AuthorizationMiddleware = require('../Authorization/AuthorizationMiddleware') const AuthenticationController = require('../Authentication/AuthenticationController') const ProjectUploadController = require('./ProjectUploadController') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const RateLimiterMiddleware = require('../Security/RateLimiterMiddleware') const Settings = require('@overleaf/settings') +const rateLimiters = { + projectUpload: new RateLimiter('project-upload', { + points: 20, + duration: 60, + }), + fileUpload: new RateLimiter('file-upload', { + points: 200, + duration: 60 * 15, + }), +} + module.exports = { apply(webRouter) { webRouter.post( '/project/new/upload', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimit({ - endpointName: 'project-upload', - maxRequests: 20, - timeInterval: 60, - }), + RateLimiterMiddleware.rateLimit(rateLimiters.projectUpload), ProjectUploadController.multerMiddleware, ProjectUploadController.uploadProject ) const fileUploadEndpoint = '/Project/:Project_id/upload' - const fileUploadRateLimit = RateLimiterMiddleware.rateLimit({ - endpointName: 'file-upload', - params: ['Project_id'], - maxRequests: 200, - timeInterval: 60 * 15, - }) + const fileUploadRateLimit = RateLimiterMiddleware.rateLimit( + rateLimiters.fileUpload, + { + params: ['Project_id'], + } + ) if (Settings.allowAnonymousReadAndWriteSharing) { webRouter.post( fileUploadEndpoint, diff --git a/services/web/app/src/Features/UserMembership/UserMembershipRouter.js b/services/web/app/src/Features/UserMembership/UserMembershipRouter.js index f5492fe0ee..c88d53257b 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipRouter.js +++ b/services/web/app/src/Features/UserMembership/UserMembershipRouter.js @@ -1,16 +1,21 @@ -// TODO: This file was created by bulk-decaffeinate. -// Sanity-check the conversion and remove this comment. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const UserMembershipMiddleware = require('./UserMembershipMiddleware') const UserMembershipController = require('./UserMembershipController') const SubscriptionGroupController = require('../Subscription/SubscriptionGroupController') const TeamInvitesController = require('../Subscription/TeamInvitesController') +const { RateLimiter } = require('../../infrastructure/RateLimiter') const RateLimiterMiddleware = require('../Security/RateLimiterMiddleware') +const rateLimiters = { + createTeamInvite: new RateLimiter('create-team-invite', { + points: 200, + duration: 60, + }), + exportTeamCsv: new RateLimiter('export-team-csv', { + points: 30, + duration: 60, + }), +} + module.exports = { apply(webRouter) { // group members routes @@ -22,11 +27,7 @@ module.exports = { webRouter.post( '/manage/groups/:id/invites', UserMembershipMiddleware.requireGroupManagementAccess, - RateLimiterMiddleware.rateLimit({ - endpointName: 'create-team-invite', - maxRequests: 200, - timeInterval: 60, - }), + RateLimiterMiddleware.rateLimit(rateLimiters.createTeamInvite), TeamInvitesController.createInvite ) webRouter.delete( @@ -42,11 +43,7 @@ module.exports = { webRouter.get( '/manage/groups/:id/members/export', UserMembershipMiddleware.requireGroupManagementAccess, - RateLimiterMiddleware.rateLimit({ - endpointName: 'export-team-csv', - maxRequests: 30, - timeInterval: 60, - }), + RateLimiterMiddleware.rateLimit(rateLimiters.exportTeamCsv), UserMembershipController.exportCsv ) diff --git a/services/web/app/src/infrastructure/RateLimiter.js b/services/web/app/src/infrastructure/RateLimiter.js index 6bac414898..46137995db 100644 --- a/services/web/app/src/infrastructure/RateLimiter.js +++ b/services/web/app/src/infrastructure/RateLimiter.js @@ -1,38 +1,10 @@ -const settings = require('@overleaf/settings') +const Settings = require('@overleaf/settings') const Metrics = require('@overleaf/metrics') const logger = require('@overleaf/logger') const RedisWrapper = require('./RedisWrapper') -const rclient = RedisWrapper.client('ratelimiter') -const RollingRateLimiter = require('rolling-rate-limiter') const RateLimiterFlexible = require('rate-limiter-flexible') -const { callbackify } = require('util') -async function addCount(opts) { - if (settings.disableRateLimits) { - return true - } - const namespace = `RateLimit:${opts.endpointName}:` - const k = `{${opts.subjectName}}` - const limiter = new RollingRateLimiter.RedisRateLimiter({ - client: rclient, - namespace, - interval: opts.timeInterval * 1000, - maxInInterval: opts.throttle, - }) - const rateLimited = await limiter.limit(k) - if (rateLimited) { - Metrics.inc('rate-limit-hit', 1, { - path: opts.endpointName, - }) - } - return !rateLimited -} - -async function clearRateLimit(endpointName, subject) { - // same as the key which will be built by RollingRateLimiter (namespace+k) - const keyName = `RateLimit:${endpointName}:{${subject}}` - await rclient.del(keyName) -} +const rclient = RedisWrapper.client('ratelimiter') /** * Wrapper over the RateLimiterRedis class @@ -63,6 +35,15 @@ class RateLimiter { } async consume(key, points = 1, options = {}) { + if (Settings.disableRateLimits) { + // Return a fake result in case it's used somewhere + return { + msBeforeNext: 0, + remainingPoints: 100, + consumedPoints: 0, + isFirstInDuration: false, + } + } try { const res = await this._rateLimiter.consume(key, points, options) return res @@ -71,8 +52,9 @@ class RateLimiter { throw err } else { // Only log the first time we exceed the rate limit for a given key and - // duration - if (err.consumedPoints === this._rateLimiter.points + points) { + // duration. This happens when the previous amount of consumed points + // was below the threshold. + if (err.consumedPoints - points <= this._rateLimiter.points) { logger.warn({ path: this.name, key }, 'rate limit exceeded') } Metrics.inc('rate-limit-hit', 1, { path: this.name }) @@ -80,14 +62,29 @@ class RateLimiter { } } } + + async delete(key) { + return await this._rateLimiter.delete(key) + } } +/* + * Shared rate limiters + */ + +const openProjectRateLimiter = new RateLimiter('open-project', { + points: 15, + duration: 60, +}) + +// Keep in sync with the can-skip-captcha options. +const overleafLoginRateLimiter = new RateLimiter('overleaf-login', { + points: 20, + duration: 60, +}) + module.exports = { - addCount: callbackify(addCount), - clearRateLimit: callbackify(clearRateLimit), RateLimiter, - promises: { - addCount, - clearRateLimit, - }, + openProjectRateLimiter, + overleafLoginRateLimiter, } diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index 0b5bb1d59a..ba4d7706ae 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -35,7 +35,10 @@ const PasswordResetRouter = require('./Features/PasswordReset/PasswordResetRoute const StaticPagesRouter = require('./Features/StaticPages/StaticPagesRouter') const ChatController = require('./Features/Chat/ChatController') const Modules = require('./infrastructure/Modules') -const { RateLimiter } = require('./infrastructure/RateLimiter') +const { + RateLimiter, + openProjectRateLimiter, +} = require('./infrastructure/RateLimiter') const RateLimiterMiddleware = require('./Features/Security/RateLimiterMiddleware') const InactiveProjectController = require('./Features/InactiveData/InactiveProjectController') const ContactRouter = require('./Features/Contacts/ContactRouter') @@ -68,8 +71,6 @@ const { plainTextResponse } = require('./infrastructure/Response') const PublicAccessLevels = require('./Features/Authorization/PublicAccessLevels') const UserContentDomainController = require('./Features/UserContentDomainCheck/UserContentDomainController') -module.exports = { initialize } - const rateLimiters = { addEmail: new RateLimiter('add-email', { points: 10, @@ -166,10 +167,6 @@ const rateLimiters = { points: 30, duration: 60, }), - openProject: new RateLimiter('open-project', { - points: 15, - duration: 60, - }), readAndWriteToken: new RateLimiter('read-and-write-token', { points: 15, duration: 60, @@ -206,6 +203,17 @@ const rateLimiters = { points: 10, duration: 60, }), + userContentDomainAccessCheckResult: new RateLimiter( + 'user-content-domain-a-c-r', + { + points: 15, + duration: 60, + } + ), + userContentDomainFallbackUsage: new RateLimiter('user-content-fb-u', { + points: 15, + duration: 60, + }), } function initialize(webRouter, privateApiRouter, publicApiRouter) { @@ -231,7 +239,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/login/can-skip-captcha', // Keep in sync with the overleaf-login options. - RateLimiterMiddleware.rateLimitV2(rateLimiters.canSkipCaptcha), + RateLimiterMiddleware.rateLimit(rateLimiters.canSkipCaptcha), CaptchaMiddleware.canSkipCaptcha ) @@ -310,7 +318,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/user/password/update', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.changePassword), + RateLimiterMiddleware.rateLimit(rateLimiters.changePassword), UserController.changePassword ) webRouter.get( @@ -322,13 +330,13 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.get('/user/emails/confirm', UserEmailsController.showConfirm) webRouter.post( '/user/emails/confirm', - RateLimiterMiddleware.rateLimitV2(rateLimiters.confirmEmail), + RateLimiterMiddleware.rateLimit(rateLimiters.confirmEmail), UserEmailsController.confirm ) webRouter.post( '/user/emails/resend_confirmation', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.resendConfirmation), + RateLimiterMiddleware.rateLimit(rateLimiters.resendConfirmation), UserEmailsController.resendConfirmation ) @@ -348,13 +356,13 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/user/emails', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.addEmail), + RateLimiterMiddleware.rateLimit(rateLimiters.addEmail), UserEmailsController.add ) webRouter.post( '/user/emails/delete', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.deleteEmail), + RateLimiterMiddleware.rateLimit(rateLimiters.deleteEmail), UserEmailsController.remove ) webRouter.post( @@ -365,7 +373,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/user/emails/endorse', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.endorseEmail), + RateLimiterMiddleware.rateLimit(rateLimiters.endorseEmail), UserEmailsController.endorse ) } @@ -408,7 +416,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/user/delete', - RateLimiterMiddleware.rateLimitV2(rateLimiters.deleteUser), + RateLimiterMiddleware.rateLimit(rateLimiters.deleteUser), AuthenticationController.requireLogin(), UserController.tryDeleteUser ) @@ -451,19 +459,19 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.get( '/project', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.openDashboard), + RateLimiterMiddleware.rateLimit(rateLimiters.openDashboard), ProjectController.projectListPage ) webRouter.post( '/project/new', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.createProject), + RateLimiterMiddleware.rateLimit(rateLimiters.createProject), ProjectController.newProject ) webRouter.post( '/api/project', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.getProjects), + RateLimiterMiddleware.rateLimit(rateLimiters.getProjects), ProjectListController.getProjectsJson ) @@ -475,7 +483,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { ]) { webRouter.get( route, - RateLimiterMiddleware.rateLimitV2(rateLimiters.openProject, { + RateLimiterMiddleware.rateLimit(openProjectRateLimiter, { params: ['Project_id'], }), AuthenticationController.validateUserSession(), @@ -514,7 +522,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/project/:Project_id/compile', - RateLimiterMiddleware.rateLimitV2(rateLimiters.compileProjectHttp, { + RateLimiterMiddleware.rateLimit(rateLimiters.compileProjectHttp, { params: ['Project_id'], }), AuthorizationMiddleware.ensureUserCanReadProject, @@ -562,7 +570,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { ) // Align with limits defined in CompileController.downloadPdf - const rateLimiterMiddlewareOutputFiles = RateLimiterMiddleware.rateLimitV2( + const rateLimiterMiddlewareOutputFiles = RateLimiterMiddleware.rateLimit( rateLimiters.miscOutputDownload, { params: ['Project_id'] } ) @@ -753,7 +761,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { ) webRouter.get( '/project/:project_id/version/:version/zip', - RateLimiterMiddleware.rateLimitV2(rateLimiters.downloadProjectRevision), + RateLimiterMiddleware.rateLimit(rateLimiters.downloadProjectRevision), AuthorizationMiddleware.blockRestrictedUserFromProject, AuthorizationMiddleware.ensureUserCanReadProject, HistoryController.downloadZipOfVersion @@ -805,7 +813,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.get( '/Project/:Project_id/download/zip', - RateLimiterMiddleware.rateLimitV2(rateLimiters.zipDownload, { + RateLimiterMiddleware.rateLimit(rateLimiters.zipDownload, { params: ['Project_id'], }), AuthorizationMiddleware.ensureUserCanReadProject, @@ -814,7 +822,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.get( '/project/download/zip', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.multipleProjectsZipDownload), + RateLimiterMiddleware.rateLimit(rateLimiters.multipleProjectsZipDownload), AuthorizationMiddleware.ensureUserCanReadMultipleProjects, ProjectDownloadsController.downloadMultipleProjects ) @@ -873,7 +881,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/tag', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.createTag), + RateLimiterMiddleware.rateLimit(rateLimiters.createTag), validate({ body: Joi.object({ name: Joi.string().required(), @@ -884,7 +892,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/tag/:tagId/rename', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.renameTag), + RateLimiterMiddleware.rateLimit(rateLimiters.renameTag), validate({ body: Joi.object({ name: Joi.string().required(), @@ -895,19 +903,19 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.delete( '/tag/:tagId', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.deleteTag), + RateLimiterMiddleware.rateLimit(rateLimiters.deleteTag), TagsController.deleteTag ) webRouter.post( '/tag/:tagId/project/:projectId', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.addProjectToTag), + RateLimiterMiddleware.rateLimit(rateLimiters.addProjectToTag), TagsController.addProjectToTag ) webRouter.post( '/tag/:tagId/projects', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.addProjectsToTag), + RateLimiterMiddleware.rateLimit(rateLimiters.addProjectsToTag), validate({ body: Joi.object({ projectIds: Joi.array().items(Joi.string()).required(), @@ -918,13 +926,13 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.delete( '/tag/:tagId/project/:projectId', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.removeProjectFromTag), + RateLimiterMiddleware.rateLimit(rateLimiters.removeProjectFromTag), TagsController.removeProjectFromTag ) webRouter.delete( '/tag/:tagId/projects', AuthenticationController.requireLogin(), - RateLimiterMiddleware.rateLimitV2(rateLimiters.removeProjectsFromTag), + RateLimiterMiddleware.rateLimit(rateLimiters.removeProjectsFromTag), validate({ body: Joi.object({ projectIds: Joi.array().items(Joi.string()).required(), @@ -1083,20 +1091,20 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { '/project/:project_id/messages', AuthorizationMiddleware.blockRestrictedUserFromProject, AuthorizationMiddleware.ensureUserCanReadProject, - RateLimiterMiddleware.rateLimitV2(rateLimiters.sendChatMessage), + RateLimiterMiddleware.rateLimit(rateLimiters.sendChatMessage), ChatController.sendMessage ) webRouter.post( '/project/:Project_id/references/index', AuthorizationMiddleware.ensureUserCanReadProject, - RateLimiterMiddleware.rateLimitV2(rateLimiters.indexProjectReferences), + RateLimiterMiddleware.rateLimit(rateLimiters.indexProjectReferences), ReferencesController.index ) webRouter.post( '/project/:Project_id/references/indexAll', AuthorizationMiddleware.ensureUserCanReadProject, - RateLimiterMiddleware.rateLimitV2(rateLimiters.indexAllProjectReferences), + RateLimiterMiddleware.rateLimit(rateLimiters.indexAllProjectReferences), ReferencesController.indexAll ) @@ -1141,7 +1149,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { ) publicApiRouter.post( '/api/institutions/confirm_university_domain', - RateLimiterMiddleware.rateLimitV2(rateLimiters.confirmUniversityDomain), + RateLimiterMiddleware.rateLimit(rateLimiters.confirmUniversityDomain), AuthenticationController.requirePrivateApiAuth(), InstitutionsController.confirmDomain ) @@ -1260,7 +1268,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.get( '/status/compiler/:Project_id', - RateLimiterMiddleware.rateLimitV2(rateLimiters.statusCompiler), + RateLimiterMiddleware.rateLimit(rateLimiters.statusCompiler), AuthorizationMiddleware.ensureUserCanReadProject, function (req, res) { const projectId = req.params.Project_id @@ -1336,26 +1344,22 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { succeeded: Joi.number().min(0).max(6), }), }), - RateLimiterMiddleware.rateLimit({ - endpointName: 'user-content-domain-a-c-r', - maxRequests: 15, - timeInterval: 60, - }), + RateLimiterMiddleware.rateLimit( + rateLimiters.userContentDomainAccessCheckResult + ), UserContentDomainController.recordCheckResult ) webRouter.post( '/record-user-content-domain-fallback-usage', - RateLimiterMiddleware.rateLimit({ - endpointName: 'user-content-domain-fb-u', - maxRequests: 15, - timeInterval: 60, - }), + RateLimiterMiddleware.rateLimit( + rateLimiters.userContentDomainFallbackUsage + ), UserContentDomainController.recordFallbackUsage ) webRouter.get( `/read/:token(${TokenAccessController.READ_ONLY_TOKEN_PATTERN})`, - RateLimiterMiddleware.rateLimitV2(rateLimiters.readOnlyToken), + RateLimiterMiddleware.rateLimit(rateLimiters.readOnlyToken), AnalyticsRegistrationSourceMiddleware.setSource( 'collaboration', 'link-sharing' @@ -1366,7 +1370,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.get( `/:token(${TokenAccessController.READ_AND_WRITE_TOKEN_PATTERN})`, - RateLimiterMiddleware.rateLimitV2(rateLimiters.readAndWriteToken), + RateLimiterMiddleware.rateLimit(rateLimiters.readAndWriteToken), AnalyticsRegistrationSourceMiddleware.setSource( 'collaboration', 'link-sharing' @@ -1377,13 +1381,13 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( `/:token(${TokenAccessController.READ_AND_WRITE_TOKEN_PATTERN})/grant`, - RateLimiterMiddleware.rateLimitV2(rateLimiters.grantTokenAccessReadWrite), + RateLimiterMiddleware.rateLimit(rateLimiters.grantTokenAccessReadWrite), TokenAccessController.grantTokenAccessReadAndWrite ) webRouter.post( `/read/:token(${TokenAccessController.READ_ONLY_TOKEN_PATTERN})/grant`, - RateLimiterMiddleware.rateLimitV2(rateLimiters.grantTokenAccessReadOnly), + RateLimiterMiddleware.rateLimit(rateLimiters.grantTokenAccessReadOnly), TokenAccessController.grantTokenAccessReadOnly ) @@ -1391,3 +1395,5 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.get('*', ErrorController.notFound) } + +module.exports = { initialize, rateLimiters } diff --git a/services/web/package.json b/services/web/package.json index 7787549a2e..8ce337777f 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -232,7 +232,6 @@ "request": "^2.88.2", "requestretry": "^7.1.0", "rimraf": "2.2.6", - "rolling-rate-limiter": "^0.2.10", "sanitize-html": "^1.27.1", "scroll-into-view-if-needed": "^2.2.25", "tsscmp": "^1.0.6", diff --git a/services/web/test/acceptance/src/PasswordUpdateTests.js b/services/web/test/acceptance/src/PasswordUpdateTests.js index 2872684903..c82b2af4f8 100644 --- a/services/web/test/acceptance/src/PasswordUpdateTests.js +++ b/services/web/test/acceptance/src/PasswordUpdateTests.js @@ -1,14 +1,11 @@ const { expect } = require('chai') -const RateLimiter = require('../../../app/src/infrastructure/RateLimiter') +const PasswordResetRouter = require('../../../app/src/Features/PasswordReset/PasswordResetRouter') const UserHelper = require('./helpers/UserHelper') describe('PasswordUpdate', function () { let email, password, response, user, userHelper afterEach(async function () { - await RateLimiter.promises.clearRateLimit( - 'password_reset_rate_limit', - '127.0.0.1' - ) + await PasswordResetRouter.rateLimiter.delete('127.0.0.1') }) beforeEach(async function () { userHelper = new UserHelper() diff --git a/services/web/test/smoke/src/steps/001_clearRateLimits.js b/services/web/test/smoke/src/steps/001_clearRateLimits.js index d090b93293..8ed81bd823 100644 --- a/services/web/test/smoke/src/steps/001_clearRateLimits.js +++ b/services/web/test/smoke/src/steps/001_clearRateLimits.js @@ -1,31 +1,21 @@ -const OError = require('@overleaf/o-error') const Settings = require('@overleaf/settings') -const RateLimiter = require('../../../../app/src/infrastructure/RateLimiter') - -async function clearRateLimit(endpointName, subject) { - try { - await RateLimiter.promises.clearRateLimit(endpointName, subject) - } catch (err) { - throw new OError( - 'error clearing rate limit', - { endpointName, subject }, - err - ) - } -} +const { + overleafLoginRateLimiter, + openProjectRateLimiter, +} = require('../../../../app/src/infrastructure/RateLimiter') +const LoginRateLimiter = require('../../../../app/src/Features/Security/LoginRateLimiter') async function clearLoginRateLimit() { - await clearRateLimit('login', Settings.smokeTest.user) + await LoginRateLimiter.promises.recordSuccessfulLogin(Settings.smokeTest.user) } async function clearOverleafLoginRateLimit() { if (!Settings.overleaf) return - await clearRateLimit('overleaf-login', Settings.smokeTest.rateLimitSubject) + await overleafLoginRateLimiter.delete(Settings.smokeTest.rateLimitSubject) } async function clearOpenProjectRateLimit() { - await clearRateLimit( - 'open-project', + await openProjectRateLimiter.delete( `${Settings.smokeTest.projectId}:${Settings.smokeTest.userId}` ) } diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js index 15a634dcf4..a22672220f 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js @@ -33,7 +33,12 @@ describe('CollaboratorsInviteController', function () { }, } - this.RateLimiter = { addCount: sinon.stub } + this.rateLimiter = { + consume: sinon.stub().resolves(), + } + this.RateLimiter = { + RateLimiter: sinon.stub().returns(this.rateLimiter), + } this.LimitationsManager = {} this.UserGetter = { @@ -1322,11 +1327,12 @@ describe('CollaboratorsInviteController', function () { }) it('should callback with `true` when rate limit under', function (done) { - this.RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true) return this.CollaboratorsInviteController._checkRateLimit( this.sendingUserId, (err, result) => { - this.RateLimiter.addCount.called.should.equal(true) + expect(this.rateLimiter.consume).to.have.been.calledWith( + this.sendingUserId + ) result.should.equal(true) return done() } @@ -1334,51 +1340,59 @@ describe('CollaboratorsInviteController', function () { }) it('should callback with `false` when rate limit hit', function (done) { - this.RateLimiter.addCount = sinon.stub().callsArgWith(1, null, false) + this.rateLimiter.consume.rejects({ remainingPoints: 0 }) return this.CollaboratorsInviteController._checkRateLimit( this.sendingUserId, (err, result) => { - this.RateLimiter.addCount.called.should.equal(true) + expect(this.rateLimiter.consume).to.have.been.calledWith( + this.sendingUserId + ) result.should.equal(false) return done() } ) }) - it('should call rate limiter with 10x the collaborators', function (done) { - this.RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true) - return this.CollaboratorsInviteController._checkRateLimit( + it('should allow 10x the collaborators', function (done) { + this.CollaboratorsInviteController._checkRateLimit( this.sendingUserId, (err, result) => { - this.RateLimiter.addCount.args[0][0].throttle.should.equal(170) + expect(this.rateLimiter.consume).to.have.been.calledWith( + this.sendingUserId, + Math.floor(40000 / 170) + ) return done() } ) }) - it('should call rate limiter with 200 when collaborators is -1', function (done) { + it('should allow 200 requests when collaborators is -1', function (done) { this.LimitationsManager.allowedNumberOfCollaboratorsForUser .withArgs(this.sendingUserId) .yields(null, -1) - this.RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true) - return this.CollaboratorsInviteController._checkRateLimit( + this.CollaboratorsInviteController._checkRateLimit( this.sendingUserId, (err, result) => { - this.RateLimiter.addCount.args[0][0].throttle.should.equal(200) + expect(this.rateLimiter.consume).to.have.been.calledWith( + this.sendingUserId, + Math.floor(40000 / 200) + ) return done() } ) }) - it('should call rate limiter with 10 when user has no collaborators set', function (done) { + it('should allow 10 requests when user has no collaborators set', function (done) { this.LimitationsManager.allowedNumberOfCollaboratorsForUser .withArgs(this.sendingUserId) .yields(null) - this.RateLimiter.addCount = sinon.stub().callsArgWith(1, null, true) return this.CollaboratorsInviteController._checkRateLimit( this.sendingUserId, (err, result) => { - this.RateLimiter.addCount.args[0][0].throttle.should.equal(10) + expect(this.rateLimiter.consume).to.have.been.calledWith( + this.sendingUserId, + Math.floor(40000 / 10) + ) return done() } ) diff --git a/services/web/test/unit/src/Compile/CompileControllerTests.js b/services/web/test/unit/src/Compile/CompileControllerTests.js index bf331cda32..9d1ca505d1 100644 --- a/services/web/test/unit/src/Compile/CompileControllerTests.js +++ b/services/web/test/unit/src/Compile/CompileControllerTests.js @@ -20,7 +20,12 @@ describe('CompileController', function () { this.CompileManager = { compile: sinon.stub() } this.ClsiManager = {} this.UserGetter = { getUser: sinon.stub() } - this.RateLimiter = { addCount: sinon.stub() } + this.rateLimiter = { + consume: sinon.stub().resolves(), + } + this.RateLimiter = { + RateLimiter: sinon.stub().returns(this.rateLimiter), + } this.settings = { apis: { clsi: { @@ -357,7 +362,6 @@ describe('CompileController', function () { describe('when downloading for embedding', function () { beforeEach(function () { this.CompileController.proxyToClsi = sinon.stub() - this.RateLimiter.addCount.callsArgWith(1, null, true) this.CompileController.downloadPdf(this.req, this.res, this.next) }) @@ -398,7 +402,6 @@ describe('CompileController', function () { beforeEach(function () { this.req.params.build_id = this.buildId = '1234-5678' this.CompileController.proxyToClsi = sinon.stub() - this.RateLimiter.addCount.callsArgWith(1, null, true) this.CompileController.downloadPdf(this.req, this.res, this.next) }) @@ -418,9 +421,8 @@ describe('CompileController', function () { describe('when the pdf is not going to be used in pdfjs viewer', function () { it('should check the rate limiter when pdfng is not set', function (done) { this.req.query = {} - this.RateLimiter.addCount.callsArgWith(1, null, true) this.CompileController.proxyToClsi = (projectId, url) => { - this.RateLimiter.addCount.args[0][0].throttle.should.equal(1000) + expect(this.rateLimiter.consume).to.have.been.called done() } this.CompileController.downloadPdf(this.req, this.res) @@ -428,9 +430,8 @@ describe('CompileController', function () { it('should check the rate limiter when pdfng is false', function (done) { this.req.query = { pdfng: false } - this.RateLimiter.addCount.callsArgWith(1, null, true) this.CompileController.proxyToClsi = (projectId, url) => { - this.RateLimiter.addCount.args[0][0].throttle.should.equal(1000) + expect(this.rateLimiter.consume).to.have.been.called done() } this.CompileController.downloadPdf(this.req, this.res) diff --git a/services/web/test/unit/src/Compile/CompileManagerTests.js b/services/web/test/unit/src/Compile/CompileManagerTests.js index 18d1b9085e..52c7d6e017 100644 --- a/services/web/test/unit/src/Compile/CompileManagerTests.js +++ b/services/web/test/unit/src/Compile/CompileManagerTests.js @@ -1,12 +1,17 @@ const { expect } = require('chai') const sinon = require('sinon') -const modulePath = '../../../../app/src/Features/Compile/CompileManager.js' const SandboxedModule = require('sandboxed-module') +const MODULE_PATH = '../../../../app/src/Features/Compile/CompileManager.js' + describe('CompileManager', function () { beforeEach(function () { - this.rateLimitGetStub = sinon.stub() - this.ratelimiter = { addCount: sinon.stub() } + this.rateLimiter = { + consume: sinon.stub().resolves(), + } + this.RateLimiter = { + RateLimiter: sinon.stub().returns(this.rateLimiter), + } this.timer = { done: sinon.stub(), } @@ -14,7 +19,7 @@ describe('CompileManager', function () { Timer: sinon.stub().returns(this.timer), inc: sinon.stub(), } - this.CompileManager = SandboxedModule.require(modulePath, { + this.CompileManager = SandboxedModule.require(MODULE_PATH, { requires: { '@overleaf/settings': (this.settings = { apis: { clsi: { defaultBackendClass: 'e2' } }, @@ -28,7 +33,7 @@ describe('CompileManager', function () { '../Project/ProjectGetter': (this.ProjectGetter = {}), '../User/UserGetter': (this.UserGetter = {}), './ClsiManager': (this.ClsiManager = {}), - '../../infrastructure/RateLimiter': this.ratelimiter, + '../../infrastructure/RateLimiter': this.RateLimiter, '@overleaf/metrics': this.Metrics, '../SplitTests/SplitTestHandler': { getAssignmentForMongoUser: (this.getAssignmentForMongoUser = sinon @@ -419,7 +424,6 @@ describe('CompileManager', function () { describe('_checkIfAutoCompileLimitHasBeenHit', function () { it('should be able to compile if it is not an autocompile', function (done) { - this.ratelimiter.addCount.callsArgWith(2, null, true) this.CompileManager._checkIfAutoCompileLimitHasBeenHit( false, 'everyone', @@ -433,8 +437,7 @@ describe('CompileManager', function () { ) }) - it('should be able to compile if rate limit has remianing', function (done) { - this.ratelimiter.addCount.callsArgWith(1, null, true) + it('should be able to compile if rate limit has remaining', function (done) { this.CompileManager._checkIfAutoCompileLimitHasBeenHit( true, 'everyone', @@ -442,11 +445,7 @@ describe('CompileManager', function () { if (err) { return done(err) } - const args = this.ratelimiter.addCount.args[0][0] - args.throttle.should.equal(12.5) - args.subjectName.should.be.oneOf(['everyone-b-one', 'everyone-b-two']) - args.timeInterval.should.equal(20) - args.endpointName.should.equal('auto_compile') + expect(this.rateLimiter.consume).to.have.been.calledWith('global') canCompile.should.equal(true) done() } @@ -454,7 +453,7 @@ describe('CompileManager', function () { }) it('should be not able to compile if rate limit has no remianing', function (done) { - this.ratelimiter.addCount.callsArgWith(1, null, false) + this.rateLimiter.consume.rejects({ remainingPoints: 0 }) this.CompileManager._checkIfAutoCompileLimitHasBeenHit( true, 'everyone', @@ -469,7 +468,7 @@ describe('CompileManager', function () { }) it('should return false if there is an error in the rate limit', function (done) { - this.ratelimiter.addCount.callsArgWith(1, 'error') + this.rateLimiter.consume.rejects(new Error('BOOM!')) this.CompileManager._checkIfAutoCompileLimitHasBeenHit( true, 'everyone', diff --git a/services/web/test/unit/src/Email/EmailSenderTests.js b/services/web/test/unit/src/Email/EmailSenderTests.js index 185311971a..daf1f0b515 100644 --- a/services/web/test/unit/src/Email/EmailSenderTests.js +++ b/services/web/test/unit/src/Email/EmailSenderTests.js @@ -10,10 +10,11 @@ const MODULE_PATH = path.join( describe('EmailSender', function () { beforeEach(function () { + this.rateLimiter = { + consume: sinon.stub().resolves(), + } this.RateLimiter = { - promises: { - addCount: sinon.stub(), - }, + RateLimiter: sinon.stub().returns(this.rateLimiter), } this.Settings = { @@ -93,7 +94,7 @@ describe('EmailSender', function () { it('should not send an email when the rate limiter says no', async function () { this.opts.sendingUser_id = '12321312321' - this.RateLimiter.promises.addCount.resolves(false) + this.rateLimiter.consume.rejects({ remainingPoints: 0 }) await expect(this.EmailSender.promises.sendEmail(this.opts)).to.be .rejected expect(this.sesClient.sendMail).not.to.have.been.called @@ -101,7 +102,6 @@ describe('EmailSender', function () { it('should send the email when the rate limtier says continue', async function () { this.opts.sendingUser_id = '12321312321' - this.RateLimiter.promises.addCount.resolves(true) await this.EmailSender.promises.sendEmail(this.opts) expect(this.sesClient.sendMail).to.have.been.called }) @@ -109,7 +109,7 @@ describe('EmailSender', function () { it('should not check the rate limiter when there is no sendingUser_id', async function () { this.EmailSender.sendEmail(this.opts, () => { expect(this.sesClient.sendMail).to.have.been.called - expect(this.RateLimiter.promises.addCount).not.to.have.been.called + expect(this.rateLimiter.consume).not.to.have.been.called }) }) diff --git a/services/web/test/unit/src/Security/LoginRateLimiterTests.js b/services/web/test/unit/src/Security/LoginRateLimiterTests.js index 7f6f3e8fd3..645002400e 100644 --- a/services/web/test/unit/src/Security/LoginRateLimiterTests.js +++ b/services/web/test/unit/src/Security/LoginRateLimiterTests.js @@ -1,15 +1,3 @@ -/* eslint-disable - n/handle-callback-err, - max-len, - no-return-assign, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const SandboxedModule = require('sandboxed-module') const sinon = require('sinon') const { expect } = require('chai') @@ -21,113 +9,76 @@ const modulePath = require('path').join( describe('LoginRateLimiter', function () { beforeEach(function () { this.email = 'bob@bob.com' + this.rateLimiter = { + consume: sinon.stub().resolves(), + delete: sinon.stub().resolves(), + } this.RateLimiter = { - clearRateLimit: sinon.stub(), - addCount: sinon.stub(), + RateLimiter: sinon.stub().withArgs('login').returns(this.rateLimiter), } - return (this.LoginRateLimiter = SandboxedModule.require(modulePath, { + this.LoginRateLimiter = SandboxedModule.require(modulePath, { requires: { '../../infrastructure/RateLimiter': this.RateLimiter, }, - })) + }) }) describe('processLoginRequest', function () { - beforeEach(function () { - return (this.RateLimiter.addCount = sinon - .stub() - .callsArgWith(1, null, true)) - }) - - it('should call RateLimiter.addCount', function (done) { - return this.LoginRateLimiter.processLoginRequest( - this.email, - (err, allow) => { - this.RateLimiter.addCount.callCount.should.equal(1) - expect( - this.RateLimiter.addCount.lastCall.args[0].endpointName - ).to.equal('login') - expect( - this.RateLimiter.addCount.lastCall.args[0].subjectName - ).to.equal(this.email) - return done() + it('should consume points', function (done) { + this.LoginRateLimiter.processLoginRequest(this.email, (err, allow) => { + if (err) { + return done(err) } - ) + expect(this.rateLimiter.consume).to.have.been.calledWith(this.email) + done() + }) }) describe('when login is allowed', function () { - beforeEach(function () { - return (this.RateLimiter.addCount = sinon - .stub() - .callsArgWith(1, null, true)) - }) - it('should call pass allow=true', function (done) { - return this.LoginRateLimiter.processLoginRequest( - this.email, - (err, allow) => { - expect(err).to.equal(null) - expect(allow).to.equal(true) - return done() - } - ) + this.LoginRateLimiter.processLoginRequest(this.email, (err, allow) => { + expect(err).to.equal(null) + expect(allow).to.equal(true) + done() + }) }) }) describe('when login is blocked', function () { beforeEach(function () { - return (this.RateLimiter.addCount = sinon - .stub() - .callsArgWith(1, null, false)) + this.rateLimiter.consume.rejects({ remainingPoints: 0 }) }) it('should call pass allow=false', function (done) { - return this.LoginRateLimiter.processLoginRequest( - this.email, - (err, allow) => { - expect(err).to.equal(null) - expect(allow).to.equal(false) - return done() - } - ) + this.LoginRateLimiter.processLoginRequest(this.email, (err, allow) => { + expect(err).to.equal(null) + expect(allow).to.equal(false) + done() + }) }) }) - describe('when addCount produces an error', function () { + describe('when consume produces an error', function () { beforeEach(function () { - return (this.RateLimiter.addCount = sinon - .stub() - .callsArgWith(1, new Error('woops'))) + this.rateLimiter.consume.rejects(new Error('woops')) }) it('should produce an error', function (done) { - return this.LoginRateLimiter.processLoginRequest( - this.email, - (err, allow) => { - expect(err).to.not.equal(null) - expect(err).to.be.instanceof(Error) - return done() - } - ) + this.LoginRateLimiter.processLoginRequest(this.email, (err, allow) => { + expect(err).to.not.equal(null) + expect(err).to.be.instanceof(Error) + done() + }) }) }) }) describe('recordSuccessfulLogin', function () { - beforeEach(function () { - return (this.RateLimiter.clearRateLimit = sinon - .stub() - .callsArgWith(2, null)) - }) - - it('should call clearRateLimit', function (done) { - return this.LoginRateLimiter.recordSuccessfulLogin(this.email, () => { - this.RateLimiter.clearRateLimit.callCount.should.equal(1) - this.RateLimiter.clearRateLimit - .calledWith('login', this.email) - .should.equal(true) - return done() + it('should clear the rate limit', function (done) { + this.LoginRateLimiter.recordSuccessfulLogin(this.email, () => { + expect(this.rateLimiter.delete).to.have.been.calledWith(this.email) + done() }) }) }) diff --git a/services/web/test/unit/src/Security/RateLimiterMiddlewareTests.js b/services/web/test/unit/src/Security/RateLimiterMiddlewareTests.js index 75f358353f..05ab4260e5 100644 --- a/services/web/test/unit/src/Security/RateLimiterMiddlewareTests.js +++ b/services/web/test/unit/src/Security/RateLimiterMiddlewareTests.js @@ -10,13 +10,9 @@ describe('RateLimiterMiddleware', function () { this.SessionManager = { getLoggedInUserId: () => this.req.session?.user?._id, } - this.RateLimiter = { - addCount: sinon.stub().yields(null, true), - } this.RateLimiterMiddleware = SandboxedModule.require(modulePath, { requires: { '@overleaf/settings': (this.settings = {}), - '../../infrastructure/RateLimiter': this.RateLimiter, './LoginRateLimiter': {}, '../Authentication/SessionManager': this.SessionManager, }, @@ -31,162 +27,15 @@ describe('RateLimiterMiddleware', function () { }) describe('rateLimit', function () { - beforeEach(function () { - this.middleware = this.RateLimiterMiddleware.rateLimit({ - endpointName: 'test-endpoint', - params: ['project_id', 'doc_id'], - timeInterval: 42, - maxRequests: 12, - }) - this.req.params = { - project_id: (this.project_id = 'project-id'), - doc_id: (this.doc_id = 'doc-id'), - } - }) - - describe('when there is no session', function () { - beforeEach(function () { - this.req.ip = this.ip = '1.2.3.4' - this.middleware(this.req, this.res, this.next) - }) - - it('should call the rate limiter backend with the ip address', function () { - this.RateLimiter.addCount - .calledWith({ - endpointName: 'test-endpoint', - timeInterval: 42, - throttle: 12, - subjectName: `${this.project_id}:${this.doc_id}:${this.ip}`, - }) - .should.equal(true) - }) - - it('should pass on to next()', function () {}) - }) - - describe('when smoke test user', function () { - beforeEach(function () { - this.req.session = { - user: { - _id: (this.user_id = 'smoke-test-user-id'), - }, - } - this.settings.smokeTest = { userId: this.user_id } - this.middleware(this.req, this.res, this.next) - }) - - it('should not call the rate limiter backend with the user_id', function () { - this.RateLimiter.addCount - .calledWith({ - endpointName: 'test-endpoint', - timeInterval: 42, - throttle: 12, - subjectName: `${this.project_id}:${this.doc_id}:${this.user_id}`, - }) - .should.equal(false) - this.RateLimiter.addCount.callCount.should.equal(0) - }) - - it('should pass on to next()', function () { - this.next.called.should.equal(true) - }) - }) - - describe('when under the rate limit with logged in user', function () { - beforeEach(function () { - this.req.session = { - user: { - _id: (this.user_id = 'user-id'), - }, - } - this.middleware(this.req, this.res, this.next) - }) - - it('should call the rate limiter backend with the user_id', function () { - this.RateLimiter.addCount - .calledWith({ - endpointName: 'test-endpoint', - timeInterval: 42, - throttle: 12, - subjectName: `${this.project_id}:${this.doc_id}:${this.user_id}`, - }) - .should.equal(true) - }) - - it('should pass on to next()', function () { - this.next.called.should.equal(true) - }) - }) - - describe('when under the rate limit with anonymous user', function () { - beforeEach(function () { - this.req.ip = this.ip = '1.2.3.4' - this.middleware(this.req, this.res, this.next) - }) - - it('should call the rate limiter backend with the ip address', function () { - this.RateLimiter.addCount - .calledWith({ - endpointName: 'test-endpoint', - timeInterval: 42, - throttle: 12, - subjectName: `${this.project_id}:${this.doc_id}:${this.ip}`, - }) - .should.equal(true) - }) - - it('should pass on to next()', function () { - this.next.called.should.equal(true) - }) - }) - - describe('when over the rate limit', function () { - beforeEach(function () { - this.req.session = { - user: { - _id: (this.user_id = 'user-id'), - }, - } - this.RateLimiter.addCount.yields(null, false) - this.middleware(this.req, this.res, this.next) - }) - - it('should return a 429', function () { - this.res.status.calledWith(429).should.equal(true) - this.res.end.called.should.equal(true) - }) - - it('should not continue', function () { - this.next.called.should.equal(false) - }) - - it('should log a warning', function () { - this.logger.warn - .calledWith( - { - endpointName: 'test-endpoint', - timeInterval: 42, - throttle: 12, - subjectName: `${this.project_id}:${this.doc_id}:${this.user_id}`, - }, - 'rate limit exceeded' - ) - .should.equal(true) - }) - }) - }) - - describe('rateLimitV2', function () { beforeEach(function () { this.projectId = 'project-id' this.docId = 'doc-id' this.rateLimiter = { consume: sinon.stub().resolves({ remainingPoints: 2 }), } - this.middleware = this.RateLimiterMiddleware.rateLimitV2( - this.rateLimiter, - { params: ['projectId', 'docId'] } - ) + this.middleware = this.RateLimiterMiddleware.rateLimit(this.rateLimiter, { + params: ['projectId', 'docId'], + }) this.req.params = { projectId: this.projectId, docId: this.docId } })