feat: validation check for double-counting hits for a single request (#…

…364)

* Validation check for double-counting hits for a single request
This implements the idea discussed in #356 (comment) to check for cases of double-counting a single request.

There's probably some situations that this doesn't account for, but I tried to cover the main ones.

We'll want to update the PreciseMemoryStore and set it to `localKeys = true` after merging this.

---------

Co-authored-by: Vedant K <gamemaker0042@gmail.com>

source/lib.ts

@@ -307,6 +307,8 @@ const rateLimit = (
 			// Increment the client's hit counter by one
 			const { totalHits, resetTime } = await config.store.increment(key)
 
+			config.validations.singleCount(request, config.store, key)
+
 			// Get the quota (max number of hits) for each client
 			const retrieveQuota =
 				typeof config.max === 'function'

source/memory-store.ts

@@ -46,6 +46,12 @@ export default class MemoryStore implements Store {
 	 */
 	interval?: NodeJS.Timer
 
+	/**
+	 * Confirmation that the keys incremented in once instance of MemoryStore
+	 * cannot affect other instances.
+	 */
+	localKeys = true
+
 	/**
 	 * Method that initializes the store.
 	 *

source/types.ts

@@ -162,6 +162,15 @@ export type Store = {
 	 * Method to shutdown the store, stop timers, and release all resources.
 	 */
 	shutdown?: () => Promise<void> | void
+
+	/**
+	 * Flag to indicate that keys incremented in one instance of this store can
+	 * not affect other instances. Typically false if a database is used, true for
+	 * MemoryStore.
+	 *
+	 * Used to help detect double-counting misconfigurations.
+	 */
+	localKeys?: boolean
 }
 
 /**

source/validations.ts

@@ -3,6 +3,7 @@
 
 import { isIP } from 'node:net'
 import type { Request } from 'express'
+import type { Store } from './types'
 
 /**
  * An error thrown/returned when a validation error occurs.
@@ -34,6 +35,21 @@ class ValidationError extends Error {
  * The validations that can be run, as well as the methods to run them.
  */
 export class Validations {
+	/**
+	 * Maps the key used in a store for a certain request, and ensures that the
+	 * same key isn't used more than once per request.
+	 *
+	 * The store can be any one of the following:
+	 *  - An instance, for stores like the MemoryStore where two instances do not
+	 *    share state.
+	 *  - A string (class name), for stores where multiple instances
+	 *    typically share state, such as the Redis store.
+	 */
+	private static readonly singleCountKeys = new WeakMap<
+		Request,
+		Map<Store | string, string[]>
+	>()
+
 	// eslint-disable-next-line @typescript-eslint/parameter-properties
 	enabled: boolean
 
@@ -121,6 +137,41 @@ export class Validations {
 		})
 	}
 
+	/**
+	 * Ensures a given key is incremented only once per request.
+	 *
+	 * @param request {Request} - The Express request object.
+	 * @param store {Store} - The store class.
+	 * @param key {string} - The key used to store the client's hit count.
+	 *
+	 * @returns {void}
+	 */
+	singleCount(request: Request, store: Store, key: string) {
+		this.wrap(() => {
+			let storeKeys = Validations.singleCountKeys.get(request)
+			if (!storeKeys) {
+				storeKeys = new Map()
+				Validations.singleCountKeys.set(request, storeKeys)
+			}
+
+			const storeKey = store.localKeys ? store : store.constructor.name
+			let keys = storeKeys.get(storeKey)
+			if (!keys) {
+				keys = []
+				storeKeys.set(storeKey, keys)
+			}
+
+			if (keys.includes(key)) {
+				throw new ValidationError(
+					'ERR_ERL_DOUBLE_COUNT',
+					`The hit count for ${key} was incremented more than once for a single request.`,
+				)
+			}
+
+			keys.push(key)
+		})
+	}
+
 	private wrap(validation: () => void) {
 		if (!this.enabled) {
 			return

test/library/validation-test.ts

@@ -10,6 +10,7 @@ import {
 	afterEach,
 } from '@jest/globals'
 import { Validations } from '../../source/validations.js'
+import type { Store } from '../../source/types'
 
 describe('validations tests', () => {
 	let validations = new Validations(true)
@@ -59,6 +60,7 @@ describe('validations tests', () => {
 			validations.trustProxy({ app: { get: () => true } } as any)
 			expect(console.error).toBeCalled()
 		})
+
 		it('should not log an error on "trust proxy" != true', () => {
 			validations.trustProxy({ app: { get: () => false } } as any)
 			validations.trustProxy({ app: { get: () => '1.2.3.4' } } as any)
@@ -83,6 +85,7 @@ describe('validations tests', () => {
 				headers: {},
 			} as any)
 			expect(console.error).not.toBeCalled()
+
 			validations.xForwardedForHeader({
 				app: { get: () => false },
 				headers: { 'x-forwarded-for': '1.2.3.4' },
@@ -91,6 +94,66 @@ describe('validations tests', () => {
 		})
 	})
 
+	describe('singleCount', () => {
+		class TestExternalStore {} // eslint-disable-line @typescript-eslint/no-extraneous-class
+
+		it('should log an error if a request is double-counted with a MemoryStore', () => {
+			const request = {}
+			const store = { localKeys: true }
+			const key = '1.2.3.4'
+
+			validations.singleCount(request as any, store as Store, key)
+			expect(console.error).not.toBeCalled()
+			validations.singleCount(request as any, store as Store, key)
+			expect(console.error).toBeCalled()
+		})
+
+		it('should log an error if a request is double-counted with an external store', () => {
+			const request = {}
+			const store = new TestExternalStore()
+			const key = '1.2.3.4'
+
+			validations.singleCount(request as any, store as Store, key)
+			expect(console.error).not.toBeCalled()
+			validations.singleCount(request as any, store as Store, key)
+			expect(console.error).toBeCalled()
+		})
+
+		it('should not log an error if a request is double-counted with separate instances of MemoryStore', () => {
+			const request = {}
+			const store1 = { localKeys: true }
+			const store2 = { localKeys: true }
+			const key = '1.2.3.4'
+
+			validations.singleCount(request as any, store1 as Store, key)
+			validations.singleCount(request as any, store2 as Store, key)
+			expect(console.error).not.toBeCalled()
+		})
+
+		it('should log an error if a request is double-counted with separate instances of an external store', () => {
+			const request = {}
+			const store1 = new TestExternalStore()
+			const store2 = new TestExternalStore()
+			const key = '1.2.3.4'
+
+			validations.singleCount(request as any, store1 as Store, key)
+			validations.singleCount(request as any, store2 as Store, key)
+			expect(console.error).toBeCalled()
+		})
+
+		it('should not log an error for multiple requests from the same key', () => {
+			const request1 = {}
+			const request2 = {}
+			const store = { localKeys: true }
+			const key = '1.2.3.4'
+
+			validations.singleCount(request1 as any, store as Store, key)
+			expect(console.error).not.toBeCalled()
+			validations.singleCount(request2 as any, store as Store, key)
+			expect(console.error).not.toBeCalled()
+		})
+	})
+
 	describe('disable', () => {
 		it('should initialize disabled when passed false', () => {
 			const disabledValidator = new Validations(false)