Update logic for forwardedForHeadersWorking

As discussed in #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>
nextcloud · Oct 25, 2018 · 5cf8f4a · 5cf8f4a
1 parent 986f4df
commit 5cf8f4a
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 28 deletions.
diff --git a/settings/Controller/CheckSetupController.php b/settings/Controller/CheckSetupController.php
@@ -287,12 +287,11 @@ private function isPhpSupported() {
 	 */
 	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
@@ -295,38 +295,41 @@ public function testIsPhpSupportedTrue() {
 		);
 	}
 
-	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 @@ public function testCheck() {
 			->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')