Fix getCurrentHost

[bernardn]

fix getCurrentHost method that failed to return the correct host when used behind a Proxy because it called an IP-related method.
add getOriginalHostFromHeader method for that purpose

[mattab]

What is the _SERVER value that "fails" in your case ? because removing this function call is not correct

[bernardn]

Dear mattab, it seems you closed this topic a liitle bit too fast. Let's at least take a look at how the software behaves when calling that function: it always return the default value because this function is not intended to be called there : it's an IP-related function, which is intended to process Client IP addresses and not server hostnames. It fails when PIWIK is running behind a proxy, because the value has to be retrieved by that function in the proxy-provided headers. It doesnt fail in an usual setup because the faulty code always return the default value.

[mattab]

This function call is there for a reason: some piwik hosts run on IP
adresses, so the header can contain one, or even a list of IP address.

On 04/02/13 01:11, Bernard Nauwelaerts wrote:

Dear mattab, it seems you closed this topic a liitle bit too fast.
Let's at least take a look at how the software behaves when calling
that function: it always return the default value because this
function is not intended to be called there : it's an IP-related
function, which is intended to process Client IP addresses and not
server hostnames. It fails when PIWIK is running behind a proxy,
because the value has to be retrieved by that function in the
proxy-provided headers. It doesnt fail in an usual setup because the
faulty code always return the default value.

—
Reply to this email directly or view it on GitHub
#16 (comment).

[bernardn]

This function call has no reason to be at THAT place... It's absolutely a technical non-sense. This make your system UNUSABLE behind a proxy because it builds ABSOLUTE URLs based on the Host information that can only retrieved in headers given by the proxy, whose names are stored in config, which CAN NOT be retrieved by a function that processes client IP addresses. But you obviously have not taken any serious look at the problem. So, please let some advised person review this issue instead of closing it. Thanks.

[mattab]

The code is unit tested at: https://github.com/piwik/piwik/blob/master/tests/PHPUnit/Core/UrlTest.php#L133

you patch breask the test, plus I cant replicate the bug. So please answer the first question: What is the _SERVER value that "fails" in your case ? and what is your config file? Ideally include in pull request the test update that shows the issue.

PS: we are working on having builds executed on each pull request, this will be helpful

[robocoder]

-1

[mattab]

@bernard next time, try submit tests + better explanation for your pull requests ,

Cheers

[bernardn]

@mattab The unit test failed because it tries to get the Host among a list of names. I adapted the code to return the first element. By the way, it seems that these tests contain an error : the proxy_ips config parameter is set, causing the function GetCurrentHost returning the wanted value. However, this is wrong because the proxy_ips config parameter is intended to exclude proxy IP addresses from information retrieved from x-forwarded-for header. This is very different from the x-forwarded-host header, which is intended to carry host names.

I'll resubmit a pull request with these changes.

Regards

[bernardn]

@mattab, I don't understand your question related to _SERVER value. My piwik setup is located behind a reverse-proxy and piwik is unable to retrieve the original Host header because it is processed by a function intended to process IP addresses and not hosts.

Here is the related config :

[General]
assume_secure_protocol = 1
proxy_client_headers[] = "HTTP_X_FORWARDED_FOR"
proxy_host_headers[] = "HTTP_X_ORIGINAL_HOST"
proxy_ips[] = "10.9.70.42"
proxy_ips[] = "10.9.70.41"

[robocoder]

The convention for X-Forwarded-Host is that proxies append their IP address to the header. So the code takes the last element that isn't in the exclusion list. This is the least likely to be spoofed, so is preferred over the first element.

If X-Original-Host behaves differently, then I can see why you'd want to change the code, but this would break the more common configuration.

BTW if you look at Piwik_IP::getNonProxyIpFromHeader(), it doesn't actually care if the element is an IP address or host.

[bernardn]

@robocoder
The header the proxies append their IPs in is X-Forwarded-For

X-Forwarded-Host contains the Original Host header, i.e. the requested site domain name, because the reverse proxy is very likely to pass the request to a server whose name differs. e.g. www.example.org <proxy> www.lan.example.org

These are very different things, that serve different purposes.