Improved https recognition behind load balancer

[janfejtek]

• new feature
• BC break? no

When server is behind reverse proxy/load balancer, checking of HTTPS variable is not enough

[dg]

Thank you. Can you add a test to RequestFactory.scheme.phpt?

[janfejtek]

I had to modify port tests too. I know that I should not change existing tests, but thanks to this comment 4d8a1fc, I'm not sure if tests were ok in the first place?

[dg]

Altering tests is probably ok, but it would be nice if @grongor could take a look at it.

[grongor]

Sorry, I haven't been working with PHP for some time now...

[dg]

@grongor I get it, but you remember how it works, right?

[janfejtek]

I did some overview of variables, I'm missing Apache because we are not using it.

	php -S	nginx/Docker	nginx/OpenShift	IIS
HTTP_HOST	localhost:8080	localhost:8080	example.com	example.com
SERVER_NAME	localhost	localhost	localhost	example.com
HTTP_X_FORWARDED_PORT			443
SERVER_PORT	8080	8080	8080	443
HTTP_X_FORWARDED_PROTO			https
HTTPS				on
REQUEST_SCHEME	http	http	http
expected result	http / 8080	http / 8080	https / 443	https / 443

So I can add tests to cover these possibilities?

[JanTvrdik]

You definitely cannot read X-Forwarded- header without checking they come from trusted proxy. Isn't this all already handled by the useNonstandardProxy() / useForwardedProxy() methods?

[dg]

@janfejtek thanks for the PR, but the truth is I don't understand this. So I can't answer you, and I can't even merge it. I just need to get it approved by someone who understands and who I know.

Or I can research it, but unfortunately the average wait time for solving issue can be over a year.

[janfejtek]

@JanTvrdik Thank you, you were right with the useForwardedProxy, I was looking in the wrong place, because I did not notice that getClient method modifies original Url object

@dg do you think now it would be possible? The actual issue is that there is no port in host key in HTTP_FORWARDED (https://github.com/nette/http/pull/221/files#diff-9b124d5a3d4427bd63acc9495c8b666c5637c227b108d7ed679f78ef37bc6077R77), HTTP_HOST does not have a port specified so the default SERVER_PORT is used, and that is the internal 8080 port

[JanTvrdik]

That's much better, but still wrong.

1. You should not mix information from Forwaded and X-Forwaded-* headers.
2. The tests pass even when your change is removed.

The corrent way to fix this (imho) is to use default port based on schema, when explicit port is missing in the host key. The same thing is already done when using X-Forwaded- headers.

	private function useForwardedProxy(Url $url, &$remoteAddr, &$remoteHost): void
	{
		$forwardParams = preg_split('/[,;]/', $_SERVER['HTTP_FORWARDED']);
		foreach ($forwardParams as $forwardParam) {
			[$key, $value] = explode('=', $forwardParam, 2) + [1 => ''];
			$proxyParams[strtolower(trim($key))][] = trim($value, " \t\"");
		}

		if (isset($proxyParams['for'])) {
			$address = $proxyParams['for'][0];
			$remoteAddr = str_contains($address, '[')
				? substr($address, 1, strpos($address, ']') - 1) // IPv6
				: explode(':', $address)[0];  // IPv4
		}

		if (isset($proxyParams['proto']) && count($proxyParams['proto']) === 1) {
			$url->setScheme(strcasecmp($proxyParams['proto'][0], 'https') === 0 ? 'https' : 'http');
			$url->setPort($url->getScheme() === 'https' ? 443 : 80);
		}

		if (isset($proxyParams['host']) && count($proxyParams['host']) === 1) {
			$host = $proxyParams['host'][0];
			$startingDelimiterPosition = strpos($host, '[');
			if ($startingDelimiterPosition === false) { //IPv4
				$remoteHostArr = explode(':', $host);
				$remoteHost = $remoteHostArr[0];
				$url->setHost($remoteHost);
				if (isset($remoteHostArr[1])) {
					$url->setPort((int) $remoteHostArr[1]);
				}
			} else { //IPv6
				$endingDelimiterPosition = strpos($host, ']');
				$remoteHost = substr($host, strpos($host, '[') + 1, $endingDelimiterPosition - 1);
				$url->setHost($remoteHost);
				$remoteHostArr = explode(':', substr($host, $endingDelimiterPosition));
				if (isset($remoteHostArr[1])) {
					$url->setPort((int) $remoteHostArr[1]);
				}
			}
		}
	}

[JanTvrdik]

@dg now the fix looks good to me

[dg]

Thanks!