Skip to content

Logger middleware: remote-addr (client IP) should use X-Forwarded-For if present #700

Closed
aseemk opened this Issue Dec 5, 2012 · 9 comments

5 participants

@aseemk
aseemk commented Dec 5, 2012

When the server is behind a load balancer -- which is the case on any cloud platform -- the logger middleware's client IP address is always the load balancer's.

The fix is simple: use req.headers['x-forwarded-for'] if present. It's a one-line change to the logger middleware, so I'm happy to submit a pull request with it, but just wanted to ask if you guys agree this should be handled within this middleware. It would be nice. =)

@langpavel

There is app.enable('trust proxy'); but it's related to express.

I wish have this in connect but opt in like in express.

@tj
Sencha Labs member
tj commented Dec 6, 2012

@aseemk sure, a PR would be great

@jmaurice
jmaurice commented Dec 6, 2012

Wouldn't this allow malicious clients to specify any IP address they want by adding a X-Forwarded-For header to their request? IMHO, I think the implementation is fine as-is since you can access req.['x-forwarded-for'] if you know your app is behind a load balancer.

@langpavel

@jmaurice You are right, this is reason for opt in. But for generic middleware (loggers…) this is the only chance to behave correctly behind proxy.

@aseemk
aseemk commented Dec 6, 2012

I'm not sure if the logger's output is a standardized format, but if it isn't, another option is to output both: always output the socket's remote address, and additionally output the X-Forwarded-For if present.

Happy to do either way. Let me know.

@tj
Sencha Labs member
tj commented Dec 6, 2012

we could just add a proxy option to the logger(), and then base the ip off that header field appropriately. Definitely tougher to handle this stuff nicely when it has to be specified all over the place but oh well, small price to pay for modularity

@langpavel

I'm not talking about logger output, but input. Modules written in generic way should recognize real IP endpoint.
In this case may be better introduce new property (or overridable prop getter) for modules to figure out X-Forwarded-For, because this is per-administrator decision what header their proxy set. (may be cascade of proxy)

Default behavior should be to reflect req.connection.remoteAddress. There may be helper which override this to reflect req.getHeader('X-Forwarded-For')

in Express is req.ips which can be good example

@tj
Sencha Labs member
tj commented Dec 7, 2012

sure, that's not really a concern for any single given middleware though, that implies global behaviour/coupling, which if there's strong feelings towards something like that it really belongs in node not connect, node falls short for many things like this since it simply doesn't care

@jonathanong jonathanong closed this Feb 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.