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

Extract the first IP from HTTP_X_FORWARDED_FOR and HTTP_CLIENT_IP and HTTP_CF_CONNECTING_IP and HTTP_X_FORWARDED_HOST when there is more than one IP #10404

Merged
merged 3 commits into from Sep 19, 2016

Conversation

Projects
None yet
2 participants
@mattab
Copy link
Member

commented Aug 16, 2016

Fixes #10342

  • It's hard to know 100% whether this is a correct patch.
  • The RFC at https://tools.ietf.org/html/rfc7239 makes it clear that we should extract the first IP from the header HTTP_X_FORWARDED_FOR
  • assuming we have had this bug for 5+ years, some users may have mis-configured their proxy setup especially just for Piwik to detect the "last IP" correctly. Once we start reading the first IP from the list, their geo-location and other features depending on IP address may break -> solution would be to configure the Proxy headers correctly so the first IP returned is the IP of the client.
  • it is unclear whether it is correct to do so for others as well ie. HTTP_CLIENT_IP and HTTP_CF_CONNECTING_IP and HTTP_X_FORWARDED_HOST , but I assume that it is correct
  • the CI tests failures are not related to the changes in this PR
@tsteur

This comment has been minimized.

Copy link
Member

commented Aug 30, 2016

Are you waiting for a user test here? LGTM if you think it's working as expected.

I think this is something that we should mention in #10454 as it may break users system when updating and it would be hard to find.

@tsteur tsteur referenced this pull request Aug 30, 2016

Closed

Piwik 3 Blog post about some important changes #10454

3 of 3 tasks complete
@mattab

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2016

I think this is something that we should mention in #10454 as it may break users system when updating and it would be hard to find.

👍

@mattab mattab self-assigned this Sep 19, 2016

@mattab mattab merged commit 19d711f into 3.x-dev Sep 19, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@mattab mattab deleted the 10342b branch Sep 19, 2016

@mattab mattab added the Enhancement label Oct 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.