fix(23804): unable to add QR code account #23903

dawnseeker8 · 2024-04-09T08:17:49Z

Description

The connection via QR code on Hardware wallet is not proceeding past scanning the QR code. Functionality is working in the release version v11.13.1.

After debuggin the code and discover that the issue is caused by Keyring Controller upgraded to 13.0.0. and #addQRKeyring function have bug to pass a empty accounts array as parameters to keyring, cause the Keystone MetamaskKeyring default properties all are undefined during deserialise() process.

This fix patch the @metamask/keyring-controller to remove the empty array passed to KeyStone MetamaskKeyring so that all default properies in keyring will remain the same.

Related issues

Fixes: #23804

Manual testing steps

On your hardware wallet, fetch the QR code for MetaMask
Proceed to add hardware wallet from the account drop-down selector
Select QR-based and Continue
Scan the QR code from the hardware wallet
user should be able to select QR code account and imported

Screenshots/Recordings

Before

Screen.Recording.2024-03-26.at.21.01.01.mov

After

Screen.Recording.2024-04-09.at.09.15.28.mov

Pre-merge author checklist

I’ve followed MetaMask Coding Standards.
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 format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

…tion issue.

github-actions · 2024-04-09T08:18:09Z

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.

socket-security · 2024-04-09T08:18:57Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@npmcli/fs@2.1.2	filesystem	`+1`	43.1 kB	gar
npm/aproba@2.0.0	None	`0`	8.05 kB	iarna
npm/cacache@16.1.3	filesystem	`+7`	142 kB	lukekarrys
npm/make-fetch-happen@10.2.1	environment, network	`+8`	298 kB	gar
npm/minipass-fetch@2.1.2	environment, network	`+3`	526 kB	gar
npm/nopt@6.0.0	environment	`+1`	28.7 kB	gar
npm/npmlog@6.0.2	None	`0`	17.1 kB	lukekarrys
npm/socks@2.8.1	network	`+3`	518 kB	joshglazebrook
npm/ssri@9.0.1	None	`0`	37.5 kB	nlf
npm/unique-filename@2.0.1	None	`+2`	18 kB	lukekarrys

🚮 Removed packages: npm/socks@2.8.3

View full report↗︎

socket-security · 2024-04-09T08:19:00Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/@npmcli/fs@2.1.2, npm/@types/pbkdf2@3.1.0, npm/@types/secp256k1@4.0.3, npm/aproba@2.0.0, npm/cacache@16.1.3, npm/make-fetch-happen@10.2.1, npm/minipass-collect@1.0.2, npm/minipass-fetch@2.1.2, npm/minipass@3.3.5, npm/node-gyp@9.3.0, npm/nopt@6.0.0, npm/npmlog@6.0.2, npm/socks@2.8.1, npm/ssri@9.0.1, npm/unique-filename@2.0.1, npm/unique-slug@3.0.0

View full report↗︎

Next steps

Take a deeper look at the dependency

Take 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 package

If 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 risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

package.json

.yarn/patches/@metamask-keyring-controller-npm-13.0.0-d94816a680.patch

gantunesr

Dimiss approval

codecov · 2024-04-09T14:29:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.46%. Comparing base (255a961) to head (bffd110).
Report is 3 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #23903   +/-   ##
========================================
  Coverage    67.46%   67.46%           
========================================
  Files         1244     1244           
  Lines        48777    48777           
  Branches     12724    12724           
========================================
  Hits         32907    32907           
  Misses       15870    15870

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

metamaskbot · 2024-04-09T14:52:47Z

Builds ready [6d909ed]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (469 ± 360 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	74	158	108	23	11
		domContentLoaded	11	67	33	15	7
		load	62	2190	469	750	360
		domInteractive	11	67	33	15	7

Bundle size diffs [🚀 Bundle size reduced!]

background: -14 Bytes (-0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

…ller version ini package.json.

gantunesr · 2024-04-10T08:52:30Z

@metamaskbot update-policies

metamaskbot · 2024-04-10T09:01:45Z

Policies updated

metamaskbot · 2024-04-10T09:21:41Z

Builds ready [eab6341]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (581 ± 414 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	69	218	106	31	15
		domContentLoaded	12	48	26	10	5
		load	56	2245	581	861	414
		domInteractive	12	48	26	10	5

Bundle size diffs [🚀 Bundle size reduced!]

background: -14 Bytes (-0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

package.json

benjisclowder

QA passed.

…been downgraded.

benjisclowder

QA passed

Gudahtt · 2024-04-10T13:51:59Z

@metamaskbot update-policies

metamaskbot · 2024-04-10T14:01:35Z

Policies updated

metamaskbot · 2024-04-10T14:22:44Z

Builds ready [7f90d82]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (664 ± 492 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	59	207	100	37	18
		domContentLoaded	9	44	20	9	4
		load	47	2735	664	1025	492
		domInteractive	9	44	20	9	4

Bundle size diffs [🚀 Bundle size reduced!]

background: -14 Bytes (-0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

benjisclowder

QA passed.

metamaskbot · 2024-04-11T10:41:16Z

Builds ready [bffd110]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (794 ± 513 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	71	350	134	71	34
		domContentLoaded	10	74	30	17	8
		load	57	2584	794	1069	513
		domInteractive	10	73	30	17	8

Bundle size diffs [🚀 Bundle size reduced!]

background: -14 Bytes (-0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

danjm · 2024-04-11T14:46:41Z

@SocketSecurity ignore-all

The new author warnings are safe. We have approved those authors in the past, and ther are github employees and/or long time contributors to the npm ecosystem.

The network access is necessary for make-fetch-happen to work, and that has been part of our dependency tree for a long time.

fix: patch the keyringController to fix the QR code keyring intializa…

af60e84

…tion issue.

dawnseeker8 added the team-hardware-wallets-deprecated DEPRECATED: please use "team-accounts-framework" instead label Apr 9, 2024

dawnseeker8 requested a review from a team as a code owner April 9, 2024 08:17

dawnseeker8 added Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. release-blocker This bug is blocking the next release labels Apr 9, 2024

dawnseeker8 added the e2e-test End to end test for the MetaMask extension label Apr 9, 2024

Merge branch 'develop' into fix/23804-add-qr-connection-issue

c2deeb0

dawnseeker8 added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 9, 2024

danjm reviewed Apr 9, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

danjm reviewed Apr 9, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

gantunesr approved these changes Apr 9, 2024

View reviewed changes

.yarn/patches/@metamask-keyring-controller-npm-13.0.0-d94816a680.patch Show resolved Hide resolved

gantunesr self-requested a review April 9, 2024 12:33

gantunesr requested changes Apr 9, 2024

View reviewed changes

Merge branch 'develop' into fix/23804-add-qr-connection-issue

107a492

Merge branch 'develop' into fix/23804-add-qr-connection-issue

6d909ed

fix: Add comments in the patch and remove not relative keyring-contro…

b39a413

…ller version ini package.json.

dawnseeker8 requested review from danjm and gantunesr April 9, 2024 15:19

fix: run yarn lavamoat:auto to auto fix the policy

d5f17a7

dawnseeker8 requested review from a team as code owners April 10, 2024 01:06

Merge branch 'develop' into fix/23804-add-qr-connection-issue

9ff7551

gantunesr changed the title ~~fix: 23804 unable to add QR code account in metamask extension~~ fix(23804): unable to add QR code account Apr 10, 2024

Update LavaMoat policies

eab6341

Gudahtt reviewed Apr 10, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

benjisclowder previously approved these changes Apr 10, 2024

View reviewed changes

dawnseeker8 added 2 commits April 10, 2024 14:15

Merge branch 'develop' into fix/23804-add-qr-connection-issue

b25741f

fix: rebase from develop version and then check any libraries have …

2c33a02

…been downgraded.

dawnseeker8 dismissed benjisclowder’s stale review via 2c33a02 April 10, 2024 13:21

dawnseeker8 requested review from Gudahtt and benjisclowder April 10, 2024 13:35

benjisclowder previously approved these changes Apr 10, 2024

View reviewed changes

Update LavaMoat policies

7f90d82

metamaskbot dismissed benjisclowder’s stale review via 7f90d82 April 10, 2024 14:01

dawnseeker8 requested a review from benjisclowder April 10, 2024 14:30

dawnseeker8 added 2 commits April 11, 2024 01:49

Merge branch 'develop' into fix/23804-add-qr-connection-issue

63c7990

Merge branch 'develop' into fix/23804-add-qr-connection-issue

bffd110

benjisclowder approved these changes Apr 11, 2024

View reviewed changes

danjm approved these changes Apr 11, 2024

View reviewed changes

dawnseeker8 added the QA Passed label Apr 11, 2024

gantunesr approved these changes Apr 11, 2024

View reviewed changes

dawnseeker8 merged commit b8159d4 into develop Apr 11, 2024

dawnseeker8 deleted the fix/23804-add-qr-connection-issue branch April 11, 2024 16:23

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 11, 2024

github-actions bot locked and limited conversation to collaborators Apr 11, 2024

metamaskbot added the release-11.15.0 Issue or pull request that will be included in release 11.15.0 label Apr 11, 2024

fix(23804): unable to add QR code account #23903

fix(23804): unable to add QR code account #23903

Uh oh!

Conversation

dawnseeker8 commented Apr 9, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Apr 9, 2024

Uh oh!

socket-security bot commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

socket-security bot commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Next steps

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gantunesr left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Apr 9, 2024

Uh oh!

gantunesr commented Apr 10, 2024

Uh oh!

metamaskbot commented Apr 10, 2024

Uh oh!

metamaskbot commented Apr 10, 2024

Uh oh!

Uh oh!

benjisclowder left a comment

Choose a reason for hiding this comment

Uh oh!

benjisclowder left a comment

Choose a reason for hiding this comment

Uh oh!

Gudahtt commented Apr 10, 2024

Uh oh!

metamaskbot commented Apr 10, 2024

Uh oh!

metamaskbot commented Apr 10, 2024

Uh oh!

benjisclowder left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Apr 11, 2024

Uh oh!

danjm commented Apr 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dawnseeker8 commented Apr 9, 2024 •

edited by github-actions bot

Loading

socket-security bot commented Apr 9, 2024 •

edited

Loading

socket-security bot commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 9, 2024 •

edited

Loading