From d749d95dbdd57e59a2243450cedb55a7b5f3daa3 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 24 Jul 2024 12:42:13 +0200 Subject: [PATCH 1/4] fix: await for iframe load on init --- src/ledger-iframe-bridge.test.ts | 45 ++++++++++++++++++++++-------- src/ledger-iframe-bridge.ts | 48 +++++++++----------------------- 2 files changed, 47 insertions(+), 46 deletions(-) diff --git a/src/ledger-iframe-bridge.test.ts b/src/ledger-iframe-bridge.test.ts index 9c12b628..9c68f002 100644 --- a/src/ledger-iframe-bridge.test.ts +++ b/src/ledger-iframe-bridge.test.ts @@ -50,6 +50,19 @@ async function simulateIFrameLoad(iframe?: HTMLIFrameElementShim) { return await iframe.onload(); } +/** + * Initializes the bridge with a simulated iframe. + * + * @param bridge - The bridge to initialize. + */ +async function initializeBridge(bridge: LedgerIframeBridge) { + // The promise returned by the init method is resolved when the iframe + // is loaded, but as we are using a shim, we need to resolve it manually. + const initHandle = bridge.init(); + await simulateIFrameLoad(bridge.iframe); + return initHandle; +} + const LEDGER_IFRAME_ID = 'LEDGER-IFRAME'; const BRIDGE_URL = 'https://metamask.github.io/eth-ledger-bridge-keyring'; const INVALID_URL_ERROR = 'bridgeURL is not a valid URL'; @@ -79,8 +92,7 @@ describe('LedgerIframeBridge', function () { bridge = new LedgerIframeBridge({ bridgeUrl: BRIDGE_URL, }); - await bridge.init(); - await simulateIFrameLoad(bridge.iframe); + await initializeBridge(bridge); }); afterEach(function () { @@ -122,16 +134,14 @@ describe('LedgerIframeBridge', function () { describe('init', function () { it('sets up the listener and iframe', async function () { - bridge = new LedgerIframeBridge(); - const addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); + bridge = new LedgerIframeBridge(); - await bridge.init(); + // `initializeBridge` is used to call `init` while simulating the iframe loading + // in a way that does not cause the test to hang. + await initializeBridge(bridge); expect(addEventListenerSpy).toHaveBeenCalledTimes(1); - expect(bridge.iframeLoaded).toBe(false); - - await simulateIFrameLoad(bridge.iframe); expect(bridge.iframeLoaded).toBe(true); }); }); @@ -227,6 +237,14 @@ describe('LedgerIframeBridge', function () { expect(bridge.iframe?.contentWindow?.postMessage).toHaveBeenCalled(); }); + it('throws an error when the bridge is not initialized', async function () { + bridge = new LedgerIframeBridge(); + + await expect(bridge.updateTransportMethod('u2f')).rejects.toThrow( + 'The iframe is not loaded yet', + ); + }); + it('throws an error when a ledger-update-transport message is not successful', async function () { bridge.iframeLoaded = true; @@ -526,14 +544,19 @@ describe('LedgerIframeBridge', function () { removeEventListenerSpy = jest.spyOn(global.window, 'removeEventListener'); addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); bridge = new LedgerIframeBridge(); - await bridge.init(); - await simulateIFrameLoad(bridge.iframe); + await initializeBridge(bridge); }); describe('when configurate bridge url', function () { describe('when given bridge url is different with current', function () { beforeEach(async () => { - await bridge.setOptions({ bridgeUrl: 'https://metamask.io' }); + // since the bridge will be re-initialized, we need to + // simulate the iframe loading again + const setOptionHandle = bridge.setOptions({ + bridgeUrl: 'https://metamask.io', + }); + await simulateIFrameLoad(bridge.iframe); + await setOptionHandle; }); it('should set bridgeUrl correctly', async function () { diff --git a/src/ledger-iframe-bridge.ts b/src/ledger-iframe-bridge.ts index f87aca5c..a73987e9 100644 --- a/src/ledger-iframe-bridge.ts +++ b/src/ledger-iframe-bridge.ts @@ -110,12 +110,6 @@ export class LedgerIframeBridge messageCallbacks: Record void> = {}; - delayedPromise?: { - resolve: (value: boolean) => void; - reject: (error: unknown) => void; - transportType: string; - }; - constructor( opts: LedgerIframeBridgeOptions = { bridgeUrl: 'https://metamask.github.io/eth-ledger-bridge-keyring', @@ -128,7 +122,7 @@ export class LedgerIframeBridge } async init() { - this.#setupIframe(this.#opts.bridgeUrl); + await this.#setupIframe(this.#opts.bridgeUrl); this.eventListener = this.#eventListener.bind(this, this.#opts.bridgeUrl); @@ -178,12 +172,7 @@ export class LedgerIframeBridge // If the iframe isn't loaded yet, let's store the desired transportType value and // optimistically return a successful promise if (!this.iframeLoaded) { - this.delayedPromise = { - resolve, - reject, - transportType, - }; - return; + throw new Error('The iframe is not loaded yet'); } this.#sendMessage( @@ -282,28 +271,17 @@ export class LedgerIframeBridge }); } - #setupIframe(bridgeUrl: string) { - this.iframe = document.createElement('iframe'); - this.iframe.src = bridgeUrl; - this.iframe.allow = `hid 'src'`; - this.iframe.onload = async () => { - // If the ledger live preference was set before the iframe is loaded, - // set it after the iframe has loaded - this.iframeLoaded = true; - if (this.delayedPromise) { - try { - const result = await this.updateTransportMethod( - this.delayedPromise.transportType, - ); - this.delayedPromise.resolve(result); - } catch (error) { - this.delayedPromise.reject(error); - } finally { - delete this.delayedPromise; - } - } - }; - document.head.appendChild(this.iframe); + async #setupIframe(bridgeUrl: string): Promise { + return new Promise((resolve) => { + this.iframe = document.createElement('iframe'); + this.iframe.src = bridgeUrl; + this.iframe.allow = `hid 'src'`; + this.iframe.onload = async () => { + this.iframeLoaded = true; + resolve(); + }; + document.head.appendChild(this.iframe); + }); } #getOrigin(bridgeUrl: string) { From 2f538976a7dfd521bd41f71140b1d4f06e2f9c04 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 25 Jul 2024 13:43:48 +0200 Subject: [PATCH 2/4] refactor: `documentShim` automatically calls iframe onload --- src/ledger-iframe-bridge.test.ts | 43 +++----------------------------- test/document.shim.ts | 41 +++++++++++++----------------- 2 files changed, 21 insertions(+), 63 deletions(-) diff --git a/src/ledger-iframe-bridge.test.ts b/src/ledger-iframe-bridge.test.ts index 9c68f002..5d96f6e0 100644 --- a/src/ledger-iframe-bridge.test.ts +++ b/src/ledger-iframe-bridge.test.ts @@ -36,33 +36,6 @@ function isIFrameValid( ); } -/** - * Simulates the loading of an iframe by calling the onload function. - * - * @param iframe - The iframe to simulate the loading of. - * @returns Returns a promise that resolves when the onload function is called. - */ -async function simulateIFrameLoad(iframe?: HTMLIFrameElementShim) { - if (!isIFrameValid(iframe)) { - throw new Error('the iframe is not valid'); - } - // we call manually the onload event to simulate the iframe loading - return await iframe.onload(); -} - -/** - * Initializes the bridge with a simulated iframe. - * - * @param bridge - The bridge to initialize. - */ -async function initializeBridge(bridge: LedgerIframeBridge) { - // The promise returned by the init method is resolved when the iframe - // is loaded, but as we are using a shim, we need to resolve it manually. - const initHandle = bridge.init(); - await simulateIFrameLoad(bridge.iframe); - return initHandle; -} - const LEDGER_IFRAME_ID = 'LEDGER-IFRAME'; const BRIDGE_URL = 'https://metamask.github.io/eth-ledger-bridge-keyring'; const INVALID_URL_ERROR = 'bridgeURL is not a valid URL'; @@ -92,7 +65,7 @@ describe('LedgerIframeBridge', function () { bridge = new LedgerIframeBridge({ bridgeUrl: BRIDGE_URL, }); - await initializeBridge(bridge); + await bridge.init(); }); afterEach(function () { @@ -137,9 +110,7 @@ describe('LedgerIframeBridge', function () { const addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); bridge = new LedgerIframeBridge(); - // `initializeBridge` is used to call `init` while simulating the iframe loading - // in a way that does not cause the test to hang. - await initializeBridge(bridge); + await bridge.init(); expect(addEventListenerSpy).toHaveBeenCalledTimes(1); expect(bridge.iframeLoaded).toBe(true); @@ -210,8 +181,6 @@ describe('LedgerIframeBridge', function () { describe('updateTransportMethod', function () { it('sends and processes a successful ledger-update-transport message', async function () { - bridge.iframeLoaded = true; - const transportType = 'u2f'; stubKeyringIFramePostMessage(bridge, (message) => { @@ -544,19 +513,15 @@ describe('LedgerIframeBridge', function () { removeEventListenerSpy = jest.spyOn(global.window, 'removeEventListener'); addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); bridge = new LedgerIframeBridge(); - await initializeBridge(bridge); + await bridge.init(); }); describe('when configurate bridge url', function () { describe('when given bridge url is different with current', function () { beforeEach(async () => { - // since the bridge will be re-initialized, we need to - // simulate the iframe loading again - const setOptionHandle = bridge.setOptions({ + await bridge.setOptions({ bridgeUrl: 'https://metamask.io', }); - await simulateIFrameLoad(bridge.iframe); - await setOptionHandle; }); it('should set bridgeUrl correctly', async function () { diff --git a/test/document.shim.ts b/test/document.shim.ts index ceba08c3..42c4983a 100644 --- a/test/document.shim.ts +++ b/test/document.shim.ts @@ -1,32 +1,25 @@ // eslint-disable-next-line import/no-mutable-exports let documentShim: any; -try { - documentShim = document || { - head: { - appendChild: () => false, +const shim = { + head: { + appendChild: (child: { onload?: () => void }) => { + child.onload?.(); }, - createElement: () => ({ - src: false, - contentWindow: { - postMessage: () => false, - }, - }), - addEventListener: () => false, - }; -} catch (error) { - documentShim = { - head: { - appendChild: () => false, + }, + createElement: () => ({ + src: false, + contentWindow: { + postMessage: () => false, }, - createElement: () => ({ - src: false, - contentWindow: { - postMessage: () => false, - }, - }), - addEventListener: () => false, - }; + }), + addEventListener: () => false, +}; + +try { + documentShim = document || shim; +} catch (error) { + documentShim = shim; } export default documentShim; From 9c33d8d776c84a8617622a52a6c21d4d650c15d7 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 25 Jul 2024 13:53:49 +0200 Subject: [PATCH 3/4] chore: bump coverage threshold --- jest.config.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jest.config.js b/jest.config.js index 36153ef4..03f9794e 100644 --- a/jest.config.js +++ b/jest.config.js @@ -41,10 +41,10 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 89.28, - functions: 95.91, - lines: 91.32, - statements: 91.41, + branches: 90.64, + functions: 95.95, + lines: 93.02, + statements: 93.09, }, }, From 2f3362b5d6cd28c010317bcae33f10d075780d04 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Thu, 25 Jul 2024 14:01:11 +0200 Subject: [PATCH 4/4] refactor: roll back unneeded changes --- src/ledger-iframe-bridge.test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/ledger-iframe-bridge.test.ts b/src/ledger-iframe-bridge.test.ts index 5d96f6e0..877da92f 100644 --- a/src/ledger-iframe-bridge.test.ts +++ b/src/ledger-iframe-bridge.test.ts @@ -107,8 +107,8 @@ describe('LedgerIframeBridge', function () { describe('init', function () { it('sets up the listener and iframe', async function () { - const addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); bridge = new LedgerIframeBridge(); + const addEventListenerSpy = jest.spyOn(global.window, 'addEventListener'); await bridge.init(); @@ -519,9 +519,7 @@ describe('LedgerIframeBridge', function () { describe('when configurate bridge url', function () { describe('when given bridge url is different with current', function () { beforeEach(async () => { - await bridge.setOptions({ - bridgeUrl: 'https://metamask.io', - }); + await bridge.setOptions({ bridgeUrl: 'https://metamask.io' }); }); it('should set bridgeUrl correctly', async function () {