feat(auto-prepend): Move custom error handler logic to auto-prepend file

Author: julienloizelet

.github/workflows/test-suite.yml

@@ -164,10 +164,77 @@ jobs:
             exit 1
           fi
 
+      - name: Run "Display error with bad settings" test
+        run: |
+          cd ${{ github.workspace }}/${{env.EXTENSION_PATH}}
+          sed -i  's/\x27cache_system\x27 => Constants::CACHE_SYSTEM_PHPFS/\x27cache_system\x27 => 1/g' scripts/auto-prepend/settings.php
+          cat scripts/auto-prepend/settings.php
+          cd ${{ github.workspace }}/${{env.EXTENSION_PATH}}/tests/end-to-end/
+          ./__scripts__/run-tests.sh ci "./__tests__/6-display-error-on.js"
+          PENDING_TESTS=$(grep -oP '"numPendingTests":\K(.*),"numRuntimeErrorTestSuites"' .test-results.json | sed  's/,"numRuntimeErrorTestSuites"//g')
+          if [[ $PENDING_TESTS == "0" ]]
+          then
+            echo "No pending tests: OK"
+          else
+            echo "There are pending tests: $PENDING_TESTS (KO)"
+            exit 1
+          fi
+
+      - name: Run "No display error with bad settings" test
+        run: |
+          cd ${{ github.workspace }}/${{env.EXTENSION_PATH}}
+          sed -i  's/\x27cache_system\x27 => Constants::CACHE_SYSTEM_PHPFS/\x27cache_system\x27 => 1/g' scripts/auto-prepend/settings.php
+          sed -i  's/\x27display_errors\x27 => true/\x27display_errors\x27 => false/g' scripts/auto-prepend/settings.php
+          cat scripts/auto-prepend/settings.php
+          cd ${{ github.workspace }}/${{env.EXTENSION_PATH}}/tests/end-to-end/
+          ./__scripts__/run-tests.sh ci "./__tests__/5-display-error-off.js"
+          PENDING_TESTS=$(grep -oP '"numPendingTests":\K(.*),"numRuntimeErrorTestSuites"' .test-results.json | sed  's/,"numRuntimeErrorTestSuites"//g')
+          if [[ $PENDING_TESTS == "0" ]]
+          then
+            echo "No pending tests: OK"
+          else
+            echo "There are pending tests: $PENDING_TESTS (KO)"
+            exit 1
+          fi
+
+      - name: Run "No display error with error while bouncing" test
+        run: |
+          cd ${{ github.workspace }}/${{env.EXTENSION_PATH}}
+          sed -i  's/\x27cache_system\x27 => 1/\x27cache_system\x27 => Constants::CACHE_SYSTEM_PHPFS/g' scripts/auto-prepend/settings.php
+          sed -i  's/\x27forced_test_ip\x27 => \x27\x27/\x27forced_test_ip\x27 => \x27bad-ip\x27/g' scripts/auto-prepend/settings.php
+          cat scripts/auto-prepend/settings.php
+          cd ${{ github.workspace }}/${{env.EXTENSION_PATH}}/tests/end-to-end/
+          ./__scripts__/run-tests.sh ci "./__tests__/5-display-error-off.js"
+          PENDING_TESTS=$(grep -oP '"numPendingTests":\K(.*),"numRuntimeErrorTestSuites"' .test-results.json | sed  's/,"numRuntimeErrorTestSuites"//g')
+          if [[ $PENDING_TESTS == "0" ]]
+          then
+            echo "No pending tests: OK"
+          else
+            echo "There are pending tests: $PENDING_TESTS (KO)"
+            exit 1
+          fi
+
+      - name: Run "Display error with error while bouncing" test
+        run: |
+          cd ${{ github.workspace }}/${{env.EXTENSION_PATH}}
+          sed -i  's/\x27display_errors\x27 => false/\x27display_errors\x27 => true/g' scripts/auto-prepend/settings.php
+          cat scripts/auto-prepend/settings.php
+          cd ${{ github.workspace }}/${{env.EXTENSION_PATH}}/tests/end-to-end/
+          ./__scripts__/run-tests.sh ci "./__tests__/6-display-error-on.js"
+          PENDING_TESTS=$(grep -oP '"numPendingTests":\K(.*),"numRuntimeErrorTestSuites"' .test-results.json | sed  's/,"numRuntimeErrorTestSuites"//g')
+          if [[ $PENDING_TESTS == "0" ]]
+          then
+            echo "No pending tests: OK"
+          else
+            echo "There are pending tests: $PENDING_TESTS (KO)"
+            exit 1
+          fi      
+
       - name: Run "live mode with cURL and without geolocation" test
         run: |
           cd ${{ github.workspace }}/${{env.EXTENSION_PATH}}
           sed -i  's/\x27use_curl\x27 => false/\x27use_curl\x27 => true/g' scripts/auto-prepend/settings.php
+          sed -i  's/\x27forced_test_ip\x27 => \x27bad-ip\x27/\x27forced_test_ip\x27 => \x27\x27/g' scripts/auto-prepend/settings.php
           cat scripts/auto-prepend/settings.php
           cd ${{ github.workspace }}/${{env.EXTENSION_PATH}}/tests/end-to-end/
           ./__scripts__/run-tests.sh ci "./__tests__/1-live-mode.js"    
@@ -332,7 +399,6 @@ jobs:
             echo "There are pending tests: $PENDING_TESTS (KO)"
             exit 1
           fi
-          
 
       - name: Run "stream mode with TLS auth and cURL and Memcached" test
         run: |

docs/USER_GUIDE.md

@@ -192,7 +192,7 @@ class MyCustomBouncer extends AbstractBouncer
 
 
 Once you have implemented these methods, you could retrieve all required configurations to instantiate your 
-bouncer and then call the `safelyBounce` method to apply a bounce for the current detected IP.
+bouncer and then call the `run` method to apply a bounce for the current detected IP.
 
 In order to instantiate the bouncer, you will have to create at least a `CrowdSec\RemediationEngine\LapiRemediation` 
 object too. 
@@ -211,7 +211,7 @@ $remediationEngine = new LapiRemediation($configs, $client, $cacheStorage);
 
 $bouncer = new MyCustomBouncer($configs, $remediationEngine);
 
-$bouncer->safelyBounce();
+$bouncer->run();
 
 ```
 

scripts/auto-prepend/bounce.php

@@ -9,7 +9,20 @@
 require_once __DIR__ . '/../../vendor/autoload.php';
 require_once __DIR__ . '/settings.php';
 
+use CrowdSecBouncer\BouncerException;
 use CrowdSecBouncer\StandaloneBouncer;
 
-$bouncer = new StandaloneBouncer($crowdSecStandaloneBouncerConfig);
-$bouncer->safelyBounce();
+// If there is any technical problem while bouncing, don't block the user.
+set_error_handler(function ($errno, $errstr) {
+    throw new BouncerException("$errstr (Error level: $errno)");
+});
+try {
+    $bouncer = new StandaloneBouncer($crowdSecStandaloneBouncerConfig);
+    $bouncer->run();
+} catch (\Throwable $e) {
+    $displayErrors = $crowdSecStandaloneBouncerConfig['display_errors'] ?? false;
+    if (true === $displayErrors) {
+        throw $e;
+    }
+}
+restore_error_handler();

scripts/auto-prepend/settings.example.php

@@ -17,7 +17,7 @@
      */
     'bouncing_level' => Constants::BOUNCING_LEVEL_NORMAL,
 
-    /** If you use a CDN, a reverse proxy or a load balancer, you can use this setting to whitelist their IPs
+    /** If you use a CDN, a reverse proxy or a load balancer, you can use this setting to whitelist their IPs.
      *
      * For other IPs, the bouncer will not trust the X-Forwarded-For header.
      *
@@ -26,7 +26,6 @@
      * [['001.002.003.004', '001.002.003.004'], ['005.006.007.008', '005.006.006.007']]
      *
      * If you use your own bouncer, you should have to set directly an array of comparable IPs arrays
-     *
      */
     'trust_ip_forward_array' => [],
     /**

src/AbstractBouncer.php

@@ -202,20 +202,17 @@ public function refreshBlocklistCache(): array
     }
 
     /**
-     * If there is any technical problem while bouncing, don't block the user. Bypass bouncing and log the error.
+     * Handle a bounce for current IP
      *
      * @return bool
      * @throws BouncerException
      * @throws CacheException
      * @throws InvalidArgumentException
      * @throws \Symfony\Component\Cache\Exception\InvalidArgumentException
      */
-    public function safelyBounce(): bool
+    public function run(): bool
     {
         $result = false;
-        set_error_handler(function ($errno, $errstr) {
-            throw new BouncerException("$errstr (Error level: $errno)");
-        });
         try {
             if ($this->shouldBounceCurrentIp()) {
                 $this->bounceCurrentIp();
@@ -230,10 +227,9 @@ public function safelyBounce(): bool
                 'line' => $e->getLine(),
             ]);
             if (true === $this->getConfig('display_errors')) {
-                throw $e;
+                throw new BouncerException($e->getMessage());
             }
         }
-        restore_error_handler();
 
         return $result;
     }

tests/Integration/IpVerificationTest.php

@@ -46,7 +46,7 @@
  * @covers \CrowdSecBouncer\AbstractBouncer::shouldTrustXforwardedFor
  * @covers \CrowdSecBouncer\StandaloneBouncer::getHttpRequestHeader
  * @covers \CrowdSecBouncer\StandaloneBouncer::getRemoteIp
- * @covers \CrowdSecBouncer\StandaloneBouncer::safelyBounce
+ * @covers \CrowdSecBouncer\StandaloneBouncer::run
  * @covers \CrowdSecBouncer\StandaloneBouncer::shouldBounceCurrentIp
  * @covers \CrowdSecBouncer\StandaloneBouncer::getHttpMethod
  * @covers \CrowdSecBouncer\StandaloneBouncer::getPostedVariable
@@ -187,6 +187,7 @@ private function addTlsConfig(&$bouncerConfigs, $tlsPath)
 
     public function testConstructAndSomeMethods()
     {
+        unset($_SERVER['REMOTE_ADDR'] );
         $bouncer = new StandaloneBouncer(array_merge($this->configs, ['unexpected_config' => 'test']));
         $this->assertEquals('', $bouncer->getRemoteIp(), 'Should return empty string');
         $_SERVER['REMOTE_ADDR'] = '5.6.7.8';
@@ -730,7 +731,7 @@ public function testCaptchaFlow()
         );
     }
 
-    public function testSafelyBounce()
+    public function testRun()
     {
         $this->assertEquals(
             false,
@@ -749,7 +750,7 @@ public function testSafelyBounce()
         // Test 2: not bouncing exclude URI
         $_SERVER['REMOTE_ADDR'] = '127.0.0.2';
         $_SERVER['REQUEST_URI'] = self::EXCLUDED_URI;
-        $this->assertEquals(false, $bouncer->safelyBounce(), 'Should not bounce excluded uri');
+        $this->assertEquals(false, $bouncer->run(), 'Should not bounce excluded uri');
         PHPUnitUtil::assertRegExp(
             $this,
             '/.*100.*Will not bounce as URI is excluded/',
@@ -760,12 +761,12 @@ public function testSafelyBounce()
         // Test 3: bouncing URI
         $_SERVER['REMOTE_ADDR'] = '127.0.0.3';
         $_SERVER['REQUEST_URI'] = '/home';
-        $this->assertEquals(true, $bouncer->safelyBounce(), 'Should bounce uri');
+        $this->assertEquals(true, $bouncer->run(), 'Should bounce uri');
         // Test 4:  not bouncing URI if disabled
         $_SERVER['REMOTE_ADDR'] = '127.0.0.4';
         $bouncer = new StandaloneBouncer(array_merge($this->configs, ['bouncing_level' =>
             Constants::BOUNCING_LEVEL_DISABLED]), $this->logger);
-        $this->assertEquals(false, $bouncer->safelyBounce(), 'Should not bounce if disabled');
+        $this->assertEquals(false, $bouncer->run(), 'Should not bounce if disabled');
 
         PHPUnitUtil::assertRegExp(
             $this,
@@ -789,7 +790,7 @@ public function testSafelyBounce()
         $error = '';
 
         try {
-            $bouncer->safelyBounce();
+            $bouncer->run();
         } catch (BouncerException $e) {
             $error = $e->getMessage();
         }
@@ -822,7 +823,7 @@ public function testSafelyBounce()
         $error = '';
 
         try {
-            $bouncer->safelyBounce();
+            $bouncer->run();
         } catch (BouncerException $e) {
             $error = $e->getMessage();
         }
@@ -845,7 +846,7 @@ public function testSafelyBounce()
             ),
             $this->logger
         );
-        $bouncer->safelyBounce();
+        $bouncer->run();
         PHPUnitUtil::assertRegExp(
             $this,
             '/.*100.*X-Forwarded-for usage is disabled/',
@@ -862,7 +863,7 @@ public function testSafelyBounce()
                 ]
             ), $this->logger
         );
-        $bouncer->safelyBounce();
+        $bouncer->run();
         PHPUnitUtil::assertRegExp(
             $this,
             '/.*100.*X-Forwarded-for usage is forced.*"x_forwarded_for_ip":"1.2.3.5"/',
@@ -877,7 +878,7 @@ public function testSafelyBounce()
                 $this->configs
             ), $this->logger
         );
-        $bouncer->safelyBounce();
+        $bouncer->run();
         PHPUnitUtil::assertRegExp(
             $this,
             '/.*300.*Detected IP is not allowed for X-Forwarded-for usage.*"x_forwarded_for_ip":"1.2.3.5"/',
@@ -892,7 +893,7 @@ public function testSafelyBounce()
                 $this->configs
             ), $this->logger
         );
-        $bouncer->safelyBounce();
+        $bouncer->run();
         PHPUnitUtil::assertRegExp(
             $this,
             '/.*100.*Detected IP is allowed for X-Forwarded-for usage.*"original_ip":"5.6.7.8","x_forwarded_for_ip":"127.0.0.10"/',

tests/Unit/StandaloneBouncerTest.php

@@ -52,7 +52,7 @@
  * @covers \CrowdSecBouncer\AbstractBouncer::shouldTrustXforwardedFor
  * @covers \CrowdSecBouncer\StandaloneBouncer::getHttpRequestHeader
  * @covers \CrowdSecBouncer\StandaloneBouncer::getRemoteIp
- * @covers \CrowdSecBouncer\StandaloneBouncer::safelyBounce
+ * @covers \CrowdSecBouncer\StandaloneBouncer::run
  * @covers \CrowdSecBouncer\StandaloneBouncer::shouldBounceCurrentIp
  *
  * @covers \CrowdSecBouncer\StandaloneBouncer::__construct

tests/end-to-end/__tests__/5-display-error-off.js

@@ -0,0 +1,9 @@
+/* eslint-disable no-undef */
+const { publicHomepageShouldBeAccessible } = require("../utils/helpers");
+
+describe(`Should not display errors`, () => {
+    it("Should not display error", async () => {
+        await publicHomepageShouldBeAccessible();
+        await expect(page).not.toHaveText("body", "Fatal error");
+    });
+});

tests/end-to-end/__tests__/6-display-error-on.js

@@ -0,0 +1,9 @@
+/* eslint-disable no-undef */
+const { goToPublicPage } = require("../utils/helpers");
+
+describe(`Should not display errors`, () => {
+    it("Should display error (if settings ko or something wrong while bouncing)", async () => {
+        await goToPublicPage();
+        await expect(page).toHaveText("body", "Fatal error");
+    });
+});