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.

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

Persist phishing state controller state. #16643

Merged
merged 3 commits into from
Jan 18, 2023
Merged

Conversation

segun
Copy link
Contributor

@segun segun commented Nov 22, 2022

Explanation

In MV3, when you restart Metamask (or when SW restarts), all the phishing lists are not loaded. This means some phishing sites are not blocked on the first run after a restart.

This PR fixes this issue by persisting PhishingController state.

Manual Testing Steps

  1. Start MM in mv3 mode yarn start:mv3
  2. Load MM extension
  3. Load MM UI
  4. Go to https://poccoinss.com/ or https://polygonteechnology.info
  5. The page should load without the phishing warning the first time you load metamask
  6. Loading the page again, you should see the Phishing Warning page
  7. Go to extension manager and click on the refresh button to reload metamask
  8. Go to https://poccoinss.com/ or https://polygonteechnology.info
  9. The page should load without the phishing warning the first time you load metamask
  10. Loading the page again, you should see the Phishing Warning page
  11. Checkout this branch and start MM in mv3 mode
  12. Repeat the above steps, refresh MM from Extensions Manager and load the phishing pages
  13. You should see the phishing warning immediately.

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.

@segun segun added the MV3 label Nov 22, 2022
@segun segun requested a review from a team as a code owner November 22, 2022 22:21
@segun segun self-assigned this Nov 22, 2022
@segun segun requested a review from jpuri November 22, 2022 22:21
@segun segun marked this pull request as draft November 22, 2022 22:21
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@hilvmason hilvmason removed the MV3 label Nov 24, 2022
@segun segun marked this pull request as ready for review December 7, 2022 21:45
@segun segun force-pushed the dev-olu-mv3-fix-phishing-list branch 4 times, most recently from 39716db to f72e718 Compare December 12, 2022 17:30
@danjm danjm force-pushed the dev-olu-mv3-fix-phishing-list branch from f72e718 to 705dfdf Compare December 14, 2022 15:45
@segun segun force-pushed the dev-olu-mv3-fix-phishing-list branch from 705dfdf to 96e7b66 Compare December 16, 2022 17:40
Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>

update phishing controller to latest version

Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>

Rebase, Yarn3, Lavamoat

Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>

run allow-scrips.

Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>

lavamoat:auto and linter

Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@danjm danjm force-pushed the dev-olu-mv3-fix-phishing-list branch from 96e7b66 to ac338ff Compare January 5, 2023 14:42
Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@metamaskbot
Copy link
Collaborator

Builds ready [4d5f120]
Page Load Metrics (1548 ± 122 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1011721292010
domContentLoaded126521611524243117
load129222801548254122
domInteractive126521611524243117
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -7998 bytes
  • ui: 0 bytes
  • common: 0 bytes

@metamaskbot
Copy link
Collaborator

Builds ready [1b2d3bf]
Page Load Metrics (1574 ± 116 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1051961362412
domContentLoaded119821121549234112
load119821121574242116
domInteractive119821121549234112
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -8021 bytes
  • ui: 0 bytes
  • common: 0 bytes

Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

LGTM

@segun segun merged commit d7e829e into develop Jan 18, 2023
@segun segun deleted the dev-olu-mv3-fix-phishing-list branch January 18, 2023 15:44
@github-actions github-actions bot locked and limited conversation to collaborators Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: [MV3] Loading a detected phishing page for the first time does not show the phishing page warning
6 participants