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

mattab
Copy link
Member

@mattab mattab 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

@mattab mattab added the Needs Review PRs that need a code review label Aug 16, 2016
@mattab mattab added this to the 3.0.0-b1 milestone Aug 16, 2016
@tsteur
Copy link
Member

tsteur 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.

@mattab
Copy link
Member Author

mattab 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
@mattab mattab deleted the 10342b branch September 19, 2016 08:09
@mattab mattab added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants