Requesting flooding bypasses throttle limit

[kkoomen]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

Hi, maintainer of the redis storage provider here. I got an issue regarding the fact that when using e.g. @Throttle(5,1) and then a simple script that triggers 1000 requests ASAP then there are requests that should receive a 429 but they are coming through. From 1000 requests, there are ±400 coming through, while perhaps only 5 are allowed. This is not a problem with the builtin throttler service, but it becomes an issue with redis and perhaps other external storage providers. It does require changes in this package, which I'll explain below.

I did explain my full problem here: kkoomen/vim-doge#1064.

Minimum reproduction code

https://github.com/kkoomen/nestjs-throttler-redis-demo

Steps to reproduce

Open 3 terminals: 1 for redis-server, 1 for nest application and 1 to trigger the requests

terminal 1: run redis-server

terminal 2:

1. git clone https://github.com/kkoomen/nestjs-throttler-redis-demo
2. cd nestjs-throttler-redis-demo
3. yarn install
4. yarn start:dev

terminal 3: inside the nestjs-throttler-redis-demo repo, run ./trigger_requests.js

The output should give you something like this:

AxiosError: Request failed with status code 429
AxiosError: Request failed with status code 429
...
default: 1
default: 2
...
default: 400
..
AxiosError: Request failed with status code 429
AxiosError: Request failed with status code 429

as you see, based on the default: 400, it was able to successfully run 400 times, while only 5 are allowed.

Expected behavior

A correct output with @Throttle(5, 1) using the trigger_requests.js script looks like this:

default: 1
default: 2
default: 3
default: 4
default: 5
AxiosError: Request failed with status code 429
AxiosError: Request failed with status code 429
AxiosError: Request failed with status code 429
...
AxiosError: Request failed with status code 429

In order to fix this, the logic of the throttler package should change (please note that in order to understand the problem, read my full comment here).

Proposed solution: the getRecord should be completely removed and this part should be merged into addRecord(). The addRecord() should do 2 things respectively as described below:

1. add the record to the storage and set expiration time (just how it is now)
2. fetch and return the list of TTLs (the result which is returned from getRecord()) the total number of requests done by a single user (partially based on IP) along with the expiration time, this is also how express-rate-limit does it and as far as I, this works best in terms of performance and resolves these nasty issues.

This will solve the issue as I described in my comment here because the first call will immediately increment the amount of requests done, so the next request will receive the +1 from the previous one (which is not the case when being flooded with requests)

This week I will make a PR with changes (along with updates tests) with the proposed change.

This should have a slight impact on the tests (nothing to worry about as far as I see) and an API change in the next version bump.

Package version

3.1.0

NestJS version

9.0.0

Node.js version

16.6.0

In which operating systems have you tested?

• macOS
• Windows
• Linux

Other

No response

[nadavkaner]

Hey do we have an ETA on resolving this issue?

[jmcdo29]

Fixed in version 4.0.0