Merge pull request #41526 from nextcloud/bugfix/noid/fix-detection-of…

…-trusted-proxies Reverse X-Forwarded-For list to read the correct proxy remote address
nextcloud · Nov 16, 2023 · 6716b3f · 6716b3f
2 parents 04c3fea + 2fa78f6
commit 6716b3f
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
@@ -593,16 +593,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 (str_starts_with($IP, '[') && str_ends_with($IP, ']')) {
 							$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
@@ -628,7 +628,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() {
@@ -657,7 +683,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() {
@@ -670,9 +696,9 @@ public function testGetRemoteAddressVerifyPriorityHeader() {
 			)-> 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 @@ public function testGetRemoteAddressIPv6VerifyPriorityHeader() {
 			)-> 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',
 				],
 			);