- Issue created by @berdir
- Status changed to Needs review
about 1 year ago 9:03am 19 October 2023 - last update
about 1 year ago 2 pass - Status changed to Active
about 1 year ago 8:14am 20 October 2023 - First commit to issue fork.
- last update
about 1 year ago 3 pass - Status changed to Needs review
about 1 year ago 1:26pm 22 October 2023 - 🇨ðŸ‡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.
- 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 10:51am 10 January 2024 - Status changed to Needs review
11 months ago 6:26am 11 January 2024 - 🇸🇪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.