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

@@ -53,8 +53,9 @@
       "port": "8080",
       "rateLimit" : {
         "public": {
-          "windowMs": "900000",
-          "max": "100"
+          "limitApiKey": "120",
+          "limitFrontend": "500",
+          "windowMs": "60000",
         }
       },
       "encryption": {

app/config/default.json

@@ -53,8 +53,9 @@
     "port": "8080",
     "rateLimit": {
       "public": {
-        "windowMs": "60000",
-        "max": "120"
+        "limitApiKey": "120",
+        "limitFrontend": "500",
+        "windowMs": "60000"
       }
     },
     "encryption": {

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",

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',

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:
 

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();
     });