Normalize ipfs gateway links to CIDs

[umar-ahmed]

• Update clean:blocklist script to normalize and extract CIDs from IPFS subdomain gateway links
• Run script to clean up existing config and remove some duplicates as a result
• Add a test case to cover the IPFS normalization codepath

[mindofmar]

Why do we want to only store the IPFS CIDs instead of the full URL? Will this break existing functionality?

[umar-ahmed]

Why do we want to only store the IPFS CIDs instead of the full URL?

From my reading of the IPFS code merged into core, it looks like that's how they are expected to be stored in the blocklist.

We currently have duplicate entries in the blocklist as a result of not normalizing, and I'm frankly not even sure if the IPFS blocking is working as intended as a result.

Will this break existing functionality?

I think it will make the blocklist work more effectively for different gateways. So I'd consider this more of a bug fix than a breaking change.

---

Aside: One thing I didn't fix yet is normalizing the CIDs to a canonical version. It's probably best to normalize to CIDv1 because it has a case-insensitive base32 encoding (https://cid.ipfs.tech/#QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR). But to do this, the detector code in core will need to be updated as well to apply the same transformation from CIDv0 to CIDv1, so I've kept it out of scope for now

[mindofmar]

@umar-ahmed ah I see. So the goal is to be able to block asdf as an IPFS CID regardless of whether a user navigates to it from ipfs.io/ipfs/asdf or asdf.ipfs.dweb.link?

@409H can you confirm that the Phishing Controller now supports just CID as input to block these as intended above?

[umar-ahmed]

@umar-ahmed ah I see. So the goal is to be able to block asdf as an IPFS CID regardless of whether a user navigates to it from ipfs.io/ipfs/asdf or asdf.ipfs.dweb.link?

Yup 👍

Those should theoretically be the same content, just available at different gateways, so I think it makes most sense that EPD and PhishingController treat them as such for better coverage

[409H]

lgtm

[409H]

@mindofmar yes looks like it was merged in https://github.com/MetaMask/core/blob/main/packages/phishing-controller/src/PhishingDetector.ts#L119 (MetaMask/phishing-controller@12.0.3)

[409H]

We will wait to merge until rollout complete

[mindofmar]

Hey @umar-ahmed would you mind fixing the merge conflict? We are now ready to deploy this.

[umar-ahmed]

Hey @umar-ahmed would you mind fixing the merge conflict? We are now ready to deploy this.

Yup, I can fix those up

[umar-ahmed]

@mindofmar I've rebased this PR on latest main branch. I decided to remove the changes to config.json from running yarn clean:blocklist to avoid the risk of future conflicts

Once this is merged in, I can create a separate PR to clean up the config.json file

[AugmentedMode]

lgtm