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

New CSP headers caused breaking damages to existing NFTs #175

Closed
tpae opened this issue Aug 17, 2022 · 17 comments
Closed

New CSP headers caused breaking damages to existing NFTs #175

tpae opened this issue Aug 17, 2022 · 17 comments
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization

Comments

@tpae
Copy link

tpae commented Aug 17, 2022

Reference to #172 that was recently merged. @vasco-santos

This update caused a significant amount of NFTs to become useless on major marketplace platforms.

NFTs can be rendered as an iframe with HTML/CSS/JS:

Animation_url also supports HTML pages, allowing you to build rich experiences and interactive NFTs using JavaScript canvas, WebGL, and more. Scripts and relative paths within the HTML page are now supported. However, access to browser extensions is not supported.
https://docs.opensea.io/docs/metadata-standards#metadata-structure

This change forced content-security-policy headers to be overwritten; now, all of those NFTs relying on iframe rendering cannot load content.

While I agree with the sentiment mentioned in #172 (wrt @Gozala), IPFS (nftstorage.link) as a protocol should not be responsible for setting security standards that could be damaging to legitimate use cases.

@tpae tpae added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Aug 17, 2022
@dchoi27
Copy link
Contributor

dchoi27 commented Aug 22, 2022

Hey, thanks for opening this issue. Just a heads up that nftstorage.link is not a protocol (it's not IPFS!) - it's an IPFS gateway. If it's helpful, you can always read data off IPFS via HTTP using other public gateways (or even using your own).

@dchoi27
Copy link
Contributor

dchoi27 commented Aug 22, 2022

Could you share more info on where you're using nftstorage.link and what's broken as a result? Many major marketplaces use their own dedicated gateways (Pinata offers this option), so would love to hear more about what you mean by "significant amount of NFTs to become useless on major marketplace platforms."

We offer nftstorage.link as a fast way for public users to read NFTs off of IPFS, but preventing its use for malicious content was becoming something we couldn't ignore (and since nftstorage.link is public infrastructure, unfortunately we had to make a call that could harm some current users - again, at the infrastructure level, not the protocol level, which we alone don't define). We're looking into offering our own dedicated gateways to users as well.

@Gozala
Copy link
Contributor

Gozala commented Aug 22, 2022

This change forced content-security-policy headers to be overwritten; now, all of those NFTs relying on iframe rendering cannot load content.

@tpae can you elaborate on this point please ? CSPs do prevent loading external URLs (from other origins) but they do not prevent ones from the same origin. E.g. relative URL would work just fine.

It is also worth pointing out that it is not just about phishing attacks. If NFT requires a third party URL to function, it is unable to take advantage of the content addressing. There is no guarantee that URL will stick around or that it will continue to serve the intended resource long term.

I think we should try and work out a solution that can help building a more resilient NFTs. As for existing NFTs that got broken by this change we could probably work out solution to fix those as well (although that fix may not apply to new NFTs)

@melMass
Copy link

melMass commented Aug 22, 2022

Hey,

We are also experiencing this for interactive OBJKTs that use WASM (not all strangely), here is an example, uploaded through NFT.storage:

WORKS -> https://ipfs.io/ipfs/bafybeid3pwll6qkarrian3ulcua5ve3qyqqqfkxn7whilr3a5hocrtrubi/
DOES NOT -> https://bafybeid3pwll6qkarrian3ulcua5ve3qyqqqfkxn7whilr3a5hocrtrubi.ipfs.nftstorage.link/

@mikeal
Copy link

mikeal commented Aug 22, 2022

we should be able to whitelist this for WASM given the security constraints in WASM

@Gozala
Copy link
Contributor

Gozala commented Aug 22, 2022

Thanks for reporting @melMass, I believe https://github.com/nftstorage/nftstorage.link/pull/176/files should address this.

@tpae
Copy link
Author

tpae commented Aug 22, 2022

Hey, thanks for opening this issue. Just a heads up that nftstorage.link is not a protocol (it's not IPFS!) - it's an IPFS gateway. If it's helpful, you can always read data off IPFS via HTTP using other public gateways (or even using your own).

@dchoi27 Makes sense, but even after switching the gateway to ipfs.io, the CSP headers were still there, preventing it from loading. nftstorage.link is the most reliable than the others since we're using nft.storage to pin all of our data.

Could you share more info on where you're using nftstorage.link and what's broken as a result? Many major marketplaces use their own dedicated gateways (Pinata offers this option), so would love to hear more about what you mean by "significant amount of NFTs to become useless on major marketplace platforms."

@dchoi27 Here's a broken link: https://opensea.io/assets/ethereum/0x2afb30418504d3c6ecfa2cb40012804e52ced20a/3643
https://looksrare.org/collections/0x2AFb30418504d3C6EcfA2cb40012804E52ced20a/3643
For iframes, they are not hosting it on their gateway. It's displaying from nftstorage.link.

I think we should try and work out a solution that can help building a more resilient NFTs. As for existing NFTs that got broken by this change we could probably work out solution to fix those as well (although that fix may not apply to new NFTs)

I'm open to ideas, but not using nftstorage.link is probably not a good idea. We rely on nft.storage to host all of our content, and not having this support is a deal breaker.

@dchoi27
Copy link
Contributor

dchoi27 commented Aug 22, 2022

Sorry you're experiencing this. We're planning on implementing in the coming weeks an allowlist of CIDs where we've verified the content is OK but would otherwise be blocked.

Clarifying a few things:

even after switching the gateway to ipfs.io, the CSP headers were still there, preventing it from loading
Could you provide an example? https://bafybeic6e4jlsuy5bhhcssdw7zgf6y6tovvv7jhj5nbmnr3vtbg4duh43y.ipfs.dweb.link/3643/ for instance loads fine for me.

nftstorage.link is the most reliable than the others since we're using nft.storage to pin all of our data
nftstorage.link isn't directly connected to NFT.Storage today. It is a caching layer on top of existing public gateways to get improved performance.

We rely on nft.storage to host all of our content, and not having this support is a deal breaker
We would love for nftstorage.link to be able to support your use case, so hopefully the allowlist solution coming in a few weeks works for you, but if it doesn't, unfortunately the best we can recommend for now is using another public gateway. As mentioned, since nftstorage.link is public infrastructure, we need to balance how it's being used across all our users.

To add on to @Gozala, you might be able to use ipfs:// URIs instead of gateway URLs in the animation content and get it working on Opensea + wallets in the short-term. At least in OpenSea's case, I think they convert ipfs:// into Pinata gateway URLs and then cache the data. But not 100% how they implement things in practice. Ideally it shouldn't matter if you use a gateway or ipfs:// URI, but today in practice not all environments that are resolving these URIs know how to handle them, so it can be a bit messy.

melMass added a commit to teia-community/teia-ui that referenced this issue Aug 22, 2022
@tpae
Copy link
Author

tpae commented Aug 23, 2022

@dchoi27 ipfs:// is still not fully adopted across all marketplaces, and having an HTTP gateway is helpful for faster resolution of NFTs on respective platforms.

relative URL would work just fine.

@Gozala it doesn't work that way when it's embedded in iframes.

It is also worth pointing out that it is not just about phishing attacks. If NFT requires a third party URL to function, it is unable to take advantage of the content addressing. There is no guarantee that URL will stick around or that it will continue to serve the intended resource long term.

This is true in an idealistic sense, but in real-world use, NFTs are still early in their concept, and not all marketplaces and platforms currently adopt the best practices. Our goal is to service our collectors and pick the most reliable gateway (so far nftstorage.link has been the best), but this restriction makes it hard for us to leverage it anymore.

We will devise a way to deal with this, but it will involve moving away from nftstorage.link gateway as a solution.

I still think adding these headers is restrictive, and as a public gateway, it should leave it up to the users to make the decisions.

@dchoi27
Copy link
Contributor

dchoi27 commented Aug 23, 2022

Totally understand! But just want to flag that thinking that users can make choices on determining what's phishing content for all content they're grabbing through an NFT / gateway is also idealistic - and these users are often ones that we need to protect. The good thing is you have other choices (other gateway providers, dedicated gateways, running your own) that can be used. And check back in next week or so to see if nftstorage.link's allowlist could work for you.

Leaving this issue open until we've merged a PR for the allowlist

@tpae
Copy link
Author

tpae commented Aug 24, 2022

I was able to fix this issue by having relative paths within the iframe, it worked!

@melMass
Copy link

melMass commented Aug 24, 2022

I was able to fix this issue by having relative paths within the iframe, it worked!

Relative paths always worked. You must just remove the leading slash (that's the case for all IPFS afaik)

@dchoi27
Copy link
Contributor

dchoi27 commented Aug 24, 2022

woo! closing this then

@dchoi27 dchoi27 closed this as completed Aug 24, 2022
vasco-santos added a commit that referenced this issue Aug 30, 2022
Addresses wasm module loading reported in #175
For details see https://github.com/WebAssembly/content-security-policy/blob/main/proposals/CSP.md#the-wasm-unsafe-eval-source-directive

Co-authored-by: Vasco Santos <santos.vasco10@gmail.com>
@pichiste
Copy link

pichiste commented Nov 6, 2022

Hi @dchoi27, we are running into a related issue with some web-based nfts on versum.xyz. The issue specifically is that connect-src is missing blob:, which breaks the functionality of model/texture loaders in three.js, a library used by a lot of creators.

I notice blob: was added to default-src, but is missing in connect-src, which is causing the issue:
https://github.com/nftstorage/nftstorage.link/blob/main/packages/edge-gateway/src/gateway.js#L60

Any chance of getting that added? Thanks!

@dchoi27
Copy link
Contributor

dchoi27 commented Nov 6, 2022

@vasco-santos mind taking a look?

@vasco-santos
Copy link
Member

Hey @pichiste
We just released #195 with blob: support in connect-src

@pichiste
Copy link

pichiste commented Nov 7, 2022

@vasco-santos Thank you! Really appreciate it! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

7 participants