Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#35894 closed defect (bug) (fixed)

Post embeds are useless with X-Frame-Options: SAMEORIGIN

Reported by: ethitter's profile ethitter Owned by: swissspidy's profile swissspidy
Milestone: 4.4.3 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch needs-testing fixed-major commit
Focuses: Cc:

Description

I've set X-Frame-Options: SAMEORIGIN for my WordPress network, which means that other sites can't embed any of my posts using the embeds feature from 4.4. I won't change the header to fix this, and I'm sure others are in a similar situation, breaking this feature in an awkward way.

Before revealing the embed iframe (it's display: none in source), couldn't the JS detect if the iframe failed to load, replacing it with a link to the original post, or at least something more useful than a blank frame? Right now, the blocked iframe expands and if a user didn't check the browser console, they'd have no idea what went wrong.

Attachments (3)

Missing embed in Twenty Sixteen.png (36.0 KB) - added by ethitter 8 years ago.
35894.diff (842 bytes) - added by swissspidy 8 years ago.
35894.2.diff (3.2 KB) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (19)

@swissspidy
8 years ago

#1 @swissspidy
8 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 4.5

That does indeed sound like a bug to me.

Right now we're actually revealing the iframe when we successfully retrieve a message from it. It just has no effect because this was already done by some leftover code in the onLoad() function of the embed script.

35894.diff should fix this.

#2 @pento
8 years ago

  • Milestone changed from 4.5 to 4.4.3

Oh, fun times.

Patch looks good to me. It can be merged to the 4.4 branch, too, if 4.4.3 happens.

#3 @pento
8 years ago

@ethitter - Side note that may be relevant to your interests (if you'd like to maintain the SAMEORIGIN restriction whilst allowing embeds) - URLs intended to embedded in an iframe include an X-WP-embed: true header, which you can detect in nginx and remove the X-Frame-Options: SAMEORIGIN rule for that case.

#4 follow-up: @netweb
8 years ago

In a somewhat related issue, over on https://bbpress.org if a user adds a link to their site that is http:// and *not* https:// then the embed will not show in Google Chrome due to Mixed Content error/warning for example:

  • Results in" Mixed Content: The page at 'https://bbpress.org/forums/topic/add-a-featured-image-to-a-forum/#post-171825' was loaded over HTTPS, but requested an insecure resource 'http://www.hippressurecooking.com/forum/embed/'. This request has been blocked; the content must be served over HTTPS.

I've just had a Slack chat with @pento regarding the above and he believes that 35894.diff will also fix this issue, in that rather than the embed not showing at all a link will be shown instead which works for me :+1

This ticket was mentioned in Slack in #bbpress by netweb. View the logs.


8 years ago

#6 in reply to: ↑ 4 @swissspidy
8 years ago

In a somewhat related issue, over on https://bbpress.org if a user adds a link to their site that is http:// and *not* https:// then the embed will not show in Google Chrome due to Mixed Content error/warning.

See also #28507

#7 @swissspidy
8 years ago

  • Owner set to swissspidy
  • Resolution set to fixed
  • Status changed from new to closed

In 36648:

Embeds: Only display an iframe when it was successfully loaded.

This prevents showing a blank iframe by first checking if a message was successfully received from it.

Fixes #35894.

#8 follow-up: @swissspidy
8 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#9 in reply to: ↑ 8 @netweb
8 years ago

Noting, reopened for the 4.4.3 milestone/branch for pending branch commit

#10 follow-up: @ocean90
8 years ago

After [36648] Firefox doesn't display the site icon and featured image anymore. Restoring source.style.display = ''; fixes it for @DrewAPicture and me.

Last edited 8 years ago by ocean90 (previous) (diff)

#11 @swissspidy
8 years ago

Okay, that's strange! So the iframe gets correctly shown after the embedding site receives a message, but the assets won't load?

@swissspidy
8 years ago

#12 in reply to: ↑ 10 @swissspidy
8 years ago

Replying to ocean90:

After [36648] Firefox doesn't display the site icon and featured image anymore. Restoring source.style.display = ''; fixes it for @DrewAPicture and me.

Restoring that line would basically be a revert of the fix.

I think I tracked it down to this bug in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=548397

When an iframe is hidden using display: none;, window.getComputedStyle() is null and assets are not loaded because they don't have any width / height set.

This can be avoided by using an alternate approach to initially hide the iframe, like position: absolute; clip: rect(1px, 1px, 1px, 1px);.

Patch attached.

#14 @DrewAPicture
8 years ago

  • Keywords commit added

#15 @swissspidy
8 years ago

In 36708:

Embeds: Use a more accessible way to initially hide the iframe.

This fixes a bug in Firefox where assets inside the iframe aren't being displayed because they have no computed style.

See #35894.

#16 @swissspidy
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 37093:

Embeds: Improve how iframes are loaded after being initially hidden.

Use a more accessible way to initially hide the iframe. After that, only display an iframe when it was successfully loaded.

Merge of [36648] and [36708] to the 4.4 branch.

Fixes #35894.

Note: See TracTickets for help on using tickets.