From 7a7204a476bdc761a115e0d597815bc18e2e507c Mon Sep 17 00:00:00 2001
From: Hari Venu V <hari.venu@qed42.com>
Date: Thu, 9 Jan 2025 16:11:22 +0530
Subject: [PATCH 1/4] Issue #3425451: Multiple IPs returning, split and use 1st
one
---
.../ReverseProxyHeaderClientIpRestore.php | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php b/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php
index f521745..6560a6d 100644
--- a/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php
+++ b/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php
@@ -55,7 +55,8 @@ class ReverseProxyHeaderClientIpRestore implements EventSubscriberInterface {
* {@inheritdoc}
*/
public static function getSubscribedEvents() {
- // Priority is set to 350 (to run before RouterListener::onKernelRequest).
+ // Priority is set to 350, so it runs
+ // before RouterListener::onKernelRequest.
// Which allows AuthenticationSubscriber methods to use the request IP.
// @see \Symfony\Component\HttpKernel\EventListener\RouterListener::getSubscribedEvents
// @see \Drupal\Core\EventSubscriber\AuthenticationSubscriber::getSubscribedEvents
@@ -78,7 +79,21 @@ class ReverseProxyHeaderClientIpRestore implements EventSubscriberInterface {
// Check the header value.
$current_request = $event->getRequest();
$connecting_ip = $current_request->server->get($reverse_proxy_header_name);
- if ($this->isInvalidIpAddress($connecting_ip, $reverse_proxy_header_name)) {
+
+ // Split and extract the first IP address.
+ $connecting_ips = array_map('trim', explode(',', $connecting_ip));
+ if (count($connecting_ips) > 1) {
+ foreach ($connecting_ips as $ip) {
+ if ($this->isInvalidIpAddress($connecting_ip, $reverse_proxy_header_name)) {
+ continue;
+ }
+ $connecting_ip = $ip;
+ break;
+ }
+ }
+
+ if (empty($connecting_ip)) {
+ $this->logger->notice('No valid IP address found in the @header_name header.', ['@header_name' => $reverse_proxy_header_name]);
return;
}
--
GitLab
From 557aa95ab53a32654253697727c1848632b6f7d1 Mon Sep 17 00:00:00 2001
From: Bohdan Artemchuk <5767-bohart@users.noreply.drupalcode.org>
Date: Thu, 9 Jan 2025 22:04:01 +0200
Subject: [PATCH 2/4] Issue #3425451 by bohart: Added tests coverage.
---
tests/src/Kernel/ClientIpRestoreTest.php | 76 ++++++++++++-------
.../src/Kernel/ReverseProxyHeaderTestBase.php | 26 ++-----
2 files changed, 54 insertions(+), 48 deletions(-)
diff --git a/tests/src/Kernel/ClientIpRestoreTest.php b/tests/src/Kernel/ClientIpRestoreTest.php
index 9d40e2d..5fb9e4a 100644
--- a/tests/src/Kernel/ClientIpRestoreTest.php
+++ b/tests/src/Kernel/ClientIpRestoreTest.php
@@ -14,17 +14,17 @@ class ClientIpRestoreTest extends ReverseProxyHeaderTestBase {
*
* @param string $header_name
* The header name.
+ * @param string $header_value
+ * The header value.
* @param string $valid_ip
- * The valid client IP address.
- * @param string $invalid_ip
- * The invalid client IP address.
+ * The valid expected client IP address.
*
- * @dataProvider providerClientIpsHeaders
+ * @dataProvider providerClientValidIps
*/
public function testValidIps(
string $header_name,
+ string $header_value,
string $valid_ip,
- string $invalid_ip,
): void {
$request = $this->reverseProxyHeaderRequest($header_name, $valid_ip);
$this->assertEquals($request->getClientIp(), $valid_ip);
@@ -35,50 +35,70 @@ class ClientIpRestoreTest extends ReverseProxyHeaderTestBase {
*
* @param string $header_name
* The header name.
- * @param string $valid_ip
- * The valid client IP address.
- * @param string $invalid_ip
- * The invalid client IP address.
+ * @param string $header_value
+ * The header value.
*
- * @dataProvider providerClientIpsHeaders
+ * @dataProvider providerClientInvalidIps
*/
public function testInvalidIps(
string $header_name,
- string $valid_ip,
- string $invalid_ip,
+ string $header_value,
): void {
- $this->reverseProxyHeaderRequest($header_name, $invalid_ip);
- $this->assertInvalidIpNoticeExist($invalid_ip, $header_name);
+ $this->reverseProxyHeaderRequest($header_name, $header_value);
+ $this->assertInvalidIpNoticeExist($header_name);
}
/**
- * Provider for testing ClientIpRestoreTest.
+ * Provider for testing ClientIpRestoreTest::testValidIps.
*
* @return array
* Test Data to simulate incoming IP address.
*/
- public static function providerClientIpsHeaders(): array {
+ public static function providerClientValidIps(): array {
return [
[
// It should work well with default headers as well.
- 'HEADER_X_FORWARDED_FOR',
- '127.0.0.1',
- '',
+ 'header_name' => 'HEADER_X_FORWARDED_FOR',
+ 'header_value' => '127.0.0.1',
+ 'valid_ip' => '127.0.0.1',
],
[
- 'CUSTOM_HEADER_2',
- '10.0.10.10',
- '',
+ 'header_name' => 'HEADER_X_CUSTOM_HEADER_NAME',
+ 'header_value' => '8.8.8.8',
+ 'valid_ip' => '8.8.8.8',
+ ],
+ [
+ 'header_name' => 'HEADER_WITH_ONE_INVALID_IP',
+ 'header_value' => '127.0.0.1,not-an-ip,127.0.0.2',
+ 'valid_ip' => '127.0.0.1',
+ ],
+ [
+ 'header_name' => 'HEADER_WITH_INVALID_IP_FIRST',
+ 'header_value' => 'not-an-ip,10.0.10.10',
+ 'valid_ip' => '10.0.10.10',
+ ],
+ ];
+ }
+
+ /**
+ * Provider for testing ClientIpRestoreTest::testInvalidIps.
+ *
+ * @return array
+ * Test Data to simulate incoming IP address.
+ */
+ public static function providerClientInvalidIps(): array {
+ return [
+ [
+ 'header_name' => 'HEADER_X_FORWARDED_FOR',
+ 'header_value' => '444.555.666.777',
],
[
- 'HEADER_CUSTOM_HEADER_3',
- '1.1.1.1',
- '444.555.666.777',
+ 'header_name' => 'HEADER_X_CUSTOM_HEADER_2',
+ 'header_value' => 'this_is_a_text_not_ip',
],
[
- 'HEADER_X_CUSTOM_HEADER_4',
- '8.8.8.8',
- 'this_is_a_text_not_ip',
+ 'header_name' => 'HEADER_WITH_ALL_INVALID_IPS',
+ 'header_value' => 'some-value,444.555.666.777',
],
];
}
diff --git a/tests/src/Kernel/ReverseProxyHeaderTestBase.php b/tests/src/Kernel/ReverseProxyHeaderTestBase.php
index 47036f1..3e61126 100644
--- a/tests/src/Kernel/ReverseProxyHeaderTestBase.php
+++ b/tests/src/Kernel/ReverseProxyHeaderTestBase.php
@@ -77,30 +77,16 @@ abstract class ReverseProxyHeaderTestBase extends KernelTestBase {
}
/**
- * Check if exist notice for current IP address.
+ * Checks if the notice exist for current invalid IP address.
*
- * @param string $ip_address
- * IP address.
* @param string $header_name
* The header name.
*/
- protected function assertInvalidIpNoticeExist(
- string $ip_address,
- string $header_name,
- ): void {
- // Prepare the message and its arguments for testing.
- if (empty($ip_address)) {
- $message = 'Empty IP address value retrieved from @header_name header is invalid.';
- }
- else {
- $message = 'IP address `@ip_address` retrieved from @header_name header is invalid.';
- }
- $arguments = [
- '@ip_address' => $ip_address,
- '@header_name' => $header_name,
- ];
- $parser = new LogMessageParser();
- $variables = $parser->parseMessagePlaceholders($message, $arguments);
+ protected function assertInvalidIpNoticeExist(string $header_name): void {
+ // Prepares the message and its arguments for testing.
+ $message = 'No valid IP address found in the @header_name header';
+ $arguments = ['@header_name' => $header_name];
+ $variables = (new LogMessageParser())->parseMessagePlaceholders($message, $arguments);
// Select the expected notice from the database.
$query = Database::getConnection()->select('watchdog');
--
GitLab
From aa2ae20060b59a6c95c36de96f35f79c5c96bb2d Mon Sep 17 00:00:00 2001
From: Bohdan Artemchuk <5767-bohart@users.noreply.drupalcode.org>
Date: Thu, 9 Jan 2025 22:21:23 +0200
Subject: [PATCH 3/4] Issue #3425451 by bohart: Added tests coverage.
---
.../ReverseProxyHeaderClientIpRestore.php | 40 +++++++++----------
tests/src/Kernel/ClientIpRestoreTest.php | 2 +-
2 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php b/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php
index 9d8e0cd..8d38c3e 100644
--- a/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php
+++ b/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php
@@ -80,32 +80,28 @@ class ReverseProxyHeaderClientIpRestore implements EventSubscriberInterface {
}
// Checks the header value.
- $current_request = $event->getRequest();
- $connecting_ip = $current_request->server->get($reverse_proxy_header_name);
+ $connecting_ips = $event->getRequest()->server->get($reverse_proxy_header_name);
+ if (empty($connecting_ips)) {
+ $this->logger->notice('Empty value retrieved from @header_name header.', ['@header_name' => $reverse_proxy_header_name]);
+ return;
+ }
// Extracts the first valid IP address from the header.
- $connecting_ips = array_map('trim', explode(',', $connecting_ip));
- if (count($connecting_ips) > 1) {
- foreach ($connecting_ips as $ip) {
- if ($this->isInvalidIpAddress($connecting_ip, $reverse_proxy_header_name)) {
- continue;
- }
- $connecting_ip = $ip;
- break;
+ $connecting_ips = array_map('trim', explode(',', $connecting_ips));
+ foreach ($connecting_ips as $connecting_ip) {
+ if (!$this->isInvalidIpAddress($connecting_ip, $reverse_proxy_header_name)) {
+
+ // As the changed remote address will make it impossible to determine
+ // a trusted proxy, we need to make sure we set the right protocol as well.
+ // @see \Symfony\Component\HttpFoundation\Request::isSecure()
+ $event->getRequest()->server->set('HTTPS', $event->getRequest()->isSecure() ? 'on' : 'off');
+ $event->getRequest()->server->set('REMOTE_ADDR', $connecting_ip);
+ $event->getRequest()->overrideGlobals();
+ return;
}
}
- if (empty($connecting_ip)) {
- $this->logger->notice('No valid IP address found in the @header_name header.', ['@header_name' => $reverse_proxy_header_name]);
- return;
- }
-
- // As the changed remote address will make it impossible to determine
- // a trusted proxy, we need to make sure we set the right protocol as well.
- // @see \Symfony\Component\HttpFoundation\Request::isSecure()
- $event->getRequest()->server->set('HTTPS', $event->getRequest()->isSecure() ? 'on' : 'off');
- $event->getRequest()->server->set('REMOTE_ADDR', $connecting_ip);
- $event->getRequest()->overrideGlobals();
+ $this->logger->notice('No valid IP address found in the @header_name header.', ['@header_name' => $reverse_proxy_header_name]);
}
/**
@@ -129,7 +125,7 @@ class ReverseProxyHeaderClientIpRestore implements EventSubscriberInterface {
];
if (empty($ip_address)) {
- $this->logger->notice('Empty IP address value retrieved from @header_name header is invalid.', $variables);
+ $this->logger->notice('Empty IP address value retrieved from @header_name header.', $variables);
return TRUE;
}
diff --git a/tests/src/Kernel/ClientIpRestoreTest.php b/tests/src/Kernel/ClientIpRestoreTest.php
index 5fb9e4a..891bf83 100644
--- a/tests/src/Kernel/ClientIpRestoreTest.php
+++ b/tests/src/Kernel/ClientIpRestoreTest.php
@@ -26,7 +26,7 @@ class ClientIpRestoreTest extends ReverseProxyHeaderTestBase {
string $header_value,
string $valid_ip,
): void {
- $request = $this->reverseProxyHeaderRequest($header_name, $valid_ip);
+ $request = $this->reverseProxyHeaderRequest($header_name, $header_value);
$this->assertEquals($request->getClientIp(), $valid_ip);
}
--
GitLab
From 566b4b8d0c263757608ec43e48550df7cec62e27 Mon Sep 17 00:00:00 2001
From: Bohdan Artemchuk <5767-bohart@users.noreply.drupalcode.org>
Date: Thu, 9 Jan 2025 22:26:07 +0200
Subject: [PATCH 4/4] Issue #3425451 by bohart: Added tests coverage.
---
src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php | 2 +-
tests/src/Kernel/ReverseProxyHeaderTestBase.php | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php b/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php
index 8d38c3e..736ffa6 100644
--- a/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php
+++ b/src/EventSubscriber/ReverseProxyHeaderClientIpRestore.php
@@ -92,7 +92,7 @@ class ReverseProxyHeaderClientIpRestore implements EventSubscriberInterface {
if (!$this->isInvalidIpAddress($connecting_ip, $reverse_proxy_header_name)) {
// As the changed remote address will make it impossible to determine
- // a trusted proxy, we need to make sure we set the right protocol as well.
+ // a trusted proxy, we need to make sure we set the right protocol.
// @see \Symfony\Component\HttpFoundation\Request::isSecure()
$event->getRequest()->server->set('HTTPS', $event->getRequest()->isSecure() ? 'on' : 'off');
$event->getRequest()->server->set('REMOTE_ADDR', $connecting_ip);
diff --git a/tests/src/Kernel/ReverseProxyHeaderTestBase.php b/tests/src/Kernel/ReverseProxyHeaderTestBase.php
index 3e61126..609a29b 100644
--- a/tests/src/Kernel/ReverseProxyHeaderTestBase.php
+++ b/tests/src/Kernel/ReverseProxyHeaderTestBase.php
@@ -84,7 +84,7 @@ abstract class ReverseProxyHeaderTestBase extends KernelTestBase {
*/
protected function assertInvalidIpNoticeExist(string $header_name): void {
// Prepares the message and its arguments for testing.
- $message = 'No valid IP address found in the @header_name header';
+ $message = 'No valid IP address found in the @header_name header.';
$arguments = ['@header_name' => $header_name];
$variables = (new LogMessageParser())->parseMessagePlaceholders($message, $arguments);
--
GitLab