diff --git a/source/lib.ts b/source/lib.ts index 1aea05bb..abb02377 100644 --- a/source/lib.ts +++ b/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' diff --git a/source/memory-store.ts b/source/memory-store.ts index d88840d0..d5f677a5 100644 --- a/source/memory-store.ts +++ b/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. * diff --git a/source/types.ts b/source/types.ts index 755be846..0f90a6c8 100644 --- a/source/types.ts +++ b/source/types.ts @@ -162,6 +162,15 @@ export type Store = { * Method to shutdown the store, stop timers, and release all resources. */ shutdown?: () => Promise | 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 } /** diff --git a/source/validations.ts b/source/validations.ts index 8a1c1699..705148d3 100644 --- a/source/validations.ts +++ b/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 + >() + // 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 diff --git a/test/library/validation-test.ts b/test/library/validation-test.ts index c963dff2..a2a8476b 100644 --- a/test/library/validation-test.ts +++ b/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)