From ec7e1a0a405496b3bfbf7f57b02a4fe297d3cd74 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 29 Aug 2024 16:35:10 -0230 Subject: [PATCH 1/2] fix: Improve migration 121.1 state validation Migration 121.1 has been updated to include more state validation, so that it does not throw an error in the event that state is corrupted in some unexpected way. Unexpected corrupted state is now reported to Sentry as well. --- app/scripts/migrations/121.1.test.ts | 195 +++++++++++++++++++++++++-- app/scripts/migrations/121.1.ts | 125 ++++++++++++++--- 2 files changed, 291 insertions(+), 29 deletions(-) diff --git a/app/scripts/migrations/121.1.test.ts b/app/scripts/migrations/121.1.test.ts index 56d14bdcf415..1d2d6f9ab70e 100644 --- a/app/scripts/migrations/121.1.test.ts +++ b/app/scripts/migrations/121.1.test.ts @@ -1,7 +1,14 @@ import { AccountsControllerState } from '@metamask/accounts-controller'; +import { cloneDeep } from 'lodash'; import { createMockInternalAccount } from '../../../test/jest/mocks'; import { migrate, version } from './121.1'; +const sentryCaptureExceptionMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, +}; + const oldVersion = 121; const mockInternalAccount = createMockInternalAccount(); @@ -25,7 +32,8 @@ describe('migration #121.1', () => { }, }; - const newStorage = await migrate(oldStorage); + const newStorage = await migrate(cloneDeep(oldStorage)); + expect(newStorage.meta).toStrictEqual({ version }); }); @@ -43,14 +51,15 @@ describe('migration #121.1', () => { }, }; - const newStorage = await migrate(oldStorage); - const { - internalAccounts: { selectedAccount }, - } = newStorage.data.AccountsController as AccountsControllerState; - expect(selectedAccount).toStrictEqual(mockInternalAccount.id); - expect(newStorage.data.AccountsController).toStrictEqual( - mockAccountsControllerState, - ); + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data.AccountsController).toStrictEqual({ + ...mockAccountsControllerState, + internalAccounts: { + ...mockAccountsControllerState.internalAccounts, + selectedAccount: mockInternalAccount.id, + }, + }); }); it('does nothing if the selectedAccount is found in the list of accounts', async () => { @@ -61,9 +70,169 @@ describe('migration #121.1', () => { }, }; - const newStorage = await migrate(oldStorage); - expect(newStorage.data.AccountsController).toStrictEqual( - mockAccountsControllerState, - ); + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('does nothing if AccountsController state is missing', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + OtherController: {}, + }, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('does nothing if there are no accounts', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + AccountsController: { + ...mockAccountsControllerState, + internalAccounts: { + ...mockAccountsControllerState.internalAccounts, + accounts: {}, + }, + }, + }, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('does nothing if selectedAccount is unset', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + AccountsController: { + ...mockAccountsControllerState, + internalAccounts: { + ...mockAccountsControllerState.internalAccounts, + selectedAccount: '', + }, + }, + }, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data).toStrictEqual(oldStorage.data); }); + + const invalidState = [ + { + errorMessage: `Migration ${version}: Invalid AccountsController state of type 'string'`, + label: 'AccountsController type', + state: { AccountsController: 'invalid' }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController state, missing internalAccounts`, + label: 'Missing internalAccounts', + state: { AccountsController: {} }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts state of type 'string'`, + label: 'Invalid internalAccounts', + state: { AccountsController: { internalAccounts: 'invalid' } }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts state, missing selectedAccount`, + label: 'Missing selectedAccount', + state: { AccountsController: { internalAccounts: { accounts: {} } } }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.selectedAccount state of type 'object'`, + label: 'Invalid selectedAccount', + state: { + AccountsController: { + internalAccounts: { accounts: {}, selectedAccount: {} }, + }, + }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts state, missing accounts`, + label: 'Missing accounts', + state: { + AccountsController: { + internalAccounts: { selectedAccount: '' }, + }, + }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state of type 'string'`, + label: 'Missing accounts', + state: { + AccountsController: { + internalAccounts: { accounts: 'invalid', selectedAccount: '' }, + }, + }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found of type 'string'`, + label: 'Account entry type', + state: { + AccountsController: { + internalAccounts: { + accounts: { [mockInternalAccount.id]: 'invalid' }, + selectedAccount: 'unknown id', + }, + }, + }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found that is missing an id`, + label: 'Account entry missing ID', + state: { + AccountsController: { + internalAccounts: { + accounts: { [mockInternalAccount.id]: {} }, + selectedAccount: 'unknown id', + }, + }, + }, + }, + { + errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found with an id of type 'object'`, + label: 'Account entry missing ID', + state: { + AccountsController: { + internalAccounts: { + accounts: { [mockInternalAccount.id]: { id: {} } }, + selectedAccount: 'unknown id', + }, + }, + }, + }, + ]; + + // @ts-expect-error 'each' function missing from type definitions, but it does exist + it.each(invalidState)( + 'captures error when state is invalid due to: $label', + async ({ + errorMessage, + state, + }: { + errorMessage: string; + state: Record; + }) => { + const oldStorage = { + meta: { version: oldVersion }, + data: state, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(errorMessage), + ); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }, + ); }); diff --git a/app/scripts/migrations/121.1.ts b/app/scripts/migrations/121.1.ts index 0e979297b6b4..49b90ff9b79d 100644 --- a/app/scripts/migrations/121.1.ts +++ b/app/scripts/migrations/121.1.ts @@ -1,5 +1,6 @@ -import { AccountsControllerState } from '@metamask/accounts-controller'; -import { cloneDeep } from 'lodash'; +import { hasProperty } from '@metamask/utils'; +import { cloneDeep, isObject } from 'lodash'; +import log from 'loglevel'; type VersionedData = { meta: { version: number }; @@ -28,22 +29,114 @@ export async function migrate( return versionedData; } -function transformState(state: Record) { - const accountsControllerState = state?.AccountsController as - | AccountsControllerState - | undefined; +function transformState(state: Record): void { + if (!hasProperty(state, 'AccountsController')) { + return; + } + + const accountsControllerState = state.AccountsController; + + if (!isObject(accountsControllerState)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController state of type '${typeof accountsControllerState}'`, + ), + ); + return; + } else if (!hasProperty(accountsControllerState, 'internalAccounts')) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController state, missing internalAccounts`, + ), + ); + return; + } else if (!isObject(accountsControllerState.internalAccounts)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts state of type '${typeof accountsControllerState.internalAccounts}'`, + ), + ); + return; + } else if ( + !hasProperty(accountsControllerState.internalAccounts, 'selectedAccount') + ) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts state, missing selectedAccount`, + ), + ); + return; + } else if ( + typeof accountsControllerState.internalAccounts.selectedAccount !== 'string' + ) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts.selectedAccount state of type '${typeof accountsControllerState + .internalAccounts.selectedAccount}'`, + ), + ); + return; + } else if ( + !hasProperty(accountsControllerState.internalAccounts, 'accounts') + ) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts state, missing accounts`, + ), + ); + return; + } else if (!isObject(accountsControllerState.internalAccounts.accounts)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts.accounts state of type '${typeof accountsControllerState + .internalAccounts.accounts}'`, + ), + ); + return; + } + + if ( + Object.keys(accountsControllerState.internalAccounts.accounts).length === 0 + ) { + log.warn(`Migration ${version}: Skipping, no accounts found`); + return; + } else if (accountsControllerState.internalAccounts.selectedAccount === '') { + log.warn(`Migration ${version}: Skipping, no selected account set`); + return; + } + + const firstAccount = Object.values( + accountsControllerState.internalAccounts.accounts, + )[0]; + if (!isObject(firstAccount)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found of type '${typeof firstAccount}'`, + ), + ); + return; + } else if (!hasProperty(firstAccount, 'id')) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found that is missing an id`, + ), + ); + return; + } else if (typeof firstAccount.id !== 'string') { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found with an id of type '${typeof firstAccount.id}'`, + ), + ); + return; + } if ( - accountsControllerState && - Object.values(accountsControllerState?.internalAccounts.accounts).length > - 0 && - !accountsControllerState?.internalAccounts.accounts[ - accountsControllerState?.internalAccounts.selectedAccount - ] + !hasProperty( + accountsControllerState.internalAccounts.accounts, + accountsControllerState.internalAccounts.selectedAccount, + ) ) { - accountsControllerState.internalAccounts.selectedAccount = Object.values( - accountsControllerState?.internalAccounts.accounts, - )[0].id; + accountsControllerState.internalAccounts.selectedAccount = firstAccount.id; } - return state; } From afa79765ce74f3f6916447e27eace3ceaa573530 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 29 Aug 2024 17:13:32 -0230 Subject: [PATCH 2/2] Update description leftover from another migration --- app/scripts/migrations/121.1.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/scripts/migrations/121.1.ts b/app/scripts/migrations/121.1.ts index 49b90ff9b79d..f81893bb00e8 100644 --- a/app/scripts/migrations/121.1.ts +++ b/app/scripts/migrations/121.1.ts @@ -10,7 +10,8 @@ type VersionedData = { export const version = 121.1; /** - * This migration removes depreciated `Txcontroller` key if it is present in state. + * Fix AccountsController state corruption, where the `selectedAccount` state is set to an invalid + * ID. * * @param originalVersionedData - Versioned MetaMask extension state, exactly * what we persist to dist.