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

fix: properly decrypt legacy state blobs #2472

Merged
merged 4 commits into from
Jun 10, 2024

Conversation

FrederikBolding
Copy link
Member

Fixes a problem where encrypted state blobs produced before keyMetadata was added would fail to decrypt.

The encryptor would attempt to use the latest key derivation parameters when no keyMetadata was passed in, causing a failure to decrypt: https://github.com/MetaMask/metamask-extension/blob/ed5ec2d28f7e118803c6fdc74abe90db4e00f480/app/scripts/lib/encryptor-factory.ts#L78-L82

This PR simply falls back to the legacy key derivation parameters whenever keyMetadata isn't present strictly for decryption purposes. The default key derivation parameters are still in use for all encryption.

@FrederikBolding FrederikBolding requested a review from a team as a code owner June 10, 2024 09:46
Mrtenz
Mrtenz previously approved these changes Jun 10, 2024
@@ -454,6 +454,13 @@ export const DEFAULT_ENCRYPTION_KEY_DERIVATION_OPTIONS = {
},
};

export const LEGACY_ENCRYPTION_KEY_DERIVATION_OPTIONS = {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this to the Snap controller file (or a constants file) and use it as the fallback, to remove some duplication?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.34%. Comparing base (1bf7e4f) to head (9b09c18).

Current head 9b09c18 differs from pull request most recent head f4c1e30

Please upload reports for the commit f4c1e30 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2472   +/-   ##
=======================================
  Coverage   94.34%   94.34%           
=======================================
  Files         438      438           
  Lines        9052     9053    +1     
  Branches     1391     1392    +1     
=======================================
+ Hits         8540     8541    +1     
  Misses        512      512           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FrederikBolding FrederikBolding merged commit e717139 into main Jun 10, 2024
151 checks passed
@FrederikBolding FrederikBolding deleted the fb/fix-decryption-of-legacy-state branch June 10, 2024 10:15
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.

2 participants