Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

johnnyshields
Copy link

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

…nd instead catches errors at the top level.
@@ -128,6 +128,17 @@ def call(env)
configuration.tracked?(request)
@app.call(env)
end
rescue *allowed_errors
Copy link
Collaborator

@grzuy grzuy Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, wouldn't this also rescue dalli and redis errors ocurring inside other middlewares down the line called in @app.call(env)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I've refactored the scope which needs error handling into its own method.

@johnnyshields
Copy link
Author

johnnyshields commented Nov 25, 2023

@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
Copy link
Collaborator

santib commented Nov 26, 2023

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 👍

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
Copy link
Author

johnnyshields commented Nov 26, 2023

@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
Copy link
Author

@santib can this be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants