From 986f4df2a59547ae08359fe7907147577222a8a7 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Thu, 25 Oct 2018 22:26:49 +0200 Subject: [PATCH 1/2] Add REMOTE_ADDR to getHeader Signed-off-by: Daniel Kesselberg --- lib/private/AppFramework/Http/Request.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php index 0485d178b4..1d5a29f74f 100644 --- a/lib/private/AppFramework/Http/Request.php +++ b/lib/private/AppFramework/Http/Request.php @@ -320,14 +320,18 @@ class Request implements \ArrayAccess, \Countable, IRequest { // There's a few headers that seem to end up in the top-level // server array. - switch($name) { + switch ($name) { case 'CONTENT_TYPE' : case 'CONTENT_LENGTH' : if (isset($this->server[$name])) { return $this->server[$name]; } break; - + case 'REMOTE_ADDR' : + if (isset($this->server[$name])) { + return $this->server[$name]; + } + break; } return ''; From 5cf8f4a407787151a8aa47c35e9aa2aa5a3d443c Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Thu, 25 Oct 2018 22:30:42 +0200 Subject: [PATCH 2/2] Update logic for forwardedForHeadersWorking As discussed in https://github.com/nextcloud/server/issues/11594 when discovering if x-forwarded-for is working properly its not possible to use getRemoteAddr because the "client ip" is returned. For this check the ip of the last hop would be required. Signed-off-by: Daniel Kesselberg --- settings/Controller/CheckSetupController.php | 7 ++- .../Controller/CheckSetupControllerTest.php | 52 ++++++++++--------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/settings/Controller/CheckSetupController.php b/settings/Controller/CheckSetupController.php index 42bb473987..fa4ed57ab9 100644 --- a/settings/Controller/CheckSetupController.php +++ b/settings/Controller/CheckSetupController.php @@ -287,12 +287,11 @@ class CheckSetupController extends Controller { */ private function forwardedForHeadersWorking() { $trustedProxies = $this->config->getSystemValue('trusted_proxies', []); - $remoteAddress = $this->request->getRemoteAddress(); + $remoteAddress = $this->request->getHeader('REMOTE_ADDR'); - if (is_array($trustedProxies) && in_array($remoteAddress, $trustedProxies)) { - return false; + if (\is_array($trustedProxies) && \in_array($remoteAddress, $trustedProxies)) { + return $remoteAddress !== $this->request->getRemoteAddress(); } - // either not enabled or working correctly return true; } diff --git a/tests/Settings/Controller/CheckSetupControllerTest.php b/tests/Settings/Controller/CheckSetupControllerTest.php index 6706573a1a..34c7d19bd8 100644 --- a/tests/Settings/Controller/CheckSetupControllerTest.php +++ b/tests/Settings/Controller/CheckSetupControllerTest.php @@ -295,38 +295,41 @@ class CheckSetupControllerTest extends TestCase { ); } - public function testForwardedForHeadersWorkingFalse() { + /** + * @dataProvider dataForwardedForHeadersWorking + * + * @param array $trustedProxies + * @param string $remoteAddrNoForwarded + * @param string $remoteAddr + * @param bool $result + */ + public function testForwardedForHeadersWorking(array $trustedProxies, string $remoteAddrNoForwarded, string $remoteAddr, bool $result) { $this->config->expects($this->once()) ->method('getSystemValue') ->with('trusted_proxies', []) - ->willReturn(['1.2.3.4']); + ->willReturn($trustedProxies); $this->request->expects($this->once()) + ->method('getHeader') + ->with('REMOTE_ADDR') + ->willReturn($remoteAddrNoForwarded); + $this->request->expects($this->any()) ->method('getRemoteAddress') - ->willReturn('1.2.3.4'); + ->willReturn($remoteAddr); - $this->assertFalse( - self::invokePrivate( - $this->checkSetupController, - 'forwardedForHeadersWorking' - ) + $this->assertEquals( + $result, + self::invokePrivate($this->checkSetupController, 'forwardedForHeadersWorking') ); } - public function testForwardedForHeadersWorkingTrue() { - $this->config->expects($this->once()) - ->method('getSystemValue') - ->with('trusted_proxies', []) - ->willReturn(['1.2.3.4']); - $this->request->expects($this->once()) - ->method('getRemoteAddress') - ->willReturn('4.3.2.1'); - - $this->assertTrue( - self::invokePrivate( - $this->checkSetupController, - 'forwardedForHeadersWorking' - ) - ); + public function dataForwardedForHeadersWorking() { + 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], + ]; } public function testCheck() { @@ -348,7 +351,8 @@ class CheckSetupControllerTest extends TestCase { ->will($this->returnValue(false)); $this->request->expects($this->once()) - ->method('getRemoteAddress') + ->method('getHeader') + ->with('REMOTE_ADDR') ->willReturn('4.3.2.1'); $client = $this->getMockBuilder('\OCP\Http\Client\IClient')