Merge pull request #43552 from nextcloud/bugfix/noid/rfc7239-compatible-proxy-handling

fix(request): Handle reverse proxy setting a port in Forwarded-For

lib/private/AppFramework/Http/Request.php

@@ -607,10 +607,15 @@ class Request implements \ArrayAccess, \Countable, IRequest {
 				if (isset($this->server[$header])) {
 					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);
+						$colons = substr_count($IP, ':');
+						if ($colons > 1) {
+							// Extract IP from string with brackets and optional port
+							if (preg_match('/^\[(.+?)\](?::\d+)?$/', $IP, $matches) && isset($matches[1])) {
+								$IP = $matches[1];
+							}
+						} elseif ($colons === 1) {
+							// IPv4 with port
+							$IP = substr($IP, 0, strpos($IP, ':'));
 						}
 
 						if ($this->isTrustedProxy($trustedProxies, $IP)) {

tests/lib/AppFramework/Http/RequestTest.php

@@ -549,357 +549,188 @@ class RequestTest extends \Test\TestCase {
 		$this->assertEquals('3', $request->getParams()['id']);
 	}
 
-	public function testGetRemoteAddressWithoutTrustedRemote() {
-		$this->config
-			->expects($this->once())
-			->method('getSystemValue')
-			->with('trusted_proxies')
-			->willReturn([]);
-
-		$request = new Request(
-			[
-				'server' => [
+	public function dataGetRemoteAddress(): array {
+		return [
+			'IPv4 without trusted remote' => [
+				[
 					'REMOTE_ADDR' => '10.0.0.2',
 					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233',
 				],
+				[],
+				[],
+				'10.0.0.2',
 			],
-			$this->requestId,
-			$this->config,
-			$this->csrfTokenManager,
-			$this->stream
-		);
-
-		$this->assertSame('10.0.0.2', $request->getRemoteAddress());
-	}
-
-	public function testGetRemoteAddressWithNoTrustedHeader() {
-		$this->config
-			->expects($this->exactly(2))
-			->method('getSystemValue')
-			->withConsecutive(
-				['trusted_proxies'],
-				['forwarded_for_headers'],
-			)->willReturnOnConsecutiveCalls(
-				['10.0.0.2'],
-				[]
-			);
-
-		$request = new Request(
-			[
-				'server' => [
+			'IPv4 without trusted headers' => [
+				[
 					'REMOTE_ADDR' => '10.0.0.2',
 					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233',
 				],
+				['10.0.0.2'],
+				[],
+				'10.0.0.2',
 			],
-			$this->requestId,
-			$this->config,
-			$this->csrfTokenManager,
-			$this->stream
-		);
-
-		$this->assertSame('10.0.0.2', $request->getRemoteAddress());
-	}
-
-	public function testGetRemoteAddressWithSingleTrustedRemote() {
-		$this->config
-			->expects($this->exactly(2))
-			->method('getSystemValue')
-			->withConsecutive(
-				['trusted_proxies'],
-				['forwarded_for_headers'],
-			)-> willReturnOnConsecutiveCalls(
+			'IPv4 with single trusted remote' => [
+				[
+					'REMOTE_ADDR' => '10.0.0.2',
+					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233',
+				],
 				['10.0.0.2'],
 				['HTTP_X_FORWARDED'],
-			);
-
-		$request = new Request(
-			[
-				'server' => [
-					'REMOTE_ADDR' => '10.0.0.2',
+				'10.4.0.4',
+			],
+			'IPv6 with single trusted remote' => [
+				[
+					'REMOTE_ADDR' => '2001:db8:85a3:8d3:1319:8a2e:370:7348',
 					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233',
 				],
+				['2001:db8:85a3:8d3:1319:8a2e:370:7348'],
+				['HTTP_X_FORWARDED'],
+				'10.4.0.4',
 			],
-			$this->requestId,
-			$this->config,
-			$this->csrfTokenManager,
-			$this->stream
-		);
-
-		$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' => [
+			'IPv4 with multiple trusted remotes' => [
+				[
 					'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'
+					'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() {
-		$this->config
-			->expects($this->exactly(2))
-			->method('getSystemValue')
-			->withConsecutive(
-				['trusted_proxies'],
-				['forwarded_for_headers'],
-			)-> willReturnOnConsecutiveCalls(
-				['2001:db8:85a3:8d3:1319:8a2e:370:7348'],
+				['10.0.0.2', '::1'],
 				['HTTP_X_FORWARDED'],
-			);
-
-		$request = new Request(
-			[
-				'server' => [
-					'REMOTE_ADDR' => '2001:db8:85a3:8d3:1319:8a2e:370:7348',
+				'10.4.0.4',
+			],
+			'IPv4 order of forwarded-for headers' => [
+				[
+					'REMOTE_ADDR' => '10.0.0.2',
 					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+					'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 testGetRemoteAddressVerifyPriorityHeader() {
-		$this->config
-			->expects($this->exactly(2))
-			->method('getSystemValue')
-			->withConsecutive(
-				['trusted_proxies'],
-				['forwarded_for_headers'],
-			)-> willReturnOnConsecutiveCalls(
 				['10.0.0.2'],
 				[
 					'HTTP_X_FORWARDED',
 					'HTTP_X_FORWARDED_FOR',
 					'HTTP_CLIENT_IP',
 				],
-			);
-
-		$request = new Request(
-			[
-				'server' => [
+				'192.168.0.233',
+			],
+			'IPv4 order of forwarded-for headers (reversed)' => [
+				[
 					'REMOTE_ADDR' => '10.0.0.2',
 					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233',
 				],
+				['10.0.0.2'],
+				[
+					'HTTP_CLIENT_IP',
+					'HTTP_X_FORWARDED_FOR',
+					'HTTP_X_FORWARDED',
+				],
+				'10.4.0.4',
 			],
-			$this->requestId,
-			$this->config,
-			$this->csrfTokenManager,
-			$this->stream
-		);
-
-		$this->assertSame('192.168.0.233', $request->getRemoteAddress());
-	}
-
-	public function testGetRemoteAddressIPv6VerifyPriorityHeader() {
-		$this->config
-			->expects($this->exactly(2))
-			->method('getSystemValue')
-			->withConsecutive(
-				['trusted_proxies'],
-				['forwarded_for_headers'],
-			)-> willReturnOnConsecutiveCalls(
+			'IPv6 order of forwarded-for headers' => [
+				[
+					'REMOTE_ADDR' => '2001:db8:85a3:8d3:1319:8a2e:370:7348',
+					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233',
+				],
 				['2001:db8:85a3:8d3:1319:8a2e:370:7348'],
 				[
 					'HTTP_X_FORWARDED',
 					'HTTP_X_FORWARDED_FOR',
 					'HTTP_CLIENT_IP',
 				],
-			);
-
-		$request = new Request(
-			[
-				'server' => [
-					'REMOTE_ADDR' => '2001:db8:85a3:8d3:1319:8a2e:370:7348',
+				'192.168.0.233',
+			],
+			'IPv4 matching CIDR of trusted proxy' => [
+				[
+					'REMOTE_ADDR' => '192.168.3.99',
 					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233',
 				],
-			],
-			$this->requestId,
-			$this->config,
-			$this->csrfTokenManager,
-			$this->stream
-		);
-
-		$this->assertSame('192.168.0.233', $request->getRemoteAddress());
-	}
-
-	public function testGetRemoteAddressWithMatchingCidrTrustedRemote() {
-		$this->config
-			->expects($this->exactly(2))
-			->method('getSystemValue')
-			->withConsecutive(
-				['trusted_proxies'],
-				['forwarded_for_headers'],
-			)-> willReturnOnConsecutiveCalls(
 				['192.168.2.0/24'],
 				['HTTP_X_FORWARDED_FOR'],
-			);
-
-		$request = new Request(
-			[
-				'server' => [
-					'REMOTE_ADDR' => '192.168.2.99',
-					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
-				],
+				'192.168.3.99',
 			],
-			$this->requestId,
-			$this->config,
-			$this->csrfTokenManager,
-			$this->stream
-		);
-
-		$this->assertSame('192.168.0.233', $request->getRemoteAddress());
-	}
-
-	public function testGetRemoteAddressWithNotMatchingCidrTrustedRemote() {
-		$this->config
-			->expects($this->once())
-			->method('getSystemValue')
-			->with('trusted_proxies')
-			->willReturn(['192.168.2.0/24']);
-
-		$request = new Request(
-			[
-				'server' => [
-					'REMOTE_ADDR' => '192.168.3.99',
+			'IPv6 matching CIDR of trusted proxy' => [
+				[
+					'REMOTE_ADDR' => '2001:db8:85a3:8d3:1319:8a21:370:7348',
 					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233',
 				],
-			],
-			$this->requestId,
-			$this->config,
-			$this->csrfTokenManager,
-			$this->stream
-		);
-
-		$this->assertSame('192.168.3.99', $request->getRemoteAddress());
-	}
-
-	public function testGetRemoteIpv6AddressWithMatchingIpv6CidrTrustedRemote() {
-		$this->config
-			->expects($this->exactly(2))
-			->method('getSystemValue')
-			->withConsecutive(
-				['trusted_proxies'],
-				['forwarded_for_headers']
-			)->willReturnOnConsecutiveCalls(
 				['2001:db8:85a3:8d3:1319:8a20::/95'],
-				['HTTP_X_FORWARDED_FOR']
-			);
-
-		$request = new Request(
-			[
-				'server' => [
-					'REMOTE_ADDR' => '2001:db8:85a3:8d3:1319:8a21:370:7348',
+				['HTTP_X_FORWARDED_FOR'],
+				'192.168.0.233',
+			],
+			'IPv6 not matching CIDR of trusted proxy' => [
+				[
+					'REMOTE_ADDR' => '2001:db8:85a3:8d3:1319:8a2e:370:7348',
 					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233',
 				],
+				['fd::/8'],
+				[],
+				'2001:db8:85a3:8d3:1319:8a2e:370:7348',
 			],
-			$this->requestId,
-			$this->config,
-			$this->csrfTokenManager,
-			$this->stream
-		);
-
-		$this->assertSame('192.168.0.233', $request->getRemoteAddress());
-	}
-
-	public function testGetRemoteAddressIpv6WithNotMatchingCidrTrustedRemote() {
-		$this->config
-			->expects($this->once())
-			->method('getSystemValue')
-			->with('trusted_proxies')
-			->willReturn(['fd::/8']);
-
-		$request = new Request(
-			[
-				'server' => [
+			'IPv6 with invalid trusted proxy' => [
+				[
 					'REMOTE_ADDR' => '2001:db8:85a3:8d3:1319:8a2e:370:7348',
 					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+					'HTTP_X_FORWARDED_FOR' => '192.168.0.233',
 				],
+				['fx::/8'],
+				[],
+				'2001:db8:85a3:8d3:1319:8a2e:370:7348',
 			],
-			$this->requestId,
-			$this->config,
-			$this->csrfTokenManager,
-			$this->stream
-		);
-
-		$this->assertSame('2001:db8:85a3:8d3:1319:8a2e:370:7348', $request->getRemoteAddress());
-	}
-
-	public function testGetRemoteAddressIpv6WithInvalidTrustedProxy() {
-		$this->config
-			->expects($this->once())
-			->method('getSystemValue')
-			->with('trusted_proxies')
-			->willReturn(['fx::/8']);
-
-		$request = new Request(
-			[
-				'server' => [
+			'IPv4 forwarded for IPv6' => [
+				[
+					'REMOTE_ADDR' => '192.168.2.99',
+					'HTTP_X_FORWARDED_FOR' => '[2001:db8:85a3:8d3:1319:8a2e:370:7348]',
+				],
+				['192.168.2.0/24'],
+				['HTTP_X_FORWARDED_FOR'],
+				'2001:db8:85a3:8d3:1319:8a2e:370:7348',
+			],
+			'IPv4 with port' => [
+				[
 					'REMOTE_ADDR' => '2001:db8:85a3:8d3:1319:8a2e:370:7348',
-					'HTTP_X_FORWARDED' => '10.4.0.5, 10.4.0.4',
-					'HTTP_X_FORWARDED_FOR' => '192.168.0.233'
+					'HTTP_X_FORWARDED_FOR' => '192.168.2.99:8080',
 				],
+				['2001:db8::/8'],
+				['HTTP_X_FORWARDED_FOR'],
+				'192.168.2.99',
 			],
-			$this->requestId,
-			$this->config,
-			$this->csrfTokenManager,
-			$this->stream
-		);
-
-		$this->assertSame('2001:db8:85a3:8d3:1319:8a2e:370:7348', $request->getRemoteAddress());
+			'IPv6 with port' => [
+				[
+					'REMOTE_ADDR' => '192.168.2.99',
+					'HTTP_X_FORWARDED_FOR' => '[2001:db8:85a3:8d3:1319:8a2e:370:7348]:8080',
+				],
+				['192.168.2.0/24'],
+				['HTTP_X_FORWARDED_FOR'],
+				'2001:db8:85a3:8d3:1319:8a2e:370:7348',
+			],
+		];
 	}
 
-	public function testGetRemoteAddressWithXForwardedForIPv6() {
+	/**
+	 * @dataProvider dataGetRemoteAddress
+	 */
+	public function testGetRemoteAddress(array $headers, array $trustedProxies, array $forwardedForHeaders, string $expected): void {
 		$this->config
-			->expects($this->exactly(2))
 			->method('getSystemValue')
 			->withConsecutive(
 				['trusted_proxies'],
 				['forwarded_for_headers'],
-			)-> willReturnOnConsecutiveCalls(
-				['192.168.2.0/24'],
-				['HTTP_X_FORWARDED_FOR'],
+			)
+			->willReturnOnConsecutiveCalls(
+				$trustedProxies,
+				$forwardedForHeaders,
 			);
 
 		$request = new Request(
 			[
-				'server' => [
-					'REMOTE_ADDR' => '192.168.2.99',
-					'HTTP_X_FORWARDED_FOR' => '[2001:db8:85a3:8d3:1319:8a2e:370:7348]',
-				],
+				'server' => $headers,
 			],
 			$this->requestId,
 			$this->config,
@@ -907,7 +738,7 @@ class RequestTest extends \Test\TestCase {
 			$this->stream
 		);
 
-		$this->assertSame('2001:db8:85a3:8d3:1319:8a2e:370:7348', $request->getRemoteAddress());
+		$this->assertSame($expected, $request->getRemoteAddress());
 	}
 
 	/**