From 67d7aae27edc353bcf61ca6fe50e631c1cb8701a Mon Sep 17 00:00:00 2001 From: Walter Moar Date: Wed, 25 Sep 2024 10:34:54 -0700 Subject: [PATCH] fix: FORMS-1303 api rate limiting for all users (#1504) * fix: FORMS-1303 api rate limiting for all users Applying API rate limiting also to the web application users, not just those using an API Key. * tests: FORMS-1303 fix the unit tests for new code * docs: use active voice, clarifications * fix: rate limit too low for frontend, use different values --- .devcontainer/chefs_local/local.json.sample | 5 +- app/config/default.json | 5 +- app/package-lock.json | 14 +++--- app/package.json | 2 +- .../forms/common/middleware/rateLimiter.js | 35 ++++++++++++-- app/tests/unit/README.md | 31 ++++++------- .../common/middleware/rateLimiter.spec.js | 46 ++++++++++++++----- 7 files changed, 94 insertions(+), 44 deletions(-) diff --git a/.devcontainer/chefs_local/local.json.sample b/.devcontainer/chefs_local/local.json.sample index 676f2edd9..345c3b721 100644 --- a/.devcontainer/chefs_local/local.json.sample +++ b/.devcontainer/chefs_local/local.json.sample @@ -53,8 +53,9 @@ "port": "8080", "rateLimit" : { "public": { - "windowMs": "900000", - "max": "100" + "limitApiKey": "120", + "limitFrontend": "500", + "windowMs": "60000", } }, "encryption": { diff --git a/app/config/default.json b/app/config/default.json index 931c2592b..d2f8eb24f 100644 --- a/app/config/default.json +++ b/app/config/default.json @@ -53,8 +53,9 @@ "port": "8080", "rateLimit": { "public": { - "windowMs": "60000", - "max": "120" + "limitApiKey": "120", + "limitFrontend": "500", + "windowMs": "60000" } }, "encryption": { diff --git a/app/package-lock.json b/app/package-lock.json index 7a5139aa5..9a3c48f0e 100644 --- a/app/package-lock.json +++ b/app/package-lock.json @@ -23,7 +23,7 @@ "cryptr": "^6.3.0", "express": "^4.21.0", "express-basic-auth": "^1.2.1", - "express-rate-limit": "^7.2.0", + "express-rate-limit": "^7.4.0", "express-winston": "^4.2.0", "falsey": "^1.0.0", "fs-extra": "^10.1.0", @@ -7695,9 +7695,9 @@ } }, "node_modules/express-rate-limit": { - "version": "7.2.0", - "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-7.2.0.tgz", - "integrity": "sha512-T7nul1t4TNyfZMJ7pKRKkdeVJWa2CqB8NA1P8BwYaoDI5QSBZARv5oMS43J7b7I5P+4asjVXjb7ONuwDKucahg==", + "version": "7.4.0", + "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-7.4.0.tgz", + "integrity": "sha512-v1204w3cXu5gCDmAvgvzI6qjzZzoMWKnyVDk3ACgfswTQLYiGen+r8w0VnXnGMmzEN/g8fwIQ4JrFFd4ZP6ssg==", "engines": { "node": ">= 16" }, @@ -19232,9 +19232,9 @@ } }, "express-rate-limit": { - "version": "7.2.0", - "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-7.2.0.tgz", - "integrity": "sha512-T7nul1t4TNyfZMJ7pKRKkdeVJWa2CqB8NA1P8BwYaoDI5QSBZARv5oMS43J7b7I5P+4asjVXjb7ONuwDKucahg==", + "version": "7.4.0", + "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-7.4.0.tgz", + "integrity": "sha512-v1204w3cXu5gCDmAvgvzI6qjzZzoMWKnyVDk3ACgfswTQLYiGen+r8w0VnXnGMmzEN/g8fwIQ4JrFFd4ZP6ssg==", "requires": {} }, "express-winston": { diff --git a/app/package.json b/app/package.json index de52307fa..164e997e8 100644 --- a/app/package.json +++ b/app/package.json @@ -61,7 +61,7 @@ "cryptr": "^6.3.0", "express": "^4.21.0", "express-basic-auth": "^1.2.1", - "express-rate-limit": "^7.2.0", + "express-rate-limit": "^7.4.0", "express-winston": "^4.2.0", "falsey": "^1.0.0", "fs-extra": "^10.1.0", diff --git a/app/src/forms/common/middleware/rateLimiter.js b/app/src/forms/common/middleware/rateLimiter.js index b31d9ec69..5c254b51d 100644 --- a/app/src/forms/common/middleware/rateLimiter.js +++ b/app/src/forms/common/middleware/rateLimiter.js @@ -1,14 +1,41 @@ const config = require('config'); const rateLimit = require('express-rate-limit'); +/** + * Returns true if the caller is using an API Key (Basic auth). Note that the + * web application will have no auth for unauthenticated (public) users, or + * Bearer auth for authenticated users. + * + * @param {string} authorizationHeader the HTTP request's authorization header. + * @returns a boolean that is true if the caller is using Basic auth. + */ +const _isApiKeyUser = (authorizationHeader) => { + return authorizationHeader && authorizationHeader.startsWith('Basic '); +}; + +/** + * Gets the rate limit to use depending on whether or not the call is using an + * API Key (Basic auth). + * + * @param {string} authorizationHeader the HTTP request's authorization header. + */ +const _getRateLimit = (authorizationHeader) => { + let rateLimit; + + if (_isApiKeyUser(authorizationHeader)) { + rateLimit = config.get('server.rateLimit.public.limitApiKey'); + } else { + rateLimit = config.get('server.rateLimit.public.limitFrontend'); + } + + return rateLimit; +}; + const apiKeyRateLimiter = rateLimit({ // Instead of legacy headers use the standardHeaders version defined below. legacyHeaders: false, - limit: config.get('server.rateLimit.public.max'), - - // Skip everything except Basic auth so that CHEFS app users are not limited. - skip: (req) => !req.headers?.authorization || !req.headers.authorization.startsWith('Basic '), + limit: (req) => _getRateLimit(req.headers?.authorization), // Use the latest draft of the IETF standard for rate limiting headers. standardHeaders: 'draft-7', diff --git a/app/tests/unit/README.md b/app/tests/unit/README.md index ae2c15b5f..1fb6e88a7 100644 --- a/app/tests/unit/README.md +++ b/app/tests/unit/README.md @@ -56,31 +56,30 @@ Similar to `.only` is the `.skip` modifier to skip a test or group of tests. ## Testing Strategy -The testing strategy for the backend unit tests can be broken down into the different layers of the backend. For all tests we should: +The testing strategy for the backend unit tests can be broken down into the different layers of the backend. For all tests: -- ensure that the tests are consistent -- ensure that we have 100% test coverage -- ensure that we have complete test coverage: we should be testing additional corner cases even once we reach 100% test coverage -- test the interface, not the implementation -- test the unit under test, not its dependencies +- Ensure that the tests are consistent: be sure to completely understand similar tests before creating a new test +- Ensure that we have 100% test coverage +- Ensure that we have complete test coverage: we should be testing additional corner cases even once we reach 100% test coverage +- Test the interface, not the implementation +- Test the unit under test, not its dependencies ### Middleware Testing -The tests for the middleware files should: +The tests for the middleware files: -- mock all services calls used by the middleware, including both exception and minimal valid results -- test all response codes produced by the middleware +- Mock all services calls used by the middleware, including both exception and minimal valid results +- Test all response codes produced by the middleware ### Route Testing -The tests for the `route.js` files should: +The tests for the `route.js` files: -- mock all middleware used by the file -- each route test should check that every middleware is called the proper number of times -- each route test should mock the controller function that it calls -- mock controller functions with `res.sendStatus(200)`, as doing something like `next()` will call multiple controller functions when route paths are overloaded -- check that the mocked controller function is called - this will catch when a new route path accidentally overloads an old one -- for consistency and ease of comparison, alphabetize the expect clauses ("alphabetize when possible") +- Mock middleware used by the file +- Each route test mocks its controller function with `res.sendStatus(200)`, as doing something like `next()` calls multiple controller functions when route paths are overloaded +- Check that the route calls every middleware the proper number of times - should be 0 or 1 +- Check that the route calls the expected controller function - this will catch when a new route path accidentally overloads an old one +- Alphabetize the expect clauses ("alphabetize when possible") for consistency and ease of comparison Note: diff --git a/app/tests/unit/forms/common/middleware/rateLimiter.spec.js b/app/tests/unit/forms/common/middleware/rateLimiter.spec.js index 8268c5fd8..1fd44b948 100644 --- a/app/tests/unit/forms/common/middleware/rateLimiter.spec.js +++ b/app/tests/unit/forms/common/middleware/rateLimiter.spec.js @@ -3,14 +3,19 @@ const uuid = require('uuid'); const { apiKeyRateLimiter } = require('../../../../../src/forms/common/middleware'); -const rateLimit = 7; +const rateLimitApiKey = 5; +const rateLimitFrontend = 7; const rateWindowSeconds = 11; jest.mock('config', () => { return { get: jest.fn((key) => { - if (key === 'server.rateLimit.public.max') { - return rateLimit; + if (key === 'server.rateLimit.public.limitApiKey') { + return rateLimitApiKey; + } + + if (key === 'server.rateLimit.public.limitFrontend') { + return rateLimitFrontend; } if (key === 'server.rateLimit.public.windowMs') { @@ -22,9 +27,11 @@ jest.mock('config', () => { // Headers for Draft 7 of the standard. const rateLimitName = 'RateLimit'; -const rateLimitValue = `limit=${rateLimit}, remaining=${rateLimit - 1}, reset=${rateWindowSeconds}`; +const rateLimitValueApiKey = `limit=${rateLimitApiKey}, remaining=${rateLimitApiKey - 1}, reset=${rateWindowSeconds}`; +const rateLimitValueFrontend = `limit=${rateLimitFrontend}, remaining=${rateLimitFrontend - 1}, reset=${rateWindowSeconds}`; const rateLimitPolicyName = 'RateLimit-Policy'; -const rateLimitPolicyValue = `${rateLimit};w=${rateWindowSeconds}`; +const rateLimitPolicyValueApiKey = `${rateLimitApiKey};w=${rateWindowSeconds}`; +const rateLimitPolicyValueFrontend = `${rateLimitFrontend};w=${rateWindowSeconds}`; const ipAddress = '1.2.3.4'; @@ -54,8 +61,8 @@ describe('apiKeyRateLimiter', () => { expect(res.setHeader).toBeCalledTimes(2); // These also test that the rate limiter uses our custom config values. - expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValue); - expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValue); + expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueApiKey); + expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueApiKey); expect(next).toBeCalledTimes(1); expect(next).toBeCalledWith(); }); @@ -70,7 +77,10 @@ describe('apiKeyRateLimiter', () => { await apiKeyRateLimiter(req, res, next); - expect(res.setHeader).toBeCalledTimes(0); + expect(res.setHeader).toBeCalledTimes(2); + // These also test that the rate limiter uses our custom config values. + expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueFrontend); + expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueFrontend); expect(next).toBeCalledTimes(1); expect(next).toBeCalledWith(); }); @@ -85,7 +95,10 @@ describe('apiKeyRateLimiter', () => { await apiKeyRateLimiter(req, res, next); - expect(res.setHeader).toBeCalledTimes(0); + expect(res.setHeader).toBeCalledTimes(2); + // These also test that the rate limiter uses our custom config values. + expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueFrontend); + expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueFrontend); expect(next).toBeCalledTimes(1); expect(next).toBeCalledWith(); }); @@ -102,7 +115,10 @@ describe('apiKeyRateLimiter', () => { await apiKeyRateLimiter(req, res, next); - expect(res.setHeader).toBeCalledTimes(0); + expect(res.setHeader).toBeCalledTimes(2); + // These also test that the rate limiter uses our custom config values. + expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueFrontend); + expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueFrontend); expect(next).toBeCalledTimes(1); expect(next).toBeCalledWith(); }); @@ -119,7 +135,10 @@ describe('apiKeyRateLimiter', () => { await apiKeyRateLimiter(req, res, next); - expect(res.setHeader).toBeCalledTimes(0); + expect(res.setHeader).toBeCalledTimes(2); + // These also test that the rate limiter uses our custom config values. + expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueFrontend); + expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueFrontend); expect(next).toBeCalledTimes(1); expect(next).toBeCalledWith(); }); @@ -136,7 +155,10 @@ describe('apiKeyRateLimiter', () => { await apiKeyRateLimiter(req, res, next); - expect(res.setHeader).toBeCalledTimes(0); + expect(res.setHeader).toBeCalledTimes(2); + // These also test that the rate limiter uses our custom config values. + expect(res.setHeader).toHaveBeenNthCalledWith(1, rateLimitPolicyName, rateLimitPolicyValueFrontend); + expect(res.setHeader).toHaveBeenNthCalledWith(2, rateLimitName, rateLimitValueFrontend); expect(next).toBeCalledTimes(1); expect(next).toBeCalledWith(); });