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

Add html generation for photo type embed previews #1334

Merged
merged 2 commits into from
Jun 22, 2017
Merged

Conversation

notnownikki
Copy link
Member

No description provided.

@mkaz mkaz self-requested a review June 21, 2017 19:20
Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, works as expected 👍

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One moment, I have a possible concern here I'd like some time to explore.

@aduth
Copy link
Member

aduth commented Jun 21, 2017

XSS is fun:

  1. Navigate to Gutenberg
  2. Insert Embed block
  3. Insert as URL: http://i1070.photobucket.com/albums/u489/aduthie7/sad-baby_zpsnscz2ovs.jpeg
  4. Click Embed
  5. Get hacked (not really)

This is one benefit of using React elements over concatenating strings, as it takes care of escaping attribute values. Also why dangerouslySetInnerHTML is named as it is.

In this case, we must not allow user values (photo.title) to be inserted into the generated markup without proper escaping.

@aduth aduth added the [Feature] Blocks Overall functionality of blocks label Jun 21, 2017
@notnownikki
Copy link
Member Author

@aduth aha, I did try to find a service where that was a problem, but it looked like the embed API returned safe values. Thanks for finding one that didn't, will change things 😄

@notnownikki
Copy link
Member Author

Ok, switched to JSX for the photo preview. I was under the impression that the embed API supplied clean values, but now I see it was actually cloudup sanitising things.

@aduth
Copy link
Member

aduth commented Jun 21, 2017

One-off sanitization scares me a bit. I'm looking at HtmlEmbed and wondering if we should be doing more sandboxing. I'm also curious to know if the oEmbed endpoint performs discovery, which would open up html to be injected by any properly-configured website.

@notnownikki
Copy link
Member Author

Yes, we absolutely should be doing sandboxing, but it's difficult. On the initial version of this block, we agreed that we'd just inject the html into a div for now, so we could get the block working, and look at sandboxing once it was done.

I do not like the current implementation of HtmlEmbed, but the amount of work needed to get sandboxed iframes working correctly is not a small amount. Calypso has a resizing iframe, but it does not work by itself, there is more code needed to correctly calculate the height of embedded content for previews, and that code needs to be looked at and brought over into the HtmlEmbed component. It's not simple :( I'm looking at it though.

@notnownikki
Copy link
Member Author

I opened #1348 to track the sandboxing issue.

@notnownikki
Copy link
Member Author

Got the ok from @mtias to merge this, as long as we're working on the sandboxing.

@notnownikki notnownikki merged commit 541d6b9 into master Jun 22, 2017
@mtias mtias deleted the fix/embed-photos branch June 23, 2017 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants