Merge pull request #41530 from nextcloud/backport/41526/stable24

[stable24] Reverse X-Forwarded-For list to read the correct proxy remote address
nextcloud · Nov 16, 2023 · ba87099 · ba87099
2 parents 2dd8861 + 2fd3a37
commit ba87099
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 8 deletions.
diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php
@@ -626,16 +626,22 @@ public function getRemoteAddress(): string {
 				// 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
 						if (strpos($IP, '[') === 0 && substr($IP, -1) === ']') {
 							$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
@@ -445,7 +445,33 @@ public function testGetRemoteAddressWithSingleTrustedRemote() {
 			$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() {
@@ -474,7 +500,7 @@ public function testGetRemoteAddressIPv6WithSingleTrustedRemote() {
 			$this->stream
 		);
 
-		$this->assertSame('10.4.0.5', $request->getRemoteAddress());
+		$this->assertSame('10.4.0.4', $request->getRemoteAddress());
 	}
 
 	public function testGetRemoteAddressVerifyPriorityHeader() {
@@ -488,9 +514,9 @@ public function testGetRemoteAddressVerifyPriorityHeader() {
 			->method('getSystemValue')
 			->with('forwarded_for_headers')
 			->willReturn([
-				'HTTP_CLIENT_IP',
+				'HTTP_X_FORWARDED',
 				'HTTP_X_FORWARDED_FOR',
-				'HTTP_X_FORWARDED'
+				'HTTP_CLIENT_IP',
 			]);
 
 		$request = new Request(
@@ -521,9 +547,9 @@ public function testGetRemoteAddressIPv6VerifyPriorityHeader() {
 			->method('getSystemValue')
 			->with('forwarded_for_headers')
 			->willReturn([
-				'HTTP_CLIENT_IP',
+				'HTTP_X_FORWARDED',
 				'HTTP_X_FORWARDED_FOR',
-				'HTTP_X_FORWARDED'
+				'HTTP_CLIENT_IP'
 			]);
 
 		$request = new Request(