-
Notifications
You must be signed in to change notification settings - Fork 5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: remove old phishfort list from clients (#27743)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR fixes an issue that prevented users from receiving the updated hotlist from ETH Phishing Detect. While the client still fetched the hotlist, the `PhishingDetector` was unable to update with the new URLs included in the hotlist. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27743?quickstart=1) Fixes: #27737 - [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. - [ ] 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. --------- Co-authored-by: Mark Stacey <[email protected]>
- Loading branch information
1 parent
13c758a
commit f9eb9df
Showing
3 changed files
with
197 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
import { migrate, version } from './126.1'; | ||
|
||
const oldVersion = 126.1; | ||
|
||
const mockPhishingListMetaMask = { | ||
allowlist: [], | ||
blocklist: ['malicious1.com'], | ||
c2DomainBlocklist: ['malicious2.com'], | ||
fuzzylist: [], | ||
tolerance: 0, | ||
version: 1, | ||
lastUpdated: Date.now(), | ||
name: 'MetaMask', | ||
}; | ||
|
||
const mockPhishingListPhishfort = { | ||
allowlist: [], | ||
blocklist: ['phishfort1.com'], | ||
c2DomainBlocklist: ['phishfort2.com'], | ||
fuzzylist: [], | ||
tolerance: 0, | ||
version: 1, | ||
lastUpdated: Date.now(), | ||
name: 'Phishfort', | ||
}; | ||
|
||
describe(`migration #${version}`, () => { | ||
it('updates the version metadata', async () => { | ||
const oldStorage = { | ||
meta: { version: oldVersion }, | ||
data: {}, | ||
}; | ||
|
||
const newStorage = await migrate(oldStorage); | ||
|
||
expect(newStorage.meta).toStrictEqual({ version }); | ||
}); | ||
|
||
it('keeps only the MetaMask phishing list in PhishingControllerState', async () => { | ||
const oldState = { | ||
PhishingController: { | ||
phishingLists: [mockPhishingListMetaMask, mockPhishingListPhishfort], | ||
whitelist: [], | ||
hotlistLastFetched: 0, | ||
stalelistLastFetched: 0, | ||
c2DomainBlocklistLastFetched: 0, | ||
}, | ||
}; | ||
|
||
const transformedState = await migrate({ | ||
meta: { version: oldVersion }, | ||
data: oldState, | ||
}); | ||
|
||
const updatedPhishingController = transformedState.data | ||
.PhishingController as Record<string, unknown>; | ||
|
||
expect(updatedPhishingController.phishingLists).toStrictEqual([ | ||
mockPhishingListMetaMask, | ||
]); | ||
}); | ||
|
||
it('removes all phishing lists if MetaMask is not present', async () => { | ||
const oldState = { | ||
PhishingController: { | ||
phishingLists: [mockPhishingListPhishfort], | ||
whitelist: [], | ||
hotlistLastFetched: 0, | ||
stalelistLastFetched: 0, | ||
c2DomainBlocklistLastFetched: 0, | ||
}, | ||
}; | ||
|
||
const transformedState = await migrate({ | ||
meta: { version: oldVersion }, | ||
data: oldState, | ||
}); | ||
|
||
const updatedPhishingController = transformedState.data | ||
.PhishingController as Record<string, unknown>; | ||
|
||
expect(updatedPhishingController.phishingLists).toStrictEqual([]); | ||
}); | ||
|
||
it('does nothing if PhishingControllerState is empty', async () => { | ||
const oldState = { | ||
PhishingController: { | ||
phishingLists: [], | ||
whitelist: [], | ||
hotlistLastFetched: 0, | ||
stalelistLastFetched: 0, | ||
c2DomainBlocklistLastFetched: 0, | ||
}, | ||
}; | ||
|
||
const transformedState = await migrate({ | ||
meta: { version: oldVersion }, | ||
data: oldState, | ||
}); | ||
|
||
const updatedPhishingController = transformedState.data | ||
.PhishingController as Record<string, unknown>; | ||
|
||
expect(updatedPhishingController.phishingLists).toStrictEqual([]); | ||
}); | ||
|
||
it('does nothing if PhishingController is not in the state', async () => { | ||
const oldState = { | ||
NetworkController: { | ||
providerConfig: { | ||
chainId: '0x1', | ||
}, | ||
}, | ||
}; | ||
|
||
const transformedState = await migrate({ | ||
meta: { version: oldVersion }, | ||
data: oldState, | ||
}); | ||
|
||
expect(transformedState.data).toStrictEqual(oldState); | ||
}); | ||
|
||
it('does nothing if phishingLists is not an array (null)', async () => { | ||
const oldState: Record<string, unknown> = { | ||
PhishingController: { | ||
phishingLists: null, | ||
whitelist: [], | ||
hotlistLastFetched: 0, | ||
stalelistLastFetched: 0, | ||
c2DomainBlocklistLastFetched: 0, | ||
}, | ||
}; | ||
|
||
const transformedState = await migrate({ | ||
meta: { version: oldVersion }, | ||
data: oldState, | ||
}); | ||
|
||
expect(transformedState.data).toStrictEqual(oldState); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
import { hasProperty, isObject } from '@metamask/utils'; | ||
import { cloneDeep } from 'lodash'; | ||
|
||
type VersionedData = { | ||
meta: { version: number }; | ||
data: Record<string, unknown>; | ||
}; | ||
|
||
export const version = 126.1; | ||
|
||
/** | ||
* This migration removes `providerConfig` from the network controller state. | ||
* | ||
* @param originalVersionedData - Versioned MetaMask extension state, exactly what we persist to dist. | ||
* @param originalVersionedData.meta - State metadata. | ||
* @param originalVersionedData.meta.version - The current state version. | ||
* @param originalVersionedData.data - The persisted MetaMask state, keyed by controller. | ||
* @returns Updated versioned MetaMask extension state. | ||
*/ | ||
export async function migrate( | ||
originalVersionedData: VersionedData, | ||
): Promise<VersionedData> { | ||
const versionedData = cloneDeep(originalVersionedData); | ||
versionedData.meta.version = version; | ||
transformState(versionedData.data); | ||
return versionedData; | ||
} | ||
|
||
function transformState( | ||
state: Record<string, unknown>, | ||
): Record<string, unknown> { | ||
if ( | ||
hasProperty(state, 'PhishingController') && | ||
isObject(state.PhishingController) && | ||
hasProperty(state.PhishingController, 'phishingLists') | ||
) { | ||
const phishingController = state.PhishingController; | ||
|
||
if (!Array.isArray(phishingController.phishingLists)) { | ||
console.error( | ||
`Migration ${version}: Invalid PhishingController.phishingLists state`, | ||
); | ||
return state; | ||
} | ||
|
||
phishingController.phishingLists = phishingController.phishingLists.filter( | ||
(list) => list.name === 'MetaMask', | ||
); | ||
|
||
state.PhishingController = phishingController; | ||
} | ||
|
||
return state; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters