feat(mock ip): Remove useless configuration

julienloizelet · julienloizelet · commit 77246edbbba2 · 2022-07-07T13:07:27.000+09:00
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md
@@ -137,10 +137,8 @@ Here is the list of available settings:
 - `forced_test_ip`: Only for test or debug purpose. Default to empty. If not empty, it will be used instead of the 
   real remote ip.
 
-- `forced_test_forwarded_ip`: Only for test or debug purpose. Default to empty. If not empty, it will be used instead of the real forwarded ip.
-
-- `forced_test_never_use_forwarded`:  Only for test or debug purpose. Default to false. Set to true if you never 
-  want to use the x-forwarded-for mechanism.
+- `forced_test_forwarded_ip`: Only for test or debug purpose. Default to empty. If not empty, it will be used 
+  instead of the real forwarded ip. If set to `no_forward`, the x-forwarded-for mechanism will not be used at all.
 
 ##### Bouncer behavior
 
diff --git a/scripts/auto-prepend/settings.example.php b/scripts/auto-prepend/settings.example.php
@@ -42,14 +42,10 @@
     /** Only for test or debug purpose. Default to empty.
      *
      * If not empty, it will be used instead of the real forwarded ip.
-     */
-    'forced_test_forwarded_ip' => '',
-
-    /** Only for test or debug purpose. Default to false.
+     * If set to "no_forward", the x-forwarded-for mechanism will not be used at all.
      *
-     * Set to true if you never want to use the x-forwarded-for mechanism.
      */
-    'forced_test_never_use_forwarded' => false,
+    'forced_test_forwarded_ip' => '',
 
     /** Select from 'bouncing_disabled', 'normal_bouncing' or 'flex_bouncing'.
      *
diff --git a/src/AbstractBounce.php b/src/AbstractBounce.php
@@ -12,6 +12,7 @@
 use Monolog\Formatter\LineFormatter;
 use Monolog\Handler\RotatingFileHandler;
 use Monolog\Logger;
+use Psr\Cache\CacheException;
 use Psr\Cache\InvalidArgumentException;
 use Psr\Log\LoggerInterface;
 
@@ -65,7 +66,7 @@ protected function getArraySettings(string $name): array
     /**
      * Run a bounce.
      *
-     * @throws Exception|InvalidArgumentException
+     * @throws Exception|InvalidArgumentException|CacheException
      */
     public function run(): void
     {
@@ -112,18 +113,55 @@ protected function initLoggerHelper(string $logDirectoryPath, string $loggerName
     }
 
     /**
-     * Decide if we use forward (default behavior) or if it depends on test settings
+     * Handle X-Forwarded-For HTTP header to retrieve the IP to bounce
      *
-     * @param $settings
-     * @return bool
+     * @param $ip
+     * @return false|mixed
      */
-    protected function shouldUseForward($settings)
+    protected function handleForwardedFor($ip)
     {
-        return empty($settings['forced_test_never_use_forwarded']);
+        if (empty($this->settings['forced_test_forwarded_ip'])) {
+            $XForwardedForHeader = $this->getHttpRequestHeader('X-Forwarded-For');
+            if (null !== $XForwardedForHeader) {
+                $ipList = array_map('trim', array_values(array_filter(explode(',', $XForwardedForHeader))));
+                $forwardedIp = end($ipList);
+                if ($this->shouldTrustXforwardedFor($ip)) {
+                    $ip = $forwardedIp;
+                } else {
+                    $this->logger->warning('', [
+                        'type' => 'NON_AUTHORIZED_X_FORWARDED_FOR_USAGE',
+                        'original_ip' => $ip,
+                        'x_forwarded_for_ip' => $forwardedIp,
+                    ]);
+                }
+            }
+        } else if ($this->settings['forced_test_forwarded_ip'] === Constants::X_FORWARDED_DISABLED) {
+            $this->logger->debug('', [
+                'type' => 'DISABLED_X_FORWARDED_FOR_USAGE',
+                'original_ip' => $ip,
+            ]);
+        } else {
+            $forwardedIp = $this->settings['forced_test_forwarded_ip'];
+            if ($this->shouldTrustXforwardedFor($ip)) {
+                $ip = $forwardedIp;
+            } else {
+                $this->logger->warning('', [
+                    'type' => 'NON_AUTHORIZED_TEST_X_FORWARDED_FOR_USAGE',
+                    'original_ip' => $ip,
+                    'x_forwarded_for_ip_for_test' => $forwardedIp,
+                ]);
+            }
+        }
+
+        return $ip;
     }
 
     /**
-     * @throws Exception|InvalidArgumentException
+     * Bounce process
+     *
+     * @return void
+     * @throws InvalidArgumentException|CacheException
+     * @throws Exception
      */
     protected function bounceCurrentIp(): void
     {
@@ -133,38 +171,13 @@ protected function bounceCurrentIp(): void
             }
             // Retrieve the current IP (even if it is a proxy IP) or a testing IP
             $ip = !empty($this->settings['forced_test_ip']) ? $this->settings['forced_test_ip'] : $this->getRemoteIp();
-            if ($this->shouldUseForward($this->settings)) {
-                // Retrieve the forwarded IP (testing one or real)
-                if (!empty($this->settings['forced_test_forwarded_ip'])) {
-                    $forwardedIp = $this->settings['forced_test_forwarded_ip'];
-                } elseif ($XForwardedForHeader = $this->getHttpRequestHeader('X-Forwarded-For')) {
-                    $ipList = array_map('trim', array_values(array_filter(explode(',', $XForwardedForHeader))));
-                    $forwardedIp = end($ipList);
-                }
-                if (isset($forwardedIp)) {
-                    if ($this->shouldTrustXforwardedFor($ip)) {
-                        $this->logger->debug('', [
-                            'type' => 'AUTHORIZED_X_FORWARDED_FOR_USAGE',
-                            'original_ip' => $ip,
-                        ]);
-                        $ip = $forwardedIp;
-                    } else {
-                        $this->logger->warning('', [
-                            'type' => 'NON_AUTHORIZED_X_FORWARDED_FOR_USAGE',
-                            'original_ip' => $ip,
-                            'x_forwarded_for_ip' => $forwardedIp ?? 'undefined',
-                        ]);
-                    }
-                } else {
-                    $this->logger->debug('', ['type' => 'X_FORWARDED_FOR_NOT_FOUND']);
-                }
-            }
+            $ip = $this->handleForwardedFor($ip);
             $remediation = $this->bouncer->getRemediationForIp($ip);
             $this->handleRemediation($remediation, $ip);
         } catch (Exception $e) {
             $this->logger->warning('', [
                 'type' => 'UNKNOWN_EXCEPTION_WHILE_BOUNCING',
-                'ip' => $ip,
+                'ip' => $ip ?? '',
                 'message' => $e->getMessage(),
                 'code' => $e->getCode(),
                 'file' => $e->getFile(),
@@ -197,6 +210,9 @@ protected function shouldTrustXforwardedFor(string $ip): bool
         return false;
     }
 
+    /**
+     * @throws InvalidArgumentException
+     */
     protected function displayCaptchaWall(string $ip): void
     {
         $options = $this->getCaptchaWallOptions();
@@ -222,7 +238,10 @@ protected function handleBanRemediation(): void
     }
 
     /**
+     * @param string $ip
      * @return void
+     * @throws InvalidArgumentException
+     * @throws CacheException
      */
     protected function handleCaptchaResolutionForm(string $ip)
     {
@@ -305,6 +324,8 @@ protected function handleCaptchaResolutionForm(string $ip)
      * @param string $ip
      *
      * @return void
+     * @throws InvalidArgumentException
+     * @throws CacheException
      */
     protected function handleCaptchaRemediation(string $ip)
     {
@@ -344,6 +365,8 @@ protected function handleCaptchaRemediation(string $ip)
      * @param string $remediation
      * @param string $ip
      * @return void
+     * @throws InvalidArgumentException
+     * @throws CacheException
      */
     protected function handleRemediation(string $remediation, string $ip)
     {
@@ -368,7 +391,7 @@ protected function handleRemediation(string $remediation, string $ip)
      * @return array
      * @throws InvalidArgumentException
      */
-    public function getIpVariables(string $cacheTag, array $names, string $ip)
+    public function getIpVariables(string $cacheTag, array $names, string $ip): array
     {
         if (!$this->bouncer) {
             throw new BouncerException('Bouncer must be instantiated to get cache data.');
@@ -386,7 +409,7 @@ public function getIpVariables(string $cacheTag, array $names, string $ip)
      * @param string $ip
      * @return void
      * @throws InvalidArgumentException
-     * @throws \Psr\Cache\CacheException
+     * @throws CacheException
      */
     public function setIpVariables(string $cacheTag, array $pairs, string $ip): void
     {
@@ -405,7 +428,7 @@ public function setIpVariables(string $cacheTag, array $pairs, string $ip): void
      * @param string $ip
      * @return void
      * @throws InvalidArgumentException
-     * @throws \Psr\Cache\CacheException
+     * @throws CacheException
      */
     public function unsetIpVariables(string $cacheTag, array $names, string $ip): void
     {
diff --git a/src/Configuration.php b/src/Configuration.php
@@ -36,7 +36,6 @@ public function getConfigTreeBuilder(): TreeBuilder
                 // Debug
                 ->scalarNode('forced_test_ip')->defaultValue('')->end()
                 ->scalarNode('forced_test_forwarded_ip')->defaultValue('')->end()
-                ->booleanNode('forced_test_never_use_forwarded')->defaultValue(false)->end()
                 ->booleanNode('debug_mode')->defaultValue(false)->end()
                 ->scalarNode('log_directory_path')->end()
                 ->booleanNode('display_errors')->defaultValue(false)->end()
diff --git a/src/Constants.php b/src/Constants.php
@@ -96,4 +96,7 @@ class Constants
 
     /** @var string The Maxmind "City" database type */
     public const MAXMIND_CITY = 'city';
+
+    /** @var string The "disabled" bouncing level */
+    public const X_FORWARDED_DISABLED = 'no_forward';
 }
diff --git a/src/StandaloneBounce.php b/src/StandaloneBounce.php
@@ -170,7 +170,6 @@ public function getBouncerInstance(array $settings, bool $forceReload = false):
             'log_directory_path' => $this->getStringSettings('log_directory_path'),
             'forced_test_ip' => $this->getStringSettings('forced_test_ip'),
             'forced_test_forwarded_ip' => $this->getStringSettings('forced_test_forwarded_ip'),
-            'forced_test_never_use_forwarded' => $this->getBoolSettings('forced_test_never_use_forwarded'),
             'display_errors' => $this->getBoolSettings('display_errors'),
             // Bouncer
             'bouncing_level' => $bouncingLevel,
diff --git a/tests/end-to-end/settings/base.php.dist b/tests/end-to-end/settings/base.php.dist
@@ -15,7 +15,6 @@ $crowdSecStandaloneBouncerConfig = [
     'fs_cache_path' => __DIR__.'/.cache',
     'forced_test_ip' => 'REPLACE_FORCED_IP',
     'forced_test_forwarded_ip' => 'REPLACE_FORCED_FORWARDED_IP',
-    'forced_test_never_use_forwarded' => false,
     // Bouncer
     'bouncing_level' => Constants::BOUNCING_LEVEL_NORMAL,
     'stream_mode' => false,

Original file line number	Diff line number	Diff line change
`@@ -42,14 +42,10 @@`
`42`	`42`	`/** Only for test or debug purpose. Default to empty.`
`43`	`43`	`*`
`44`	`44`	`* If not empty, it will be used instead of the real forwarded ip.`
`45`		`- */`
`46`		`- 'forced_test_forwarded_ip' => '',`
`47`		`-`
`48`		`- /** Only for test or debug purpose. Default to false.`
	`45`	`+ * If set to "no_forward", the x-forwarded-for mechanism will not be used at all.`
`49`	`46`	`*`
`50`		`- * Set to true if you never want to use the x-forwarded-for mechanism.`
`51`	`47`	`*/`
`52`		`- 'forced_test_never_use_forwarded' => false,`
	`48`	`+ 'forced_test_forwarded_ip' => '',`
`53`	`49`
`54`	`50`	`/** Select from 'bouncing_disabled', 'normal_bouncing' or 'flex_bouncing'.`
`55`	`51`	`*`
Original file line number	Diff line number	Diff line change
`@@ -96,4 +96,7 @@ class Constants`
`96`	`96`
`97`	`97`	`/** @var string The Maxmind "City" database type */`
`98`	`98`	`public const MAXMIND_CITY = 'city';`
	`99`	`+`
	`100`	`+ /** @var string The "disabled" bouncing level */`
	`101`	`+ public const X_FORWARDED_DISABLED = 'no_forward';`
`99`	`102`	`}`