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

[phishing-controller] Add PhishingDetector from eth-phishing-detector #4137

Merged
merged 17 commits into from
Jun 10, 2024

Conversation

mikesposito
Copy link
Member

@mikesposito mikesposito commented Apr 8, 2024

Explanation

This PR adds the PhishingDetector class from eth-phising-detector, along with some utility functions into the @metamask/phishing-controller package, converting all to Typescript and Jest.

References

Changelog

@metamask/phishing-detector

  • ADDED: Added PhishingDetector from eth-phising-detector
    • The class, utils and tests have been converted to typescript

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

Copy link

socket-security bot commented Apr 8, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/[email protected] None 0 21.3 kB ka-weihe

View full report↗︎

@mikesposito
Copy link
Member Author

Main files from eth-phishing-detector original package have been added in this commit 1775471 for an easier diff

Comment on lines +56 to +58
#configs: PhishingDetectorConfiguration[];

#legacyConfig: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

These two were originally public in eth-phishing-detector, but looks like they are not used outside of this class - we should be able to prepend # to these without issues

Comment on lines +38 to +45
export type PhishingDetectorConfiguration = {
name?: string;
version?: number | string;
allowlist: string[][];
blocklist: string[][];
fuzzylist: string[][];
tolerance: number;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

This might be confusing, but the main difference between the options passed to the constructor, and the configs saved in #configs (PhishingDetectorConfiguration) is that domains are divided in parts, so each list becomes a matrix

@mikesposito mikesposito marked this pull request as ready for review April 9, 2024 13:55
@mikesposito mikesposito requested a review from a team as a code owner April 9, 2024 13:55
return result;
}

#check(domain: string): PhishingDetectorResult {
Copy link
Member Author

Choose a reason for hiding this comment

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

This can most likely be merged into the public check method since all the public method does is handle the differences with the legacy result type, so that we don't have two methods with the same name without a clear responsibility distinction.

I'm unsure about doing it in this PR though, since there are many things to review already

@NicholasEllul
Copy link
Contributor

@mikesposito Is the plan to eventually remove the relevant code from https://github.com/MetaMask/eth-phishing-detect/blob/main/src/detector.js and centralize everything here instead?

@mikesposito
Copy link
Member Author

@NicholasEllul exactly! the plan is to leave the eth-phishing-detect package for the configuration only. We'll remove the PhishingDetector implementation from that package once we have it here

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Nice work! A lot of comments here, but most are minor. Only major comments are some questions about the logic in check.

packages/phishing-controller/src/PhishingDetector.ts Outdated Show resolved Hide resolved
packages/phishing-controller/src/PhishingDetector.ts Outdated Show resolved Hide resolved
packages/phishing-controller/src/PhishingDetector.ts Outdated Show resolved Hide resolved
packages/phishing-controller/src/PhishingDetector.ts Outdated Show resolved Hide resolved
packages/phishing-controller/src/utils.ts Outdated Show resolved Hide resolved
@mikesposito mikesposito requested a review from a team as a code owner May 30, 2024 13:57
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

LGTM!

@mikesposito mikesposito merged commit 1fe8a66 into main Jun 10, 2024
113 checks passed
@mikesposito mikesposito deleted the feat/add-detector branch June 10, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants