fix: add a more aggressive rate limit for spammy users

[wa0x6e]

🧿 Current issues / What's wrong ?

From https://github.com/orgs/snapshot-labs/projects/12/views/7?pane=issue&itemId=43069387

Current rate limit is pretty generous, with 100 requests every minute. This is fine for legitimate users, but some bad actors are abusing that limit by trying to send multiple requests within that limit, but with all requests failing, as they're trying to brute force something.

Thus, some users are sending 100 bad requests per minutes, for hours straight.

This traffic is of low quality, and is from bot. Example: a bot trying to vote on all proposals. This kind of requests will always fail with various errors, such as "proposal closed", or "no voting power".

💊 Fixes / Solution

We should decrease the rpm for users sending a high number of failed requests.
The express-rate-limit middleware is already providing the logic for that, and can be used by just tweaking its config.

This can also serve as a protection when the service is returning a high number of errors due to internal service issues (pineapple, score-api, etc ...)

🚧 Changes

• Add additional rate limit middleware, to limit the maximum number of failed requests to 15 every 15seconds. (effectively dropping the limit to 60 rpm if users only send failed requests)
• Upgrade express-rate-limit to latest version, as current version has a bug when using multiple instance of rateLimit.

Limit is still pretty generous, and will be further be adjusted depending on how well it works.

🛠️ Tests

• Send some successful requests => you should be able to send 100 requests per minute
• Send a mix of successful and failing requests => you should be able to send 100 requests per minute, as long as you don't send more than 15 failing requests within 15s
• Send more than 15 failing requests within 15 seconds => it should return a 429 error from the 16th requests, and accept requests again only after the 15 seconds window closes.

[codecov]

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (c3e79e1) 49.50% compared to head (c37f0d2) 49.97%.

Files	Patch %	Lines
src/helpers/rateLimit.ts	0.00%	29 Missing ⚠️
src/index.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   49.50%   49.97%   +0.47%     
==========================================
  Files          33       33              
  Lines        2117     2135      +18     
  Branches      195      199       +4     
==========================================
+ Hits         1048     1067      +19     
  Misses       1063     1063              
+ Partials        6        5       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Todmy]

I have tried it and looks the case

Send some successful requests => you should be able to send 100 requests per minute

broken for me. After a short investigation, I see that the second rate limit you have added somehow overrides windowMs time for the first middleware.

[wa0x6e]

I have tried it and looks the case

Send some successful requests => you should be able to send 100 requests per minute

broken for me. After a short investigation, I see that the second rate limit you have added somehow overrides windowMs time for the first middleware.

What do you mean by override? It resets the time?

[wa0x6e]

The second, more aggressive rate limit will take precedence in case it is triggered.

First rate limit will work as usual, until there is 15 errored requests in a 15 seconds window, in which case, the user is locked completely until the 15 second window closes (every requests return 429, success or not)

[Todmy]

I have tried it and looks the case

Send some successful requests => you should be able to send 100 requests per minute

broken for me. After a short investigation, I see that the second rate limit you have added somehow overrides windowMs time for the first middleware.

What do you mean by override? It resets the time?

you can try it by yourself. With the current code, the first middleware doesn't work.
I said override because I have inspect the keys stored in redis and found out that both keys are using 15s window

[Todmy]

The second, more aggressive rate limit will take precedence in case it is triggered.

First rate limit will work as usual, until there is 15 errored requests in a 15 seconds window, in which case, the user is locked completely until the 15 second window closes (every requests return 429, success or not)

ok, for me the regular(regularRateLimit) rate limit doesn't work. Does it work for you? But highErroredRateLimit works as expected

[Todmy]

@wa0x6e ^^^

[wa0x6e]

@wa0x6e ^^^

Just tested, and confirming that the latest define rateLimit's windowMs is used. Swapping the order, and every limiter are using 60s instead.

Will see why

[wa0x6e]

Fixed, seems like a known bug from rate-limit itself express-rate-limit/rate-limit-redis#171

[Todmy]

I can't test. Could you @ChaituVR test take it?

[wa0x6e]

I can't test. Could you @ChaituVR test take it?

Will do, thank you

reviewer change