Reverse X-Forwarded-For list to read the correct proxy remote address

Signed-off-by: Joas Schilling <coding@schilljs.com>

2 files changed, 40 insertions, 8 deletions

diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php
index 52abb909b60..fdc475bd165 100644
--- a/lib/private/AppFramework/Http/Request.php
+++ b/lib/private/AppFramework/Http/Request.php
@@ -597,9 +597,11 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 				// only have one default, so we cannot ship an insecure product out of the box
 			]);
 
-			foreach ($forwardedForHeaders as $header) {
+			// Read the x-forwarded-for headers and values in reverse order as per
+			// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For#selecting_an_ip_address
+			foreach (array_reverse($forwardedForHeaders) as $header) {
 				if (isset($this->server[$header])) {
-					foreach (explode(',', $this->server[$header]) as $IP) {
+					foreach (array_reverse(explode(',', $this->server[$header])) as $IP) {
 						$IP = trim($IP);
 
 						// remove brackets from IPv6 addresses
@@ -607,6 +609,10 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 							$IP = substr($IP, 1, -1);
 						}
 
+						if ($this->isTrustedProxy($trustedProxies, $IP)) {
+							continue;
+						}
+
 						if (filter_var($IP, FILTER_VALIDATE_IP) !== false) {
 							return $IP;
 						}
diff --git a/tests/lib/AppFramework/Http/RequestTest.php b/tests/lib/AppFramework/Http/RequestTest.php
index 839c7ad4338..e042d933871 100644
--- a/tests/lib/AppFramework/Http/RequestTest.php
+++ b/tests/lib/AppFramework/Http/RequestTest.php
@@ -628,7 +628,33 @@ class RequestTest extends \Test\TestCase {
 			$this->stream
 		);
 
-		$this->assertSame('10.4.0.5', $request->getRemoteAddress());
+		$this->assertSame('10.4.0.4', $request->getRemoteAddress());
+	}
+
+	public function testGetRemoteAddressWithMultipleTrustedRemotes() {
+		$this->config
+			->expects($this->exactly(2))
+			->method('getSystemValue')
+			->willReturnMap([
+				['trusted_proxies', [], ['10.0.0.2', '::1']],
+				['forwarded_for_headers', ['HTTP_X_FORWARDED_FOR'], ['HTTP_X_FORWARDED']],
+			]);
+
+		$request = new Request(
+			[
+				'server' => [
+					'REMOTE_ADDR' => '10.0.0.2',
+					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4, ::1',
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+				],
+			],
+			$this->requestId,
+			$this->config,
+			$this->csrfTokenManager,
+			$this->stream
+		);
+
+		$this->assertSame('10.4.0.4', $request->getRemoteAddress());
 	}
 
 	public function testGetRemoteAddressIPv6WithSingleTrustedRemote() {
@@ -657,7 +683,7 @@ class RequestTest extends \Test\TestCase {
 			$this->stream
 		);
 
-		$this->assertSame('10.4.0.5', $request->getRemoteAddress());
+		$this->assertSame('10.4.0.4', $request->getRemoteAddress());
 	}
 
 	public function testGetRemoteAddressVerifyPriorityHeader() {
@@ -670,9 +696,9 @@ class RequestTest extends \Test\TestCase {
 			)-> willReturnOnConsecutiveCalls(
 				['10.0.0.2'],
 				[
-					'HTTP_CLIENT_IP',
-					'HTTP_X_FORWARDED_FOR',
 					'HTTP_X_FORWARDED',
+					'HTTP_X_FORWARDED_FOR',
+					'HTTP_CLIENT_IP',
 				],
 			);
 
@@ -703,9 +729,9 @@ class RequestTest extends \Test\TestCase {
 			)-> willReturnOnConsecutiveCalls(
 				['2001:db8:85a3:8d3:1319:8a2e:370:7348'],
 				[
-					'HTTP_CLIENT_IP',
+					'HTTP_X_FORWARDED',
 					'HTTP_X_FORWARDED_FOR',
-					'HTTP_X_FORWARDED'
+					'HTTP_CLIENT_IP',
 				],
 			);