Add an event on a successful match

Created on 23 September 2023, about 1 year ago
Updated 11 January 2024, 11 months ago

Problem/Motivation

An event would have a lot of interesting use cases and beside allowing other modules to act, it could also include a flag to skip the ban. For example, I'm thinking that instead of banning on a single bad request, it might make sense to use flood API to only do so after N bad requests. That could be a feature in this module if the maintainers are interested but if not, it could easily be implemented in an event subscriber.

Other use cases are delegating the IP ban to infrastructure/CDN, distributing it across a site and more.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

✨ Feature request
Status

Needs review

Version

3.0

Component

Code

Created by

🇨🇭Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @berdir
  • Status changed to Needs review about 1 year ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update about 1 year ago
    2 pass
  • 🇸🇮Slovenia primsi

    Initial patch for this.

  • Status changed to Active about 1 year ago
  • 🇨🇭Switzerland berdir Switzerland
  • First commit to issue fork.
  • Merge request !11Dispatch an event on banning → (Open) created by alayham
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update about 1 year ago
    3 pass
  • Status changed to Needs review about 1 year ago
  • 🇨🇭Switzerland berdir Switzerland

    Nice work. One thing that's not implemented yet is my suggestion to be able to stop the ban through the event, so a method on it that's checked then after the dispatch.

    Also, it's already there but you're moving that code, there's really no need for those Xss::filter(), the placeholder does that, and $_SERVER should instead use the request object.

  • 🇸🇪Sweden alayham

    The ability to stop the ban can confuse other modules. Some subscribers might not know that the ban was aborted by another subscriber.
    If you think that is a valuable use case, then another event fired before the ban would be useful, and we keep this event to inform subscribers that a ban has actually happened.

    Regarding the XSS filtering, that is a code writing habit I got long ago, I consider it "defence in depth", so even if the something passes the previous layers of security, I might be able to stop it here. It is probably not needed here, but we are dealing with a request that we know to be nasty or malicious, and I prefer to be more careful. Anyway, does it hurt?

    I will be happy to add another "before banning" that allows aborting the ban if you think that is useful, but that would wait until I have some free time. I would suggest to merge the new tests in https://www.drupal.org/project/perimeter/issues/3304472 ✨ Write functionality tests Needs review and finish this one first.

  • 🇨🇭Switzerland berdir Switzerland

    Your call, I think the abort feature would be useful, several open feature requests could easily be implemented in an event subscriber *if* that is added. ✨ Use flood service Needs work for example if you decide not to that, or a whitelist. Being able to abort processing in an event subscriber is a very common use case, for example the Request and Response events do exactly that, see \Symfony\Component\HttpKernel\Event\RequestEvent::setResponse, the stopPropagation() calls then means that no further event subscribers will be called and the response is returned immediately.

    > Regarding the XSS filtering, that is a code writing habit I got long ago, I consider it "defence in depth", so even if the something passes the previous layers of security, I might be able to stop it here. It is probably not needed here, but we are dealing with a request that we know to be nasty or malicious, and I prefer to be more careful. Anyway, does it hurt?

    It does, actually. If there really is something in there that needs to be escaped then it will be escaped twice, because all translate and twig placeholders are safe by default unless it's a Markup object. Minor issue in a log message, but a real problem if it happens on node titles or similar things that visitors look at. Xss:filter() is *very* rarely needed in Drupal 8+, pretty much the only cases I know is when you're bypassing the render system, for example through javascript, like autocomplete callbacks.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update about 1 year ago
    3 pass
  • 🇸🇪Sweden alayham

    I tried overriding the default stopPropagation method to allow passing the module name and the reason for stopping, but I got weird issues in the tests. No more time today, so I am going with the default signature.
    I think it is useful to pass who and why the ban was stopped, and for later, here is the issue I faced with the test:

    PHPUnit 9.6.13 by Sebastian Bergmann and contributors.
    
    Testing /var/www/drupal/web/modules/contrib/perimeter
    FR.                                                                 3 / 3 (100%).
    
    Time: 00:11.462, Memory: 8.00 MB
    
    There was 1 failure:
    
    1) Drupal\Tests\perimeter\Functional\PerimeterBanEventsTest::testBanEventNotFound
    Test was run in child process and ended unexpectedly
    
    /var/www/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
    /var/www/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
    /var/www/drupal/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
    /var/www/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:144
    /var/www/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:97
    
    --
    
    There was 1 risky test:
    
    1) Drupal\Tests\perimeter\Functional\PerimeterBanEventsTest::testBanEventNotFound
    This test did not perform any assertions
    
    /var/www/drupal/web/core/tests/Drupal/Tests/Listeners/DrupalListener.php:65
    /var/www/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:452
    /var/www/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
    /var/www/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
    /var/www/drupal/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
    /var/www/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:144
    /var/www/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:97
    
    FAILURES!
    Tests: 3, Assertions: 16, Failures: 1, Risky: 1.
    
  • Status changed to Needs work 11 months ago
  • 🇩🇪Germany Anybody Porta Westfalica

    As of #11. @alayham are you planning to finish this?

  • Status changed to Needs review 11 months ago
  • 🇸🇪Sweden alayham

    I was able to pass the test by encapsulating the stopPropagation method with an abort method that accepts arguments.

      public function abort(string $module, string $reason): void {
        $this->module = $module;
        $this->reason = $reason;
        $this->stopPropagation();
      }
    

    Not sure whether that is an acceptable Symfony practice. Comments are welcome.

Production build 0.71.5 2024