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 limiting API routes doesn't work or is very inconsistent #281

Closed
georgesaliba opened this issue Nov 5, 2023 · 18 comments · Fixed by #429
Closed

Rate limiting API routes doesn't work or is very inconsistent #281

georgesaliba opened this issue Nov 5, 2023 · 18 comments · Fixed by #429
Labels
awaiting details Waiting for feedback from the issue author, i.e. reproduction bug Something isn't working

Comments

@georgesaliba
Copy link

georgesaliba commented Nov 5, 2023

Version

nuxt-security: v1.0.0-rc.3
nuxt: v3.8.0

Reproduction Link

https://stackblitz.com/edit/nuxt-starter-lytnxe?file=app.vue

Steps to reproduce

I have a routeRule set up to try to rate limit an API endpoint. It looks like the following in nuxt.config.ts:

'/api/test/**': { 
      security: {
        rateLimiter: {
          tokensPerInterval: 3,
          interval: 86400000
        }
      }
    }

Unfortunately it's just very inconsistent. If I start up the server and hit the endpoint directly, and refresh it more than 3 times, it will correctly throw a 429. But if I'm navigating around the site, where the pages are calling the same API via $fetch, it won't trigger. Additionally, after navigating around the site, going back to the endpoint and refreshing more than 3 times will no longer throw the 429 any more.

For my own scenario, I'm not even sure if rate limiting the API is a good idea (i.e., I have a cloud function which does the server-side rendering calling the same API; I would not want that one to be rate limited). But I expected it to be rate limited with that configuration.

I also may be misunderstanding the routeRule itself. I thought '/api/test/**' rate limiting would count the number of requests to any endpoint that started with /api/test, but maybe it only rate limits each unique endpoint that starts with? For example, would /api/test/A and /api/test/B each get 3 requests per day, or would they get 3 requests combined per day? Either way, it doesn't appear to work right.

What is Expected?

I'm expecting it to correctly and consistently rate limit the API route - whether I'm loading it directly or loading it from a page.

What is actually happening?

It doesn't consistently rate limit.

@georgesaliba georgesaliba added the bug Something isn't working label Nov 5, 2023
@georgesaliba georgesaliba changed the title Rate limiting routes is very inconsistent Rate limiting API routes doesn't work or is very inconsistent Nov 5, 2023
@Baroshem
Copy link
Owner

Baroshem commented Nov 5, 2023

Hey @georgesaliba

Thanks for this issue. This new rate limiter was actually introduced as a part of RC1 version.

I am looking for all possible feedback regarding it so your issue and comment is more than welcome!

You have done a great work analysing what could be improved. Maybe you already have an idea how it could be fixed?

If you like, feel free to create a PR with proposed change. I will be more than happy to help you with it so that we can make it better together :)

@Baroshem
Copy link
Owner

Any more info @georgesaliba ?

@georgesaliba
Copy link
Author

Hi @Baroshem ! I actually have been trying to move from client-side API calls to server-side API calls using server components, as to not expose these APIs at all rather than trying to rate limit them. Unfortunately, the rateLimiter middleware seems to actually be limiting the calls being made from server components, so I've largely been disabling rate limiting for my use case as a workaround.

@Baroshem
Copy link
Owner

Hey @georgesaliba

Maybe a better option would be to increase the amount of tokensPerInterval rather than disabling it?

@georgesaliba
Copy link
Author

Yes and no for my use case.

I'll use an example which isn't my use case but demonstrates it well enough. Imagine I have a website that hosts recipes. Each recipe is just a list of ingredients, and there are many recipes in the database - all of them public. So I have one paginated summary API that gives me a list of recipes, and a details API that gives me the ingredients in any one recipe.

Now I don't want bots to just scrap my database for all the recipes, and all the subsequent ingredients in each recipe. So I put rate limits on the recipe details page (the one that lists ingredients) that was reasonable for a real human (e.g., let's say most people don't view more than 100 recipes a day), and that worked. However, the bots were still hitting the recipe details API, and able to continue to scrape all the recipes from the database. So I tried to put a rate limit on that API, but it didn't work (hence why I opened this issue).

In my case, however, I don't even really want to even expose that the recipes details API to the public. I would rather it only be exposed to my own server as an API, and always presented to my users as HTML listing the ingredients on details page (where the limit works). However, to do this, I need to use Nuxt server components. I didn't realize, however, that these seemingly server components were going through the same nuxt-security middleware (presumably it calls the API server-to-server, stores the HTML, then is making an async call just to fetch that HTML down to the client). I had built the server components as each line item in the recipe, so even viewing a few recipes quickly blew through the rateLimit (e.g., a recipe with 100 ingredients was making 100 calls on page load).

So I can disable the rate limit on the server components folder (/_nuxt_island/** I believe), but it's likely better if I make it so the entire recipe is a single server component instead. So that's my next step, at which point I'll look at getting rid of the rate limit bypass on the server components folder. But in this scenario, I'm not really worried about the API rate limiting not working anymore.

@Baroshem
Copy link
Owner

I see. Is there anything we can do from the module side to help you?

@georgesaliba
Copy link
Author

Following up on this, I'm seeing a slightly different (but maybe related?) issue, but it's confusing me.

So I've removed all my API route-specific rate limiting, so it should just be using the default rate limits. What I'm seeing is when I hit a Nuxt page route directly, I'm getting a 429 response from the API, even though I've only hit the page once. But if I navigate to that same Nuxt page from another page, it works fine.

I'm pretty sure what's happening is the server-side call from my Nuxt page to the Nuxt server API is getting rate limited even though it's an internal call, while the call that happens when I navigate to the page is happening from the client-side, meaning it's mapping to the customer's IP and rate limiting per user correctly.

Is there any way to exclude internal API traffic from the rate limits, while keeping the rate limits on the same API when being called from the client-side?

@Baroshem
Copy link
Owner

Baroshem commented Dec 5, 2023

Hey, I think it could be a bug on the implementation side.

Would you be up for contributing and fixing this issue? :)

@Baroshem
Copy link
Owner

@georgesaliba are you open for creating a Pull Request with a fix for that? :)

@Baroshem Baroshem added the awaiting details Waiting for feedback from the issue author, i.e. reproduction label Dec 11, 2023
@vejja
Copy link
Collaborator

vejja commented Dec 13, 2023

Hi @Baroshem
I'm happy to try tackling this one.

However would love to get your input first on independent vs cumulative limits first, as we were discussing in: #292 (comment)

I don't know how other rate limiters approach this question, also happy to look at reference implementations. If there is a standard rateLimiter package that you like, let me know

@Baroshem
Copy link
Owner

Hey @vejja

Yes please. If you have time, feel free to take a look at this case.

Regarding

Say you have a limit of 10 requests/sec at the global level, but /route1 has a limit of 5 req/s. What happens if you hit index 5 times, can you still hit /route1 ? In other words, are the limits cumulative or independent ?

I would say that in general rate limiters are supposed to help avoid attacks such as DOS and DDOS. And in case of this attacks, you are keen on blocking whole user (device) from accessing your website no matter what endpoint this bot is trying to access.

So IMO, if a user or device manages to reach a limit of any routes it should be blocked from accessing the whole website. That is why, by default rate limiter has a limit of 150 requests per minute.

Regarding the examples, here are some useful tools that do the same:

@georgesaliba
Copy link
Author

Hey guys, was out on holiday, but following up. I tried to take a look at the rate limiter code, but it looked fine - at least as far as I could understand it.

I have a feeling that the root cause is that the rate limiter is looking at "x-forwarded-for", but that's not what Google uses when Nuxt is hosted on Google Firebase (executed in a Google Cloud Function behind the scenes). Apparently Firebase is behind the Fastly CDN, so the originating IP address is stored in "fastly-client-ip" instead (per this). I suspect that could apply to any server behind Fastly CDN, though not sure. I've also found some conflicting info about exactly which string this info is stored, apparaently based on the specific technology solution, so probably makes the most sense to have that lookup string be a configurable array for strings, where they get tried in some order if the previous was wasn't found.

As a result, the IP-based rate limiting is just limiting the sum of all traffic from any user on the site when going through the Cloud Function, not rate limiting specific users. As the user population grows, there can be 429s on any call for any user, which basically breaks functionality at random throughout the site.

@vejja
Copy link
Collaborator

vejja commented Jan 2, 2024

unjs/h3 now have getRequestIP since v1.8 and getRequestFingerprint since v1.9
Not sure if this addresses your concerns but worth checking

BTW in the GitHub discussion they mention this module as an example of why they think it is preferrable to use getRequestIP: unjs/h3#503

@georgesaliba
Copy link
Author

georgesaliba commented Jan 3, 2024

As an aside, after digging through the logs a bit, it ends up the client IP address isn't in either "x-forwarded-for" nor "fastly-client-ip", but instead in "cf-connecting-ip" (and "fastly-temp-xff"). In my case, Cloudflare is fronting Firebase, which sits behind Fastly CDN. So Cloudflare's header is the accurate source of the client IP (fastly-client-ip only contains the Cloudflare IP, and x-forwarded-for contains only the Cloudflare IP and Google's IP, not client IP).

I'm not sure if a better option here is to make the header where the IP address is pulled from configurable, or if my use case is unique enough I should be rolling my own rate limiter.

@Baroshem
Copy link
Owner

Baroshem commented Jan 9, 2024

Hey @georgesaliba

Thanks for additional details. We could add an option to set the header name that you want to set in order to get a proper rate limiting.

It would be an additional configuration option for rateLimiter middleware that would overwrite default behavior of the middleware.

Would you be interested in creating a Proof of Concept Pull Request with that change? :)

@Baroshem
Copy link
Owner

@georgesaliba

Any feedback from you? :)

@georgesaliba
Copy link
Author

Hey @Baroshem ! Given the complexity and very specific requirements I had, I ended up creating my own rate limiter, which worked for my use case. But then I quickly decided to scrap the whole idea of rate limiting at the middleware, and replaced it with Cloudflare rate limiting (basically what is actually called out as a recommendation in the Nuxt Security docs).

@Baroshem
Copy link
Owner

Hey @georgesaliba

Yes, I think this is the right way to go in the production applications as solutions on the application level are already too late to handle rate limiting so the built it rate limiting is aimed at simpler web apps.

Do you think we could close the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting details Waiting for feedback from the issue author, i.e. reproduction bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants