Skip to content

Commit 761562a

Browse files
GudahttGudahtt
authored
fix: Improve migration 121.1 state validation (#26773)
## **Description** 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. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26773?quickstart=1) ## **Related issues** Related: #26377 ## **Manual testing steps** The unit tests outline the scenarios that the added validation are meant to cover. Probably not worth anyone's time to manually test those. To test the migration in general though, here are the steps: * Create a dev build from v12.1.0 * Install the dev build from the `dist/chrome` directory and proceed through onboarding * Run this command in the background console: ``` chrome.storage.local.get( null, (state) => { state.data.AccountsController.internalAccounts.selectedAccount = 'unknown id'; chrome.storage.local.set(state, () => chrome.runtime.reload()); } ); ``` * The extension should now be in a broken state * Note that you can use this same script to corrupt state in other ways to test other scenarios * Disable the extension * Switch to this branch and create a dev build * Enable and reload the extension * You should see in the console that migration 121.1 has run without error * You can run `chrome.storage.local.get(console.log)` to check that the AccountsController state is now valid * The extension should no longer be broken ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
1 parent e177ba0 commit 761562a

File tree

2 files changed

+293
-30
lines changed

2 files changed

+293
-30
lines changed

‎app/scripts/migrations/121.1.test.ts

+182-13
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,14 @@
11
import { AccountsControllerState } from '@metamask/accounts-controller';
2+
import { cloneDeep } from 'lodash';
23
import { createMockInternalAccount } from '../../../test/jest/mocks';
34
import { migrate, version } from './121.1';
45

6+
const sentryCaptureExceptionMock = jest.fn();
7+
8+
global.sentry = {
9+
captureException: sentryCaptureExceptionMock,
10+
};
11+
512
const oldVersion = 121;
613

714
const mockInternalAccount = createMockInternalAccount();
@@ -25,7 +32,8 @@ describe('migration #121.1', () => {
2532
},
2633
};
2734

28-
const newStorage = await migrate(oldStorage);
35+
const newStorage = await migrate(cloneDeep(oldStorage));
36+
2937
expect(newStorage.meta).toStrictEqual({ version });
3038
});
3139

@@ -43,14 +51,15 @@ describe('migration #121.1', () => {
4351
},
4452
};
4553

46-
const newStorage = await migrate(oldStorage);
47-
const {
48-
internalAccounts: { selectedAccount },
49-
} = newStorage.data.AccountsController as AccountsControllerState;
50-
expect(selectedAccount).toStrictEqual(mockInternalAccount.id);
51-
expect(newStorage.data.AccountsController).toStrictEqual(
52-
mockAccountsControllerState,
53-
);
54+
const newStorage = await migrate(cloneDeep(oldStorage));
55+
56+
expect(newStorage.data.AccountsController).toStrictEqual({
57+
...mockAccountsControllerState,
58+
internalAccounts: {
59+
...mockAccountsControllerState.internalAccounts,
60+
selectedAccount: mockInternalAccount.id,
61+
},
62+
});
5463
});
5564

5665
it('does nothing if the selectedAccount is found in the list of accounts', async () => {
@@ -61,9 +70,169 @@ describe('migration #121.1', () => {
6170
},
6271
};
6372

64-
const newStorage = await migrate(oldStorage);
65-
expect(newStorage.data.AccountsController).toStrictEqual(
66-
mockAccountsControllerState,
67-
);
73+
const newStorage = await migrate(cloneDeep(oldStorage));
74+
75+
expect(newStorage.data).toStrictEqual(oldStorage.data);
76+
});
77+
78+
it('does nothing if AccountsController state is missing', async () => {
79+
const oldStorage = {
80+
meta: { version: oldVersion },
81+
data: {
82+
OtherController: {},
83+
},
84+
};
85+
86+
const newStorage = await migrate(cloneDeep(oldStorage));
87+
88+
expect(newStorage.data).toStrictEqual(oldStorage.data);
89+
});
90+
91+
it('does nothing if there are no accounts', async () => {
92+
const oldStorage = {
93+
meta: { version: oldVersion },
94+
data: {
95+
AccountsController: {
96+
...mockAccountsControllerState,
97+
internalAccounts: {
98+
...mockAccountsControllerState.internalAccounts,
99+
accounts: {},
100+
},
101+
},
102+
},
103+
};
104+
105+
const newStorage = await migrate(cloneDeep(oldStorage));
106+
107+
expect(newStorage.data).toStrictEqual(oldStorage.data);
108+
});
109+
110+
it('does nothing if selectedAccount is unset', async () => {
111+
const oldStorage = {
112+
meta: { version: oldVersion },
113+
data: {
114+
AccountsController: {
115+
...mockAccountsControllerState,
116+
internalAccounts: {
117+
...mockAccountsControllerState.internalAccounts,
118+
selectedAccount: '',
119+
},
120+
},
121+
},
122+
};
123+
124+
const newStorage = await migrate(cloneDeep(oldStorage));
125+
126+
expect(newStorage.data).toStrictEqual(oldStorage.data);
68127
});
128+
129+
const invalidState = [
130+
{
131+
errorMessage: `Migration ${version}: Invalid AccountsController state of type 'string'`,
132+
label: 'AccountsController type',
133+
state: { AccountsController: 'invalid' },
134+
},
135+
{
136+
errorMessage: `Migration ${version}: Invalid AccountsController state, missing internalAccounts`,
137+
label: 'Missing internalAccounts',
138+
state: { AccountsController: {} },
139+
},
140+
{
141+
errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts state of type 'string'`,
142+
label: 'Invalid internalAccounts',
143+
state: { AccountsController: { internalAccounts: 'invalid' } },
144+
},
145+
{
146+
errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts state, missing selectedAccount`,
147+
label: 'Missing selectedAccount',
148+
state: { AccountsController: { internalAccounts: { accounts: {} } } },
149+
},
150+
{
151+
errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.selectedAccount state of type 'object'`,
152+
label: 'Invalid selectedAccount',
153+
state: {
154+
AccountsController: {
155+
internalAccounts: { accounts: {}, selectedAccount: {} },
156+
},
157+
},
158+
},
159+
{
160+
errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts state, missing accounts`,
161+
label: 'Missing accounts',
162+
state: {
163+
AccountsController: {
164+
internalAccounts: { selectedAccount: '' },
165+
},
166+
},
167+
},
168+
{
169+
errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state of type 'string'`,
170+
label: 'Missing accounts',
171+
state: {
172+
AccountsController: {
173+
internalAccounts: { accounts: 'invalid', selectedAccount: '' },
174+
},
175+
},
176+
},
177+
{
178+
errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found of type 'string'`,
179+
label: 'Account entry type',
180+
state: {
181+
AccountsController: {
182+
internalAccounts: {
183+
accounts: { [mockInternalAccount.id]: 'invalid' },
184+
selectedAccount: 'unknown id',
185+
},
186+
},
187+
},
188+
},
189+
{
190+
errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found that is missing an id`,
191+
label: 'Account entry missing ID',
192+
state: {
193+
AccountsController: {
194+
internalAccounts: {
195+
accounts: { [mockInternalAccount.id]: {} },
196+
selectedAccount: 'unknown id',
197+
},
198+
},
199+
},
200+
},
201+
{
202+
errorMessage: `Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found with an id of type 'object'`,
203+
label: 'Account entry missing ID',
204+
state: {
205+
AccountsController: {
206+
internalAccounts: {
207+
accounts: { [mockInternalAccount.id]: { id: {} } },
208+
selectedAccount: 'unknown id',
209+
},
210+
},
211+
},
212+
},
213+
];
214+
215+
// @ts-expect-error 'each' function missing from type definitions, but it does exist
216+
it.each(invalidState)(
217+
'captures error when state is invalid due to: $label',
218+
async ({
219+
errorMessage,
220+
state,
221+
}: {
222+
errorMessage: string;
223+
state: Record<string, unknown>;
224+
}) => {
225+
const oldStorage = {
226+
meta: { version: oldVersion },
227+
data: state,
228+
};
229+
230+
const newStorage = await migrate(cloneDeep(oldStorage));
231+
232+
expect(sentryCaptureExceptionMock).toHaveBeenCalledWith(
233+
new Error(errorMessage),
234+
);
235+
expect(newStorage.data).toStrictEqual(oldStorage.data);
236+
},
237+
);
69238
});

‎app/scripts/migrations/121.1.ts

+111-17
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
import { AccountsControllerState } from '@metamask/accounts-controller';
2-
import { cloneDeep } from 'lodash';
1+
import { hasProperty } from '@metamask/utils';
2+
import { cloneDeep, isObject } from 'lodash';
3+
import log from 'loglevel';
34

45
type VersionedData = {
56
meta: { version: number };
@@ -9,7 +10,8 @@ type VersionedData = {
910
export const version = 121.1;
1011

1112
/**
12-
* This migration removes depreciated `Txcontroller` key if it is present in state.
13+
* Fix AccountsController state corruption, where the `selectedAccount` state is set to an invalid
14+
* ID.
1315
*
1416
* @param originalVersionedData - Versioned MetaMask extension state, exactly
1517
* what we persist to dist.
@@ -28,22 +30,114 @@ export async function migrate(
2830
return versionedData;
2931
}
3032

31-
function transformState(state: Record<string, unknown>) {
32-
const accountsControllerState = state?.AccountsController as
33-
| AccountsControllerState
34-
| undefined;
33+
function transformState(state: Record<string, unknown>): void {
34+
if (!hasProperty(state, 'AccountsController')) {
35+
return;
36+
}
37+
38+
const accountsControllerState = state.AccountsController;
39+
40+
if (!isObject(accountsControllerState)) {
41+
global.sentry?.captureException(
42+
new Error(
43+
`Migration ${version}: Invalid AccountsController state of type '${typeof accountsControllerState}'`,
44+
),
45+
);
46+
return;
47+
} else if (!hasProperty(accountsControllerState, 'internalAccounts')) {
48+
global.sentry?.captureException(
49+
new Error(
50+
`Migration ${version}: Invalid AccountsController state, missing internalAccounts`,
51+
),
52+
);
53+
return;
54+
} else if (!isObject(accountsControllerState.internalAccounts)) {
55+
global.sentry?.captureException(
56+
new Error(
57+
`Migration ${version}: Invalid AccountsController internalAccounts state of type '${typeof accountsControllerState.internalAccounts}'`,
58+
),
59+
);
60+
return;
61+
} else if (
62+
!hasProperty(accountsControllerState.internalAccounts, 'selectedAccount')
63+
) {
64+
global.sentry?.captureException(
65+
new Error(
66+
`Migration ${version}: Invalid AccountsController internalAccounts state, missing selectedAccount`,
67+
),
68+
);
69+
return;
70+
} else if (
71+
typeof accountsControllerState.internalAccounts.selectedAccount !== 'string'
72+
) {
73+
global.sentry?.captureException(
74+
new Error(
75+
`Migration ${version}: Invalid AccountsController internalAccounts.selectedAccount state of type '${typeof accountsControllerState
76+
.internalAccounts.selectedAccount}'`,
77+
),
78+
);
79+
return;
80+
} else if (
81+
!hasProperty(accountsControllerState.internalAccounts, 'accounts')
82+
) {
83+
global.sentry?.captureException(
84+
new Error(
85+
`Migration ${version}: Invalid AccountsController internalAccounts state, missing accounts`,
86+
),
87+
);
88+
return;
89+
} else if (!isObject(accountsControllerState.internalAccounts.accounts)) {
90+
global.sentry?.captureException(
91+
new Error(
92+
`Migration ${version}: Invalid AccountsController internalAccounts.accounts state of type '${typeof accountsControllerState
93+
.internalAccounts.accounts}'`,
94+
),
95+
);
96+
return;
97+
}
98+
99+
if (
100+
Object.keys(accountsControllerState.internalAccounts.accounts).length === 0
101+
) {
102+
log.warn(`Migration ${version}: Skipping, no accounts found`);
103+
return;
104+
} else if (accountsControllerState.internalAccounts.selectedAccount === '') {
105+
log.warn(`Migration ${version}: Skipping, no selected account set`);
106+
return;
107+
}
108+
109+
const firstAccount = Object.values(
110+
accountsControllerState.internalAccounts.accounts,
111+
)[0];
112+
if (!isObject(firstAccount)) {
113+
global.sentry?.captureException(
114+
new Error(
115+
`Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found of type '${typeof firstAccount}'`,
116+
),
117+
);
118+
return;
119+
} else if (!hasProperty(firstAccount, 'id')) {
120+
global.sentry?.captureException(
121+
new Error(
122+
`Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found that is missing an id`,
123+
),
124+
);
125+
return;
126+
} else if (typeof firstAccount.id !== 'string') {
127+
global.sentry?.captureException(
128+
new Error(
129+
`Migration ${version}: Invalid AccountsController internalAccounts.accounts state, entry found with an id of type '${typeof firstAccount.id}'`,
130+
),
131+
);
132+
return;
133+
}
35134

36135
if (
37-
accountsControllerState &&
38-
Object.values(accountsControllerState?.internalAccounts.accounts).length >
39-
0 &&
40-
!accountsControllerState?.internalAccounts.accounts[
41-
accountsControllerState?.internalAccounts.selectedAccount
42-
]
136+
!hasProperty(
137+
accountsControllerState.internalAccounts.accounts,
138+
accountsControllerState.internalAccounts.selectedAccount,
139+
)
43140
) {
44-
accountsControllerState.internalAccounts.selectedAccount = Object.values(
45-
accountsControllerState?.internalAccounts.accounts,
46-
)[0].id;
141+
accountsControllerState.internalAccounts.selectedAccount = firstAccount.id;
47142
}
48-
return state;
49143
}

0 commit comments

Comments
 (0)