Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Improve migration 121.1 state validation #26773

Merged
merged 2 commits into from
Aug 29, 2024
Merged

fix: Improve migration 121.1 state validation #26773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec7e1a0
fix: Improve migration 121.1 state validation
Gudahtt Aug 29, 2024
afa7976
Update description leftover from another migration
Gudahtt Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 182 additions & 13 deletions app/scripts/migrations/121.1.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
Expand All @@ -25,7 +32,8 @@ describe('migration #121.1', () => {
},
};

const newStorage = await migrate(oldStorage);
const newStorage = await migrate(cloneDeep(oldStorage));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the existing tests to use cloneDeep to ensure we don't accidentally forget to clone state in the migration itself. The clone happens in the migration in the copy+pasted portion, so it's not likely to be missed, but it's easy to cover in tests so why not.


expect(newStorage.meta).toStrictEqual({ version });
});

Expand All @@ -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 () => {
Expand All @@ -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 () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few tests for these "expected missing state" cases, which were already handled correctly but were not tested

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<string, unknown>;
}) => {
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);
},
);
});
128 changes: 111 additions & 17 deletions app/scripts/migrations/121.1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 };
Expand All @@ -9,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.
Expand All @@ -28,22 +30,114 @@ export async function migrate(
return versionedData;
}

function transformState(state: Record<string, unknown>) {
const accountsControllerState = state?.AccountsController as
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of using unknown was to force runtime validation of types, rather than assuming state is in the correct shape. We've seen unexpected state corruption in the past, so we try to be more cautious about this. Using a type cast here is defeating the purpose of using unknown, as it lets you skip past this validation.

This also goes against our general TypeScript guidelines, see here for more information: https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md#avoid-type-assertions-by-using-type-guards-to-improve-type-inference

| AccountsControllerState
| undefined;
function transformState(state: Record<string, unknown>): 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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two cases might be encountered for users who have not completed onboarding. It seemed fine to skip the migration in this case, and the types of AccountsController seem to allow it as well. A console log was added just in case this happens unexpectedly.

) {
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}'`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't strictly need to validate the presence and type of the id property, TypeScript didn't demand it of me. But it seemed like a good idea. If the id was corrupted somehow, we wouldn't want to propagate that.

),
);
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;
}