Merge pull request #14261 from nextcloud/show-warning-if-x-forwarded-set-with-empty-trusted-proxies

Warning if x-forwarded-host present but trusted_proxies empty

2 files changed, 39 insertions, 9 deletions

diff --git a/settings/Controller/CheckSetupController.php b/settings/Controller/CheckSetupController.php
index d21874e025d..920bf59ea55 100644
--- a/settings/Controller/CheckSetupController.php
+++ b/settings/Controller/CheckSetupController.php
@@ -289,9 +289,14 @@ class CheckSetupController extends Controller {
 		$trustedProxies = $this->config->getSystemValue('trusted_proxies', []);
 		$remoteAddress = $this->request->getHeader('REMOTE_ADDR');
 
-		if (\is_array($trustedProxies) && \in_array($remoteAddress, $trustedProxies)) {
+		if (empty($trustedProxies) && $this->request->getHeader('X-Forwarded-Host')) {
+			return false;
+		}
+
+		if (\is_array($trustedProxies) && \in_array($remoteAddress, $trustedProxies, true)) {
 			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 7efc6c56bc4..b1b451aa9e2 100644
--- a/tests/Settings/Controller/CheckSetupControllerTest.php
+++ b/tests/Settings/Controller/CheckSetupControllerTest.php
@@ -310,19 +310,21 @@ class CheckSetupControllerTest extends TestCase {
 	 * @dataProvider dataForwardedForHeadersWorking
 	 *
 	 * @param array $trustedProxies
-	 * @param string $remoteAddrNoForwarded
+	 * @param string $remoteAddrNotForwarded
 	 * @param string $remoteAddr
 	 * @param bool $result
 	 */
-	public function testForwardedForHeadersWorking(array $trustedProxies, string $remoteAddrNoForwarded, string $remoteAddr, 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);
-		$this->request->expects($this->once())
+		$this->request->expects($this->atLeastOnce())
 			->method('getHeader')
-			->with('REMOTE_ADDR')
-			->willReturn($remoteAddrNoForwarded);
+			->willReturnMap([
+				['REMOTE_ADDR', $remoteAddrNotForwarded],
+				['X-Forwarded-Host', '']
+			]);
 		$this->request->expects($this->any())
 			->method('getRemoteAddress')
 			->willReturn($remoteAddr);
@@ -343,6 +345,27 @@ class CheckSetupControllerTest extends TestCase {
 		];
 	}
 
+	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')
@@ -365,10 +388,12 @@ class CheckSetupControllerTest extends TestCase {
 			->with('appstoreenabled', true)
 			->will($this->returnValue(false));
 
-		$this->request->expects($this->once())
+		$this->request->expects($this->atLeastOnce())
 			->method('getHeader')
-			->with('REMOTE_ADDR')
-			->willReturn('4.3.2.1');
+			->willReturnMap([
+				['REMOTE_ADDR', '4.3.2.1'],
+				['X-Forwarded-Host', '']
+			]);
 
 		$client = $this->getMockBuilder('\OCP\Http\Client\IClient')
 			->disableOriginalConstructor()->getMock();