Merge pull request #79 from julienloizelet/feature/78-fix-session-and-redirect-issues

julienloizelet · web-flow · commit 3d2156cb7234 · 2022-03-24T14:38:20.000+09:00
Feature/78 fix session and redirect issues
diff --git a/.github/workflows/test-suite.yml b/.github/workflows/test-suite.yml
@@ -1,10 +1,5 @@
 name: Test suite
 on:
-  push:
-    branches:
-      - main
-    paths-ignore:
-      - '**.md'
   pull_request:
     branches:
       - main
@@ -33,7 +28,7 @@ jobs:
 
       - name: Install DDEV
         env:
-          DDEV_VERSION: v1.18.2
+          DDEV_VERSION: v1.19.1
         run: |
           # @see https://ddev.readthedocs.io/en/stable/#installationupgrade-script-linux-and-macos-armarm64-and-amd64-architectures
           sudo apt-get -qq update
diff --git a/.gitignore b/.gitignore
@@ -9,6 +9,7 @@ vendor
 super-linter.log
 .php-cs-fixer.cache
 .php-cs-fixer.php
+tools/php-cs-fixer/composer.lock
 
 # App
 /var/
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
 
+## [0.19.0] - 2022-03-24
+
+### Added
+
+- Add `excluded_uris` configuration to exclude some uris (was hardcoded to `/favicon.ico`)
+
+### Changed
+- Change the redirection after captcha resolution to `/` (was `$_SERVER['REQUEST_URI']'`)
+
+### Fixed
+- Fix Standalone bouncer session handling
+
+
 ## [0.18.0] - 2022-03-18
 ### Changed
 - *Breaking change*: Change `trust_ip_forward_array` symfony configuration node to an array of array.
diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md
@@ -145,6 +145,8 @@ Here is the list of available settings:
 
 - `trust_ip_forward_array`:  If you use a CDN, a reverse proxy or a load balancer, set an array of IPs. For other IPs, the bouncer will not trust the X-Forwarded-For header.
 
+- `excluded_uris`: array of URIs that will not be bounced
+
 ##### Cache
 
 - `cache_system`: Select from `phpfs` (File system cache), `redis` or `memcached`.
@@ -156,9 +158,9 @@ Here is the list of available settings:
 
 - `memcached_dsn`: Will be used only if you choose Memcached as cache_system
 
-- `cache_expiration_for_clean_ip`: Set the duration we keep in cache the fact that an IP is clean. In seconds. Defaults to 5.
+- `clean_ip_cache_duration`: Set the duration we keep in cache the fact that an IP is clean. In seconds. Defaults to 5.
 
-- `cache_expiration_for_bad_ip`: Set the duration we keep in cache the fact that an IP is bad. In seconds. Defaults to 20.
+- `bad_ip_cache_duration`: Set the duration we keep in cache the fact that an IP is bad. In seconds. Defaults to 20.
 
 - `stream_mode`: true to enable stream mode, false to enable the live mode. Default to false. By default, the `live mode` is enabled. The first time a stranger connects to your website, this mode means that the IP will be checked directly by the CrowdSec API. The rest of your user’s browsing will be even more transparent thanks to the fully customizable cache system. But you can also activate the `stream mode`. This mode allows you to constantly feed the bouncer with the malicious IP list via a background task (CRON), making it to be even faster when checking the IP of your visitors. Besides, if your site has a lot of unique visitors at the same time, this will not influence the traffic to the API of your CrowdSec instance.
 
@@ -181,25 +183,45 @@ Here is the list of available settings:
 - Wording and css settings: 
 
   `theme_color_text_primary`
+
   `theme_color_text_secondary`
+
   `theme_color_text_button`
+
   `theme_color_text_error_message`
+
   `theme_color_background_page`
+
   `theme_color_background_container`
+
   `theme_color_background_button`
+
   `theme_color_background_button_hover`
+
   `theme_custom_css`
+
   `theme_text_captcha_wall_tab_title`
+
   `theme_text_captcha_wall_title`
+
   `theme_text_captcha_wall_subtitle`
+
   `theme_text_captcha_wall_refresh_image_link`
+
   `theme_text_captcha_wall_captcha_placeholder`
+
   `theme_text_captcha_wall_send_button`
+
   `theme_text_captcha_wall_error_message`
+
   `theme_text_captcha_wall_footer`
+
   `theme_text_ban_wall_tab_title`
+
   `theme_text_ban_wall_title`
+
   `theme_text_ban_wall_subtitle`
+
   `theme_text_ban_wall_footer`
 
 
diff --git a/scripts/auto-prepend/bounce.php b/scripts/auto-prepend/bounce.php
@@ -8,7 +8,9 @@
 
 use CrowdSecBouncer\StandaloneBounce;
 
+
+
 $bounce = new StandaloneBounce();
 
 /** @var $crowdSecStandaloneBouncerConfig */
-$bounce->safelyBounce($crowdSecStandaloneBouncerConfig);
+$bounce->safelyBounce($crowdSecStandaloneBouncerConfig);
diff --git a/scripts/auto-prepend/settings.example.php b/scripts/auto-prepend/settings.example.php
@@ -7,7 +7,7 @@
      *
      * Key generated by the cscli (CrowdSec cli) command like "cscli bouncers add bouncer-php-library"
      */
-    'api_key'=> 'YOUR_BOUNCER_API_KEY',
+    'api_key' => 'YOUR_BOUNCER_API_KEY',
 
     /** Define the URL to your LAPI server, default to http://localhost:8080.
      *
@@ -37,7 +37,7 @@
      *
      * If not empty, it will be used for all remediation and geolocation processes.
      */
-    'forced_test_ip' => '1.2.3.4',
+    'forced_test_ip' => '',
 
     /** Select from 'bouncing_disabled', 'normal_bouncing' or 'flex_bouncing'.
      *
@@ -68,6 +68,11 @@
      */
     'trust_ip_forward_array' => [],
 
+    /**
+     * array of URIs that will not be bounced
+     */
+    'excluded_uris' => ['/favicon.ico'],
+
     // Select from 'phpfs' (File system cache), 'redis' or 'memcached'.
     'cache_system' => Constants::CACHE_SYSTEM_PHPFS,
 
@@ -84,10 +89,10 @@
     'memcached_dsn' => 'memcached://localhost:11211',
 
     // Set the duration we keep in cache the fact that an IP is clean. In seconds. Defaults to 5.
-    'cache_expiration_for_clean_ip'=> Constants::CACHE_EXPIRATION_FOR_CLEAN_IP,
+    'clean_ip_cache_duration'=> Constants::CACHE_EXPIRATION_FOR_CLEAN_IP,
 
     // Optional. Set the duration we keep in cache the fact that an IP is bad. In seconds. Defaults to 20.
-    'cache_expiration_for_bad_ip'=> Constants::CACHE_EXPIRATION_FOR_BAD_IP,
+    'bad_ip_cache_duration'=> Constants::CACHE_EXPIRATION_FOR_BAD_IP,
 
     /** true to enable stream mode, false to enable the live mode. Default to false.
      *
diff --git a/src/AbstractBounce.php b/src/AbstractBounce.php
@@ -266,13 +266,12 @@ protected function handleCaptchaRemediation($ip)
 
         if (null === $this->getSessionVariable('crowdsec_captcha_has_to_be_resolved')) {
             // Set up the first captcha remediation.
-
             $this->storeNewCaptchaCoupleInSession();
             $this->setSessionVariable('crowdsec_captcha_has_to_be_resolved', true);
             $this->setSessionVariable('crowdsec_captcha_resolution_failed', false);
             $this->setSessionVariable('crowdsec_captcha_resolution_redirect', 'POST' === $this->getHttpMethod() &&
                                                                               !empty($_SERVER['HTTP_REFERER']) ?
-                $_SERVER['HTTP_REFERER'] : $_SERVER['REQUEST_URI']);
+                $_SERVER['HTTP_REFERER'] : '/');
         }
 
         // Display captcha page if this is required.
diff --git a/src/Bouncer.php b/src/Bouncer.php
@@ -74,8 +74,8 @@ public function configure(array $config): void
             $finalConfig['api_timeout'],
             $finalConfig['api_user_agent'],
             $finalConfig['api_key'],
-            $finalConfig['cache_expiration_for_clean_ip'],
-            $finalConfig['cache_expiration_for_bad_ip'],
+            $finalConfig['clean_ip_cache_duration'],
+            $finalConfig['bad_ip_cache_duration'],
             $finalConfig['fallback_remediation'],
             $finalConfig['geolocation']
         );
diff --git a/src/Configuration.php b/src/Configuration.php
@@ -56,6 +56,9 @@ public function getConfigTreeBuilder(): TreeBuilder
                         ->scalarPrototype()->end()
                     ->end()
                 ->end()
+                ->arrayNode('excluded_uris')
+                    ->scalarPrototype()->end()
+                ->end()
                 // Cache
                 ->booleanNode('stream_mode')->defaultValue(false)->end()
                 ->enumNode('cache_system')
@@ -65,10 +68,10 @@ public function getConfigTreeBuilder(): TreeBuilder
                 ->scalarNode('fs_cache_path')->end()
                 ->scalarNode('redis_dsn')->end()
                 ->scalarNode('memcached_dsn')->end()
-                ->integerNode('cache_expiration_for_clean_ip')
+                ->integerNode('clean_ip_cache_duration')
                     ->defaultValue(Constants::CACHE_EXPIRATION_FOR_CLEAN_IP)
                 ->end()
-                ->integerNode('cache_expiration_for_bad_ip')
+                ->integerNode('bad_ip_cache_duration')
                     ->defaultValue(Constants::CACHE_EXPIRATION_FOR_BAD_IP)
                 ->end()
                 // Geolocation
diff --git a/src/Constants.php b/src/Constants.php
@@ -20,7 +20,7 @@ class Constants
     public const DEFAULT_LAPI_URL = 'http://localhost:8080';
 
     /** @var string The last version of this library */
-    public const VERSION = 'v0.18.0';
+    public const VERSION = 'v0.19.0';
 
     /** @var string The user agent used to send request to LAPI */
     public const BASE_USER_AGENT = 'PHP CrowdSec Bouncer/'.self::VERSION;
diff --git a/src/RestClient.php b/src/RestClient.php
@@ -99,7 +99,7 @@ public function request(
             'method' => $method,
             'uri' => $this->baseUri.$endpoint,
             'content' => 'POST' === $method ? $config['http']['content'] : null,
-            //'header' => $header, # Do not display header to avoid logging sensible data
+            // 'header' => $header, # Do not display header to avoid logging sensible data
         ]);
 
         $response = file_get_contents($this->baseUri.$endpoint, false, $context);
diff --git a/src/StandaloneBounce.php b/src/StandaloneBounce.php
@@ -30,7 +30,7 @@ class StandaloneBounce extends AbstractBounce implements IBounce
     /**
      * @var string
      */
-    protected $session_name;
+    const SESSION_NAME = 'crowdsec';
 
     /**
      * Initialize the bouncer.
@@ -41,10 +41,6 @@ class StandaloneBounce extends AbstractBounce implements IBounce
      */
     public function init(array $configs, array $forcedConfigs = []): Bouncer
     {
-        if (\PHP_SESSION_NONE === session_status()) {
-            $this->session_name = session_name('crowdsec');
-            session_start();
-        }
         // Convert array of string to array of array with comparable IPs
         if (\is_array(($configs['trust_ip_forward_array']))) {
             $forwardConfigs = $configs['trust_ip_forward_array'];
@@ -182,8 +178,8 @@ public function getBouncerInstance(array $settings, bool $forceReload = false):
             'fs_cache_path' => $this->getStringSettings('fs_cache_path'),
             'redis_dsn' => $this->getStringSettings('redis_dsn'),
             'memcached_dsn' => $this->getStringSettings('memcached_dsn'),
-            'cache_expiration_for_clean_ip' => $this->getIntegerSettings('clean_ip_cache_duration'),
-            'cache_expiration_for_bad_ip' => $this->getIntegerSettings('bad_ip_cache_duration'),
+            'clean_ip_cache_duration' => $this->getIntegerSettings('clean_ip_cache_duration'),
+            'bad_ip_cache_duration' => $this->getIntegerSettings('bad_ip_cache_duration'),
             // Geolocation
             'geolocation' => $this->getArraySettings('geolocation'),
         ]);
@@ -345,11 +341,15 @@ public function getPostedVariable(string $name): ?string
      */
     public function shouldBounceCurrentIp(): bool
     {
-        // Don't bounce favicon calls
-        if ('/favicon.ico' === $_SERVER['REQUEST_URI']) {
+        $excludedURIs = $this->getArraySettings('excluded_uris');
+        if (\in_array($_SERVER['REQUEST_URI'], $excludedURIs)) {
+            $this->logger->debug('', [
+                'type' => 'SHOULD_NOT_BOUNCE',
+                'message' => 'This URI is excluded from bouncing: '.$_SERVER['REQUEST_URI'],
+            ]);
+
             return false;
         }
-
         $bouncingDisabled = (Constants::BOUNCING_LEVEL_DISABLED === $this->getStringSettings('bouncing_level'));
         if ($bouncingDisabled) {
             $this->logger->debug('', [
@@ -407,6 +407,10 @@ public function safelyBounce(array $configs): bool
             throw new BouncerException("$errstr (Error level: $errno)");
         });
         try {
+            if (\PHP_SESSION_NONE === session_status()) {
+                session_name(self::SESSION_NAME);
+                session_start();
+            }
             $this->init($configs);
             $this->run();
             $result = true;
@@ -421,11 +425,6 @@ public function safelyBounce(array $configs): bool
             if ($this->displayErrors) {
                 throw $e;
             }
-        } finally {
-            if (\PHP_SESSION_NONE !== session_status()) {
-                session_write_close();
-                session_name($this->session_name);
-            }
         }
         restore_error_handler();
 
diff --git a/tests/LoadPaginatedDecisionsTest.php b/tests/LoadPaginatedDecisionsTest.php
@@ -10,7 +10,7 @@ final class LoadPaginatedDecisionsTest extends TestCase
      */
     public function testCanLoad10FirstDecisions(): void
     {
-        //...(0, 10)
+        // ...(0, 10)
         $this->markTestIncomplete('This test has not been implemented yet.');
     }
 
@@ -19,7 +19,7 @@ public function testCanLoad10FirstDecisions(): void
      */
     public function testCanLoad10LastDecisions(): void
     {
-        //...(-10)
+        // ...(-10)
         $this->markTestIncomplete('This test has not been implemented yet.');
     }
 
@@ -28,7 +28,7 @@ public function testCanLoad10LastDecisions(): void
      */
     public function testCanLoad5FirstDecisions(): void
     {
-        //...(-10, 0)
+        // ...(-10, 0)
         $this->markTestIncomplete('This test has not been implemented yet.');
     }
 
@@ -37,7 +37,7 @@ public function testCanLoad5FirstDecisions(): void
      */
     public function testCanLoadAllDecisions(): void
     {
-        //...(0)
+        // ...(0)
         $this->markTestIncomplete('This test has not been implemented yet.');
     }
 
@@ -46,7 +46,7 @@ public function testCanLoadAllDecisions(): void
      */
     public function testCanLoadTheSecondDecision(): void
     {
-        //...(0, 2)
+        // ...(0, 2)
         $this->markTestIncomplete('This test has not been implemented yet.');
     }
 }
diff --git a/tests/LoadPaginatedLogsTest.php b/tests/LoadPaginatedLogsTest.php
@@ -10,7 +10,7 @@ final class LoadPaginatedLogsTest extends TestCase
      */
     public function testCanLoad10FirstLogInputs(): void
     {
-        //...(0, 10)
+        // ...(0, 10)
         $this->markTestIncomplete('This test has not been implemented yet.');
     }
 
@@ -19,7 +19,7 @@ public function testCanLoad10FirstLogInputs(): void
      */
     public function testCanLoad10LastLogInputs(): void
     {
-        //...(-10)
+        // ...(-10)
         $this->markTestIncomplete('This test has not been implemented yet.');
     }
 
@@ -28,7 +28,7 @@ public function testCanLoad10LastLogInputs(): void
      */
     public function testCanLoad5FirstLogInputs(): void
     {
-        //...(-10, 0)
+        // ...(-10, 0)
         $this->markTestIncomplete('This test has not been implemented yet.');
     }
 
@@ -37,7 +37,7 @@ public function testCanLoad5FirstLogInputs(): void
      */
     public function testCanLoadAllLogInputs(): void
     {
-        //...(0)
+        // ...(0)
         $this->markTestIncomplete('This test has not been implemented yet.');
     }
 
@@ -46,7 +46,7 @@ public function testCanLoadAllLogInputs(): void
      */
     public function testCanLoadTheSecondLogInput(): void
     {
-        //...(0, 2)
+        // ...(0, 2)
         $this->markTestIncomplete('This test has not been implemented yet.');
     }
 }
diff --git a/tools/php-cs-fixer/composer.lock b/tools/php-cs-fixer/composer.lock
