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(x-forwarded-for): Follow AWS Load Balancer spec for X-Forwarded-For Header #33

Closed
wants to merge 4 commits into from

Conversation

austince
Copy link
Contributor

@austince austince commented May 3, 2018

Closes #25.
AWS ELB Docs

Still works with the unknown case, just searches backwards through the IP list instead of forwards.

NOTE: This might be considered a breaking change.

@austince
Copy link
Contributor Author

austince commented May 4, 2018

While I still think this should be merged in, I've run into some issues with the AWS ELB in api configurations that have an AWS API gateway in front. In that case, the X-Forwarded-For header looks like:
real-client-ip, aws-proxy-ip, aws-api-gateway-private-ip

Perhaps we should allow a config option that can choose from the list of stripped ips from the header?

// ...
getClientIp(req, {
   xForwardedForHandler: (ips) => ips[0]
}
// ...

Would also be happy to put in a PR for that feature, but we should plan it out if you're onboard with it.

@austince
Copy link
Contributor Author

austince commented May 7, 2018

Hey @pbojinov, do you have any thoughts on this?

@pbojinov
Copy link
Owner

pbojinov commented Jul 3, 2018

@austince Sorry for the delay. Thank you for this, I'll review and get back you.

@austince
Copy link
Contributor Author

austince commented Aug 3, 2018

Hi @pbojinov, any update on this?

@evdama
Copy link

evdama commented Oct 3, 2019

any reason it's not merged?

@marcuspoehls
Copy link

@austince @pbojinov Hey Austin, Hey Petar, any chance to get this merged?

I'm maintaining hapi-rate-limitor and users run into the same issue on Heroku where the client's actual IP address is the last item in the chain, not the first one.

I see conflicts on this PR that must be resolved before merging. @austince are you still interested in this feature? Would you mind resolving the conflicts?

@austince
Copy link
Contributor Author

Hey @marcuspoehls, I’d be happy to resolve the conflicts if we get a confirmation that they will be merged.

@ruscon
Copy link

ruscon commented Jul 30, 2020

dead ?

@austince
Copy link
Contributor Author

dead ?

@pbojinov 🤷

@marcuspoehls
Copy link

Just a note: I created a separate request-ip as a drop-in replacement for this package. I used this package here for my hapi.js plugins and now took the time to implement my own request-ip package. It may be worth a look.

Looks like Petar (the maintainer of this package here) can’t find the time for this request-ip package. Nonetheless, it’s a helpful package!

@pbojinov
Copy link
Owner

Sorry for the delay. I stopped getting these notifications via email due to some spam filters. Those should be cleared up now.

I’m happy to get some of these new changes merged in this week. Will review and get back shortly.

@austince
Copy link
Contributor Author

Not sure what happened here... but am going to close this since it doesn't seem like much else will.

@pbojinov
Copy link
Owner

pbojinov commented Jun 1, 2022

Fixed merge conflicts and merged in #51

@pbojinov pbojinov closed this Jun 1, 2022
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.

IP address calculated from X-Forwarded-For are insecure
5 participants