chore: update Trezor Connect to v9.4.0, remove workarounds #26749
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat's wrong with native code?Contains native code which could be a vector to obscure malicious code, and generally decrease the likelihood of reproducible or reliable installs. Ensure that native code bindings are expected. Consumers may consider pure JS and functionally similar alternatives to avoid the challenges and risks associated with native code bindings. What is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. What is new author?A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package. Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights. What is network access?This module accesses the network. Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use. What is a deprecated package?The maintainer of the package marked it as deprecated. This could indicate that a single version should not be used, or that the package is no longer maintained and any new vulnerabilities will not be fixed. Research the state of the package and determine if there are non-deprecated versions that can be used, or if it should be replaced with a new, supported solution. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
martykan
force-pushed
the
chore/update-trezor-9-4-0
branch
2 times, most recently
from
7646897
to
ffae4cf
Compare
|
Test OK for Trezor connection on this PR branch: Screen.Recording.2024-09-06.at.23.42.29.mov |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26749 +/- ##
========================================
Coverage 70.08% 70.08%
========================================
Files 1414 1414
Lines 49328 49330 +2
Branches 13781 13781
========================================
+ Hits 34568 34570 +2
Misses 14760 14760 ☔ View full report in Codecov by Sentry. |
|
Hey @martykan I noticed there are some merge conflicts. If any support is needed with updating, feel free to reach out here. |
|
Yeah, probably due to #26882. |
martykan
force-pushed
the
chore/update-trezor-9-4-0
branch
from
ffae4cf
to
3f2f9e7
Compare
martykan
force-pushed
the
chore/update-trezor-9-4-0
branch
from
3f2f9e7
to
9495a8b
Compare
|
Ok, I've updated it. There seem to be some failing checks, I don't think I can affect those. |
This PR includes changes from PR #26749 by @martykan. <!-- 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** > With MV3, MetaMask started to use Trezor Connect inside an offscreen environment, in a way that was previously unsupported and required a workaround by patching the Trezor Connect library. > > In recent versions of Trezor Connect, the library can handle working in an offscreen environment correctly, without the need for the workaround, which could cause issues with compatibility. > > This PR removes the patch and updates the Trezor Connect library to the latest version (v9.4.0). > The change in manifest.json is due to new URL parameters, Firefox needs the asterisk at the end to match the URL with them. > I haven't removed the WebUSB device request which was added on MetaMask's side in relation to the workaround, since it can improve UX of the pairing process, but it could be removed if desired. > > The dependency update affects Lavamoat, I am including the policy changes in my commit, however let me know if you would like me to remove them and handle them using your own process. [](https://codespaces.new/MetaMask/metamask-extension/pull/27112?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** > 1. Open accounts dropdown, "Add hardware wallet" > 2. Select Trezor > 3. Follow prompts to connect the device > 4. See a list of accounts from the Trezor ## **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** - [x ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x ] 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-extension/blob/develop/.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.
|
Thanks again for the contribution! This PR was re-opened as #27112 to workaround some issues we are currently having with CI and forks, and that PR has been closed. |
](https://codespaces.new/MetaMask/metamask-extension/pull/27112?quickstart=1){kind=link}
Description
With MV3, MetaMask started to use Trezor Connect inside an offscreen environment, in a way that was previously unsupported and required a workaround by patching the Trezor Connect library.
In recent versions of Trezor Connect, the library can handle working in an offscreen environment correctly, without the need for the workaround, which could cause issues with compatibility.
This PR removes the patch and updates the Trezor Connect library to the latest version (v9.4.0).
The change in
manifest.jsonis due to new URL parameters, Firefox needs the asterisk at the end to match the URL with them.I haven't removed the WebUSB device request which was added on MetaMask's side in relation to the workaround, since it can improve UX of the pairing process, but it could be removed if desired.
The dependency update affects Lavamoat, I am including the policy changes in my commit, however let me know if you would like me to remove them and handle them using your own process.
Related issues
NA
Manual testing steps
Screenshots/Recordings
NA
Pre-merge author checklist
Pre-merge reviewer checklist