From 5cf8f4a407787151a8aa47c35e9aa2aa5a3d443c Mon Sep 17 00:00:00 2001
From: Daniel Kesselberg <mail@danielkesselberg.de>
Date: Thu, 25 Oct 2018 22:30:42 +0200
Subject: [PATCH] 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 <mail@danielkesselberg.de>
---
 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')