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

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

Closed
digiwand opened this issue Nov 17, 2022 · 3 comments · Fixed by MetaMask/core#986 or #16643

Comments

@digiwand
Copy link
Contributor

@seaona observed refreshing the extension from chrome://extensions and then opening a URL detected as a phishing page will not show the phishing warning page.

Looking into this, it appears that the blocklist may not finish loading when the user first lands on a detected phishing page.

Observe breakpoint hit when list has not finished loading (eth-phishing-detect/src/detector):
Screen Shot 2022-11-17 at 05.57.15.png

Observe breakpoint when list is loaded.
Screen Shot 2022-11-17 at 05.58.48.png

Related Repos:

For some reason, I am only able to repro this issue on MV3 and not MV2.

@segun
Copy link
Contributor

segun commented Nov 21, 2022

I've been testing this using this branch #15913 and I can't replicate the issue. I always get the phising screen after a refresh.

@seaona can you please check to see if this branch fixes the issue?

@seaona
Copy link
Contributor

seaona commented Nov 22, 2022

I still can reproduce @digiwand 's behaviour mentioned above on the branch you pointed out @segun. The issue is that the phishing list does not load completely.

image

Steps to repro:

  1. Load MV3 MM
  2. Add a breakpoint on /detector.js file on line 182 return list.some...
  3. Open a phishing page i.e. solascan.name
  4. See list of blocked sites is smaller than expected

Some extra information, highlighted by @digiwand: we combine the 2 lists https://static.metafi.codefi.network/api/v1/lists/eth_phishing_detect_config.json and https://static.metafi.codefi.network/api/v1/lists/phishfort_hotlist.json of results.

I noticed that for every pause event from the debugger , we are getting different elements from the list:

  • 1266 corresponds to the whitelist array from phishingdetectconfig
  • 24379 corresponds to the blacklist array from phishingdetectconfig
  • 9583 corresponds to phishfortlist - there are 12179 in total, but here in the video not all of them were loaded, which is problematic
2022-11-22.15-14-41.mp4

@segun
Copy link
Contributor

segun commented Nov 22, 2022

@seaona Thanks. I'm now able to reproduce the bug and have a fix.

@digiwand digiwand added this to the MV3 Implementation milestone Nov 23, 2022
@hilvmason hilvmason reopened this Nov 29, 2022
gantunesr pushed a commit to MetaMask/core that referenced this issue Dec 8, 2022
**Pass in initial state to PhishingController and load the initialState
from persisted state**
In Manifest V3, there's need to 

1. Persist phishingcontroller state
2. Load phishingcontroller from persistent state

Thos PR makes sure state is loaded from persistent state and also
abstracts out the checks required before updating the lists

- BREAKING:
- All the code bases using PhishingController should continue to work as
expected
- CHANGED:
- We should call initialise before calling PhishingDetector controller.

- ADDED:

- Added `maybeUpdatePhishingLists` method so that controllers don't have
to do the check logic anymore

**Checklist**

- [ ] Tests are included if applicable
- [ ] Any added code is fully documented

**Issue**

Resolves MetaMask/metamask-extension#16551

Signed-off-by: Akintayo A. Olusegun <[email protected]>
MajorLift pushed a commit to MetaMask/core that referenced this issue Oct 11, 2023
**Pass in initial state to PhishingController and load the initialState
from persisted state**
In Manifest V3, there's need to 

1. Persist phishingcontroller state
2. Load phishingcontroller from persistent state

Thos PR makes sure state is loaded from persistent state and also
abstracts out the checks required before updating the lists

- BREAKING:
- All the code bases using PhishingController should continue to work as
expected
- CHANGED:
- We should call initialise before calling PhishingDetector controller.

- ADDED:

- Added `maybeUpdatePhishingLists` method so that controllers don't have
to do the check logic anymore

**Checklist**

- [ ] Tests are included if applicable
- [ ] Any added code is fully documented

**Issue**

Resolves MetaMask/metamask-extension#16551

Signed-off-by: Akintayo A. Olusegun <[email protected]>
MajorLift pushed a commit to MetaMask/core that referenced this issue Oct 11, 2023
**Pass in initial state to PhishingController and load the initialState
from persisted state**
In Manifest V3, there's need to 

1. Persist phishingcontroller state
2. Load phishingcontroller from persistent state

Thos PR makes sure state is loaded from persistent state and also
abstracts out the checks required before updating the lists

- BREAKING:
- All the code bases using PhishingController should continue to work as
expected
- CHANGED:
- We should call initialise before calling PhishingDetector controller.

- ADDED:

- Added `maybeUpdatePhishingLists` method so that controllers don't have
to do the check logic anymore

**Checklist**

- [ ] Tests are included if applicable
- [ ] Any added code is fully documented

**Issue**

Resolves MetaMask/metamask-extension#16551

Signed-off-by: Akintayo A. Olusegun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment