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

Force an update of the phishing warning configuration #20381

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Aug 3, 2023

Explanation

The "last fetched" state for the PhishingController has been deleted to force an immediate full update of the phishing configuration state. We're doing this because the state was cleared in v10.34.2 because the format of that state had changed.

This has been implemented in migration 92. The previous migration 92 has been renamed to 93 because it won't be included until a future release. We need the migrations to remain sequential, and this will save us from having to resolve a complex conflict when releasing this.

Manual Testing Steps

  • Install v10.34.1, onboard
  • Navigate to this test website to ensure that it's blocked: https://test.metamask-phishing.io/
  • update the directory you are loading the extension from to a v10.34.2 build
  • See that the site is still blocked

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.

@Gudahtt Gudahtt marked this pull request as ready for review August 3, 2023 13:59
@Gudahtt Gudahtt requested a review from a team as a code owner August 3, 2023 13:59
@danjm
Copy link
Contributor

danjm commented Aug 3, 2023

code looks good, will do a manual QA

@danjm
Copy link
Contributor

danjm commented Aug 3, 2023

manual QA passes. I could repro the bug on v10.34.2, but not on this branch.

I did the following:

  1. Install v10.34.1, onboard
  2. go to Add scams from ChainPatrol eth-phishing-detect#13184 and click the scam link
  3. checkout this branch
  4. edit metamask-controller.js so that a this.phishingController.setHotlistRefreshInterval(5); call is made (I edited the existing call to that function and moved it outside the if block)
  5. yarn start
  6. update the extension to this new build
  7. In the background console, see a network request to https://phishing-detection.metafi.codefi.network/v1/diffsSince/-Infinity

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

LGTM

The "last fetched" state for the `PhishingController` has been deleted
to force an immediate full update of the phishing configuration state.
We're doing this because the state was cleared in v10.34.2 because the
format of that state had changed.

This has been implemented in migration 92. The previous migration 92
has been renamed to 93 because it won't be included until a future
release. We need the migrations to remain sequential, and this will
save us from having to resolve a complex conflict when releasing this.
@Gudahtt Gudahtt force-pushed the force-phishing-config-refresh branch from 9a3bbc0 to 654ff8d Compare August 3, 2023 14:56
@Gudahtt Gudahtt merged commit a7a0865 into develop Aug 3, 2023
@Gudahtt Gudahtt deleted the force-phishing-config-refresh branch August 3, 2023 15:42
@github-actions github-actions bot locked and limited conversation to collaborators Aug 3, 2023
@metamaskbot metamaskbot added the release-10.36.0 Issue or pull request that will be included in release 10.36.0 label Aug 3, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [654ff8d]
Page Load Metrics (1723 ± 97 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1101931542412
domContentLoaded14202145172320397
load14202145172320397
domInteractive14202145172320397
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 890 Bytes (0.02%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt Gudahtt added release-10.34.2 Issue or pull request that will be included in release 10.34.2 release-10.34.3 Issue or pull request that will be included in release 10.34.3 and removed release-10.36.0 Issue or pull request that will be included in release 10.36.0 release-10.34.2 Issue or pull request that will be included in release 10.34.2 labels Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-10.34.3 Issue or pull request that will be included in release 10.34.3 team-extension-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants