From 2fd3a372f773424e1d6b3b674f92b2ca5c37b60c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 30 Oct 2023 14:14:20 +0100 Subject: [PATCH] Reverse X-Forwarded-For list to read the correct proxy remote address Signed-off-by: Joas Schilling --- lib/private/AppFramework/Http/Request.php | 10 ++++-- tests/lib/AppFramework/Http/RequestTest.php | 38 +++++++++++++++++---- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php index 770946c80d557..af6cf27356321 100644 --- a/lib/private/AppFramework/Http/Request.php +++ b/lib/private/AppFramework/Http/Request.php @@ -626,9 +626,11 @@ 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 @@ -636,6 +638,10 @@ public function getRemoteAddress(): string { $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 cd48822573905..5d2fc63daf716 100644 --- 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(