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

[PS-1137] Fix reseed storage #4543

Merged
merged 3 commits into from Jan 27, 2023
Merged

[PS-1137] Fix reseed storage #4543

merged 3 commits into from Jan 27, 2023

Conversation

djsmith85
Copy link
Contributor

@djsmith85 djsmith85 commented Jan 23, 2023

Type of change

- [X] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

On logout we call stateService.clean to remove the account. The check for vault-timeout in reseedStorage will always return null as the account was already cleaned. The check needs to be done beforehand.

Code changes

  • apps/browser/src/background/main.background.ts:
    • Retrieve vault-timeout set by user, before removing the account from stateService (.clean())

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

The check for the set vault-timeout needs to happen before all cleaning stateService
Remove check inside of reseedStorage as happens outside prior to calling it (logout/settings.component)
Execute on all browsers besides Safari as it does not support chrome.storage.local.get with an empty key

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/get
@djsmith85 djsmith85 requested a review from a team January 23, 2023 20:01
if (currentVaultTimeout == null) {
// Skipping Safari as it does not support retrieving all data from localStorage via an empty key
// https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/get
if (this.platformUtilsService.isSafari) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not equivalent to the replaced code. For example, firefox would be skipped in the previous, but not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is expected as I wanted to remove the previous limit completely with d7f71aa but had to include Safari as it would throw with an empty key

Copy link
Member

Choose a reason for hiding this comment

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

I believe the limitation was due to logging practices, not api limitations. I have not specifically looked at other browsers but to my memory this came about due to specifically chrome logging local storage values.

Is that not the case? I would prefer to not reseed storage unless needed for security.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have also not heard about any logging-issue related to this on other browser than Chrome-based ones. I had no issues testing FF with or without the changes, so thought I'd remove/loosen the check. Reverted my last commit

@djsmith85 djsmith85 added the needs-qa Marks a PR as requiring QA approval label Jan 23, 2023
@djsmith85 djsmith85 removed the needs-qa Marks a PR as requiring QA approval label Jan 27, 2023
@djsmith85 djsmith85 merged commit 75fbccb into master Jan 27, 2023
@djsmith85 djsmith85 deleted the PS-1137-fix-reseed-storage branch January 27, 2023 10:49
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.

Vulnerability: Chrome extension saves vault in persistent local storage after logging out and exiting Chrome
2 participants