Instrument fail2ban #655
Instrument fail2ban #655
Conversation
woahdae
force-pushed
the
instrument-fail2ban
branch
2 times, most recently
from
e3b7731
to
923ecd5
Compare
Previously, if a request triggered a ban, there was no instrumentation for knowing the ban occurred. This instruments bans under the `ban.rack_attack` notification.
woahdae
force-pushed
the
instrument-fail2ban
branch
from
923ecd5
to
d872acc
Compare
| request.env["rack.attack.matched"] ||= name | ||
| request.env["rack.attack.match_type"] ||= type |
There was a problem hiding this comment.
note to self: this can be considered a breaking change since it alters the way rack-attack currently notifies about blocklist.rack_attack
There was a problem hiding this comment.
I can look at this more closely tomorrow, but the reason for this change was that, without this, the request was first getting a match_type of :ban set in the environment as it was banned, then as the request was blocklisted, it was overridden here as :blocklist. I believe the blocklist should work the same as it did previously, definitely if the developer hasn't passed in the request (an awkward requirement for this to work), maybe in general. I could look at the test suite with this in mind if it's a concern.
There was a problem hiding this comment.
I just had some time to come back to this.
I opened this PR #656 with my concern about this. This gem is so widely used that I wouldn't like to change existing behavior (unless it's in a new major version). I'm wondering if we can keep the existing events + adding the new one :ban that you are introducing. Maybe we can achieve that by not modifying the request.env in https://github.com/rack/rack-attack/pull/655/files#diff-aa6124b2b316bd2edb9f054a2a3200ab0c12248c79f4bc30da5195122c949363R73 and instead passing another object to Rack::Attack.instrument ?
Previously, if a request triggered a ban, there was no instrumentation for knowing the ban occurred.
This instruments bans under the
ban.rack_attacknotification.