Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

make sure both headers are set before checking for ip spoofing #10844

Merged
merged 1 commit into from Sep 30, 2013

Conversation

Projects
None yet
8 participants
Contributor

tamird commented Jun 4, 2013

In my routing stack, HTTP_CLIENT_IPS is getting set, but HTTP_X_FORWARDED_FOR is not, and this check is blowing up.

This PR adds tests and fixes the underlying problem.

rudle commented Jun 6, 2013

👍

Member

schneems commented Jun 13, 2013

👍 seems good

Contributor

xaviershay commented Jun 13, 2013

@steveklabnik are you still doing triage on this queue? This one is pretty straightforward I think.

Member

steveklabnik commented Jun 14, 2013

I know I merged something relating to this a few months ago, but I forget all the details. :(

@carlosantoniodasilva WDYT?

Contributor

tamird commented Jul 8, 2013

Contributor

tamird commented Sep 28, 2013

Member

lukaszx0 commented Sep 30, 2013

Looks good to me. @steveklabnik what's holding us before merging it?

Contributor

pftg commented Sep 30, 2013

Does this PR need any Changelog changes?

pixeltrix added a commit that referenced this pull request Sep 30, 2013

@pixeltrix pixeltrix merged commit ccd6f8b into rails:master Sep 30, 2013

Owner

pixeltrix commented Sep 30, 2013

Thanks @tamird, sorry for the delay.

Member

lukaszx0 commented Sep 30, 2013

@pixeltrix awesome, thanks man! 👍

Owner

pixeltrix commented Sep 30, 2013

I've backported it to 4-0-stable here: fbf5ece

Unfortunately the RemoteIp code changed significantly from 3.2 to 4.0 (mainly in 75dcdbc) so I can't really backport to 3-2-stable without risking a breaking change.

Member

lukaszx0 commented Sep 30, 2013

Thanks Andrew.

I think it would be nice to backport it to 3.2 if it won't requite much of a hussle. @tamird can you look into that? Contact me if you need any help with that!

Owner

pixeltrix commented Sep 30, 2013

@strzalek the calculate_ip code doesn't parse HTTP_CLIENT_IP for multiple IP addresses but reading the code suggests the error may occur if HTTP_CLIENT_IP or HTTP_X_FORWARDED_FOR alone are set. If @tamird would like to come up with a PR against 3-2-stable that limits itself to fixing that particular issue then I'd certainly consider merging it, however it mustn't change any other behaviour.

@tamird tamird deleted the tamird:fix-ip-spoof-errors branch Sep 30, 2013

@lukaszx0 lukaszx0 referenced this pull request Sep 30, 2013

Merged

Fix ip spoof errors #12410

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment