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

default key req.ip doesn't work with Next.js #254

Closed
swalker-888 opened this issue Oct 4, 2021 · 2 comments
Closed

default key req.ip doesn't work with Next.js #254

swalker-888 opened this issue Oct 4, 2021 · 2 comments
Labels
bug A bug in the library documentation Related to documentation

Comments

@swalker-888
Copy link

Hello,

I'm using express-rate-limit for my Next.js API as a middleware. I encountered an issue where the key was being stored as 'undefined', I discovered that this is because the default key generator - 'req.ip' doesn't exist in Next, causing the undefined entry.

I've gotten around this by using the following option with the limiter:

  keyGenerator: function (req /*, res*/) {
    return req.headers["x-forwarded-for"] || req.connection.remoteAddress;
  }

Obviously the package is originally intended for express so understand why the key generator would default to req.ip, but due to it being very popular and many SO posts referencing it as a good option for a Next middleware maybe this should be included in the docs as a heads up?

@nfriedly
Copy link
Member

nfriedly commented Oct 4, 2021

Yea, that means it's probably not using express under-the-hood, as req.ip is one of the goodies that express adds.

I wonder if maybe next.js used to but doesn't any more?

I think I'm going to tweak the default keyGenerator function to throw an error if req.ip is undefined.


Also, be careful with your implementation - an attacker could add an x-forwarded-for header with a random IP that changes on each request, and essentially bypass your rate-limiting entirely.

Express's app.set('trust proxy', 1); setting means that it would accept 1 of these headers from your real reverse proxy, but then ignore all others. (I believe node.js will concatenate them if there are multiple - see https://nodejs.org/api/http.html#http_message_headers .)

@swalker-888
Copy link
Author

that's great thank you. Good point i'll look into the headers again.

nfriedly added a commit that referenced this issue Nov 6, 2021
@gamemaker1 gamemaker1 added bug A bug in the library documentation Related to documentation labels Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug in the library documentation Related to documentation
Projects
None yet
Development

No branches or pull requests

3 participants