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

Rate limiter seems not working sometimes #156

Closed
maxdzin opened this issue May 31, 2023 · 14 comments
Closed

Rate limiter seems not working sometimes #156

maxdzin opened this issue May 31, 2023 · 14 comments
Labels
bug Something isn't working

Comments

@maxdzin
Copy link

maxdzin commented May 31, 2023

Version

nuxt-security: v0.13.0
nuxt: v3.5.2

Reproduction Link

https://stackblitz.com/edit/github-uzlx5a

I'm not sure how that happens, but when the rate limiter is set to, let's say, 3 times per day, after some number of page requests (sometimes 5-10 or more), the page is rendered (i.e. returned with 200 OK response), but it must drop 429 error.

I'm not experiencing that every time, but sometimes, so it is hard to reproduce. But when I tested, that happens especially when interval is set as day.

So, the config:

security: {
  rateLimiter: {
    tokensPerInterval: 3,
    interval: 'day',
  },
},

Maybe the test here is passed because there are just 5 requests and maybe it can be increased in order to get that wrong 200 response, I'm not sure.

Steps to reproduce

Just try to refresh the page about 5 - 10 - 20 times.

What is Expected?

The rate limiter must throw 429 response after 3 successful requests all the next specified period of time (for this test case there's used 3 requests per day, as can be seen above).

What is actually happening?

After successful 3 requests (as it is set in config) it must drop 429 response according to the specified period of time in config.

@maxdzin maxdzin added the bug Something isn't working label May 31, 2023
@maxdzin
Copy link
Author

maxdzin commented Jun 1, 2023

@Baroshem additional note. It seems the rule is bypassed every 10 seconds don't matter what interval is specified (minute, day, etc., or as a number: 60000).
And after two or three cycles of dropping errors of that every 10 seconds, the rate limiter stops working at all - the page become accessible every time.

@Baroshem
Copy link
Owner

Baroshem commented Jun 2, 2023

Hey @maxdzin thanks for reporting this issue :)

This is strange indeed. I believe it can be related to the fact that rate limiter by default uses the memory storage to store ip adressess.

Maybe the memory storage cannot procees multiple requests at the same time.

To be honest, the implementation of the rate limiter here is mostly to cover the most basic cases of rate limiting. Few months ago, I had an idea to deprecate the rate limiter, mainly because for real production applications users should use dedicated software like fail2ban, iptables etc.

In prod apps, handling rate limiting on the application level (rather than network or infrastructure layers) is already too late. Also, storing rate limited ips in memory can slow down the application.

I am not sure if this issue can be solved by the module, as both rate limiter and memory storage are external packages.

The rate limiter was here from the very beginning of the existence of the module (back in October last year) so several things have proven to be more or less useful over the time. Rate Limiter unfortunately is an example of less useful.

@Baroshem
Copy link
Owner

Baroshem commented Jun 2, 2023

I could add a note to the docs about the reasons I mentioned in the previous comment. What do you think @maxdzin ?

@maxdzin
Copy link
Author

maxdzin commented Jun 7, 2023

Hi @Baroshem,
Yes, I did a bit of research on this and it seems to be related to an in-memory issue (and maybe something else).
As for the rate limiter deprecating. Well, I think that even though it's far from a somehow really good solution, it can help in some very simple cases, and by the way, I'm already using it for such things in several projects, so I can say that it would probably be better to leave the speed limiter, at least as an optional solution.

And, yes, it would be good to have some notice regarding this issue in the docs as well.

@CptJJ
Copy link

CptJJ commented Jun 25, 2023

Did you find a solution to this? @maxdzin running into the same issue working on a simple rate limiter for a reset password form, don't want users being able to blow through my email request API limit

@Baroshem
Copy link
Owner

Hey @CptJJ

There is no solution that the module could deliver. The current issues are caused by the underlying rate limiter and memory storage. These tools have some limitations.

That is why, in the production environment, you should use a separate dedicated tool just for rate limiting :)

@CptJJ
Copy link

CptJJ commented Jun 25, 2023

Fwiw I was able to implement some really robust (or so far I think) rate limiting with limiter:

import { RateLimiter } from "limiter";

const tokensPerInterval = 1
const interval = 60 * 1000
const limiter = new RateLimiter(
    {
        tokensPerInterval,
        interval,
        fireImmediately: true
    });

export default defineEventHandler(async (event) => { 
const remainingRequest = await limiter.removeTokens(1)
    if (remainingRequest < 0) {
        throw createError({ statusCode: 429, statusMessage: "Try again in 60 seconds" })
    }

    await mailerSend(body.email, "Verification Code", "xxxxxx", { otp: data.properties.email_otp });


    return "success"

})

@Baroshem
Copy link
Owner

@CptJJ

I am not sure about the snippet of code you sent here. Does it store information about specific ip adress that is trying to connect to your application or is it just blocking the amount of requests from all available IP's?

If the second one, it will only protect you from the requests in general but it wont allow you to block certain ip's only which I think is rhe most beneficial.

@maxdzin
Copy link
Author

maxdzin commented Jun 27, 2023

Did you find a solution to this? @maxdzin running into the same issue working on a simple rate limiter for a reset password form, don't want users being able to blow through my email request API limit

@CptJJ For that project was enough to tweak tokensPerInterval value. And I agree with @Baroshem - for more serious cases there's a need to use another solution that could handle the limits correctly.

@CptJJ
Copy link

CptJJ commented Jun 27, 2023

@Baroshem I have not had a chance to test this yet, however this post led me to believe that the limiter package does use the requesters IP:
https://stackoverflow.com/questions/76170872/how-to-add-rate-limiter-in-all-apis-inside-the-server-on-nuxt-3

@Baroshem
Copy link
Owner

Baroshem commented Jul 3, 2023

Keep in mind that this answer suggests using the Node middleware in Nuxt (which is not the recommended approach right now with Nuxt 3 apps as roght now Nuxt middlewares use Nitro and H3, not pure Node and Express.

As far as I remebmer (imight be wrong here) when I was implementing the rate limiting as a part of the module, I needed to implement a memory storage to store these ips.

@Baroshem
Copy link
Owner

I have added a note about built in rate limiter to the documentation. Closing the issue :)

@adriaandotcom
Copy link

I know this issue is closed, but I would like to add that the rateLimiter does not really work. When it's part of this module, you kind of expect it to work. It costs me quite a few hours before moving to the network layer (which is also warned about). It's not production ready, but also not great for a hobby project. It's very unreliable.

Besides that, I really appreciate the work on nuxt-security. It has nice defaults that force you to be more secure. Thanks for that.

@Baroshem
Copy link
Owner

Hey @adriaanvanrossum

Thanks for your comment. I developed rate limiter almost a year ago and since then, I experienced mainly the issues with the implementation. Because of that, I decided to make it optional #190 as it should be rather implemented in the network layer as you mentioned.

Could you share in the linked issue what approach have you decided to choose to implement rate limiting in your application? I would like to make the rate limiter optional and mention good practices that users can use instead of the built in limiter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants