Merge pull request #13116 from nextcloud/fix/only_trust_xforwardedhost_for_trusted_proxies

Only trust the X-FORWARDED-HOST header for trusted proxies

lib/private/AppFramework/Http/Request.php

@@ -691,7 +691,7 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 			return $this->config->getSystemValue('overwriteprotocol');
 		}
 
-		if (isset($this->server['HTTP_X_FORWARDED_PROTO'])) {
+		if ($this->fromTrustedProxy() && isset($this->server['HTTP_X_FORWARDED_PROTO'])) {
 			if (strpos($this->server['HTTP_X_FORWARDED_PROTO'], ',') !== false) {
 				$parts = explode(',', $this->server['HTTP_X_FORWARDED_PROTO']);
 				$proto = strtolower(trim($parts[0]));
@@ -862,7 +862,7 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 	 */
 	public function getInsecureServerHost(): string {
 		$host = 'localhost';
-		if (isset($this->server['HTTP_X_FORWARDED_HOST'])) {
+		if ($this->fromTrustedProxy() && isset($this->server['HTTP_X_FORWARDED_HOST'])) {
 			if (strpos($this->server['HTTP_X_FORWARDED_HOST'], ',') !== false) {
 				$parts = explode(',', $this->server['HTTP_X_FORWARDED_HOST']);
 				$host = trim(current($parts));
@@ -924,4 +924,10 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 		return null;
 	}
 
+	private function fromTrustedProxy(): bool {
+		$remoteAddress = isset($this->server['REMOTE_ADDR']) ? $this->server['REMOTE_ADDR'] : '';
+		$trustedProxies = $this->config->getSystemValue('trusted_proxies', []);
+
+		return \is_array($trustedProxies) && $this->isTrustedProxy($trustedProxies, $remoteAddress);
+	}
 }

tests/lib/AppFramework/Http/RequestTest.php

@@ -715,15 +715,20 @@ class RequestTest extends \Test\TestCase {
 
 	public function testGetServerProtocolWithProtoValid() {
 		$this->config
-			->expects($this->exactly(2))
 			->method('getSystemValue')
-			->with('overwriteprotocol')
+			->will($this->returnCallback(function($key, $default) {
-			->will($this->returnValue(''));
+				if ($key === 'trusted_proxies') {
+					return ['1.2.3.4'];
+				}
+
+				return $default;
+			}));
 
 		$requestHttps = new Request(
 			[
 				'server' => [
-					'HTTP_X_FORWARDED_PROTO' => 'HtTpS'
+					'HTTP_X_FORWARDED_PROTO' => 'HtTpS',
+					'REMOTE_ADDR' => '1.2.3.4',
 				],
 			],
 			$this->secureRandom,
@@ -734,7 +739,8 @@ class RequestTest extends \Test\TestCase {
 		$requestHttp = new Request(
 			[
 				'server' => [
-					'HTTP_X_FORWARDED_PROTO' => 'HTTp'
+					'HTTP_X_FORWARDED_PROTO' => 'HTTp',
+					'REMOTE_ADDR' => '1.2.3.4',
 				],
 			],
 			$this->secureRandom,
@@ -750,10 +756,10 @@ class RequestTest extends \Test\TestCase {
 
 	public function testGetServerProtocolWithHttpsServerValueOn() {
 		$this->config
-			->expects($this->once())
 			->method('getSystemValue')
-			->with('overwriteprotocol')
+			->will($this->returnCallback(function($key, $default) {
-			->will($this->returnValue(''));
+				return $default;
+			}));
 
 		$request = new Request(
 			[
@@ -771,10 +777,10 @@ class RequestTest extends \Test\TestCase {
 
 	public function testGetServerProtocolWithHttpsServerValueOff() {
 		$this->config
-			->expects($this->once())
 			->method('getSystemValue')
-			->with('overwriteprotocol')
+			->will($this->returnCallback(function($key, $default) {
-			->will($this->returnValue(''));
+				return $default;
+			}));
 
 		$request = new Request(
 			[
@@ -792,10 +798,10 @@ class RequestTest extends \Test\TestCase {
 
 	public function testGetServerProtocolWithHttpsServerValueEmpty() {
 		$this->config
-			->expects($this->once())
 			->method('getSystemValue')
-			->with('overwriteprotocol')
+			->will($this->returnCallback(function($key, $default) {
-			->will($this->returnValue(''));
+				return $default;
+			}));
 
 		$request = new Request(
 			[
@@ -813,10 +819,10 @@ class RequestTest extends \Test\TestCase {
 
 	public function testGetServerProtocolDefault() {
 		$this->config
-			->expects($this->once())
 			->method('getSystemValue')
-			->with('overwriteprotocol')
+			->will($this->returnCallback(function($key, $default) {
-			->will($this->returnValue(''));
+				return $default;
+			}));
 
 		$request = new Request(
 			[],
@@ -830,15 +836,20 @@ class RequestTest extends \Test\TestCase {
 
 	public function testGetServerProtocolBehindLoadBalancers() {
 		$this->config
-			->expects($this->once())
 			->method('getSystemValue')
-			->with('overwriteprotocol')
+			->will($this->returnCallback(function($key, $default) {
-			->will($this->returnValue(''));
+				if ($key === 'trusted_proxies') {
+					return ['1.2.3.4'];
+				}
+
+				return $default;
+			}));
 
 		$request = new Request(
 			[
 				'server' => [
-					'HTTP_X_FORWARDED_PROTO' => 'https,http,http'
+					'HTTP_X_FORWARDED_PROTO' => 'https,http,http',
+					'REMOTE_ADDR' => '1.2.3.4',
 				],
 			],
 			$this->secureRandom,
@@ -1025,12 +1036,23 @@ class RequestTest extends \Test\TestCase {
 	}
 
 	public function testInsecureServerHostHttpFromForwardedHeaderSingle() {
+		$this->config
+			->method('getSystemValue')
+			->will($this->returnCallback(function($key, $default) {
+				if ($key === 'trusted_proxies') {
+					return ['1.2.3.4'];
+				}
+
+				return $default;
+			}));
+
 		$request = new Request(
 			[
 				'server' => [
 					'SERVER_NAME' => 'from.server.name:8080',
 					'HTTP_HOST' => 'from.host.header:8080',
 					'HTTP_X_FORWARDED_HOST' => 'from.forwarded.host:8080',
+					'REMOTE_ADDR' => '1.2.3.4',
 				]
 			],
 			$this->secureRandom,
@@ -1043,12 +1065,23 @@ class RequestTest extends \Test\TestCase {
 	}
 
 	public function testInsecureServerHostHttpFromForwardedHeaderStacked() {
+		$this->config
+			->method('getSystemValue')
+			->will($this->returnCallback(function($key, $default) {
+				if ($key === 'trusted_proxies') {
+					return ['1.2.3.4'];
+				}
+
+				return $default;
+			}));
+
 		$request = new Request(
 			[
 				'server' => [
 					'SERVER_NAME' => 'from.server.name:8080',
 					'HTTP_HOST' => 'from.host.header:8080',
 					'HTTP_X_FORWARDED_HOST' => 'from.forwarded.host2:8080,another.one:9000',
+					'REMOTE_ADDR' => '1.2.3.4',
 				]
 			],
 			$this->secureRandom,
@@ -1062,20 +1095,16 @@ class RequestTest extends \Test\TestCase {
 
 	public function testGetServerHostWithOverwriteHost() {
 		$this->config
-			->expects($this->at(0))
 			->method('getSystemValue')
-			->with('overwritehost')
+			->will($this->returnCallback(function($key, $default) {
-			->will($this->returnValue('my.overwritten.host'));
+				if ($key === 'overwritecondaddr') {
-		$this->config
+					return '';
-			->expects($this->at(1))
+				} else if ($key === 'overwritehost') {
-			->method('getSystemValue')
+					return 'my.overwritten.host';
-			->with('overwritecondaddr')
+				}
-			->will($this->returnValue(''));
+
-		$this->config
+				return $default;
-			->expects($this->at(2))
+			}));
-			->method('getSystemValue')
-			->with('overwritehost')
-			->will($this->returnValue('my.overwritten.host'));
 
 		$request = new Request(
 			[],
@@ -1090,15 +1119,22 @@ class RequestTest extends \Test\TestCase {
 
 	public function testGetServerHostWithTrustedDomain() {
 		$this->config
-			->expects($this->at(3))
 			->method('getSystemValue')
-			->with('trusted_domains')
+			->will($this->returnCallback(function($key, $default) {
-			->will($this->returnValue(['my.trusted.host']));
+				if ($key === 'trusted_proxies') {
+					return ['1.2.3.4'];
+				} else if ($key === 'trusted_domains') {
+					return ['my.trusted.host'];
+				}
+
+				return $default;
+			}));
 
 		$request = new Request(
 			[
 				'server' => [
 					'HTTP_X_FORWARDED_HOST' => 'my.trusted.host',
+					'REMOTE_ADDR' => '1.2.3.4',
 				],
 			],
 			$this->secureRandom,
@@ -1112,20 +1148,22 @@ class RequestTest extends \Test\TestCase {
 
 	public function testGetServerHostWithUntrustedDomain() {
 		$this->config
-			->expects($this->at(3))
 			->method('getSystemValue')
-			->with('trusted_domains')
+			->will($this->returnCallback(function($key, $default) {
-			->will($this->returnValue(['my.trusted.host']));
+				if ($key === 'trusted_proxies') {
-		$this->config
+					return ['1.2.3.4'];
-			->expects($this->at(4))
+				} else if ($key === 'trusted_domains') {
-			->method('getSystemValue')
+					return ['my.trusted.host'];
-			->with('trusted_domains')
+				}
-			->will($this->returnValue(['my.trusted.host']));
+
+				return $default;
+			}));
 
 		$request = new Request(
 			[
 				'server' => [
 					'HTTP_X_FORWARDED_HOST' => 'my.untrusted.host',
+					'REMOTE_ADDR' => '1.2.3.4',
 				],
 			],
 			$this->secureRandom,
@@ -1139,20 +1177,19 @@ class RequestTest extends \Test\TestCase {
 
 	public function testGetServerHostWithNoTrustedDomain() {
 		$this->config
-			->expects($this->at(3))
 			->method('getSystemValue')
-			->with('trusted_domains')
+			->will($this->returnCallback(function($key, $default) {
-			->will($this->returnValue([]));
+				if ($key === 'trusted_proxies') {
-		$this->config
+					return ['1.2.3.4'];
-			->expects($this->at(4))
+				}
-			->method('getSystemValue')
+				return $default;
-			->with('trusted_domains')
+			}));
-			->will($this->returnValue([]));
 
 		$request = new Request(
 			[
 				'server' => [
 					'HTTP_X_FORWARDED_HOST' => 'my.untrusted.host',
+					'REMOTE_ADDR' => '1.2.3.4',
 				],
 			],
 			$this->secureRandom,