Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add a more aggressive rate limit for spammy users #226

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

wa0x6e
Copy link
Contributor

@wa0x6e wa0x6e commented Oct 30, 2023

🧿 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
Copy link

codecov bot commented Oct 30, 2023

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.

@wa0x6e wa0x6e changed the base branch from master to extract-redis-to-file October 31, 2023 08:53
@wa0x6e wa0x6e marked this pull request as ready for review October 31, 2023 10:10
@wa0x6e wa0x6e changed the title fix: add a more aggressive rate limit for high errored users fix: add a more aggressive rate limit for spammy users Oct 31, 2023
@wa0x6e wa0x6e requested a review from ChaituVR October 31, 2023 10:33
Todmy
Todmy previously requested changes Nov 13, 2023
yarn.lock Outdated Show resolved Hide resolved
@Todmy
Copy link
Contributor

Todmy commented Nov 13, 2023

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
Copy link
Contributor Author

wa0x6e commented Nov 14, 2023

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?

Base automatically changed from extract-redis-to-file to master November 14, 2023 07:12
@wa0x6e
Copy link
Contributor Author

wa0x6e commented Nov 14, 2023

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)

@wa0x6e wa0x6e requested a review from Todmy November 14, 2023 07:46
@Todmy
Copy link
Contributor

Todmy commented Nov 14, 2023

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
Copy link
Contributor

Todmy commented Nov 14, 2023

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
Copy link
Contributor

Todmy commented Nov 17, 2023

@wa0x6e ^^^

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Nov 28, 2023

@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
Copy link
Contributor Author

wa0x6e commented Nov 28, 2023

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

@Todmy
Copy link
Contributor

Todmy commented Dec 8, 2023

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

@Todmy Todmy removed their request for review December 8, 2023 11:21
@wa0x6e
Copy link
Contributor Author

wa0x6e commented Dec 8, 2023

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

Will do, thank you

@wa0x6e wa0x6e dismissed Todmy’s stale review December 22, 2023 06:23

reviewer change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants