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.

Issue fork perimeter-3349053

Command icon 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

douggreen created an issue. See original summary.

douggreen’s picture

Status: Active » Needs review
StatusFileSize
new6.11 KB

The 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.

douggreen’s picture

Attached patch is for the 2.x d9 branch.

anybody’s picture

Assigned: Unassigned » grevil

Thank you very much! @Grevil could you review this carefully please?

douggreen’s picture

Updated patches use dependency injection to create the ban.ip_manager; the 3.x patch also has a re-roll so that it applies.

douggreen’s picture

Title: Make perimeter compatible with fast404 module » Make perimeter compatible with fast404 module and use DI for ban.ip_manager
douggreen’s picture

Updated patch has a minor change to remove an unnecessary permission check.

anybody’s picture

Assigned: grevil » Unassigned
Status: Needs review » Needs work

@douggreen thank you, could you please use a MR instead of patches?

douggreen’s picture

Ping, I made a PR a while ago, please re-review.

douggreen’s picture

Status: Needs work » Needs review

Grevil made their first commit to this issue’s fork.

grevil’s picture

Status: Needs review » Reviewed & tested by the community

Wow! 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:

  1. We currently only have simple installation tests.
  2. The test would need fast404 as a dependency

I would be fine without one, what do you think @Anybody?

anybody’s picture

Thanks, 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.

grevil’s picture

No, 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! :)

grevil’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs manual testing, -Needs tests

Just 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!

  • Grevil committed e910c8e4 on 3.x authored by douggreen
    #3349053 - Make perimeter compatible with fast404 module and use DI for...
grevil’s picture

Created a new release!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

anybody’s picture

For 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