Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Trail of bits audit - Issue #3 (Improper error handling) #352

Merged
merged 13 commits into from
Oct 3, 2022

Conversation

isSerge
Copy link
Contributor

@isSerge isSerge commented Sep 30, 2022

Introduced new error titles:

"Failed to initialize autolauncher",
"Failed to initialize updater",
"Failed enabling autolauncher",
"Failed disabling autolauncher",
"Failed to get logs",
"Failed to initialize config",
"Failed to reset the application",
"Failed to start plotting"

Draft, since we need to update the current generic error message:

Please, make sure you have an Internet connection and enough disk space in order to proceed. If the problem persists, you can reach out to our team

Closes #337

@isSerge isSerge self-assigned this Sep 30, 2022
@isSerge isSerge linked an issue Sep 30, 2022 that may be closed by this pull request
10 tasks
…trail-of-bits-audit-issue-3-improper-error-handling
Copy link
Contributor

@ozgunozerk ozgunozerk left a comment

Choose a reason for hiding this comment

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

I noticed this PR will get rid of errorLogger in many places, but we also want errors to be logged.
When I looked at setError() code in store.ts, I couldn't see any call to errorLogger()?

@ozgunozerk
Copy link
Contributor

@isSerge I'm happy with it as is. However, what about embedding errorLogger(error) into the setError(error) function, so that we will only call setError() from the outside?
The code may be cleaner this way.

@isSerge
Copy link
Contributor Author

isSerge commented Oct 3, 2022

@ozgunozerk setError does not have access to the logger, unfortunately, it seems Pinia does not support dependency injection, so we'll have to provide it as a parameter as in other methods

@isSerge
Copy link
Contributor Author

isSerge commented Oct 3, 2022

@ozgunozerk also setError accepts objects with a title and optional message (which are displayed to users), while the logger logs the actual error, so I would prefer to keep them separate

@isSerge isSerge marked this pull request as ready for review October 3, 2022 15:18
@ozgunozerk ozgunozerk self-requested a review October 3, 2022 15:23
Copy link
Contributor

@ozgunozerk ozgunozerk left a comment

Choose a reason for hiding this comment

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

did inspect during draft, looks great to me

@isSerge isSerge merged commit 7020e6f into main Oct 3, 2022
@isSerge isSerge deleted the 337-trail-of-bits-audit-issue-3-improper-error-handling branch October 3, 2022 17:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trail of Bits audit - Issue #3 (Improper error handling)
2 participants