change(phishing-controller): validate hostname is provided to phishing-controller test and bypass methods

[NicholasEllul]

Explanation

This pull requests introduces validation to the phishing-controller test and bypass methods to ensure that the value being provided is a hostname. The way the code is currently written suggests to the consumer that an origin should be provided.

However, this is problematic given that our phishing list contains hostnames not domains (see here). This can also be validated checking our API endpoint.

While we currently do not pass in an origin to either of these functions, the impact of doing so would result in our phishing detection mechanisms failing. This is because https://evil.com would not match with evil.com/

In order to minimize the potential for an origin to be passed in, this PR adds validation to better catch implementation errors during development cycles. It does so by raising an error.

Reasons not to make this change

I had debated whether this change would have fit better in the eth-phishing-detect detector itself. However, I decided on implementing it here as its the place that developers will most likely be looking to for guidance on how to best implement their integrations with the phishing controller.

This does have tradeoffs, as now the controller is opinionated about the type of input provided. This may or may not be desired and is open to discussion.

In addition, we may prefer to accept full URL's and have the phishing controller do the parsing itself to minimize the burden on external consumers.

References

Changelog

@metamask/phishing-controller

• BREAKING: #test and #bypass raise an error if input other than a hostname is provided to
  • We recommend using new URL("...").hostname or other mechanisms to pass the hostname instead of the full origin to the phishing controller functions.
• BREAKING: Controller action handler renamed from PhishingController:testOrigin to PhishingController:testPhishing

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

[NicholasEllul]

Looks like I can't update the controller action handler until I update the snaps controller

node_modules/@metamask/snaps-controllers/dist/types/interface/SnapInterfaceController.d.ts:3:33 - error TS2305: Module '"@metamask/phishing-controller"' has no exported member 'TestOrigin'.

3 import type { MaybeUpdateState, TestOrigin } from '@metamask/phishing-controller';
                                  ~~~~~~~~~~

However, I can't update SnapInterfaceController until this change is released, is a patch typically required here?

https://github.com/MetaMask/snaps/blob/b572cb8c2c662ce38ca2bac1f163d3e6a7854b0a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts#L5

[legobeat]

What's the runtime performance impact of this (if any) on list operations on the production list?

In general I'd like to think that this can be assumed and users are expected to pass only input that make sense in their context. And I do think origin/protocol is out of scope.

A less strict alternative that would still throw on erroneously passing https://whatever would be to just check for the presence of / and throw if included?
That would also (just like yours, I think) a similar class of user error I've seen, which is passing something like whatever.something/an/actual/url.

[NicholasEllul]

Thanks for the eyes @legobeat. This won't be running on items found on the blocklist, only the input that is being prepared to be checked against it. This code will run once when a user loads a webpage (when MM checks if its a phishing site), and once if someone chooses to dismiss a phishing warning prompt, thus adding it to the safelist. For this reason, it shouldn't be necessary to compromise the thoroughness for performance as this will be a constant time operation.

Good call out with that edge case, i'll add it in as a unit test to capture any potential regressions

[mcmire]

@NicholasEllul Hey, sorry for the late reply on the SnapInterfaceController error you saw above.

If I'm not mistaken, what appears to be happening is that the assets-controllers package relies on snaps-controller, and snaps-controller relies on phishing-controller (which is the version of phishing-controller in your branch and not on NPM). So in order for the whole monorepo to build, phishing-controller needs to have a TestOrigin export.

What are your thoughts on keep the old event type present, but not use it in any way? Then the monorepo would build, and then we could update snaps-controller to use a new version of phishing-controller that exports TestPhishing instead of TestOrigin, and then we could remove TestOrigin in another release.

[NicholasEllul]

@mcmire that can work for this no problem 👍 I'll take that approach here