Append phishfort blocklist

[moosthuizen42]

PhishFort phishing blacklist

Included PhishFort blacklist in the default blacklist used to perform phishing detection.

Description

• ADDED:

  • Fetch of public PhishFort blacklist via cdnjs / GitHub.
  • Inclusion of entries in the PhishFort blacklist into the one used by default (published as part of MetaMask/eth-phishing-detect)
  • Some tests to check for successful detection of entries from the PhishFort blacklist.

Checklist

• Tests are included if applicable
• Any added code is fully documented

[korn101]

Submitted w/ explanation to @sethkfman via email.

[moosthuizen42]

I have updated the PR with the following:

• Removal of duplicates before appending the PhishFort hotlist.
• Graceful failure if only one list is accessible.
• Updated queryConfig to make the above possible (it no longer throws errors for certain HTTP codes and not for others).

The only thing discussed above but not yet catered for is tying back a specific blocklist entry to a list. The Detector from eth-phishing-detect would likely also need to change to do this, or alternatively the architecture of the PhishingController would need a revamp to allow for multiple PhishingDetector instances.

I would like to get this merged for the sake of getting users protected sooner rather than later, while we work out list separation separately, preferably in its own PR.

[Gudahtt]

Good point that the detector would require an update to distinguish which list the block came from. Still, I think that is important enough to block this. eth-phishing-detect is difficult enough to maintain as it is, I don't want to make their jobs any more difficult. I can take a look at updating the detector within the next few days.

[moosthuizen42]

@Gudahtt Noted. My understanding is that you intend to make the required changes to eth-phishing-detect to allow for multiple list inputs. What are you envisioning in terms of input to the PhishingController? I could suggest the following:

{
    version: number;
    tolerance: number;
    blacklist: string[];
    fuzzylist: string[];
    whitelist: string[];
    provider: string; // eg. MetaMask, PhishFort, Umbrella Corporation, etc.
    disputeUrl: string; // Or reportingUrl if that does not cause ambiguity
}[]

[Gudahtt]

I have a PR up now that implements support for multiple configurations on the phishing detector: MetaMask/eth-phishing-detect#7079

Thanks for the suggested configuration @moosthuizen42 . I mostly used that, except I omitted disputeUrl because we can consider that to be outside the responsibilities of the phishing detector. Returning the config name with the match is sufficient for the user of the detector to add whatever metadata they want, which might extend beyond just a dispute URL.

[moosthuizen42]

Thanks @Gudahtt. I'll keep an eye on #7079. Once merged, I intend to update the version of eth-phishing-detect used in our fork, and then integrate both configurations.

[Gudahtt]

v1.2.0 of eth-phishing-detect was just published with support for multiple phishing configurations. Let me know how it works out!

[Gudahtt]

That would be ideal, yeah. We do want to have all properties tested. Though it seems there aren't any fuzzylist tests right now, so those would need to be added. We could use tests for the multi-config setup as well.

[Gudahtt]

Not sure how much of the test improvements to ask you to complete here. I try to ensure that all new changes are fully tested but not to require adding tests that were already missing in the first place, as a general rule. So if you wanted to add fuzzylist tests, that would be great, but I would accept this with just additional assertions for the type property on the current test(...) tests, plus a few more tests to ensure that the "allow" and "block" cases work properly with multiple configs, with assertions for the config name.

[moosthuizen42]

Done. A sequence of fuzzylist tests added as well. @Gudahtt, once you have taken a look, please confirm that everything you expect to see is there.

Other minor changes:

• EthPhishingDetectResult was missing match?: string, fixed.
• Tests intended to verify behaviour for inaccessible configs changed to HTTP 500 instead of 304.

[Gudahtt]

Sorry I'm only just now realizing that the legacy config remains in use until the first update. Could you update the constructor to use the new format as well, passing in the config as an array? I think this would be simpler if we moved directly to the new format, so we wouldn't have to anticipate the two different return types.

[moosthuizen42]

Sorry I'm only just now realizing that the legacy config remains in use until the first update. Could you update the constructor to use the new format as well, passing in the config as an array? I think this would be simpler if we moved directly to the new format, so we wouldn't have to anticipate the two different return types.

Done. Testing references to legacy config have been replaced with default config.

[Gudahtt]

LGTM, great work! The tests look fantastic except that the fuzzylist tests don't check the match property on the result. But that's fine, I can add that later.

[Gudahtt]

Looks like I don't have permission to make commits on this branch. I can merge this after my suggestion is addressed or merged, and the lint errors are fixed.

[Gudahtt]

More lint errors. Please run yarn lint:fix locally, and double check that the build and tests pass as well.

[moosthuizen42]

Noted, haven't gotten around to linting yet. Will notify once done.

[moosthuizen42]

@Gudahtt Linting done, build and test complete successfully. Please note that I am only running the PhishingController.test.ts tests.

[Gudahtt]

LGTM! Holding off a bit on merging this, as I'm going to talk this over with the product teams first and see what it would take to get this change included.

[moosthuizen42]

@Gudahtt Any update on this?

[Gudahtt]

Hey @moosthuizen42 , sorry for the delay! Got side-tracked for a few of weeks, was about to come back to this. I have talked to both product teams and we're all on board with including this in the next release, so this can be merged now once it's brought up to date. Can you update the branch?