From 8e053b58ca4dac2dcaba73c772096de92d13b69a Mon Sep 17 00:00:00 2001 From: augmentedmode Date: Wed, 9 Oct 2024 14:53:57 -0600 Subject: [PATCH 01/10] migrate: remove old phishfort list from clients --- app/scripts/migrations/126.1.test.ts | 129 +++++++++++++++++++++++++++ app/scripts/migrations/126.1.ts | 51 +++++++++++ 2 files changed, 180 insertions(+) create mode 100644 app/scripts/migrations/126.1.test.ts create mode 100644 app/scripts/migrations/126.1.ts diff --git a/app/scripts/migrations/126.1.test.ts b/app/scripts/migrations/126.1.test.ts new file mode 100644 index 000000000000..cb92811fcb77 --- /dev/null +++ b/app/scripts/migrations/126.1.test.ts @@ -0,0 +1,129 @@ +import { List } from '@material-ui/core'; +import { migrate, version } from './126.1'; +import { + PhishingControllerState, + PhishingListState, + ListNames, +} from '@metamask/phishing-controller'; + +const oldVersion = 126.1; + +const mockPhishingListMetaMask: PhishingListState = { + allowlist: [], + blocklist: ['malicious1.com'], + c2DomainBlocklist: ['malicious2.com'], + fuzzylist: [], + tolerance: 0, + version: 1, + lastUpdated: Date.now(), + name: ListNames.MetaMask, +}; + +const mockPhishingListPhishfort: PhishingListState = { + allowlist: [], + blocklist: ['phishfort1.com'], + c2DomainBlocklist: ['phishfort2.com'], + fuzzylist: [], + tolerance: 0, + version: 1, + lastUpdated: Date.now(), + name: 'Phishfort' as ListNames, +}; + +describe(`migration #${version}`, () => { + it('updates the version metadata', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: {}, + }; + + const newStorage = await migrate(oldStorage); + + expect(newStorage.meta).toStrictEqual({ version }); + }); + + it('keeps only the MetaMask phishing list in PhishingControllerState', async () => { + const oldState = { + PhishingController: { + phishingLists: [mockPhishingListMetaMask, mockPhishingListPhishfort], + whitelist: [], + hotlistLastFetched: 0, + stalelistLastFetched: 0, + c2DomainBlocklistLastFetched: 0, + } as PhishingControllerState, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: oldState, + }); + + const updatedPhishingController = transformedState.data + .PhishingController as PhishingControllerState; + + expect(updatedPhishingController.phishingLists).toStrictEqual([ + mockPhishingListMetaMask, + ]); + }); + + it('removes all phishing lists if MetaMask is not present', async () => { + const oldState = { + PhishingController: { + phishingLists: [mockPhishingListPhishfort], + whitelist: [], + hotlistLastFetched: 0, + stalelistLastFetched: 0, + c2DomainBlocklistLastFetched: 0, + } as PhishingControllerState, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: oldState, + }); + + const updatedPhishingController = transformedState.data + .PhishingController as PhishingControllerState; + + expect(updatedPhishingController.phishingLists).toStrictEqual([]); + }); + + it('does nothing if PhishingControllerState is empty', async () => { + const oldState = { + PhishingController: { + phishingLists: [], + whitelist: [], + hotlistLastFetched: 0, + stalelistLastFetched: 0, + c2DomainBlocklistLastFetched: 0, + } as PhishingControllerState, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: oldState, + }); + + const updatedPhishingController = transformedState.data + .PhishingController as PhishingControllerState; + + expect(updatedPhishingController.phishingLists).toStrictEqual([]); + }); + + it('does nothing if PhishingController is not in the state', async () => { + const oldState = { + NetworkController: { + providerConfig: { + chainId: '0x1', + }, + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: oldState, + }); + + expect(transformedState.data).toStrictEqual(oldState); + }); +}); diff --git a/app/scripts/migrations/126.1.ts b/app/scripts/migrations/126.1.ts new file mode 100644 index 000000000000..c40471c15e8b --- /dev/null +++ b/app/scripts/migrations/126.1.ts @@ -0,0 +1,51 @@ +import { hasProperty, isObject } from '@metamask/utils'; +import { cloneDeep } from 'lodash'; +import { + PhishingControllerState, + ListNames, +} from '@metamask/phishing-controller'; + +type VersionedData = { + meta: { version: number }; + data: Record; +}; + +export const version = 126.1; + +/** + * This migration removes `providerConfig` from the network controller state. + * + * @param originalVersionedData - Versioned MetaMask extension state, exactly what we persist to dist. + * @param originalVersionedData.meta - State metadata. + * @param originalVersionedData.meta.version - The current state version. + * @param originalVersionedData.data - The persisted MetaMask state, keyed by controller. + * @returns Updated versioned MetaMask extension state. + */ +export async function migrate( + originalVersionedData: VersionedData, +): Promise { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + transformState(versionedData.data); + return versionedData; +} + +function transformState( + state: Record, +): Record { + if ( + hasProperty(state, 'PhishingController') && + isObject(state.PhishingController) + ) { + const phishingController = + state.PhishingController as PhishingControllerState; + + phishingController.phishingLists = phishingController.phishingLists.filter( + (list) => list.name === 'MetaMask', + ); + + state.PhishingController = phishingController; + } + + return state; +} From 6928c792ecadf740cec44ec4599bc715eeb3a5d1 Mon Sep 17 00:00:00 2001 From: augmentedmode Date: Wed, 9 Oct 2024 14:55:44 -0600 Subject: [PATCH 02/10] fix: type of listnames metamask --- app/scripts/migrations/126.1.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/migrations/126.1.ts b/app/scripts/migrations/126.1.ts index c40471c15e8b..45546e0eecea 100644 --- a/app/scripts/migrations/126.1.ts +++ b/app/scripts/migrations/126.1.ts @@ -41,7 +41,7 @@ function transformState( state.PhishingController as PhishingControllerState; phishingController.phishingLists = phishingController.phishingLists.filter( - (list) => list.name === 'MetaMask', + (list) => list.name === ListNames.MetaMask, ); state.PhishingController = phishingController; From 4556b1fe42ecc165085ad991797b3dfd4e5361a1 Mon Sep 17 00:00:00 2001 From: augmentedmode Date: Wed, 9 Oct 2024 15:04:42 -0600 Subject: [PATCH 03/10] feat: add extra validations --- app/scripts/migrations/126.1.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/scripts/migrations/126.1.ts b/app/scripts/migrations/126.1.ts index 45546e0eecea..2279904efe4b 100644 --- a/app/scripts/migrations/126.1.ts +++ b/app/scripts/migrations/126.1.ts @@ -35,7 +35,9 @@ function transformState( ): Record { if ( hasProperty(state, 'PhishingController') && - isObject(state.PhishingController) + isObject(state.PhishingController) && + hasProperty(state.PhishingController, 'phishingLists') && + Array.isArray(state.PhishingController.phishingLists) ) { const phishingController = state.PhishingController as PhishingControllerState; From 579f74935daacb728287b47f3bfab80f16f2dc01 Mon Sep 17 00:00:00 2001 From: AugmentedMode <31675118+AugmentedMode@users.noreply.github.com> Date: Wed, 9 Oct 2024 17:17:10 -0400 Subject: [PATCH 04/10] Update app/scripts/migrations/126.1.ts Co-authored-by: Mark Stacey --- app/scripts/migrations/126.1.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/scripts/migrations/126.1.ts b/app/scripts/migrations/126.1.ts index 2279904efe4b..7eac80469c66 100644 --- a/app/scripts/migrations/126.1.ts +++ b/app/scripts/migrations/126.1.ts @@ -36,11 +36,14 @@ function transformState( if ( hasProperty(state, 'PhishingController') && isObject(state.PhishingController) && - hasProperty(state.PhishingController, 'phishingLists') && - Array.isArray(state.PhishingController.phishingLists) + hasProperty(state.PhishingController, 'phishingLists') ) { const phishingController = state.PhishingController as PhishingControllerState; + + if (!Array.isArray(phishingController.phishingLists)) { + return state; + } phishingController.phishingLists = phishingController.phishingLists.filter( (list) => list.name === ListNames.MetaMask, From e9ecd63d273c7da26919e1ef01c3eb0ef642b949 Mon Sep 17 00:00:00 2001 From: augmentedmode Date: Wed, 9 Oct 2024 15:23:41 -0600 Subject: [PATCH 05/10] fix: remove types from controller --- app/scripts/migrations/126.1.test.ts | 26 ++++++++++---------------- app/scripts/migrations/126.1.ts | 11 +++-------- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/app/scripts/migrations/126.1.test.ts b/app/scripts/migrations/126.1.test.ts index cb92811fcb77..5aa9a43db8eb 100644 --- a/app/scripts/migrations/126.1.test.ts +++ b/app/scripts/migrations/126.1.test.ts @@ -1,14 +1,8 @@ -import { List } from '@material-ui/core'; import { migrate, version } from './126.1'; -import { - PhishingControllerState, - PhishingListState, - ListNames, -} from '@metamask/phishing-controller'; const oldVersion = 126.1; -const mockPhishingListMetaMask: PhishingListState = { +const mockPhishingListMetaMask = { allowlist: [], blocklist: ['malicious1.com'], c2DomainBlocklist: ['malicious2.com'], @@ -16,10 +10,10 @@ const mockPhishingListMetaMask: PhishingListState = { tolerance: 0, version: 1, lastUpdated: Date.now(), - name: ListNames.MetaMask, + name: 'MetaMask', }; -const mockPhishingListPhishfort: PhishingListState = { +const mockPhishingListPhishfort = { allowlist: [], blocklist: ['phishfort1.com'], c2DomainBlocklist: ['phishfort2.com'], @@ -27,7 +21,7 @@ const mockPhishingListPhishfort: PhishingListState = { tolerance: 0, version: 1, lastUpdated: Date.now(), - name: 'Phishfort' as ListNames, + name: 'Phishfort', }; describe(`migration #${version}`, () => { @@ -50,7 +44,7 @@ describe(`migration #${version}`, () => { hotlistLastFetched: 0, stalelistLastFetched: 0, c2DomainBlocklistLastFetched: 0, - } as PhishingControllerState, + }, }; const transformedState = await migrate({ @@ -59,7 +53,7 @@ describe(`migration #${version}`, () => { }); const updatedPhishingController = transformedState.data - .PhishingController as PhishingControllerState; + .PhishingController as Record; expect(updatedPhishingController.phishingLists).toStrictEqual([ mockPhishingListMetaMask, @@ -74,7 +68,7 @@ describe(`migration #${version}`, () => { hotlistLastFetched: 0, stalelistLastFetched: 0, c2DomainBlocklistLastFetched: 0, - } as PhishingControllerState, + }, }; const transformedState = await migrate({ @@ -83,7 +77,7 @@ describe(`migration #${version}`, () => { }); const updatedPhishingController = transformedState.data - .PhishingController as PhishingControllerState; + .PhishingController as Record; expect(updatedPhishingController.phishingLists).toStrictEqual([]); }); @@ -96,7 +90,7 @@ describe(`migration #${version}`, () => { hotlistLastFetched: 0, stalelistLastFetched: 0, c2DomainBlocklistLastFetched: 0, - } as PhishingControllerState, + }, }; const transformedState = await migrate({ @@ -105,7 +99,7 @@ describe(`migration #${version}`, () => { }); const updatedPhishingController = transformedState.data - .PhishingController as PhishingControllerState; + .PhishingController as Record; expect(updatedPhishingController.phishingLists).toStrictEqual([]); }); diff --git a/app/scripts/migrations/126.1.ts b/app/scripts/migrations/126.1.ts index 7eac80469c66..0b6e811c8465 100644 --- a/app/scripts/migrations/126.1.ts +++ b/app/scripts/migrations/126.1.ts @@ -1,9 +1,5 @@ import { hasProperty, isObject } from '@metamask/utils'; import { cloneDeep } from 'lodash'; -import { - PhishingControllerState, - ListNames, -} from '@metamask/phishing-controller'; type VersionedData = { meta: { version: number }; @@ -38,15 +34,14 @@ function transformState( isObject(state.PhishingController) && hasProperty(state.PhishingController, 'phishingLists') ) { - const phishingController = - state.PhishingController as PhishingControllerState; - + const phishingController = state.PhishingController; + if (!Array.isArray(phishingController.phishingLists)) { return state; } phishingController.phishingLists = phishingController.phishingLists.filter( - (list) => list.name === ListNames.MetaMask, + (list) => list.name === 'MetaMask', ); state.PhishingController = phishingController; From 6214506ade9c223b9e9d45242debe379e91e0828 Mon Sep 17 00:00:00 2001 From: AugmentedMode <31675118+AugmentedMode@users.noreply.github.com> Date: Wed, 9 Oct 2024 17:27:39 -0400 Subject: [PATCH 06/10] Update app/scripts/migrations/126.1.ts Co-authored-by: Mark Stacey --- app/scripts/migrations/126.1.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/app/scripts/migrations/126.1.ts b/app/scripts/migrations/126.1.ts index 0b6e811c8465..4fc820f5eca7 100644 --- a/app/scripts/migrations/126.1.ts +++ b/app/scripts/migrations/126.1.ts @@ -37,6 +37,7 @@ function transformState( const phishingController = state.PhishingController; if (!Array.isArray(phishingController.phishingLists)) { + console.error(`Migration ${version}: Invalid PhishingController.phishingLists state`,) return state; } From ebffa31282e7be6fc1e068381bdf4d790d28f194 Mon Sep 17 00:00:00 2001 From: augmentedmode Date: Wed, 9 Oct 2024 15:31:56 -0600 Subject: [PATCH 07/10] feat: add tests to cover condition if state is corrupted --- app/scripts/migrations/126.1.test.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/app/scripts/migrations/126.1.test.ts b/app/scripts/migrations/126.1.test.ts index 5aa9a43db8eb..0d21a675ebcc 100644 --- a/app/scripts/migrations/126.1.test.ts +++ b/app/scripts/migrations/126.1.test.ts @@ -120,4 +120,23 @@ describe(`migration #${version}`, () => { expect(transformedState.data).toStrictEqual(oldState); }); + + it('does nothing if phishingLists is not an array (null)', async () => { + const oldState: Record = { + PhishingController: { + phishingLists: null, + whitelist: [], + hotlistLastFetched: 0, + stalelistLastFetched: 0, + c2DomainBlocklistLastFetched: 0, + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: oldState, + }); + + expect(transformedState.data).toStrictEqual(oldState); + }); }); From 90c029a1fd058b19877a909daba82dde190a1ac3 Mon Sep 17 00:00:00 2001 From: AugmentedMode <31675118+AugmentedMode@users.noreply.github.com> Date: Wed, 9 Oct 2024 17:34:10 -0400 Subject: [PATCH 08/10] Update app/scripts/migrations/126.1.ts Co-authored-by: Mark Stacey --- app/scripts/migrations/126.1.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/migrations/126.1.ts b/app/scripts/migrations/126.1.ts index 4fc820f5eca7..8498ee6b77b0 100644 --- a/app/scripts/migrations/126.1.ts +++ b/app/scripts/migrations/126.1.ts @@ -37,7 +37,7 @@ function transformState( const phishingController = state.PhishingController; if (!Array.isArray(phishingController.phishingLists)) { - console.error(`Migration ${version}: Invalid PhishingController.phishingLists state`,) + console.error(`Migration ${version}: Invalid PhishingController.phishingLists state`) return state; } From 8d844239812e59607c9d03e642b4e578ac9b987c Mon Sep 17 00:00:00 2001 From: augmentedmode Date: Wed, 9 Oct 2024 15:48:13 -0600 Subject: [PATCH 09/10] fix: lint issue --- app/scripts/migrations/126.1.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/scripts/migrations/126.1.ts b/app/scripts/migrations/126.1.ts index 8498ee6b77b0..81e609e672f1 100644 --- a/app/scripts/migrations/126.1.ts +++ b/app/scripts/migrations/126.1.ts @@ -37,7 +37,9 @@ function transformState( const phishingController = state.PhishingController; if (!Array.isArray(phishingController.phishingLists)) { - console.error(`Migration ${version}: Invalid PhishingController.phishingLists state`) + console.error( + `Migration ${version}: Invalid PhishingController.phishingLists state`, + ); return state; } From dfc3964786dd1cbb36b11161f6785711a307eae6 Mon Sep 17 00:00:00 2001 From: augmentedmode Date: Wed, 9 Oct 2024 16:14:49 -0600 Subject: [PATCH 10/10] feat: add migration to index.js --- app/scripts/migrations/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 93a862b5ee02..a72fd34c3c28 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -146,6 +146,7 @@ const migrations = [ require('./125'), require('./125.1'), require('./126'), + require('./126.1'), require('./127'), require('./128'), require('./129'),