-
-
Notifications
You must be signed in to change notification settings - Fork 955
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
Normalize ipfs gateway links to CIDs #83309
base: main
Are you sure you want to change the base?
Conversation
589e197
to
e8a39ac
Compare
e8a39ac
to
1e475e4
Compare
Why do we want to only store the IPFS CIDs instead of the full URL? Will this break existing functionality? |
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.
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 |
@umar-ahmed ah I see. So the goal is to be able to block @409H can you confirm that the Phishing Controller now supports just CID as input to block these as intended above? |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@mindofmar yes looks like it was merged in https://github.com/MetaMask/core/blob/main/packages/phishing-controller/src/PhishingDetector.ts#L119 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will wait to merge until rollout complete
clean:blocklist
script to normalize and extract CIDs from IPFS subdomain gateway links