From 7d50c39d6c6e18c9112a4a7e1075b323d4547a78 Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Mon, 2 Sep 2024 13:55:45 +0200 Subject: [PATCH] fix: wait for ledger offscreen iframe load (#26225) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** In order to instantiate a functioning communication between the offscreen iframe for Ledger and the LedgerKeyring (through `LedgerOffscreenBridge`), we need to make sure that the iframe is loaded before sending any message to it. We currently wait for the offscreen page to load, but the iframe load is completely async and it will most likely be ready after the rest of the offscreen page, leaving messages proxied to the iframe hanging forever. On a higher level, this is dangerous because everytime we try to send a message to the Ledger iframe the `KeyringController` controller-level mutex is locked, and any other operation will wait for its release to proceed - this creates a deadlock situation in the case the iframe does not respond to a message. This PR makes Ledger iframe initialization into a Promise, and the offscreen page will wait for it to be resolved before sending the "ready" signal back to the extension. To avoid waiting forever, the Ledger initialisation promise races with a 5s timeout: in case of timeout, interactions with Ledger accounts will not work during the entire session (until the offscreen page is re-initialised, and another attempt is made to init Ledger). Note that the UI setup is not affected by this change, since the Offscreen initialisation is awaited only when unlocking the wallet. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26225?quickstart=1) ## **Related issues** Progresses: #26840 ## **Manual testing steps** N/A ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [ ] 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). - [ ] 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. --- app/scripts/offscreen.js | 7 +- offscreen/scripts/ledger.ts | 28 +++++--- offscreen/scripts/offscreen.ts | 78 ++++++++++++++------- shared/constants/offscreen-communication.ts | 3 + 4 files changed, 76 insertions(+), 40 deletions(-) diff --git a/app/scripts/offscreen.js b/app/scripts/offscreen.js index 159a2c8a5773..f09fcbcf0f58 100644 --- a/app/scripts/offscreen.js +++ b/app/scripts/offscreen.js @@ -1,5 +1,8 @@ import { captureException } from '@sentry/browser'; -import { OffscreenCommunicationTarget } from '../../shared/constants/offscreen-communication'; +import { + OFFSCREEN_LOAD_TIMEOUT, + OffscreenCommunicationTarget, +} from '../../shared/constants/offscreen-communication'; import { getSocketBackgroundToMocha } from '../../test/e2e/background-socket/socket-background-to-mocha'; /** @@ -65,7 +68,7 @@ export async function createOffscreen() { // In case we are in a bad state where the offscreen document is not loading, timeout and let execution continue. const timeoutPromise = new Promise((resolve) => { - setTimeout(resolve, 5000); + setTimeout(resolve, OFFSCREEN_LOAD_TIMEOUT); }); await Promise.race([loadPromise, timeoutPromise]); diff --git a/offscreen/scripts/ledger.ts b/offscreen/scripts/ledger.ts index 9fe1f4f10c37..c08fdc6ac85a 100644 --- a/offscreen/scripts/ledger.ts +++ b/offscreen/scripts/ledger.ts @@ -16,15 +16,11 @@ const LEDGER_KEYRING_IFRAME_CONNECTED_EVENT = 'ledger-connection-event'; const callbackProcessor = new CallbackProcessor(); -export default function init() { - const iframe = document.createElement('iframe'); - iframe.src = 'https://metamask.github.io/eth-ledger-bridge-keyring'; - iframe.allow = 'hid'; - document.body.appendChild(iframe); +function setupMessageListeners(iframe: HTMLIFrameElement) { // This listener receives action responses from the live ledger iframe // Then forwards the response to the offscreen bridge window.addEventListener('message', ({ origin, data, source }) => { - if (origin !== KnownOrigins.ledger || source !== iframe?.contentWindow) { + if (origin !== KnownOrigins.ledger || source !== iframe.contentWindow) { return; } @@ -64,7 +60,7 @@ export default function init() { return; } - if (!iframe?.contentWindow) { + if (!iframe.contentWindow) { const error = new Error('Ledger iframe not present'); sendResponse({ success: false, @@ -87,10 +83,7 @@ export default function init() { messageId, }; - // 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, KnownOrigins.ledger); + iframe.contentWindow.postMessage(iframeMsg, KnownOrigins.ledger); // This keeps sendResponse function valid after return // https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/onMessage @@ -99,3 +92,16 @@ export default function init() { }, ); } + +export default async function init() { + return new Promise((resolve) => { + const iframe = document.createElement('iframe'); + iframe.src = 'https://metamask.github.io/eth-ledger-bridge-keyring'; + iframe.allow = 'hid'; + iframe.onload = () => { + setupMessageListeners(iframe); + resolve(); + }; + document.body.appendChild(iframe); + }); +} diff --git a/offscreen/scripts/offscreen.ts b/offscreen/scripts/offscreen.ts index 9f611333b581..d1651eb6af08 100644 --- a/offscreen/scripts/offscreen.ts +++ b/offscreen/scripts/offscreen.ts @@ -1,6 +1,8 @@ import { BrowserRuntimePostMessageStream } from '@metamask/post-message-stream'; import { ProxySnapExecutor } from '@metamask/snaps-execution-environments'; +import { isObject } from '@metamask/utils'; import { + OFFSCREEN_LEDGER_INIT_TIMEOUT, OffscreenCommunicationEvents, OffscreenCommunicationTarget, } from '../../shared/constants/offscreen-communication'; @@ -9,41 +11,63 @@ import initLedger from './ledger'; import initTrezor from './trezor'; import initLattice from './lattice'; -initLedger(); -initTrezor(); -initLattice(); - /** * Initialize a post message stream with the parent window that is initialized * in the metamask-controller (background/serivce worker) process. This will be * utilized by snaps for communication with snaps running in the offscreen * document. */ -const parentStream = new BrowserRuntimePostMessageStream({ - name: 'child', - target: 'parent', -}); - -ProxySnapExecutor.initialize(parentStream, './snaps/index.html'); - -if (process.env.IN_TEST) { - chrome.runtime.onMessage.addListener((message) => { - if ( - message && - typeof message === 'object' && - message.event === OffscreenCommunicationEvents.metamaskBackgroundReady && - message.target === OffscreenCommunicationTarget.extension - ) { - window.document?.documentElement?.classList?.add('controller-loaded'); - } +function initializePostMessageStream() { + const parentStream = new BrowserRuntimePostMessageStream({ + name: 'child', + target: 'parent', }); + + ProxySnapExecutor.initialize(parentStream, './snaps/index.html'); } -chrome.runtime.sendMessage({ - target: OffscreenCommunicationTarget.extensionMain, - isBooted: true, +/** + * Initialize the ledger, trezor, and lattice keyring connections, and the + * post message stream for the Snaps environment. + */ +async function init(): Promise { + initializePostMessageStream(); + initTrezor(); + initLattice(); - // This message is being sent from the Offscreen Document to the Service Worker. - // The Service Worker has no way to query `navigator.webdriver`, so we send it here. - webdriverPresent: navigator.webdriver === true, + try { + const ledgerInitTimeout = new Promise((_, reject) => { + setTimeout(() => { + reject(new Error('Ledger initialization timed out')); + }, OFFSCREEN_LEDGER_INIT_TIMEOUT); + }); + await Promise.race([initLedger(), ledgerInitTimeout]); + } catch (error) { + console.error('Ledger initialization failed:', error); + } +} + +init().then(() => { + if (process.env.IN_TEST) { + chrome.runtime.onMessage.addListener((message) => { + if ( + message && + isObject(message) && + message.event === + OffscreenCommunicationEvents.metamaskBackgroundReady && + message.target === OffscreenCommunicationTarget.extension + ) { + window.document?.documentElement?.classList?.add('controller-loaded'); + } + }); + } + + chrome.runtime.sendMessage({ + target: OffscreenCommunicationTarget.extensionMain, + isBooted: true, + + // This message is being sent from the Offscreen Document to the Service Worker. + // The Service Worker has no way to query `navigator.webdriver`, so we send it here. + webdriverPresent: navigator.webdriver === true, + }); }); diff --git a/shared/constants/offscreen-communication.ts b/shared/constants/offscreen-communication.ts index 77c657943842..881a02560b2d 100644 --- a/shared/constants/offscreen-communication.ts +++ b/shared/constants/offscreen-communication.ts @@ -1,3 +1,6 @@ +export const OFFSCREEN_LEDGER_INIT_TIMEOUT = 4000; +export const OFFSCREEN_LOAD_TIMEOUT = OFFSCREEN_LEDGER_INIT_TIMEOUT + 1000; + /** * Defines legal targets for offscreen communication. These values are used to * filter and route messages to the correct target.