Update the PhishingController to v2 and update phishing warning page #17835

Gudahtt · 2023-02-20T17:53:46Z

The PhishingController has been updated to v2. This release should dramatically reduce network traffic and double the update speed of the phishing list.

This was accomplished by combining both of our phishing configurations into one list (the "stalelist"), then creating a separate list of the changes just the past few days (the "hotlist"). Now users will download a smaller list more frequently (every 30 minutes rather than every hour), whereas the full list is only updated every 4 days.

The combined configuration means that we no longer know which list was responsible for each block. The phishing warning page has been updated to dynamically look this information up, to ensure users are still directed to the correct place to dispute a block. This update to the phishing warning page also includes the recent redesign.

Screenshots/Screencaps

The architectural changes can't be seen visually, but this PR also includes the phishing warning page redesign.

You can see those differences visually by looking at this the description for this PR: MetaMask/phishing-warning#52

Manual Testing Steps

To test the new phishing warning page design, you can go through the usual steps to test the phishing warning page (see our phishing warning e2e tests for a few example workflows).

For the architectural changes to how the phishing configuration is updated, we can prepare and install a build, then wait for a phishing config update (maybe we can coordinate with one of the eth-phishing-detect maintainers to merge something at the right time). You should see the update within 0-60 minutes (max 30 minute delay on our API updating, and max 30 minute delay on the client requesting the updated config from the API). You could leave the background process dev tools open to see the network request, and confirm that it is a relatively small diff.

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

socket-security · 2023-02-20T17:55:07Z

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] [email protected]

Powered by socket.dev

Gudahtt · 2023-02-20T19:59:50Z

test/e2e/mock-e2e.js

@@ -341,6 +356,22 @@ async function setupMocking(server, testSpecificMock) {
    });

  testSpecificMock(server);
+
+  // Mocks below this line can be overridden by test-specific mocks


I wanted to set default mocks for these requests, but override them in the phishing tests. I found that this only worked if they were declared below this line. Presumably the first handler wins with mockttp.

cc @PeterYinusa , tagging you in case I'm missing something here, since you set this up.

Thanks for tagging me here. I think I should probably add some comment blocks/documentation to provide clarity on the mocks.
Personally, I feel it needs to be cleaned up a little, as the use cases increased as we added additional test cases. Here's some general info on how it's intended to work at the moment.

Global mocks: Mocks in this file are supposed to be global (across all tests). These mocks are set up before the extension is installed.

Local mocks: Some mocks are specific to a single test. In this case, we use do not put them in the global mocks file, but instead place them directly in the test. Another reason we may place the mock directly in the test, is so we can do assertions on the requests.

Local pre-installation mocks: These are mocks that need to be in place before the extension is installed and are specific to a single test.

Thanks, that all makes sense.

This case still isn't clear though. Here I want to use a global mock, but override it for a specific test. Does placing it here before the invocation of the local pre-installation mocks make sense? Should we let all global mocks be overridden in this way?

Created an issue here to tidy things up #17843

Presumably the first handler wins with mockttp.

I believe this is the case.

Does placing it here before the invocation of the local pre-installation mocks make sense? Should we let all global mocks be overridden in this way?

This makes sense to me. Then we can actually remove the usage of a reset here and make it a test-specific mock

Actually, in reference to my comment above, we would still need a way to do assertions on a request.
But your suggestion seems sound to me.

metamaskbot · 2023-02-21T20:18:46Z

Builds ready [5a05979]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1645 ± 74 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	104	148	121	11	5
		domContentLoaded	1437	2012	1603	151	73
		load	1478	2111	1645	155	74
		domInteractive	1437	2012	1603	151	73

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2949 bytes
ui: 0 bytes
common: 0 bytes

app/scripts/migrations/080.ts

danjm · 2023-02-22T18:41:09Z

@Gudahtt looking at the other PR you linked, I saw this unresolved question: https://github.com/MetaMask/phishing-warning/pull/52/files#r1106413619

Was it addressed at some point?

danjm · 2023-02-22T18:47:36Z

app/scripts/metamask-controller.js

    const mux = setupMultiplex(connectionStream);
    const phishingStream = mux.createStream('phishing');
-    phishingStream.write({ hostname, newIssueUrl });


should newIssueUrl be removed from the redirectToPhishingWarning function in contentScript as well?

metamask-extension/app/scripts/contentscript.js

Line 614 in 5a05979

const { newIssueUrl } = data;

Yes, great catch

Done here: 5ab6a37

danjm

I have a couple of questions and can approve from a code review perspective once they are resolved.

Also, I see that this has been marked for manual QA from the QA team @PeterYinusa @chloeYue @tmashuang

Gudahtt · 2023-02-22T19:48:04Z

Was it addressed at some point?

Yes, just replied to that with a link to the PR where the test was restored. Though all of those tests were later replaced by Playwright tests anyway.

metamaskbot · 2023-02-22T20:59:38Z

Builds ready [5ab6a37]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1505 ± 38 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	93	237	116	32	15
		domContentLoaded	1379	1643	1481	65	31
		load	1379	1649	1505	78	38
		domInteractive	1379	1643	1481	65	31

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2949 bytes
ui: 0 bytes
common: 0 bytes

danjm

LGTM

brad-decker

LGTM

The PhishingController has been updated to v2. This release should dramatically reduce network traffic and double the update speed of the phishing list. This was accomplished by combining both of our phishing configurations into one list (the "stalelist"), then creating a separate list of the changes just the past few days (the "hotlist"). Now users will download a smaller list more frequently (every 30 minutes rather than every hour), whereas the full list is only updated every 4 days. The combined configuration means that we no longer know which list was responsible for each block. The phishing warning page has been updated to dynamically look this information up, to ensure users are still directed to the correct place to dispute a block. This update to the phishing warning page also includes the recent redesign.

Gudahtt · 2023-02-24T13:35:06Z

Rebased to resolve a conflict in package.json

chloeYue · 2023-02-24T14:47:37Z

QA test OK, LGTM, thanks!

metamaskbot · 2023-02-24T15:01:51Z

Builds ready [37cb677]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1509 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	91	291	114	41	20
		domContentLoaded	1393	1678	1488	73	35
		load	1405	1678	1509	87	42
		domInteractive	1393	1678	1488	73	35

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2949 bytes
ui: 0 bytes
common: 0 bytes

Gudahtt force-pushed the update-phishing-controller branch 4 times, most recently from 6e7b856 to f712ac3 Compare February 20, 2023 19:55

Gudahtt commented Feb 20, 2023

View reviewed changes

Gudahtt changed the base branch from develop to update-metamask-utils February 20, 2023 20:00

This comment was marked as resolved.

Sign in to view

Gudahtt force-pushed the update-phishing-controller branch from f712ac3 to 5b8597f Compare February 20, 2023 20:04

This comment was marked as resolved.

Sign in to view

Base automatically changed from update-metamask-utils to develop February 21, 2023 14:49

Gudahtt force-pushed the update-phishing-controller branch 2 times, most recently from d7e3284 to 5a05979 Compare February 21, 2023 19:55

Gudahtt marked this pull request as ready for review February 21, 2023 19:55

Gudahtt requested a review from a team as a code owner February 21, 2023 19:55

Gudahtt requested a review from digiwand February 21, 2023 19:55

Gudahtt marked this pull request as draft February 21, 2023 21:06

This comment was marked as resolved.

Sign in to view

Gudahtt marked this pull request as ready for review February 21, 2023 22:06

Gudahtt added the needs-qa Label will automate into QA workspace label Feb 21, 2023

Gudahtt changed the title ~~Update the PhishingController to v2~~ Update the PhishingController to v2 and update phishing warning page Feb 22, 2023

danjm reviewed Feb 22, 2023

View reviewed changes

app/scripts/migrations/080.ts Show resolved Hide resolved

danjm reviewed Feb 22, 2023

View reviewed changes

Gudahtt force-pushed the update-phishing-controller branch from 5a05979 to 5ab6a37 Compare February 22, 2023 20:36

danjm previously approved these changes Feb 23, 2023

View reviewed changes

brad-decker previously approved these changes Feb 23, 2023

View reviewed changes

PeterYinusa added team-extension-client team-extension-platform labels Feb 24, 2023

Gudahtt added 2 commits February 24, 2023 10:04

Remove unused data parameter and non-existent newIssueUrl property

37cb677

Gudahtt dismissed stale reviews from brad-decker and danjm via 37cb677 February 24, 2023 13:35

Gudahtt force-pushed the update-phishing-controller branch from 5ab6a37 to 37cb677 Compare February 24, 2023 13:35

danjm approved these changes Feb 24, 2023

View reviewed changes

brad-decker approved these changes Feb 24, 2023

View reviewed changes

Gudahtt merged commit 2ccc197 into develop Feb 24, 2023

Gudahtt deleted the update-phishing-controller branch February 24, 2023 15:09

github-actions bot locked and limited conversation to collaborators Feb 24, 2023

PeterYinusa added the rc-cherry-picked label Mar 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the PhishingController to v2 and update phishing warning page #17835

Update the PhishingController to v2 and update phishing warning page #17835

Gudahtt commented Feb 20, 2023 •

edited by danjm

Loading

socket-security bot commented Feb 20, 2023 •

edited

Loading

Gudahtt Feb 20, 2023

PeterYinusa Feb 21, 2023

Gudahtt Feb 21, 2023

PeterYinusa Feb 21, 2023

PeterYinusa Feb 21, 2023

PeterYinusa Feb 21, 2023 •

edited

Loading

PeterYinusa Feb 21, 2023

This comment was marked as resolved.

This comment was marked as resolved.

metamaskbot commented Feb 21, 2023

This comment was marked as resolved.

danjm commented Feb 22, 2023

danjm Feb 22, 2023

Gudahtt Feb 22, 2023

Gudahtt Feb 22, 2023

danjm left a comment

Gudahtt commented Feb 22, 2023 •

edited

Loading

metamaskbot commented Feb 22, 2023

danjm left a comment

brad-decker left a comment

Gudahtt commented Feb 24, 2023

chloeYue commented Feb 24, 2023

metamaskbot commented Feb 24, 2023

Update the PhishingController to v2 and update phishing warning page #17835

Update the PhishingController to v2 and update phishing warning page #17835

Conversation

Gudahtt commented Feb 20, 2023 • edited by danjm Loading

Screenshots/Screencaps

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

socket-security bot commented Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterYinusa Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

metamaskbot commented Feb 21, 2023

This comment was marked as resolved.

danjm commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danjm left a comment

Choose a reason for hiding this comment

Gudahtt commented Feb 22, 2023 • edited Loading

metamaskbot commented Feb 22, 2023

danjm left a comment

Choose a reason for hiding this comment

brad-decker left a comment

Choose a reason for hiding this comment

Gudahtt commented Feb 24, 2023

chloeYue commented Feb 24, 2023

metamaskbot commented Feb 24, 2023

Gudahtt commented Feb 20, 2023 •

edited by danjm

Loading

socket-security bot commented Feb 20, 2023 •

edited

Loading

PeterYinusa Feb 21, 2023 •

edited

Loading

Gudahtt commented Feb 22, 2023 •

edited

Loading