Repository: express-rate-limit/express-rate-limit

Issue:
Multiple rate limits with different window sizes not working properly
### Description

When I have multiple rate limits with different window sizes, the ratelimit-reset is changed to the value of the previous call.

I am using RedisStore, but I think this problem also occours with default store.

Rate limits:

```ts
import rateLimit from 'express-rate-limit';
import RedisStore from 'rate-limit-redis';
import { createClient } from 'redis';

const client = createClient();

client.connect();

export const apiLimiter = rateLimit({
  windowMs: 30 * 1000, // 30 seconds
  max: 100,
  standardHeaders: true,
  legacyHeaders: false,
  store: new RedisStore({
    sendCommand: (...args: string[]) => client.sendCommand(args),
  }),
});

export const paymentLimiter = rateLimit({
  windowMs: 1000, // 1 second
  max: 1,
  standardHeaders: true,
  legacyHeaders: false,
  store: new RedisStore({
    sendCommand: (...args: string[]) => client.sendCommand(args),
  }),
});
```

Routes:

```
app.use('/users', apiLimiter);
app.use('/purchases', paymentLimiter);
```

When I call routes in these sequence:

GET /purchases
GET /users/123
GET /purchases

the value of ratelimit-reset are:
1
30
27 (considering 3 seconds after the previous call)

Maybe I didn't understand the logic behind multple rate limits in one API, but I think the ratelimit should be stored separately by route in that cases.



### Library version

6.8.0

### Node version

v18.16.1

### Typescript version (if you are using it)

4.8.4

### Module system

CommonJS

Discussion:
@gamemaker1: Hi,

You are right, the reset header should show 1 for the last request. Let me try reproducing this issue locally, then debug from there.

Regards@jeffersonfelixdev: I tested with default store, and works fine.

The problem is using Redis, but I need it, because I have a node cluster.@jeffersonfelixdev: I added a custom key generator, so now it's OK.
The problem is the key is shared between different rate limiters, so using different keys solves the problem.

In my particular case, I need a custom rate limit per route, so this is the implementation for that:

```ts
import rateLimit from 'express-rate-limit';
import RedisStore from 'rate-limit-redis';
import { createClient } from 'redis';

const client = createClient();

client.connect();

export const apiLimiter = rateLimit({
  windowMs: 30 * 1000, // 30 seconds
  max: 100,
  standardHeaders: true,
  legacyHeaders: false,
  keyGenerator: req => `[${req.method}] ${req.path}@${req.ip}`,
  store: new RedisStore({
    sendCommand: (...args: string[]) => client.sendCommand(args),
  }),
});

export const paymentLimiter = rateLimit({
  windowMs: 1000, // 1 second
  max: 1,
  standardHeaders: true,
  legacyHeaders: false,
  keyGenerator: req => `[${req.method}] ${req.path}@${req.ip}`,
  store: new RedisStore({
    sendCommand: (...args: string[]) => client.sendCommand(args),
  }),
});
```@gamemaker1: Ohkay that's great! I'll add this to the common issues section in the wiki.@tbell511: I can confirm this is still a problem. The suggestion of using a custom key fixed the issue for me - thanks for the tip!@nfriedly: @tbell511 Did you get an error logged to the console about double-counting, or did you find this first?

Solution patch:
diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
index a11985b..ba5bcd1 100644
--- a/.github/pull_request_template.md
+++ b/.github/pull_request_template.md
@@ -50,7 +50,7 @@
 
 - [ ] The issues that this PR fixes/closes have been mentioned above.
 - [ ] What this PR adds/changes/removes has been explained.
-- [ ] All tests (`npm test`) pass.
+- [ ] All library tests (`npm run test`) pass.
 - [ ] All added/modified code has been commented, and
       methods/classes/constants/types have been annotated with TSDoc comments.
 - [ ] If a new feature has been added or a bug has been fixed, tests have been
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index dbb36c7..944ef46 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -28,7 +28,7 @@ jobs:
     name: Test (Library)
     strategy:
       matrix:
-        node-version: [14, 16, 18, 19, 20]
+        node-version: [14, 16, 18, 20]
         os: [ubuntu-latest, windows-latest, macos-latest]
     runs-on: ${{ matrix.os }}
     steps:
@@ -46,7 +46,7 @@ jobs:
     name: Test (External)
     strategy:
       matrix:
-        node-version: [14, 16, 18, 19, 20]
+        node-version: [14, 16, 18, 20]
         os: [ubuntu-latest]
     runs-on: ${{ matrix.os }}
     steps:
diff --git a/contributing.md b/contributing.md
index e466f63..a9f43a5 100644
--- a/contributing.md
+++ b/contributing.md
@@ -21,10 +21,10 @@ Once you have installed the above, follow
 to
 [`fork`](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks)
 and [`clone`](https://github.com/git-guides/git-clone) the repository
-(`nfriedly/express-rate-limit`).
+(`express-rate-limit/express-rate-limit`).
 
 Once you have forked and cloned the repository, you can
-[pick out an issue](https://github.com/nfriedly/express-rate-limit/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc)
+[pick out an issue](https://github.com/express-rate-limit/express-rate-limit/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc)
 you want to fix/implement!
 
 ## Making Changes
@@ -34,28 +34,16 @@ Once you have cloned the repository to your computer (say, in
 branch:
 
 ```sh
-> git checkout -b branch-name
+> git switch -c branch-name
 ```
 
-While naming your branch, try to follow the below guidelines:
-
-1. Prefix the branch name with the type of change being made:
-   - `fix`: For a bug fix.
-   - `feat`: For a new feature.
-   - `test`: For any change related to tests.
-   - `perf`: For a performance related change.
-   - `meta`: Anything related to the build process, workflows, issue templates,
-     etc.
-   - `refc`: For any refactoring work.
-   - `docs`: For any documentation related changes.
-2. Make the branch name short but self-explanatory.
+While naming your branch, make sure the name is short and self explanatory.
 
 Once you have created a branch, you can start coding!
 
 The library is written in
-[Typescript](https://github.com/microsoft/TypeScript#readme) and
-[supports all LTS versions](https://github.com/nodejs/Release#release-schedule)
-of Node. The code is arranged as follows:
+[Typescript](https://github.com/microsoft/TypeScript#readme) and supports Node
+14, 16, 18 and 20. The code is arranged as follows:
 
 ```sh
 express-rate-limit
@@ -138,9 +126,13 @@ express-rate-limit
 
 When adding a new feature/fixing a bug, please add/update the readme and
 changelog as well as add tests for the same. Also make sure your code has been
-linted and that existing tests pass. You can run the linter using `npm lint`,
-the tests using `npm test` and try to automatically fix most lint issues using
-`npm autofix`.
+linted and that existing tests pass. You can run the linter using
+`npm run lint`, the tests using `npm run test` and try to automatically fix most
+lint issues using `npm run format`.
+
+> Note that the external tests require more CPU and RAM, and therefore are run
+> only on GitHub Actions. Please run only the library tests and the linter
+> locally using `npm run test`.
 
 Once you have made changes to the code, you will want to
 [`commit`](https://github.com/git-guides/git-commit) (basically, Git's version
@@ -182,10 +174,10 @@ changes to GitHub) your commits. To push your changes to your fork:
 ```
 
 If there are changes made to the `main` branch of the
-`nfriedly/express-rate-limit` repository, you may wish to
+`express-rate-limit/express-rate-limit` repository, you may wish to
 [`rebase`](https://docs.github.com/en/get-started/using-git/about-git-rebase)
 your branch to include those changes. To rebase, or include the changes from the
-`main` branch of the `nfriedly/express-rate-limit` repository:
+`main` branch of the `express-rate-limit/express-rate-limit` repository:
 
 ```
 > git fetch upstream main
@@ -193,8 +185,8 @@ your branch to include those changes. To rebase, or include the changes from the
 ```
 
 This will automatically add the changes from `main` branch of the
-`nfriedly/express-rate-limit` repository to the current branch. If you encounter
-any merge conflicts, follow
+`express-rate-limit/express-rate-limit` repository to the current branch. If you
+encounter any merge conflicts, follow
 [this guide](https://docs.github.com/en/get-started/using-git/resolving-merge-conflicts-after-a-git-rebase)
 to resolve them.
 
@@ -220,3 +212,5 @@ one maintainer and pass CI. After that, if there are no objections from other
 contributors, the pull request can be merged.
 
 #### Congratulations and thanks for your contribution!
+
+<!-- This contributing guide was inspired by the Electron project's contributing guide. -->
diff --git a/package.json b/package.json
index c55af80..6e4358c 100644
--- a/package.json
+++ b/package.json
@@ -63,12 +63,12 @@
 		"lint:code": "xo --ignore test/external/",
 		"lint:rest": "prettier --ignore-path .gitignore --ignore-unknown --check .",
 		"lint": "run-s lint:*",
-		"autofix:code": "npm run lint:code -- --fix",
-		"autofix:rest": "npm run lint:rest -- --write .",
-		"autofix": "run-s autofix:*",
+		"format:code": "npm run lint:code -- --fix",
+		"format:rest": "npm run lint:rest -- --write .",
+		"format": "run-s format:*",
 		"test:lib": "cross-env NODE_NO_WARNINGS=1 NODE_OPTIONS=--experimental-vm-modules jest",
 		"test:ext": "cd test/external/ && bash run-all-tests",
-		"test": "run-s lint test:*",
+		"test": "run-s lint test:lib",
 		"pre-commit": "lint-staged",
 		"prepare": "run-s compile && husky install config/husky"
 	},
diff --git a/source/lib.ts b/source/lib.ts
index 1aea05b..abb0237 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 d88840d..d5f677a 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 755be84..0f90a6c 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> | 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 8a1c169..705148d 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<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 patch:
diff --git a/test/library/validation-test.ts b/test/library/validation-test.ts
index c963dff..a2a8476 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)