Skip to content

fix: use password instead of key when upgrading vault#5601

Merged
mikesposito merged 5 commits into
mainfrom
mikesposito/fix/cached-key
Apr 7, 2025
Merged

fix: use password instead of key when upgrading vault#5601
mikesposito merged 5 commits into
mainfrom
mikesposito/fix/cached-key

Conversation

@mikesposito

@mikesposito mikesposito commented Apr 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Explanation

Alternative proposal to #5593 to invalidate the encryption key when the vault encryption needs to be updated.

References

Changelog

### Fixed

- The cached encryption key is invalidated when the vault needs to upgrade its encryption parameters ([#5601](https://github.com/MetaMask/core/pull/5601))

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

mikesposito
mikesposito commented Apr 7, 2025
View reviewed changes
Comment thread packages/keyring-controller/CHANGELOG.md Outdated
@mikesposito mikesposito marked this pull request as ready for review April 7, 2025 13:21
@mikesposito mikesposito requested review from a team as code owners April 7, 2025 13:21
@gantunesr gantunesr mentioned this pull request Apr 7, 2025
Closed
4 tasks
cryptodev-2s
cryptodev-2s approved these changes Apr 7, 2025
View reviewed changes

@cryptodev-2s cryptodev-2s left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@mikesposito mikesposito merged commit 414ddc5 into main Apr 7, 2025
@mikesposito mikesposito deleted the mikesposito/fix/cached-key branch April 7, 2025 15:12
github-merge-queue Bot pushed a commit to MetaMask/metamask-mobile that referenced this pull request Apr 25, 2025
@mikesposito @NicolasMassart
chore: cp-7.25.0 bump @metamask/keyring-controller to ^21.0.4 (#1…
6de2b94 
…4702)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

Updating `@metamask/keyring-controller` to `^21.0.4`

```markdown
## [21.0.4]

### Fixed

- Ensure no duplicate accounts are persisted ([#5710](MetaMask/core#5710))

## [21.0.3]

### Changed

- `ExportableKeyEncryptor` is now a generic type with a type parameter `EncryptionKey` ([#5395](MetaMask/core#5395))
  - The type parameter defaults to `unknown`

### Fixed

- Fixed wrong error message thrown when using the wrong password ([#5627](MetaMask/core#5627))

## [21.0.2]

### Changed

- Bump `@metamask/keyring-api` from `^17.2.0` to `^17.4.0` ([#5565](MetaMask/core#5565))
- Bump `@metamask/keyring-internal-api` from `^6.0.0` to `^6.0.1` ([#5565](MetaMask/core#5565))

### Fixed

- Ignore cached encryption key when the vault needs to upgrade its encryption parameters ([#5601](MetaMask/core#5601))


```

## **Related issues**

Fixes: #14228 

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>
runway-github Bot pushed a commit to MetaMask/metamask-mobile that referenced this pull request Apr 30, 2025
@mikesposito @NicolasMassart
chore(runway): cherry-pick chore: cp-7.25.0 bump `@metamask/keyring-c…
df5e74d 
…ontroller` to `^21.0.4` (#14702)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

Updating `@metamask/keyring-controller` to `^21.0.4`

```markdown
## [21.0.4]

### Fixed

- Ensure no duplicate accounts are persisted ([#5710](MetaMask/core#5710))

## [21.0.3]

### Changed

- `ExportableKeyEncryptor` is now a generic type with a type parameter `EncryptionKey` ([#5395](MetaMask/core#5395))
  - The type parameter defaults to `unknown`

### Fixed

- Fixed wrong error message thrown when using the wrong password ([#5627](MetaMask/core#5627))

## [21.0.2]

### Changed

- Bump `@metamask/keyring-api` from `^17.2.0` to `^17.4.0` ([#5565](MetaMask/core#5565))
- Bump `@metamask/keyring-internal-api` from `^6.0.0` to `^6.0.1` ([#5565](MetaMask/core#5565))

### Fixed

- Ignore cached encryption key when the vault needs to upgrade its encryption parameters ([#5601](MetaMask/core#5601))


```

## **Related issues**

Fixes: #14228 

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>
EtherWizard33 pushed a commit to MetaMask/metamask-mobile that referenced this pull request May 2, 2025
@mikesposito @NicolasMassart
chore: cp-7.25.0 bump @metamask/keyring-controller to ^21.0.4 (#1…
b8e0cd9 
…4702)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

Updating `@metamask/keyring-controller` to `^21.0.4`

```markdown
## [21.0.4]

### Fixed

- Ensure no duplicate accounts are persisted ([#5710](MetaMask/core#5710))

## [21.0.3]

### Changed

- `ExportableKeyEncryptor` is now a generic type with a type parameter `EncryptionKey` ([#5395](MetaMask/core#5395))
  - The type parameter defaults to `unknown`

### Fixed

- Fixed wrong error message thrown when using the wrong password ([#5627](MetaMask/core#5627))

## [21.0.2]

### Changed

- Bump `@metamask/keyring-api` from `^17.2.0` to `^17.4.0` ([#5565](MetaMask/core#5565))
- Bump `@metamask/keyring-internal-api` from `^6.0.0` to `^6.0.1` ([#5565](MetaMask/core#5565))

### Fixed

- Ignore cached encryption key when the vault needs to upgrade its encryption parameters ([#5601](MetaMask/core#5601))


```

## **Related issues**

Fixes: #14228 

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gantunesr gantunesr gantunesr approved these changes

@cryptodev-2s cryptodev-2s cryptodev-2s approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mikesposito @gantunesr @cryptodev-2s