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

Support for HTML media type #189

Merged
merged 9 commits into from
Mar 19, 2021
Merged

Support for HTML media type #189

merged 9 commits into from
Mar 19, 2021

Conversation

pichiste
Copy link
Collaborator

Hi there,

This adds support for individual HTML files as a media type, and is aimed at the use case of interactive artworks. Currently artists are using/hijacking the SVG component to embed interactive pieces. Having an HTML media type would simplify the process and make it clearer to the user. It's a close copy of the SVG component by @andrevenancio.

@pichiste pichiste mentioned this pull request Mar 19, 2021
@andrevenancio
Copy link
Contributor

Nice one I had this on my todo list. But needs @crzypatchwork to give it a green light. Because this is Pandora's box open 😂🙌🏻❤️

@pichiste
Copy link
Collaborator Author

Pandora's box indeed!

@andrevenancio
Copy link
Contributor

Give me some time until Brazil wakes up and I'll ask if I can merge this. It seems a better option that SVG embeds.

@andrevenancio andrevenancio added the enhancement New feature or request label Mar 19, 2021
@pichiste
Copy link
Collaborator Author

Thanks!! 🙏 I don't think there are any new security concerns, more just a usability thing really.

@mrdoob
Copy link
Contributor

mrdoob commented Mar 19, 2021

I was worried yesterday that people were able to embed other websites inside SVGs.
But I'm glad to see that removing allow-same-origin solved that.

Security-wise, I'm now more confident about supporting html 🤓

@mrdoob
Copy link
Contributor

mrdoob commented Mar 19, 2021

Also, I don't think this is too different from what SVGs foreignObject already allows the developer to do.
So I don't see it much as opening pandora's box but rather to stop abusing SVGs 😁

@spite
Copy link

spite commented Mar 19, 2021

Related: what do we want to do about permissions that require https://developer.mozilla.org/en-US/docs/Web/HTTP/Feature_Policy (e.g. Camera, Orientation, XR)?

Edit: more concretely, i think that if those were allowed, they should probably be disabled on the main feed and only allowed in the objkt page.

@mrdoob
Copy link
Contributor

mrdoob commented Mar 19, 2021

@spite One step at a time? 😁

@spite
Copy link

spite commented Mar 19, 2021

@mrdoob of course. but i don't think ignoring the overall security is the right call if it can be considered at the same time. specially if one thing doesn't exclude the other. better to be safe than sorry

@mrdoob
Copy link
Contributor

mrdoob commented Mar 19, 2021

As far as I understand...
None of these features are allowed unless we add, say... allow="camera; fullscreen" to the iframe.

@nickdima
Copy link

Together with security we should consider also the immutability aspect. What if you call an external script in the HTML page that's later modified?
Even if hicetnunc will have some security measures in place, once these HTML NFTs are on the blockchain you never know how other websites/apps will handle them.

@pichiste
Copy link
Collaborator Author

@spite @mrdoob Yup it does block the features by default, so they'd have to be selectively enabled with allow. I guess it would just be a matter of pairing down the feature list to one that makes sense at some point?

My wishlist 😅

accelerometer
autoplay
camera
fullscreen
gyroscope
magnetometer
microphone
xr-spatial-tracking (!!!)

Would love to just see basic HTML working first though!

@mrdoob
Copy link
Contributor

mrdoob commented Mar 19, 2021

@nickdima The iframe only has sandbox="allow-scripts". It can't access external urls.

@nickdima
Copy link

@nickdima The iframe only has sandbox="allow-scripts". It can't access external urls.

Yeah what I was thinking is that the responsibility is given to whoever loads that HTML NFT and mistakes might be made. Imagine a new marketplace loads this NFTs and they "forget" to sandbox the iframe.

@pichiste
Copy link
Collaborator Author

@nickdima @mrdoob Hm just tested and it looks like sandbox can't prevent the iframe from requesting random web resources once scripts are enabled.

I'm not convinced that external resources should be prevented though... If someone opens the direct link, they are at the same risk as visiting any webpage, no?

If we really need it to be standalone, a few possible solutions:

  • Injecting a Content Security meta tag upon upload, as suggested here.
  • Wrapping the HTML file in an iframe upon upload, so you are already getting a sandboxed iframe.
  • Upon upload, fetching all external resources and injecting them inline into the main doc.

These would happen on the frontend though so not sure how secure they really are...

With regard to this PR though, the SVG element works the same as this, so it's not introducing a new vulnerability.

@pichiste
Copy link
Collaborator Author

@nickdima The iframe only has sandbox="allow-scripts". It can't access external urls.

Yeah what I was thinking is that the responsibility is given to whoever loads that HTML NFT and mistakes might be made. Imagine a new marketplace loads this NFTs and they "forget" to sandbox the iframe.

Fair point!

@pichiste
Copy link
Collaborator Author

pichiste commented Mar 19, 2021

Testing this out, the CSP meta tag feels like the best solution. Simply injecting this into <head> upon upload:

<meta http-equiv="Content-Security-Policy" content="default-src 'self';">

Thoughts?

@mrdoob
Copy link
Contributor

mrdoob commented Mar 19, 2021

@pichiste Is it possible to remove the meta tag afterwards with JS? 😬

@mrdoob
Copy link
Contributor

mrdoob commented Mar 19, 2021

Is so... maybe double iframe would be the easiest/safest...

@pichiste
Copy link
Collaborator Author

@mrdoob Good call!

I just tested and it looks like fortunately the CSP policy takes effect on load, so it doesn't matter if it gets removed in the js.

Another thing about the double iframes is that things like the feature policy mentioned above would potentially need to be baked into the inner iframe, though maybe its safer in some ways?

@pichiste
Copy link
Collaborator Author

pichiste commented Mar 19, 2021

Ok, I've gone ahead and implemented the CSP meta tag solution. Basically these utility functions are used to inject the following meta tag into both the HTML preview and uploaded HTML file:

<meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'unsafe-inline'; style-src 'unsafe-inline'">

I think that covers external sources concern.

Anything else?

@mrdoob

This comment has been minimized.

src/utils/html.js Outdated Show resolved Hide resolved
@andrevenancio andrevenancio changed the base branch from main to develop March 19, 2021 23:18
@andrevenancio andrevenancio merged commit b9fae14 into hicetnunc2000:develop Mar 19, 2021
@andrevenancio
Copy link
Contributor

merged this to develop. but will not release just yet. need to double check with @crzypatchwork

@pichiste
Copy link
Collaborator Author

Awesome thanks!! I guess one last thing before releasing would be to make sure the author/creator query param name matches between the templates and vector/html iframe embeds.

Excited to see this live!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants