Thanks for the module, great idea!
I've noticed that many URL's aren't getting banned because https://www.drupal.org/project/fast_404 handles the request exception first, and stops propagation, thus the exception is never handled. We should add a response handler that applies the ban logic.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | perimeter--fast404-symfony4-d9--3349053-6.patch | 6.48 KB | douggreen |
| #7 | perimeter--fast404-symfony4--3349053-6.patch | 6.48 KB | douggreen |
Issue fork perimeter-3349053
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
douggreen commentedThe attached patch adds the addition exception handler.
This also has some refactoring, cleanup, and also does the Symfony 4 upgrade (see #3336029: Symfony > 4.4 uses getThrowable). I apologize for mixing so many issues. I hope that you can commit this as-is.
Comment #3
douggreen commentedAttached patch is for the 2.x d9 branch.
Comment #4
anybodyThank you very much! @Grevil could you review this carefully please?
Comment #5
douggreen commentedUpdated patches use dependency injection to create the ban.ip_manager; the 3.x patch also has a re-roll so that it applies.
Comment #6
douggreen commentedComment #7
douggreen commentedUpdated patch has a minor change to remove an unnecessary permission check.
Comment #8
anybody@douggreen thank you, could you please use a MR instead of patches?
Comment #10
douggreen commentedPing, I made a PR a while ago, please re-review.
Comment #11
douggreen commentedComment #13
grevil commentedWow! Great and clean code! Well done!
This will make the whole class way cleaner and tidier, thanks! 🙂
LGTM!
I would usually ask for a test, BUT since:
I would be fine without one, what do you think @Anybody?
Comment #14
anybodyThanks, I agree this really looks good. Best would be indeed to have tests. @douggreen could you help with that?
If not, it should please be at least tested manually to ensure nothing breaks. Did you already do that @Grevil?
Tests should be added in #3304472: Write functionality tests please.
Comment #15
grevil commentedNo, I have only reviewed the code, since apart from the fast404 support the general code hasn't changed much, just generally "moved" and refactored a bit, to be clearer, but surely I can test this! :)
Comment #16
grevil commentedJust tested it and it works great!
Enable the default "$settings['fast404_exts']" (including the .asp extension) in the settings php.
Enable perimeter without the patch applied and check that the .asp extension will get you banned.
Go to "/bla.asp" → it will throw a fast404 not found
Apply the patch and clear caches
Go to "/bla.asp" → you are banned
RTBC for testing!
I added a notice inside #3304472: Write functionality tests to test this via test case in the future, but for now, I will merge this!
Comment #18
grevil commentedCreated a new release!
Comment #20
anybodyFor the records: The approach taken here by crowdsec might also be interesting: #3337245: Change scanning detection method to also work with Fast404 and other handlers