From 8fe13dbb2355b9bcb418c611fc4a85b285cd491a Mon Sep 17 00:00:00 2001 From: Kalista Payne Date: Wed, 6 May 2026 15:35:56 -0500 Subject: [PATCH] Revert "Improve rate limit handling (#15649)" This reverts commit 1482f6c225641f593527575d9da513fc8b2fe1d4. --- test/api/unit/middlewares/rateLimiter.test.js | 36 ++--- website/server/middlewares/appRoutes.js | 18 +-- website/server/middlewares/rateLimiter.js | 133 +++++++++--------- 3 files changed, 79 insertions(+), 108 deletions(-) diff --git a/test/api/unit/middlewares/rateLimiter.test.js b/test/api/unit/middlewares/rateLimiter.test.js index 74d8f26417..f7f418e5c5 100644 --- a/test/api/unit/middlewares/rateLimiter.test.js +++ b/test/api/unit/middlewares/rateLimiter.test.js @@ -32,8 +32,7 @@ describe('rateLimiter middleware', () => { it('is disabled when the env var is not defined', () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns(undefined); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; attachRateLimiter(req, res, next); expect(next).to.have.been.calledOnce; @@ -44,8 +43,7 @@ describe('rateLimiter middleware', () => { it('is disabled when the env var is an not "true"', () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('false'); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; attachRateLimiter(req, res, next); expect(next).to.have.been.calledOnce; @@ -57,8 +55,7 @@ describe('rateLimiter middleware', () => { it('does not throw when there are available points', async () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true'); nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; await attachRateLimiter(req, res, next); expect(next).to.have.been.calledOnce; @@ -80,8 +77,7 @@ describe('rateLimiter middleware', () => { sandbox.stub(RateLimiterMemory.prototype, 'consume') .returns(Promise.reject(new Error('Unknown error.'))); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; await attachRateLimiter(req, res, next); expect(next).to.have.been.calledOnce; @@ -96,8 +92,7 @@ describe('rateLimiter middleware', () => { it('does not throw when LIVELINESS_PROBE_KEY is correct', async () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true'); nconfGetStub.withArgs('LIVELINESS_PROBE_KEY').returns('abc'); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; req.query.liveliness = 'abc'; await attachRateLimiter(req, res, next); @@ -112,8 +107,7 @@ describe('rateLimiter middleware', () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true'); nconfGetStub.withArgs('LIVELINESS_PROBE_KEY').returns('abc'); nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; req.query.liveliness = 'das'; await attachRateLimiter(req, res, next); @@ -130,8 +124,7 @@ describe('rateLimiter middleware', () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true'); nconfGetStub.withArgs('LIVELINESS_PROBE_KEY').returns(undefined); nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; await attachRateLimiter(req, res, next); @@ -147,8 +140,7 @@ describe('rateLimiter middleware', () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true'); nconfGetStub.withArgs('LIVELINESS_PROBE_KEY').returns(''); nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; req.query.liveliness = ''; await attachRateLimiter(req, res, next); @@ -164,8 +156,7 @@ describe('rateLimiter middleware', () => { it('throws when there are no available points remaining', async () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true'); nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; // call for 31 times for (let i = 0; i < 31; i += 1) { @@ -189,8 +180,7 @@ describe('rateLimiter middleware', () => { it('uses the user id if supplied or the ip address', async () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true'); nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(1); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; req.ip = 1; await attachRateLimiter(req, res, next); @@ -220,8 +210,7 @@ describe('rateLimiter middleware', () => { it('applies increased cost for registration calls with and without user id', async () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true'); nconfGetStub.withArgs('RATE_LIMITER_REGISTRATION_COST').returns(3); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; req.path = '/api/v4/user/auth/local/register'; req.ip = 1; @@ -252,8 +241,7 @@ describe('rateLimiter middleware', () => { it('applies increased cost for unauthenticated API calls', async () => { nconfGetStub.withArgs('RATE_LIMITER_ENABLED').returns('true'); nconfGetStub.withArgs('RATE_LIMITER_IP_COST').returns(10); - const setupRateLimiter = requireAgain(pathToRateLimiter).default; - const attachRateLimiter = setupRateLimiter(); + const attachRateLimiter = requireAgain(pathToRateLimiter).default; req.ip = 1; await attachRateLimiter(req, res, next); diff --git a/website/server/middlewares/appRoutes.js b/website/server/middlewares/appRoutes.js index f4530e8fe7..eb390e0e77 100644 --- a/website/server/middlewares/appRoutes.js +++ b/website/server/middlewares/appRoutes.js @@ -1,9 +1,8 @@ -import nconf from 'nconf'; import express from 'express'; import expressValidator from 'express-validator'; import path from 'path'; import setupBody from './setupBody'; -import setupRateLimiter from './rateLimiter'; +import rateLimiter from './rateLimiter'; import setupExpress from '../libs/setupExpress'; import * as routes from '../libs/routes'; @@ -11,9 +10,6 @@ const API_V3_CONTROLLERS_PATH = path.join(__dirname, '/../controllers/api-v3/'); const API_V4_CONTROLLERS_PATH = path.join(__dirname, '/../controllers/api-v4/'); const TOP_LEVEL_CONTROLLERS_PATH = path.join(__dirname, '/../controllers/top-level/'); -const RATE_LIMITER_V4_POINTS = nconf.get('RATE_LIMITER_V4_POINTS') || 100; -const RATE_LIMITER_V4_REGISTRATION_COST = nconf.get('RATE_LIMITER_V4_REGISTRATION_COST') || 10; - const app = express(); // re-set the view options because they are not inherited from the top level app @@ -29,10 +25,7 @@ app.use('/', topLevelRouter); const v3Router = express.Router(); // eslint-disable-line new-cap routes.walkControllers(v3Router, API_V3_CONTROLLERS_PATH); -const v3RateLimiter = setupRateLimiter({ - keyPrefix: 'api-v3', -}); -app.use('/api/v3', v3RateLimiter, v3Router); +app.use('/api/v3', rateLimiter, v3Router); // API v4 proxies API v3 routes by default. // It can also disable or override v3 routes @@ -56,11 +49,6 @@ const v4RouterOverrides = [ const v4Router = express.Router(); // eslint-disable-line new-cap routes.walkControllers(v4Router, API_V3_CONTROLLERS_PATH, v4RouterOverrides); routes.walkControllers(v4Router, API_V4_CONTROLLERS_PATH); -const v4RateLimiter = setupRateLimiter({ - keyPrefix: 'api-v4', - points: RATE_LIMITER_V4_POINTS, - registrationCost: RATE_LIMITER_V4_REGISTRATION_COST, -}); -app.use('/api/v4', v4RateLimiter, v4Router); +app.use('/api/v4', v4Router); export default app; diff --git a/website/server/middlewares/rateLimiter.js b/website/server/middlewares/rateLimiter.js index f6d615d6ef..b47e9e9eec 100644 --- a/website/server/middlewares/rateLimiter.js +++ b/website/server/middlewares/rateLimiter.js @@ -23,39 +23,55 @@ const REDIS_HOST = nconf.get('REDIS_HOST'); const REDIS_PASSWORD = nconf.get('REDIS_PASSWORD'); const REDIS_PORT = nconf.get('REDIS_PORT'); const LIVELINESS_PROBE_KEY = nconf.get('LIVELINESS_PROBE_KEY'); -const BASE_POINTS = nconf.get('RATE_LIMITER_BASE_POINTS') || 30; -const BASE_DURATION = nconf.get('RATE_LIMITER_BASE_DURATION') || 60; const REGISTRATION_COST = nconf.get('RATE_LIMITER_REGISTRATION_COST') || 5; const IP_RATE_LIMIT_COST = nconf.get('RATE_LIMITER_IP_COST') || 5; let redisClient; +let rateLimiter; -if (RATE_LIMITER_ENABLED && !IS_TEST) { - redisClient = redis.createClient({ - host: REDIS_HOST, - password: REDIS_PASSWORD, - port: REDIS_PORT, - enable_offline_queue: false, - }); +const rateLimiterOpts = { + keyPrefix: 'api-v3', + points: 30, // 30 requests + duration: 60, // per 1 minute by User ID or IP +}; - redisClient.on('ready', () => { - SERVER_STATUS.REDIS = true; - }); +if (RATE_LIMITER_ENABLED) { + if (IS_TEST) { + rateLimiter = new RateLimiterMemory({ + ...rateLimiterOpts, + }); + } else { + redisClient = redis.createClient({ + host: REDIS_HOST, + password: REDIS_PASSWORD, + port: REDIS_PORT, + enable_offline_queue: false, + }); - redisClient.on('reconnecting', () => { - SERVER_STATUS.REDIS = false; - }); + redisClient.on('ready', () => { + SERVER_STATUS.REDIS = true; + }); - redisClient.on('error', error => { - logger.error(error, 'Redis Error'); - }); + redisClient.on('reconnecting', () => { + SERVER_STATUS.REDIS = false; + }); + + redisClient.on('error', error => { + logger.error(error, 'Redis Error'); + }); + + rateLimiter = new RateLimiterRedis({ + ...rateLimiterOpts, + storeClient: redisClient, + }); + } } else { SERVER_STATUS.REDIS = true; } -function setResponseHeaders (res, points, rateLimiterRes) { +function setResponseHeaders (res, rateLimiterRes) { const headers = { - 'X-RateLimit-Limit': points, + 'X-RateLimit-Limit': rateLimiterOpts.points, 'X-RateLimit-Remaining': rateLimiterRes.remainingPoints, 'X-RateLimit-Reset': new Date(Date.now() + rateLimiterRes.msBeforeNext), }; @@ -67,55 +83,34 @@ function setResponseHeaders (res, points, rateLimiterRes) { res.set(headers); } -export default function setupRateLimiter (options = {}) { - const rateLimiterOpts = { - keyPrefix: options.keyPrefix || 'api', - points: options.points || BASE_POINTS, // 30 requests - duration: options.duration || BASE_DURATION, // per 1 minute by User ID or IP - }; - let rateLimiter; - if (!RATE_LIMITER_ENABLED) { - return (req, res, next) => next(); +export default function rateLimiterMiddleware (req, res, next) { + if (!RATE_LIMITER_ENABLED) return next(); + if (LIVELINESS_PROBE_KEY && req.query.liveliness === LIVELINESS_PROBE_KEY) return next(); + + const userId = req.header('x-api-user'); + + let cost = 1; + if (req.path === '/api/v4/user/auth/local/register' || req.path === '/api/v3/user/auth/local/register') { + cost = REGISTRATION_COST; + } else if (!userId) { + cost = IP_RATE_LIMIT_COST; } - if (IS_TEST) { - rateLimiter = new RateLimiterMemory({ - ...rateLimiterOpts, + + return rateLimiter.consume(userId || req.ip, cost) + .then(rateLimiterRes => { + setResponseHeaders(res, rateLimiterRes); + return next(); + }) + .catch(rateLimiterRes => { + if (rateLimiterRes instanceof RateLimiterRes) { + setResponseHeaders(res, rateLimiterRes); + return next(new TooManyRequests(apiError('clientRateLimited'))); + } + + // In case of an unhandled error we skip the middleware as it could mean + // , for example, that the connection to the redis database is not working. + // We do not want to block all requests in these cases. + logger.error(rateLimiterRes, 'Rate Limiter Error'); + return next(); }); - } else { - rateLimiter = new RateLimiterRedis({ - ...rateLimiterOpts, - storeClient: redisClient, - }); - } - return function rateLimiterMiddleware (req, res, next) { - if (!RATE_LIMITER_ENABLED) return next(); - if (LIVELINESS_PROBE_KEY && req.query.liveliness === LIVELINESS_PROBE_KEY) return next(); - - const userId = req.header('x-api-user'); - - let cost = 1; - if (req.path === '/api/v4/user/auth/local/register' || req.path === '/api/v3/user/auth/local/register') { - cost = options.registrationCost || REGISTRATION_COST; - } else if (!userId) { - cost = options.ipRateLimitCost || IP_RATE_LIMIT_COST; - } - - return rateLimiter.consume(userId || req.ip, cost) - .then(rateLimiterRes => { - setResponseHeaders(res, rateLimiterOpts.points, rateLimiterRes); - return next(); - }) - .catch(rateLimiterRes => { - if (rateLimiterRes instanceof RateLimiterRes) { - setResponseHeaders(res, rateLimiterOpts.points, rateLimiterRes); - return next(new TooManyRequests(apiError('clientRateLimited'))); - } - - // In case of an unhandled error we skip the middleware as it could mean - // , for example, that the connection to the redis database is not working. - // We do not want to block all requests in these cases. - logger.error(rateLimiterRes, 'Rate Limiter Error'); - return next(); - }); - }; }