rack / rack-attack

Rack middleware for blocking & throttling
MIT License
5.57k stars 337 forks source link

System Hardening Phase 1: Catch errors at top level, take two #641

Open johnnyshields opened 11 months ago

johnnyshields commented 11 months ago

This PR removes the rescuing blocks from Dalli/RedisProxy classes and instead catches errors at the top level.

It is a simpler version of #639

johnnyshields commented 11 months ago

@grzuy @santib Your help to review this PR is appreciated.

@santib in previous threads you had proposed introducing an intermediate error such as "StoreProxyError". I've expressed that I don't feel there is a big advantage in doing this, as it obscures the underlying errors which have valuable information. As you'll see in my end-state I'm adding a hook to report errors e.g. via Sentry/Bugsnag (user adds their own), so keeping the raw Redis/Dalli errors are valuable.

It would be great if we could merge this so I can proceed on the series of PRs that users are waiting on. If at the end there is a strong reason to introduce a StoreProxyError I'm happy to add it at the end.

santib commented 11 months ago

Hey @johnnyshields,

I’d like to make clear that not necessarily all of the proposed PRs will get merged. We’ll need to evaluate the tradeoffs of each of them.

Regarding this PR, I think the end goal it’s solving is valuable and would like to see it merged once we agree on the solution :+1:

Regarding the solution, I still think the wrapper error is better (this wouldn’t have even happened), and maybe we should even consider going a step further and do something like this. The fact that we raise a custom rack attack error doesn't mean we can't provide the original error to devs wherever we want. But I’d like to hear @grzuy thoughts on this before moving on with either solution.

johnnyshields commented 11 months ago

@santib lets keep it simple. This PR has a low-complexity implementation. We can always introduce more complexity (new error classes, Exception.cause, etc) at anytime in the future if it is warranted.

johnnyshields commented 7 months ago

@santib can this be merged?