From b1e29d0583cc2cd1c311d52302c89b47c8709f7a Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Thu, 9 Jan 2025 11:57:11 -0800 Subject: [PATCH] chore: Improve Keyring/Accounts error handling and logs (#12822) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR does the following 1. unmasks non sensitive data from the Accounts Controller and Keyring controller to help us better debug issues in prod. This means that more data will be provided in the sentry state logs. I have ensured that all addresses have remained masked 2. adds logs when creating key components such as the engine and the accounts controller. This will help us debug issues with empty state. 3. adds validation to the migrations for key components. Currently we are validating that the accounts controller and keyring controller data are sound. This validation will not block the app from initializing but will log the errors it finds. ## **Related issues** Progresses : https://github.com/MetaMask/metamask-mobile/issues/12408 ## **Manual testing steps** 1. this change only adds logs and error handling to the app so there should be no functional changes. that being said, we should still verify that the upgrade path is working. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.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-mobile/blob/main/.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/core/Engine/Engine.ts | 36 +-- .../AccountsController/logger.test.ts | 54 +++++ .../controllers/AccountsController/logger.ts | 19 ++ .../AccountsController/utils.test.ts | 36 +++ .../controllers/AccountsController/utils.ts | 2 + app/core/Engine/utils/logger.ts | 20 ++ app/core/Engine/utils/test/logger.test.ts | 111 +++++++++ app/core/EngineService/EngineService.test.ts | 37 ++- app/core/EngineService/EngineService.ts | 23 +- app/store/index.ts | 5 + .../accountsController.test.ts | 214 ++++++++++++++++++ .../validateMigration/accountsController.ts | 60 +++++ .../engineBackgroundState.test.ts | 56 +++++ .../engineBackgroundState.ts | 12 + .../keyringController.test.ts | 109 +++++++++ .../validateMigration/keyringController.ts | 42 ++++ .../validateMigration.test.ts | 87 +++++++ .../validateMigration/validateMigration.ts | 29 +++ .../validateMigration.types.ts | 10 + .../sentry/__snapshots__/utils.test.ts.snap | 58 ++++- app/util/sentry/utils.js | 65 +++++- app/util/sentry/utils.test.ts | 140 +++++++++++- 22 files changed, 1182 insertions(+), 43 deletions(-) create mode 100644 app/core/Engine/controllers/AccountsController/logger.test.ts create mode 100644 app/core/Engine/controllers/AccountsController/logger.ts create mode 100644 app/core/Engine/utils/logger.ts create mode 100644 app/core/Engine/utils/test/logger.test.ts create mode 100644 app/store/validateMigration/accountsController.test.ts create mode 100644 app/store/validateMigration/accountsController.ts create mode 100644 app/store/validateMigration/engineBackgroundState.test.ts create mode 100644 app/store/validateMigration/engineBackgroundState.ts create mode 100644 app/store/validateMigration/keyringController.test.ts create mode 100644 app/store/validateMigration/keyringController.ts create mode 100644 app/store/validateMigration/validateMigration.test.ts create mode 100644 app/store/validateMigration/validateMigration.ts create mode 100644 app/store/validateMigration/validateMigration.types.ts diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index bd5ae84de10..1494a2c033f 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -213,6 +213,7 @@ import { getGlobalChainId, getGlobalNetworkClientId, } from '../../util/networks/global-network'; +import { logEngineCreation } from './utils/logger'; const NON_EMPTY = 'NON_EMPTY'; @@ -276,6 +277,8 @@ export class Engine { initialState: Partial = {}, initialKeyringState?: KeyringControllerState | null, ) { + logEngineCreation(initialState, initialKeyringState); + this.controllerMessenger = new ExtendedControllerMessenger(); const isBasicFunctionalityToggleEnabled = () => @@ -971,8 +974,8 @@ export class Engine { disableSnaps: !isBasicFunctionalityToggleEnabled(), }), clientCryptography: { - pbkdf2Sha512: pbkdf2 - } + pbkdf2Sha512: pbkdf2, + }, }); const authenticationController = new AuthenticationController.Controller({ @@ -1173,7 +1176,7 @@ export class Engine { return Boolean( hasProperty(showIncomingTransactions, currentChainId) && - showIncomingTransactions?.[currentHexChainId], + showIncomingTransactions?.[currentHexChainId], ); }, updateTransactions: true, @@ -1305,7 +1308,9 @@ export class Engine { .addProperties({ token_standard: 'ERC20', asset_type: 'token', - chain_id: getDecimalChainId(getGlobalChainId(networkController)), + chain_id: getDecimalChainId( + getGlobalChainId(networkController), + ), }) .build(), ), @@ -1517,7 +1522,7 @@ export class Engine { if ( state.networksMetadata[state.selectedNetworkClientId].status === NetworkStatus.Available && - getGlobalChainId(networkController) !== currentChainId + getGlobalChainId(networkController) !== currentChainId ) { // We should add a state or event emitter saying the provider changed setTimeout(() => { @@ -1537,7 +1542,9 @@ export class Engine { } catch (error) { console.error( error, - `Network ID not changed, current chainId: ${getGlobalChainId(networkController)}`, + `Network ID not changed, current chainId: ${getGlobalChainId( + networkController, + )}`, ); } }, @@ -1775,7 +1782,7 @@ export class Engine { const tokenBalances = allTokenBalances?.[selectedInternalAccount.address as Hex]?.[ - chainId + chainId ] ?? {}; tokens.forEach( (item: { address: string; balance?: string; decimals: number }) => { @@ -1786,9 +1793,9 @@ export class Engine { item.balance || (item.address in tokenBalances ? renderFromTokenMinimalUnit( - tokenBalances[item.address as Hex], - item.decimals, - ) + tokenBalances[item.address as Hex], + item.decimals, + ) : undefined); const tokenBalanceFiat = balanceToFiatNumber( // TODO: Fix this by handling or eliminating the undefined case @@ -2148,12 +2155,15 @@ export default { return instance.resetState(); }, - destroyEngine() { - instance?.destroyEngineInstance(); + destroyEngine: async () => { + await instance?.destroyEngineInstance(); instance = null; }, - init(state: Partial | undefined, keyringState = null) { + init( + state: Partial | undefined, + keyringState: KeyringControllerState | null = null, + ) { instance = Engine.instance || new Engine(state, keyringState); Object.freeze(instance); return instance; diff --git a/app/core/Engine/controllers/AccountsController/logger.test.ts b/app/core/Engine/controllers/AccountsController/logger.test.ts new file mode 100644 index 00000000000..bdc58f5a83b --- /dev/null +++ b/app/core/Engine/controllers/AccountsController/logger.test.ts @@ -0,0 +1,54 @@ +import Logger from '../../../../util/Logger'; +import { logAccountsControllerCreation } from './logger'; +import { defaultAccountsControllerState } from './utils'; +import { MOCK_ACCOUNTS_CONTROLLER_STATE } from '../../../../util/test/accountsControllerTestUtils'; + +jest.mock('../../../../util/Logger'); + +describe('logAccountsControllerCreation', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('logs creation with default state when no initial state provided', () => { + logAccountsControllerCreation(); + + expect(Logger.log).toHaveBeenCalledWith( + 'Creating AccountsController with default state', + { + defaultState: defaultAccountsControllerState, + }, + ); + }); + + it('logs creation with empty initial state', () => { + const initialState = { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + }; + + logAccountsControllerCreation(initialState); + + expect(Logger.log).toHaveBeenCalledWith( + 'Creating AccountsController with provided initial state', + { + hasSelectedAccount: false, + accountsCount: 0, + }, + ); + }); + + it('logs creation with populated initial state', () => { + logAccountsControllerCreation(MOCK_ACCOUNTS_CONTROLLER_STATE); + + expect(Logger.log).toHaveBeenCalledWith( + 'Creating AccountsController with provided initial state', + { + hasSelectedAccount: true, + accountsCount: 2, + }, + ); + }); +}); diff --git a/app/core/Engine/controllers/AccountsController/logger.ts b/app/core/Engine/controllers/AccountsController/logger.ts new file mode 100644 index 00000000000..381d8e3c7a3 --- /dev/null +++ b/app/core/Engine/controllers/AccountsController/logger.ts @@ -0,0 +1,19 @@ +import { AccountsControllerState } from '@metamask/accounts-controller'; +import Logger from '../../../../util/Logger'; +import { defaultAccountsControllerState } from './utils'; + +export function logAccountsControllerCreation( + initialState?: AccountsControllerState, +) { + if (!initialState) { + Logger.log('Creating AccountsController with default state', { + defaultState: defaultAccountsControllerState, + }); + } else { + Logger.log('Creating AccountsController with provided initial state', { + hasSelectedAccount: !!initialState.internalAccounts?.selectedAccount, + accountsCount: Object.keys(initialState.internalAccounts?.accounts || {}) + .length, + }); + } +} diff --git a/app/core/Engine/controllers/AccountsController/utils.test.ts b/app/core/Engine/controllers/AccountsController/utils.test.ts index 67b640cb448..7165e1239a3 100644 --- a/app/core/Engine/controllers/AccountsController/utils.test.ts +++ b/app/core/Engine/controllers/AccountsController/utils.test.ts @@ -10,11 +10,19 @@ import { import { withScope } from '@sentry/react-native'; import { AGREED, METRICS_OPT_IN } from '../../../../constants/storage'; import StorageWrapper from '../../../../store/storage-wrapper'; +import { logAccountsControllerCreation } from './logger'; jest.mock('@sentry/react-native', () => ({ withScope: jest.fn(), })); +jest.mock('./logger', () => ({ + logAccountsControllerCreation: jest.fn(), +})); + const mockedWithScope = jest.mocked(withScope); +const mockedLogAccountsControllerCreation = jest.mocked( + logAccountsControllerCreation, +); describe('accountControllersUtils', () => { describe('createAccountsController', () => { @@ -50,12 +58,38 @@ describe('accountControllersUtils', () => { jest.resetAllMocks(); }); + it('logs creation with default state when no initial state provided', () => { + createAccountsController({ + messenger: accountsControllerMessenger, + }); + expect(mockedLogAccountsControllerCreation).toHaveBeenCalledWith( + undefined, + ); + }); + + it('logs creation with provided initial state', () => { + const initialState = { + internalAccounts: { + accounts: {}, + selectedAccount: '0x1', + }, + }; + createAccountsController({ + messenger: accountsControllerMessenger, + initialState, + }); + expect(mockedLogAccountsControllerCreation).toHaveBeenCalledWith( + initialState, + ); + }); + it('AccountsController state should be default state when no initial state is passed in', () => { const accountsController = createAccountsController({ messenger: accountsControllerMessenger, }); expect(accountsController.state).toEqual(defaultAccountsControllerState); }); + it('AccountsController state should be initial state when initial state is passed in', () => { const initialAccountsControllerState: AccountsControllerState = { internalAccounts: { @@ -69,6 +103,7 @@ describe('accountControllersUtils', () => { }); expect(accountsController.state).toEqual(initialAccountsControllerState); }); + it('AccountsController name should be AccountsController', () => { const accountsControllerName = 'AccountsController'; const accountsController = createAccountsController({ @@ -76,6 +111,7 @@ describe('accountControllersUtils', () => { }); expect(accountsController.name).toEqual(accountsControllerName); }); + it('should detect and log an error when controller fails to initialize', async () => { const brokenAccountsControllerMessenger = 'controllerMessenger' as unknown as AccountsControllerMessenger; diff --git a/app/core/Engine/controllers/AccountsController/utils.ts b/app/core/Engine/controllers/AccountsController/utils.ts index 881e86e43ef..d081bed84e5 100644 --- a/app/core/Engine/controllers/AccountsController/utils.ts +++ b/app/core/Engine/controllers/AccountsController/utils.ts @@ -4,6 +4,7 @@ import { AccountsControllerState, } from '@metamask/accounts-controller'; import Logger from '../../../../util/Logger'; +import { logAccountsControllerCreation } from './logger'; // Default AccountsControllerState export const defaultAccountsControllerState: AccountsControllerState = { @@ -28,6 +29,7 @@ export const createAccountsController = ({ initialState?: AccountsControllerState; }): AccountsController => { try { + logAccountsControllerCreation(initialState); const accountsController = new AccountsController({ messenger, state: initialState ?? defaultAccountsControllerState, diff --git a/app/core/Engine/utils/logger.ts b/app/core/Engine/utils/logger.ts new file mode 100644 index 00000000000..ceee1d234d2 --- /dev/null +++ b/app/core/Engine/utils/logger.ts @@ -0,0 +1,20 @@ +import { KeyringControllerState } from '@metamask/keyring-controller'; +import { EngineState } from '../types'; +import Logger from '../../../util/Logger'; + +export function logEngineCreation( + initialState: Partial = {}, + initialKeyringState?: KeyringControllerState | null, +) { + if (Object.keys(initialState).length === 0) { + Logger.log('Engine initialized with empty state', { + keyringStateFromBackup: !!initialKeyringState, + }); + } else { + Logger.log('Engine initialized with non-empty state', { + hasAccountsState: !!initialState.AccountsController, + hasKeyringState: !!initialState.KeyringController, + keyringStateFromBackup: !!initialKeyringState, + }); + } +} diff --git a/app/core/Engine/utils/test/logger.test.ts b/app/core/Engine/utils/test/logger.test.ts new file mode 100644 index 00000000000..0d43dfc2fd2 --- /dev/null +++ b/app/core/Engine/utils/test/logger.test.ts @@ -0,0 +1,111 @@ +import Logger from '../../../../util/Logger'; +import { logEngineCreation } from '../logger'; + +jest.mock('../../../../util/Logger'); + +describe('logEngineCreation', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('logs empty state initialization', () => { + logEngineCreation({}, null); + + expect(Logger.log).toHaveBeenCalledWith( + 'Engine initialized with empty state', + { + keyringStateFromBackup: false, + }, + ); + }); + + it('logs empty state initialization with keyring backup', () => { + logEngineCreation( + {}, + { vault: 'test-vault', keyrings: [], isUnlocked: false }, + ); + + expect(Logger.log).toHaveBeenCalledWith( + 'Engine initialized with empty state', + { + keyringStateFromBackup: true, + }, + ); + }); + + it('logs non-empty state initialization with accounts', () => { + const initialState = { + AccountsController: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + }, + }; + + logEngineCreation(initialState, null); + + expect(Logger.log).toHaveBeenCalledWith( + 'Engine initialized with non-empty state', + { + hasAccountsState: true, + hasKeyringState: false, + keyringStateFromBackup: false, + }, + ); + }); + + it('logs non-empty state initialization with keyring state', () => { + const initialState = { + KeyringController: { + vault: 'test-vault', + keyrings: [], + isUnlocked: false, + }, + }; + + logEngineCreation(initialState, null); + + expect(Logger.log).toHaveBeenCalledWith( + 'Engine initialized with non-empty state', + { + hasAccountsState: false, + hasKeyringState: true, + keyringStateFromBackup: false, + }, + ); + }); + + it('logs non-empty state initialization with both accounts and keyring', () => { + const initialState = { + AccountsController: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + }, + KeyringController: { + vault: 'test-vault', + keyrings: [], + isUnlocked: false, + }, + }; + + const keyringBackup = { + vault: 'backup-vault', + keyrings: [], + isUnlocked: false, + }; + + logEngineCreation(initialState, keyringBackup); + + expect(Logger.log).toHaveBeenCalledWith( + 'Engine initialized with non-empty state', + { + hasAccountsState: true, + hasKeyringState: true, + keyringStateFromBackup: true, + }, + ); + }); +}); diff --git a/app/core/EngineService/EngineService.test.ts b/app/core/EngineService/EngineService.test.ts index 99bc238b2f5..1afd30585b5 100644 --- a/app/core/EngineService/EngineService.test.ts +++ b/app/core/EngineService/EngineService.test.ts @@ -16,6 +16,7 @@ jest.mock('../NavigationService', () => ({ // Mock Logger jest.mock('../../util/Logger', () => ({ error: jest.fn(), + log: jest.fn(), })); jest.mock('../BackupVault', () => ({ @@ -109,7 +110,9 @@ describe('EngineService', () => { jest.clearAllMocks(); jest.resetAllMocks(); jest.spyOn(ReduxService, 'store', 'get').mockReturnValue({ - getState: () => ({ engine: { backgroundState: {} } }), + getState: () => ({ + engine: { backgroundState: { KeyringController: {} } }, + }), } as unknown as ReduxStore); engineService = new EngineService(); @@ -120,11 +123,41 @@ describe('EngineService', () => { expect(Engine.context).toBeDefined(); }); - it('should have recovered vault on redux store ', async () => { + it('should log Engine initialization with state info', () => { + engineService.start(); + expect(Logger.log).toHaveBeenCalledWith( + 'EngineService: Initializing Engine:', + { + hasState: true, + }, + ); + }); + + it('should log Engine initialization with empty state', () => { + jest.spyOn(ReduxService, 'store', 'get').mockReturnValue({ + getState: () => ({ engine: { backgroundState: {} } }), + } as unknown as ReduxStore); + + engineService.start(); + expect(Logger.log).toHaveBeenCalledWith( + 'EngineService: Initializing Engine:', + { + hasState: false, + }, + ); + }); + + it('should have recovered vault on redux store and log initialization', async () => { engineService.start(); const { success } = await engineService.initializeVaultFromBackup(); expect(success).toBeTruthy(); expect(Engine.context.KeyringController.state.vault).toBeDefined(); + expect(Logger.log).toHaveBeenCalledWith( + 'EngineService: Initializing Engine from backup:', + { + hasState: true, + }, + ); }); it('should navigate to vault recovery if Engine fails to initialize', () => { diff --git a/app/core/EngineService/EngineService.ts b/app/core/EngineService/EngineService.ts index 6ba3e827cb9..0e103d522bc 100644 --- a/app/core/EngineService/EngineService.ts +++ b/app/core/EngineService/EngineService.ts @@ -1,4 +1,4 @@ -import UntypedEngine from '../Engine'; +import Engine from '../Engine'; import AppConstants from '../AppConstants'; import { getVaultFromBackup } from '../BackupVault'; import Logger from '../../util/Logger'; @@ -12,6 +12,9 @@ import getUIStartupSpan from '../Performance/UIStartup'; import ReduxService from '../redux'; import NavigationService from '../NavigationService'; import Routes from '../../constants/navigation/Routes'; +import { KeyringControllerState } from '@metamask/keyring-controller'; + +const LOG_TAG = 'EngineService'; interface InitializeEngineResult { success: boolean; @@ -49,10 +52,11 @@ export class EngineService { tags: getTraceTags(reduxState), }); const state = reduxState?.engine?.backgroundState || {}; - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const Engine = UntypedEngine as any; try { + Logger.log(`${LOG_TAG}: Initializing Engine:`, { + hasState: Object.keys(state).length > 0, + }); + Engine.init(state); this.updateControllers(Engine); } catch (error) { @@ -235,17 +239,20 @@ export class EngineService { const keyringState = await getVaultFromBackup(); const reduxState = ReduxService.store.getState(); const state = reduxState?.engine?.backgroundState || {}; - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const Engine = UntypedEngine as any; // This ensures we create an entirely new engine await Engine.destroyEngine(); this.engineInitialized = false; if (keyringState) { - const newKeyringState = { + const newKeyringState: KeyringControllerState = { keyrings: [], vault: keyringState.vault, + isUnlocked: false, }; + + Logger.log(`${LOG_TAG}: Initializing Engine from backup:`, { + hasState: Object.keys(state).length > 0, + }); + const instance = Engine.init(state, newKeyringState); if (instance) { this.updateControllers(instance); diff --git a/app/store/index.ts b/app/store/index.ts index e158b1e5f81..7dca2b0e1c8 100644 --- a/app/store/index.ts +++ b/app/store/index.ts @@ -14,6 +14,7 @@ import persistConfig from './persistConfig'; import getUIStartupSpan from '../core/Performance/UIStartup'; import ReduxService from '../core/redux'; import { onPersistedDataLoaded } from '../actions/user'; +import { validatePostMigrationState } from './validateMigration/validateMigration'; // TODO: Improve type safety by using real Action types instead of `any` // TODO: Replace "any" with type @@ -69,6 +70,10 @@ const createStoreAndPersistor = async () => { endTrace({ name: TraceName.StoreInit }); // Signal that persisted data has been loaded store.dispatch(onPersistedDataLoaded()); + + // validate the state after migration + const currentState = store.getState(); + validatePostMigrationState(currentState); }; persistor = persistStore(store, null, onPersistComplete); diff --git a/app/store/validateMigration/accountsController.test.ts b/app/store/validateMigration/accountsController.test.ts new file mode 100644 index 00000000000..63a636f8243 --- /dev/null +++ b/app/store/validateMigration/accountsController.test.ts @@ -0,0 +1,214 @@ +import { validateAccountsController } from './accountsController'; +import { LOG_TAG } from './validateMigration.types'; +import { RootState } from '../../reducers'; +import { AccountsControllerState } from '@metamask/accounts-controller'; +import { EngineState } from '../../core/Engine/types'; +import { Json } from '@metamask/utils'; + +describe('validateAccountsController', () => { + const createMockState = ( + accountsState?: Partial, + ): Partial => ({ + engine: accountsState + ? { + backgroundState: { + AccountsController: accountsState, + } as EngineState, + } + : undefined, + }); + + const mockValidAccountsState: Partial = { + internalAccounts: { + accounts: { + 'account-1': { + id: 'account-1', + address: '0x123', + type: 'eip155:eoa', + options: {} as Record, + methods: [], + metadata: { + name: 'Account 1', + lastSelected: 0, + importTime: Date.now(), + keyring: { + type: 'HD Key Tree', + }, + }, + }, + }, + selectedAccount: 'account-1', + }, + }; + + it('returns no errors for valid state', () => { + const errors = validateAccountsController( + createMockState(mockValidAccountsState) as RootState, + ); + expect(errors).toEqual([]); + }); + + it('returns error if AccountsController state is missing', () => { + const errors = validateAccountsController( + createMockState(undefined) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController state is missing in engine backgroundState.`, + ]); + }); + + it('returns error if internalAccounts is missing', () => { + const errors = validateAccountsController(createMockState({}) as RootState); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController No internalAccounts object found on AccountsControllerState.`, + ]); + }); + + it('returns error if accounts is empty', () => { + const errors = validateAccountsController( + createMockState({ + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + }) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController No accounts found in internalAccounts.accounts.`, + ]); + }); + + it('returns error if selectedAccount is empty', () => { + const errors = validateAccountsController( + createMockState({ + internalAccounts: { + accounts: { + 'account-1': { + id: 'account-1', + address: '0x123', + type: 'eip155:eoa', + options: {} as Record, + methods: [], + metadata: { + name: 'Account 1', + lastSelected: 0, + importTime: Date.now(), + keyring: { + type: 'HD Key Tree', + }, + }, + }, + }, + selectedAccount: '', + }, + }) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController selectedAccount is missing or empty.`, + ]); + }); + + it('returns error if selectedAccount does not exist in accounts', () => { + const errors = validateAccountsController( + createMockState({ + internalAccounts: { + accounts: { + 'account-1': { + id: 'account-1', + address: '0x123', + type: 'eip155:eoa', + options: {} as Record, + methods: [], + metadata: { + name: 'Account 1', + lastSelected: 0, + importTime: Date.now(), + keyring: { + type: 'HD Key Tree', + }, + }, + }, + }, + selectedAccount: 'non-existent-account', + }, + }) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController The selectedAccount 'non-existent-account' does not exist in the accounts record.`, + ]); + }); + + it('handles undefined engine state', () => { + const errors = validateAccountsController({} as RootState); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController state is missing in engine backgroundState.`, + ]); + }); + + it('handles undefined backgroundState', () => { + const errors = validateAccountsController({ engine: {} } as RootState); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController state is missing in engine backgroundState.`, + ]); + }); + + it('does not throw with malformed state', () => { + // Test with various malformed states + const testStates = [ + undefined, + null, + {}, + { engine: undefined }, + { engine: null }, + { engine: { backgroundState: undefined } }, + { engine: { backgroundState: null } }, + { engine: { backgroundState: { AccountsController: undefined } } }, + { engine: { backgroundState: { AccountsController: null } } }, + { + engine: { + backgroundState: { + AccountsController: { internalAccounts: undefined }, + }, + }, + }, + { + engine: { + backgroundState: { AccountsController: { internalAccounts: null } }, + }, + }, + { + engine: { + backgroundState: { + AccountsController: { + internalAccounts: { + accounts: undefined, + selectedAccount: undefined, + }, + }, + }, + }, + }, + { + engine: { + backgroundState: { + AccountsController: { + internalAccounts: { accounts: null, selectedAccount: null }, + }, + }, + }, + }, + ]; + + testStates.forEach((state) => { + // Verify no throw + expect(() => { + validateAccountsController(state as unknown as RootState); + }).not.toThrow(); + + // Verify returns errors array + const errors = validateAccountsController(state as unknown as RootState); + expect(Array.isArray(errors)).toBe(true); + expect(errors.length).toBeGreaterThan(0); + }); + }); +}); diff --git a/app/store/validateMigration/accountsController.ts b/app/store/validateMigration/accountsController.ts new file mode 100644 index 00000000000..20d608fb8ef --- /dev/null +++ b/app/store/validateMigration/accountsController.ts @@ -0,0 +1,60 @@ +import { ValidationCheck, LOG_TAG } from './validateMigration.types'; + +import { AccountsControllerState } from '@metamask/accounts-controller'; + +/** + * Validates the AccountsControllerState to ensure required + * fields exist and match the expected structure. + * + * @param rootState - The Redux state to validate. + * @returns An array of error messages. If empty, the state is valid. + */ +export const validateAccountsController: ValidationCheck = (rootState) => { + const errors: string[] = []; + + const accountsState: AccountsControllerState | undefined = + rootState?.engine?.backgroundState?.AccountsController; + + // If it's missing altogether, return an error + if (!accountsState) { + errors.push( + `${LOG_TAG}: AccountsController state is missing in engine backgroundState.`, + ); + return errors; + } + + // 1. Check that internalAccounts exists + if (!accountsState.internalAccounts) { + errors.push( + `${LOG_TAG}: AccountsController No internalAccounts object found on AccountsControllerState.`, + ); + return errors; + } + + const { selectedAccount, accounts } = accountsState.internalAccounts; + + // 2. Confirm there is at least one account + if (!accounts || Object.keys(accounts).length === 0) { + errors.push( + `${LOG_TAG}: AccountsController No accounts found in internalAccounts.accounts.`, + ); + return errors; + } + + // 3. Check that selectedAccount is non-empty + if (!selectedAccount) { + errors.push( + `${LOG_TAG}: AccountsController selectedAccount is missing or empty.`, + ); + return errors; + } + + // 4. Confirm the selectedAccount ID exists in internalAccounts.accounts + if (!accounts[selectedAccount]) { + errors.push( + `${LOG_TAG}: AccountsController The selectedAccount '${selectedAccount}' does not exist in the accounts record.`, + ); + } + + return errors; +}; diff --git a/app/store/validateMigration/engineBackgroundState.test.ts b/app/store/validateMigration/engineBackgroundState.test.ts new file mode 100644 index 00000000000..d729b9cd45b --- /dev/null +++ b/app/store/validateMigration/engineBackgroundState.test.ts @@ -0,0 +1,56 @@ +import { validateEngineInitialized } from './engineBackgroundState'; +import { LOG_TAG } from './validateMigration.types'; +import { RootState } from '../../reducers'; +import { backgroundState } from '../../util/test/initial-root-state'; + +describe('validateEngineInitialized', () => { + const createMockState = ( + hasBackgroundState: boolean, + ): Partial => ({ + engine: hasBackgroundState ? { backgroundState } : undefined, + }); + + it('returns no errors when backgroundState exists', () => { + const errors = validateEngineInitialized( + createMockState(true) as RootState, + ); + expect(errors).toEqual([]); + }); + + it('returns error if engine state is missing', () => { + const errors = validateEngineInitialized({} as RootState); + expect(errors).toEqual([`${LOG_TAG}: Engine backgroundState not found.`]); + }); + + it('returns error if backgroundState is missing', () => { + const errors = validateEngineInitialized( + createMockState(false) as RootState, + ); + expect(errors).toEqual([`${LOG_TAG}: Engine backgroundState not found.`]); + }); + + it('does not throw with malformed state', () => { + // Test with various malformed states + const testStates = [ + undefined, + null, + {}, + { engine: undefined }, + { engine: null }, + { engine: { backgroundState: undefined } }, + { engine: { backgroundState: null } }, + ]; + + testStates.forEach((state) => { + // Verify no throw + expect(() => { + validateEngineInitialized(state as unknown as RootState); + }).not.toThrow(); + + // Verify returns errors array + const errors = validateEngineInitialized(state as unknown as RootState); + expect(Array.isArray(errors)).toBe(true); + expect(errors.length).toBeGreaterThan(0); + }); + }); +}); diff --git a/app/store/validateMigration/engineBackgroundState.ts b/app/store/validateMigration/engineBackgroundState.ts new file mode 100644 index 00000000000..9eda6e4f9b6 --- /dev/null +++ b/app/store/validateMigration/engineBackgroundState.ts @@ -0,0 +1,12 @@ +import { ValidationCheck, LOG_TAG } from './validateMigration.types'; + +/** + * Verifies that the engine is initialized + */ +export const validateEngineInitialized: ValidationCheck = (state) => { + const errors: string[] = []; + if (!state?.engine?.backgroundState) { + errors.push(`${LOG_TAG}: Engine backgroundState not found.`); + } + return errors; +}; diff --git a/app/store/validateMigration/keyringController.test.ts b/app/store/validateMigration/keyringController.test.ts new file mode 100644 index 00000000000..25881d55802 --- /dev/null +++ b/app/store/validateMigration/keyringController.test.ts @@ -0,0 +1,109 @@ +import { validateKeyringController } from './keyringController'; +import { LOG_TAG } from './validateMigration.types'; +import { RootState } from '../../reducers'; +import { KeyringControllerState } from '@metamask/keyring-controller'; +import { EngineState } from '../../core/Engine/types'; + +describe('validateKeyringController', () => { + const createMockState = ( + keyringState?: Partial, + ): Partial => ({ + engine: keyringState + ? { + backgroundState: { + KeyringController: keyringState, + } as EngineState, + } + : undefined, + }); + + const mockValidKeyringState: Partial = { + vault: 'mock-vault', + keyrings: [ + { + type: 'HD Key Tree', + accounts: ['0x123'], + }, + ], + }; + + it('returns no errors for valid state', () => { + const errors = validateKeyringController( + createMockState(mockValidKeyringState) as RootState, + ); + expect(errors).toEqual([]); + }); + + it('returns error if KeyringController state is missing', () => { + const errors = validateKeyringController( + createMockState(undefined) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: KeyringController state is missing in engine backgroundState.`, + ]); + }); + + it('returns error if vault is missing', () => { + const errors = validateKeyringController( + createMockState({ keyrings: [] }) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: KeyringController No vault in KeyringControllerState.`, + ]); + }); + + it('returns error if keyrings is missing or empty', () => { + const errors = validateKeyringController( + createMockState({ vault: 'mock-vault', keyrings: [] }) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: KeyringController No keyrings found.`, + ]); + }); + + it('handles undefined engine state', () => { + const errors = validateKeyringController({} as RootState); + expect(errors).toEqual([ + `${LOG_TAG}: KeyringController state is missing in engine backgroundState.`, + ]); + }); + + it('handles undefined backgroundState', () => { + const errors = validateKeyringController({ engine: {} } as RootState); + expect(errors).toEqual([ + `${LOG_TAG}: KeyringController state is missing in engine backgroundState.`, + ]); + }); + + it('does not throw with malformed state', () => { + // Test with various malformed states + const testStates = [ + undefined, + null, + {}, + { engine: undefined }, + { engine: { backgroundState: undefined } }, + { engine: { backgroundState: { KeyringController: undefined } } }, + { engine: { backgroundState: { KeyringController: {} } } }, + { + engine: { + backgroundState: { + KeyringController: { vault: undefined, keyrings: undefined }, + }, + }, + }, + ]; + + testStates.forEach((state) => { + // Verify no throw + expect(() => { + validateKeyringController(state as unknown as RootState); + }).not.toThrow(); + + // Verify returns errors array + const errors = validateKeyringController(state as unknown as RootState); + expect(Array.isArray(errors)).toBe(true); + expect(errors.length).toBeGreaterThan(0); + }); + }); +}); diff --git a/app/store/validateMigration/keyringController.ts b/app/store/validateMigration/keyringController.ts new file mode 100644 index 00000000000..fa932eb09dc --- /dev/null +++ b/app/store/validateMigration/keyringController.ts @@ -0,0 +1,42 @@ +import { KeyringControllerState } from '@metamask/keyring-controller'; +import { ValidationCheck, LOG_TAG } from './validateMigration.types'; + +/** + * Validates the KeyringControllerState to ensure required + * fields exist and match the expected structure. + * + * @param rootState - The Redux state to validate. + * @returns An array of error messages. If empty, the state is valid. + */ +export const validateKeyringController: ValidationCheck = (rootState) => { + const errors: string[] = []; + + const keyringControllerState: KeyringControllerState | undefined = + rootState?.engine?.backgroundState?.KeyringController; + + // If it's missing altogether, return an error + if (!keyringControllerState) { + errors.push( + `${LOG_TAG}: KeyringController state is missing in engine backgroundState.`, + ); + return errors; + } + + // 1. Check that vault exists + if (!keyringControllerState.vault) { + errors.push( + `${LOG_TAG}: KeyringController No vault in KeyringControllerState.`, + ); + return errors; + } + + const keyrings = keyringControllerState.keyrings; + + // 2. Confirm there is at least one account + if (!keyrings || Object.keys(keyrings).length === 0) { + errors.push(`${LOG_TAG}: KeyringController No keyrings found.`); + return errors; + } + + return errors; +}; diff --git a/app/store/validateMigration/validateMigration.test.ts b/app/store/validateMigration/validateMigration.test.ts new file mode 100644 index 00000000000..32375b18917 --- /dev/null +++ b/app/store/validateMigration/validateMigration.test.ts @@ -0,0 +1,87 @@ +import { validatePostMigrationState } from './validateMigration'; +import { RootState } from '../../reducers'; +import Logger from '../../util/Logger'; +import { validateAccountsController } from './accountsController'; +import { validateKeyringController } from './keyringController'; +import { validateEngineInitialized } from './engineBackgroundState'; + +jest.mock('../../util/Logger', () => ({ + error: jest.fn(), +})); + +jest.mock('./accountsController'); +jest.mock('./keyringController'); +jest.mock('./engineBackgroundState'); + +const TOTAL_VALIDATION_CHECKS = 3; + +describe('validatePostMigrationState', () => { + beforeEach(() => { + jest.clearAllMocks(); + (validateEngineInitialized as jest.Mock).mockReturnValue([]); + (validateAccountsController as jest.Mock).mockReturnValue([]); + (validateKeyringController as jest.Mock).mockReturnValue([]); + }); + + it('runs all validation checks', () => { + const mockState = {} as RootState; + validatePostMigrationState(mockState); + + expect(validateEngineInitialized).toHaveBeenCalledWith(mockState); + expect(validateKeyringController).toHaveBeenCalledWith(mockState); + expect(validateAccountsController).toHaveBeenCalledWith(mockState); + + const totalCalls = + (validateAccountsController as jest.Mock).mock.calls.length + + (validateKeyringController as jest.Mock).mock.calls.length + + (validateEngineInitialized as jest.Mock).mock.calls.length; + + expect(totalCalls).toBe(TOTAL_VALIDATION_CHECKS); + }); + + it('logs errors when validation checks return errors', () => { + const mockState = {} as RootState; + const mockError = 'Mock validation error'; + + // Mock one of the validation checks to return an error + (validateAccountsController as jest.Mock).mockReturnValue([mockError]); + + validatePostMigrationState(mockState); + + // Verify error was logged + expect(Logger.error).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ + message: expect.stringContaining(mockError), + }), + ); + }); + + it('does not log when no validation errors occur', () => { + const mockState = {} as RootState; + validatePostMigrationState(mockState); + + // Verify no errors were logged + expect(Logger.error).not.toHaveBeenCalled(); + }); + + it('does not throw when validation checks fail', () => { + const mockState = {} as RootState; + // Mock all validation checks to return errors + const mockErrors = ['Error 1', 'Error 2', 'Error 3']; + (validateEngineInitialized as jest.Mock).mockReturnValue([mockErrors[0]]); + (validateAccountsController as jest.Mock).mockReturnValue([mockErrors[1]]); + (validateKeyringController as jest.Mock).mockReturnValue([mockErrors[2]]); + + // Verify that calling validatePostMigrationState does not throw + expect(() => validatePostMigrationState(mockState)).not.toThrow(); + + // Verify that errors were logged but execution continued + expect(Logger.error).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ + message: expect.stringContaining(mockErrors.join(', ')), + }), + ); + }); +}); diff --git a/app/store/validateMigration/validateMigration.ts b/app/store/validateMigration/validateMigration.ts new file mode 100644 index 00000000000..0638d65b680 --- /dev/null +++ b/app/store/validateMigration/validateMigration.ts @@ -0,0 +1,29 @@ +import Logger from '../../util/Logger'; +import { RootState } from '../../reducers'; +import { ValidationCheck } from './validateMigration.types'; + +// checks +import { validateAccountsController } from './accountsController'; +import { validateKeyringController } from './keyringController'; +import { validateEngineInitialized } from './engineBackgroundState'; + +const checks: ValidationCheck[] = [ + validateEngineInitialized, + validateAccountsController, + validateKeyringController, +]; + +/** + * Runs all validations and logs any errors, but doesn’t throw. + * This makes sure your app keeps running even if some data is unexpected. + */ +export function validatePostMigrationState(state: RootState): void { + const allErrors = checks.flatMap((check) => check(state)); + + // If there are any errors, log them + if (allErrors.length > 0) { + Logger.error(new Error('Migration validation errors'), { + message: `State validation found these issues: ${allErrors.join(', ')}`, + }); + } +} diff --git a/app/store/validateMigration/validateMigration.types.ts b/app/store/validateMigration/validateMigration.types.ts new file mode 100644 index 00000000000..fb9da96148f --- /dev/null +++ b/app/store/validateMigration/validateMigration.types.ts @@ -0,0 +1,10 @@ +import { RootState } from '../../reducers'; + +/** + * Each validation check is a function that: + * - Takes the final (post-migration) Redux state, + * - Returns an array of error strings if any validations fail or an empty array otherwise. + */ +export type ValidationCheck = (state: RootState) => string[]; + +export const LOG_TAG = 'MIGRATION_VALIDATE_STATE_ERROR'; diff --git a/app/util/sentry/__snapshots__/utils.test.ts.snap b/app/util/sentry/__snapshots__/utils.test.ts.snap index 8a5cfd0b599..8af1d0305c7 100644 --- a/app/util/sentry/__snapshots__/utils.test.ts.snap +++ b/app/util/sentry/__snapshots__/utils.test.ts.snap @@ -32,8 +32,48 @@ exports[`captureSentryFeedback maskObject masks initial root state fixture 1`] = }, "AccountsController": { "internalAccounts": { - "accounts": "object", - "selectedAccount": "string", + "accounts": { + "1be55f5b-eba9-41a7-a9ed-a6a8274aca27": { + "address": "string", + "id": "1be55f5b-eba9-41a7-a9ed-a6a8274aca27", + "metadata": { + "importTime": 1720023898234, + "keyring": { + "type": "HD Key Tree", + }, + "lastSelected": 1720023898236, + "name": "Account 1", + }, + "methods": [ + "personal_sign", + "eth_signTransaction", + "eth_signTypedData_v1", + "eth_signTypedData_v3", + "eth_signTypedData_v4", + ], + "options": {}, + "type": "eip155:eoa", + }, + "2be55f5b-eba9-41a7-a9ed-a6a8274aca28": { + "address": "string", + "id": "2be55f5b-eba9-41a7-a9ed-a6a8274aca28", + "metadata": { + "importTime": 1720023898235, + "keyring": { + "type": "HD Key Tree", + }, + "lastSelected": 1720023898237, + "name": "Account 2", + }, + "methods": [ + "personal_sign", + "eth_signTransaction", + ], + "options": {}, + "type": "eip155:eoa", + }, + }, + "selectedAccount": "1be55f5b-eba9-41a7-a9ed-a6a8274aca27", }, }, "AddressBookController": { @@ -63,7 +103,19 @@ exports[`captureSentryFeedback maskObject masks initial root state fixture 1`] = }, "KeyringController": { "isUnlocked": true, - "keyrings": "object", + "keyrings": { + "0": { + "accounts": { + "0": "string", + "1": "string", + }, + "type": "HD Key Tree", + }, + "1": { + "accounts": {}, + "type": "QR Hardware Wallet Device", + }, + }, "vault": "string", }, "LoggingController": { diff --git a/app/util/sentry/utils.js b/app/util/sentry/utils.js index 44f55fc5123..87a5397d290 100644 --- a/app/util/sentry/utils.js +++ b/app/util/sentry/utils.js @@ -32,7 +32,30 @@ export const sentryStateMask = { }, AccountsController: { internalAccounts: { - [AllProperties]: false, + accounts: { + [AllProperties]: { + id: true, + address: false, + type: true, + options: true, + methods: true, + metadata: { + name: true, + importTime: true, + keyring: { + type: true, + }, + nameLastUpdatedAt: true, + snap: { + id: true, + name: true, + enabled: true, + }, + lastSelected: true, + }, + }, + }, + selectedAccount: true, }, }, AddressBookController: { @@ -53,6 +76,16 @@ export const sentryStateMask = { }, KeyringController: { isUnlocked: true, + vault: false, + keyrings: { + [AllProperties]: { + type: true, + // Each keyring contains an array of accounts (addresses), all of which should be masked + accounts: { + [AllProperties]: false, + }, + }, + }, }, LoggingController: { [AllProperties]: false, @@ -336,29 +369,39 @@ function removeSES(report) { */ export function maskObject(objectToMask, mask = {}) { if (!objectToMask) return {}; - let maskAllProperties = false; - if (Object.keys(mask).includes(AllProperties)) { - if (Object.keys(mask).length > 1) { - throw new Error('AllProperties mask key does not support sibling keys'); - } - maskAllProperties = true; - } + + // Include both string and symbol keys. + const maskKeys = Reflect.ownKeys(mask); + const allPropertiesMask = maskKeys.includes(AllProperties) + ? mask[AllProperties] + : undefined; return Object.keys(objectToMask).reduce((maskedObject, key) => { - const maskKey = maskAllProperties ? mask[AllProperties] : mask[key]; + // Start with the AllProperties mask if available + let maskKey = allPropertiesMask; + + // If a key-specific mask exists, it overrides the AllProperties mask + if (mask[key] !== undefined && mask[key] !== AllProperties) { + maskKey = mask[key]; + } + const shouldPrintValue = maskKey === true; const shouldIterateSubMask = - Boolean(maskKey) && typeof maskKey === 'object'; + Boolean(maskKey) && + typeof maskKey === 'object' && + maskKey !== AllProperties; const shouldPrintType = maskKey === undefined || maskKey === false; + if (shouldPrintValue) { maskedObject[key] = objectToMask[key]; } else if (shouldIterateSubMask) { maskedObject[key] = maskObject(objectToMask[key], maskKey); } else if (shouldPrintType) { - // Since typeof null is object, it is more valuable to us having the null instead of object + // For excluded fields, return their type or a placeholder maskedObject[key] = objectToMask[key] === null ? 'null' : typeof objectToMask[key]; } + return maskedObject; }, {}); } diff --git a/app/util/sentry/utils.test.ts b/app/util/sentry/utils.test.ts index c36fe4d6c8b..32071388370 100644 --- a/app/util/sentry/utils.test.ts +++ b/app/util/sentry/utils.test.ts @@ -180,6 +180,21 @@ describe('captureSentryFeedback', () => { options: {}, type: 'eip155:eoa', }, + '2be55f5b-eba9-41a7-a9ed-a6a8274aca28': { + address: '0x1234567890abcdef1234567890abcdef12345678', + id: '2be55f5b-eba9-41a7-a9ed-a6a8274aca28', + metadata: { + importTime: 1720023898235, + keyring: { + type: 'HD Key Tree', + }, + lastSelected: 1720023898237, + name: 'Account 2', + }, + methods: ['personal_sign', 'eth_signTransaction'], + options: {}, + type: 'eip155:eoa', + }, }, selectedAccount: '1be55f5b-eba9-41a7-a9ed-a6a8274aca27', }, @@ -213,9 +228,16 @@ describe('captureSentryFeedback', () => { isUnlocked: true, keyrings: [ { - accounts: ['0x6312c98831d74754f86dd4936668a13b7e9ba411'], + accounts: [ + '0x6312c98831d74754f86dd4936668a13b7e9ba411', + '0xf2ecb579d1225711c2c117f3ab22554357372c822', + ], type: 'HD Key Tree', }, + { + type: 'QR Hardware Wallet Device', + accounts: [], + }, ], vault: '{"cipher":""}', }, @@ -467,9 +489,9 @@ describe('captureSentryFeedback', () => { it('masks initial root state fixture', () => { const maskedState = maskObject(rootState, sentryStateMask); - expect(maskedState).toMatchSnapshot(); }); + it('handles undefined mask', () => { const maskedState = maskObject(rootState, undefined); expect(maskedState).toEqual({ @@ -502,10 +524,12 @@ describe('captureSentryFeedback', () => { wizard: 'object', }); }); + it('handles empty rootState', () => { const maskedState = maskObject({}, sentryStateMask); expect(maskedState).toEqual({}); }); + it('handles rootState with more keys than what is defined in the mask', () => { const maskedState = maskObject(rootState, { legalNotices: true }); expect(maskedState).toEqual({ @@ -541,6 +565,7 @@ describe('captureSentryFeedback', () => { wizard: 'object', }); }); + it('handles submask with { [AllProperties]: false, enabled: true }', () => { const submask = { [AllProperties]: false, @@ -577,13 +602,114 @@ describe('captureSentryFeedback', () => { wizard: 'object', }); }); + + it('masks sensitive data in AccountsController while preserving other fields', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const maskedState = maskObject(rootState, sentryStateMask) as any; + const maskedAccounts = + maskedState.engine.backgroundState.AccountsController.internalAccounts + .accounts; + + const maskedAccount1 = + maskedAccounts['1be55f5b-eba9-41a7-a9ed-a6a8274aca27']; + const maskedAccount2 = + maskedAccounts['2be55f5b-eba9-41a7-a9ed-a6a8274aca28']; + + // Verify addresses are masked + expect(maskedAccount1.address).toBe('string'); + expect(maskedAccount2.address).toBe('string'); + + // Verify non-sensitive data is preserved for account 1 + expect(maskedAccount1.id).toBe('1be55f5b-eba9-41a7-a9ed-a6a8274aca27'); + expect(maskedAccount1.type).toBe('eip155:eoa'); + expect(maskedAccount1.options).toEqual({}); + expect(maskedAccount1.methods).toEqual([ + 'personal_sign', + 'eth_signTransaction', + 'eth_signTypedData_v1', + 'eth_signTypedData_v3', + 'eth_signTypedData_v4', + ]); + expect(maskedAccount1.metadata).toEqual({ + importTime: 1720023898234, + keyring: { type: 'HD Key Tree' }, + lastSelected: 1720023898236, + name: 'Account 1', + }); + + // Verify non-sensitive data is preserved for account 2 + expect(maskedAccount2.id).toBe('2be55f5b-eba9-41a7-a9ed-a6a8274aca28'); + expect(maskedAccount2.type).toBe('eip155:eoa'); + expect(maskedAccount2.options).toEqual({}); + expect(maskedAccount2.methods).toEqual([ + 'personal_sign', + 'eth_signTransaction', + ]); + expect(maskedAccount2.metadata).toEqual({ + importTime: 1720023898235, + keyring: { type: 'HD Key Tree' }, + lastSelected: 1720023898237, + name: 'Account 2', + }); + + // Verify selected account is preserved + const selectedAccountId = + maskedState.engine.backgroundState.AccountsController.internalAccounts + .selectedAccount; + expect(selectedAccountId).toBe('1be55f5b-eba9-41a7-a9ed-a6a8274aca27'); + }); + + it('masks sensitive data in KeyringController while preserving type', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const maskedState = maskObject(rootState, sentryStateMask) as any; + + const maskedKeyringController = + maskedState.engine.backgroundState.KeyringController; + + expect(maskedKeyringController.keyrings).toEqual({ + '0': { + accounts: { + '0': 'string', + '1': 'string', + }, + type: 'HD Key Tree', + }, + '1': { + accounts: {}, + type: 'QR Hardware Wallet Device', + }, + }); + + // Verify vault is masked + expect(maskedKeyringController.vault).toBe('string'); + + // keyrings should now be an object with numeric keys instead of an array + expect(typeof maskedKeyringController.keyrings).toBe('object'); + + const firstKeyring = maskedKeyringController.keyrings['0']; + const secondKeyring = maskedKeyringController.keyrings['1']; + + // Verify first keyring type is preserved + expect(firstKeyring.type).toBe('HD Key Tree'); + + // Accounts are now an object as well + expect(typeof firstKeyring.accounts).toBe('object'); + // Check that accounts have numeric keys and their values are masked + expect(firstKeyring.accounts['0']).toBe('string'); + expect(firstKeyring.accounts['1']).toBe('string'); + + // Verify second keyring type is preserved + expect(secondKeyring.type).toBe('QR Hardware Wallet Device'); + + // The second keyring has no accounts, so it should be an empty object + expect(typeof secondKeyring.accounts).toBe('object'); + expect(Object.keys(secondKeyring.accounts).length).toBe(0); + }); }); it('handle root state with value null and mask false', () => { const submask = { - SnapsController: { - [AllProperties]: false, - }, + [AllProperties]: false, }; const maskedState = maskObject( { @@ -593,7 +719,9 @@ describe('captureSentryFeedback', () => { exampleObj: {}, }, }, - submask, + { + SnapsController: submask, + }, ); expect(maskedState).toEqual({ SnapsController: {