feat(mv3): Ledger MV3 support by brad-decker · Pull Request #22275 · MetaMask/metamask-extension

brad-decker · 2023-12-13T18:40:57Z

Description

Adds the ledger-iframe to the offscreen document and establishes MV3 support for ledger. This is done by the following method:

ledger-iframe.ts interfaces with the ledger keyring iframe, working as a middle layer between the two. Messages received by the offscreen document that target the ledger iframe will be forwarded to the ledger keyring iframe (the github pages generated content from @MetaMask/eth-ledger-bridge-keyring. A callback manager uses an incremental id to assign an id to each message which is returned in the response such that a callback can be declared only for a specific id.
ledger-offscreen-bridge.ts is a bridge for the LedgerKeyring that interfaces with the offscreen document such that when the keyring calls out for specific methods, it calls into the offscreen bridge and then the communication is sent to the offscreen document and forwarded to the keyring iframe.

In addition some more cleanup of the #18794 deprecation of u2f and ledger live. We no longer support setting any other method than the browsers lowest level method for connecting. For chrome webhid is the gold standard, for firefox which doesn't support webhid we use u2f. There were still a number of places in the code that reference the transportType from preferences and migrations were done to change the value for many users when we only need to know what the support for those two protocols are.

So firstly it changes the setLedgerTransportPreference to not take an input, but rather just uses the browsers support of navigator.hid to determine whether to use webhid or u2f. When #18794 landed service workers did not have access to webhid, now they do, so this is a viable approach. I also added deprecation messages where appropriate. In a future PR we should go through and remove any usages of the preference. Ideally we'd also set the transport type for the ledger device closer to the bridge, however i'm not familiar with the unlock mechanism of the keyring and trying to do this was causing issues with the mv3 connectivity.

Related issues

Fixes #17724 - No longer relevant because background/service workers have access to window.navigator.webhid
Fixes #21624 - Connects ledger to offscreen doc

Screenshots/Recordings

Manual testing steps

run yarn start:mv3
reload the extension in your browser.
try to add a ledger device. (Remember you must have your device unlocked and the ethereum app open on your ledger
Try to use the test dapp to send tx, sign messages, etc.

Pre-merge author checklist

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.

codecov · 2023-12-14T19:23:12Z

Codecov Report

Attention: 65 lines in your changes are missing coverage. Please review.

Comparison is base (c6d6946) 67.90% compared to head (359627f) 67.80%.

❗ Current head 359627f differs from pull request most recent head eed8495. Consider uploading reports for the commit eed8495 to get more accurate results

Files	Patch %	Lines
...ts/lib/offscreen-bridge/ledger-offscreen-bridge.ts	3.03%	64 Missing ⚠️
app/scripts/metamask-controller.js	90.91%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #22275      +/-   ##
===========================================
- Coverage    67.90%   67.80%   -0.10%     
===========================================
  Files         1071     1069       -2     
  Lines        41378    41334      -44     
  Branches     11112    11104       -8     
===========================================
- Hits         28096    28023      -73     
- Misses       13282    13311      +29

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

metamaskbot · 2023-12-14T19:34:52Z

Builds ready [0e09033]

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
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1079 ± 127 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	82	274	161	70	34
		domContentLoaded	9	184	68	75	36
		load	776	1539	1079	265	127
		domInteractive	9	184	68	75	36

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2.38 KiB (0.07%)
ui: -500 Bytes (-0.01%)
common: 187 Bytes (0.00%)

metamaskbot · 2023-12-17T19:17:27Z

Builds ready [247ff78]

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
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1356 ± 203 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	89	459	205	89	43
		domContentLoaded	11	237	83	73	35
		load	783	2291	1356	423	203
		domInteractive	11	237	83	73	35

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2.38 KiB (0.07%)
ui: -500 Bytes (-0.01%)
common: 187 Bytes (0.00%)

pedronfigueiredo · 2023-12-18T15:39:00Z

Testing with a ledger device on the latest Chrome, I see this message on the background (and offscreen) console. Can you help me understand what I may have done wrong?

pedronfigueiredo · 2023-12-18T17:59:24Z

Testing with a ledger device on the latest Chrome, I see this message on the background (and offscreen) console. Can you help me understand what I may have done wrong?

After another trial, I couldn't reproduce this error again. Tested manually and it works on both nano x and s plus. LGTM

metamaskbot · 2023-12-19T03:14:15Z

Builds ready [359627f]

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
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1344 ± 139 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	98	400	190	89	43
		domContentLoaded	12	231	72	77	37
		load	886	1889	1344	289	139
		domInteractive	12	231	72	77	37

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2.38 KiB (0.07%)
ui: -500 Bytes (-0.01%)
common: 187 Bytes (0.00%)

danjm · 2023-12-19T15:18:15Z

app/scripts/metamask-controller.js

  async getKeyringForDevice(deviceName, hdPath = null) {
    const keyringOverrides = this.opts.overrides?.keyrings;
    let keyringName = null;
+    console.log(deviceName);


console.log

danjm · 2023-12-19T15:38:20Z

app/scripts/lib/offscreen-bridge/ledger-offscreen-bridge.ts

+          action: LedgerAction.makeApp,
+        },
+        (response) => {
+          if (response.success) {


I notice this if else block is different from what is currently on the main branch on the eth-ledger-bridge-keyring repo. Is that expected?

https://github.com/MetaMask/eth-ledger-bridge-keyring/blob/main/src/ledger-iframe-bridge.ts#L132-L138

Its mostly expected, I will add an else if error with a catch for if neither success or error is defined.

replicated the else catch for a missing error

davidmurdoch · 2023-12-19T16:07:14Z

offscreen/scripts/ledger-iframe.ts

+      messageId,
+    };
+
+    // It has already been checked that they are not null above, so the


It was checked, but this code is still run since the if(!iframe?.contentWindow) block doesn't short circuit. Should it return false?

That's a really good catch @davidmurdoch :D

Thanks for that, it now returns early in this case

weizman · 2023-12-19T16:33:31Z

offscreen/scripts/ledger-iframe.ts

+    // It has already been checked that they are not null above, so the
+    // optional chaining here is for compiler typechecking only. This avoids
+    // overriding our non-null assertion rule.
+    iframe?.contentWindow?.postMessage(iframeMsg, '*');


Suggested change

iframe?.contentWindow?.postMessage(iframeMsg, '*');

iframe?.contentWindow?.postMessage(iframeMsg, LEDGER_FRAME_ORIGIN_URL);

IMPORTANT ☝️

(otherwise, external potentially breached realms can redirect the location of this iframe to a malicious one and still receive the message)

weizman · 2023-12-19T16:37:58Z

offscreen/scripts/ledger-iframe.ts

+window.addEventListener('message', ({ origin, data }) => {
+  if (origin !== LEDGER_FRAME_ORIGIN_URL) {


Suggested change

window.addEventListener('message', ({ origin, data }) => {

if (origin !== LEDGER_FRAME_ORIGIN_URL) {

window.addEventListener('message', ({ origin, data, source }) => {

if (origin !== LEDGER_FRAME_ORIGIN_URL || source !== iframe?.contentWindow) {

Would be a great touch (for that to work, const iframe = document.querySelector('iframe') should be moved to the upper scope, but that shouldn't cause too much trouble)

weizman · 2023-12-19T16:41:25Z

offscreen/offscreen.html

    <iframe src="./trezor-iframe.html"></iframe>
+    <iframe src="./ledger-iframe.html"></iframe>


Would this security hardening work?

Suggested change

<iframe src="./trezor-iframe.html"></iframe>

<iframe src="./ledger-iframe.html"></iframe>

<iframe sandbox="" src="./trezor-iframe.html"></iframe>

<iframe sandbox="" src="./ledger-iframe.html"></iframe>

Or does it clash with the rest of the logic introduced in this PR? Could be a nice touch. Weird, because the origin of the content of the iframe is at a similar trust level of the code that loads it, but if such isolation costs us nothing, it could only add security without giving up on anything.

adding this property caused two things: Before the iframe would show up in the extension manifest in the extension manager, and it received communication from the offscreen document. After setting it both those things stopped working. For those reasons I did not add this hardening to the PR

Fair enough, that was a long shot - thanks for trying!
I'm not too worried about that honestly, would have been nice but far from required

danjm

looks good to me overall

left one nit about a stray console log, and I left one question

and of course Gal's comments need to be addressed. Also, we should check whether any of Gal's comments apply to the previously merged trezor PR as well.

weizman

8c21b93 lgtm (iframe related security side of things)

danjm

Great work!

metamaskbot · 2023-12-21T16:44:45Z

Builds ready [d73f19e]

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
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1348 ± 160 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	92	402	192	86	41
		domContentLoaded	9	201	77	73	35
		load	816	2015	1348	334	160
		domInteractive	9	201	77	73	35

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2.37 KiB (0.07%)
ui: -500 Bytes (-0.01%)
common: 187 Bytes (0.00%)

feat(mv3): Ledger MV3 support

fe2cc8b

brad-decker added the team-extension-platform Extension Platform team label Dec 13, 2023

brad-decker added 3 commits December 13, 2023 12:55

restore u2f on firefox

057385e

fixup

3a57a83

remove console.log

3761a1d

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Dec 13, 2023

brad-decker marked this pull request as ready for review December 14, 2023 17:02

brad-decker requested review from a team and kumavis as code owners December 14, 2023 17:02

brad-decker removed the INVALID-PR-TEMPLATE PR's body doesn't match template label Dec 14, 2023

Merge branch 'develop' into feat/ledger-mv3-support

0e09033

metamaskbot added INVALID-PR-TEMPLATE PR's body doesn't match template and removed INVALID-PR-TEMPLATE PR's body doesn't match template labels Dec 14, 2023

brad-decker added 4 commits December 15, 2023 08:03

Merge branch 'develop' into feat/ledger-mv3-support

be96d7e

Merge branch 'develop' into feat/ledger-mv3-support

63d724e

Merge branch 'develop' into feat/ledger-mv3-support

d828591

Merge branch 'develop' into feat/ledger-mv3-support

247ff78

danjm added the team-extension-client label Dec 18, 2023

metamaskbot added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed team-extension-client labels Dec 18, 2023

pedronfigueiredo previously approved these changes Dec 18, 2023

View reviewed changes

Merge branch 'develop' into feat/ledger-mv3-support

359627f

danjm reviewed Dec 19, 2023

View reviewed changes

davidmurdoch reviewed Dec 19, 2023

View reviewed changes

weizman reviewed Dec 19, 2023

View reviewed changes

danjm suggested changes Dec 19, 2023

View reviewed changes

security hardening and fixes

8c21b93

brad-decker dismissed pedronfigueiredo’s stale review via 8c21b93 December 19, 2023 20:39

Merge branch 'develop' into feat/ledger-mv3-support

eed8495

weizman approved these changes Dec 19, 2023

View reviewed changes

Merge branch 'develop' into feat/ledger-mv3-support

d73f19e

danjm approved these changes Dec 21, 2023

View reviewed changes

brad-decker merged commit 4fb8560 into develop Dec 21, 2023

brad-decker deleted the feat/ledger-mv3-support branch December 21, 2023 16:44

github-actions bot locked and limited conversation to collaborators Dec 21, 2023

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

metamaskbot added the release-11.9.0 Issue or pull request that will be included in release 11.9.0 label Dec 21, 2023

	iframe?.contentWindow?.postMessage(iframeMsg, '*');
	iframe?.contentWindow?.postMessage(iframeMsg, LEDGER_FRAME_ORIGIN_URL);

		window.addEventListener('message', ({ origin, data }) => {
		if (origin !== LEDGER_FRAME_ORIGIN_URL) {

		<iframe src="./trezor-iframe.html"></iframe>
		<iframe src="./ledger-iframe.html"></iframe>

Conversation

brad-decker commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Screenshots/Recordings

Manual testing steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

codecov bot commented Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Dec 14, 2023

Uh oh!

metamaskbot commented Dec 17, 2023

Uh oh!

pedronfigueiredo commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedronfigueiredo commented Dec 18, 2023

Uh oh!

metamaskbot commented Dec 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danjm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weizman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Dec 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

brad-decker commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading

pedronfigueiredo commented Dec 18, 2023 •

edited

Loading

danjm left a comment •

edited

Loading

weizman left a comment •

edited

Loading