From 01118a2218e76d5fb89eb5eaeb101ce43632996f Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 12 Feb 2021 14:22:27 +0100 Subject: [PATCH] use the configured forwarded headers for the setup check instead of always checking against the same header Signed-off-by: Robin Appelman --- .../lib/Controller/CheckSetupController.php | 9 ++- .../Controller/CheckSetupControllerTest.php | 70 ++++++++----------- 2 files changed, 39 insertions(+), 40 deletions(-) diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index d732b13719..47a55591a4 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -309,7 +309,14 @@ class CheckSetupController extends Controller { $trustedProxies = $this->config->getSystemValue('trusted_proxies', []); $remoteAddress = $this->request->getHeader('REMOTE_ADDR'); - if (empty($trustedProxies) && $this->request->getHeader('X-Forwarded-Host') !== '') { + $forwardedForHeaders = $this->config->getSystemValue('forwarded_for_headers', [ + 'HTTP_X_FORWARDED_FOR' + ]); + $hasForwardedHeaderSet = array_reduce($forwardedForHeaders, function($set, $header) { + return $set || ($this->request->getHeader($header) !== ''); + }, false); + + if (empty($trustedProxies) && $hasForwardedHeaderSet) { return false; } diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index 426793df60..2020d9f19f 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -329,26 +329,37 @@ class CheckSetupControllerTest extends TestCase { } /** - * @dataProvider dataForwardedForHeadersWorking + * @dataProvider dataForwardedForHeaders * - * @param array $trustedProxies + * @param string[] $trustedProxies * @param string $remoteAddrNotForwarded * @param string $remoteAddr + * @param string[] $forwardedForHeaders + * @param array $requestHeaders * @param bool $result */ - public function testForwardedForHeadersWorking(array $trustedProxies, string $remoteAddrNotForwarded, string $remoteAddr, bool $result) { - $this->config->expects($this->once()) - ->method('getSystemValue') - ->with('trusted_proxies', []) - ->willReturn($trustedProxies); + public function testForwardedForHeaders(array $trustedProxies, string $remoteAddrNotForwarded, string $remoteAddr, array $forwardedForHeaders, array $requestHeaders, bool $result) { + $this->config->method('getSystemValue') + ->willReturnCallback(function($key, $default) use ($forwardedForHeaders, $trustedProxies) { + switch ($key) { + case 'forwarded_for_headers': + return $forwardedForHeaders; + case 'trusted_proxies': + return $trustedProxies; + default: + return $default; + } + }); + $headers = array_merge( + ['REMOTE_ADDR' => $remoteAddrNotForwarded], + $requestHeaders + ); $this->request->expects($this->atLeastOnce()) ->method('getHeader') - ->willReturnMap([ - ['REMOTE_ADDR', $remoteAddrNotForwarded], - ['X-Forwarded-Host', ''] - ]); - $this->request->expects($this->any()) - ->method('getRemoteAddress') + ->willReturnCallback(function($header) use ($headers) { + return isset($headers[$header]) ? $headers[$header] : ''; + }); + $this->request->method('getRemoteAddress') ->willReturn($remoteAddr); $this->assertEquals( @@ -357,37 +368,18 @@ class CheckSetupControllerTest extends TestCase { ); } - public function dataForwardedForHeadersWorking() { + public function dataForwardedForHeaders() { return [ // description => trusted proxies, getHeader('REMOTE_ADDR'), getRemoteAddr, expected result - 'no trusted proxies' => [[], '2.2.2.2', '2.2.2.2', true], - 'trusted proxy, remote addr not trusted proxy' => [['1.1.1.1'], '2.2.2.2', '2.2.2.2', true], - 'trusted proxy, remote addr is trusted proxy, x-forwarded-for working' => [['1.1.1.1'], '1.1.1.1', '2.2.2.2', true], - 'trusted proxy, remote addr is trusted proxy, x-forwarded-for not set' => [['1.1.1.1'], '1.1.1.1', '1.1.1.1', false], + 'no trusted proxies' => [[], '2.2.2.2', '2.2.2.2', ['HTTP_X_FORWARDED_FOR'], [], true], + 'trusted proxy, remote addr not trusted proxy' => [['1.1.1.1'], '2.2.2.2', '2.2.2.2', ['HTTP_X_FORWARDED_FOR'], [], true], + 'trusted proxy, remote addr is trusted proxy, forwarded header working' => [['1.1.1.1'], '1.1.1.1', '2.2.2.2', ['HTTP_X_FORWARDED_FOR'], [], true], + 'trusted proxy, remote addr is trusted proxy, forwarded header not set' => [['1.1.1.1'], '1.1.1.1', '1.1.1.1', ['HTTP_X_FORWARDED_FOR'], [], false], + 'no trusted proxies, but header present' => [[], '2.2.2.2', '2.2.2.2', ['HTTP_X_FORWARDED_FOR'], ['HTTP_X_FORWARDED_FOR' => '1.1.1.1'], false], + 'no trusted proxies, different header present' => [[], '2.2.2.2', '2.2.2.2', ['HTTP_X_FORWARDED_FOR'], ['FORWARDED' => '1.1.1.1'], true], ]; } - public function testForwardedHostPresentButTrustedProxiesEmpty() { - $this->config->expects($this->once()) - ->method('getSystemValue') - ->with('trusted_proxies', []) - ->willReturn([]); - $this->request->expects($this->atLeastOnce()) - ->method('getHeader') - ->willReturnMap([ - ['REMOTE_ADDR', '1.1.1.1'], - ['X-Forwarded-Host', 'nextcloud.test'] - ]); - $this->request->expects($this->any()) - ->method('getRemoteAddress') - ->willReturn('1.1.1.1'); - - $this->assertEquals( - false, - self::invokePrivate($this->checkSetupController, 'forwardedForHeadersWorking') - ); - } - public function testCheck() { $this->config->expects($this->at(0)) ->method('getAppValue')