WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#34451 closed defect (bug) (fixed)

Embeds not working for non-WP sites

Reported by: swissspidy Owned by: pento
Milestone: 4.4 Priority: normal
Severity: major Version: 4.4
Component: Embeds Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

While testing #34429 @peterwilsoncc noticed that the new embeds feature doesn't work for non-WordPress sites. In fact, it seems it didn't work for quite some time, I just didn't test it properly.

The problem is that the secret shared between the iframe and the host site is added via PHP by WordPress. It should be done in the JavaScript part to ensure it works for any website.

Luckily, that's not hard to fix. The attached patch solves it in a backwards-compatible manner.

Attachments (5)

34451.diff (3.9 KB) - added by swissspidy 3 years ago.
34451.2.diff (7.9 KB) - added by swissspidy 3 years ago.
34451.3.diff (9.3 KB) - added by swissspidy 3 years ago.
34451.4.diff (7.1 KB) - added by swissspidy 3 years ago.
34451.5.diff (8.1 KB) - added by swissspidy 3 years ago.

Download all attachments as: .zip

Change History (17)

@swissspidy
3 years ago

#1 @peterwilsoncc
3 years ago

Note: #34204 will require a refresh if this is committed first.

#2 @pento
3 years ago

  • Keywords needs-refresh added

This solution causes a race condition - if the iframe HTML loads before the parent DOMDocumentLoaded is fired, the iframe JS will try to grab the secret before its set in the URL.

If the secret hasn't been added, the iframe should sleep and retry later.

#3 @peterwilsoncc
3 years ago

iframes[ i ].src += '#?secret=' + secret; 
iframes[ i ].setAttribute( 'data-secret', secret ); 

These lines will be a problem if multiple embeds are on one page, the iframe URL will have the secret appended for each instance of the embed code. A check for data-secret before appending should fix this.

if data-secret exists, you can also continue; as that increment of the loop does not need to run.

if ( !!iframes[ i ].getAttribute( 'security' ) && (isIE10 || isIE11) ) { 

Worth reversing the attribute and IE check here, no need to look up the DOM if not IE10-11.

Could also be worth overloading the selector to iframe.wp-embedded-content to help avoid any coincidental hits.

@swissspidy
3 years ago

#4 @swissspidy
3 years ago

  • Keywords needs-testing added; needs-refresh removed

34451.2.diff is an updated patch with the improvements suggested above.

  • The getSecret() function in the iframe re-gets the secret when it was later on.
  • Change order in the IE checks.
  • Continue when data-secret attribute already exists.
  • Fixes #34462 by hiding/displaying the embed content during onLoad

Note: The editor preview seems to be broken, but will be fixed by #34204

#5 follow-up: @pento
3 years ago

We need to add the class in wp_filter_oembed_result(), not get_post_embed_html(). (Which reminds me - the same should be happening for the iframe.) We don't trust classes from the embedding site, as they could cause clickjacking-type side effects.

#6 in reply to: ↑ 5 @swissspidy
3 years ago

Replying to pento:

We need to add the class in wp_filter_oembed_result(), not get_post_embed_html(). (Which reminds me - the same should be happening for the iframe.) We don't trust classes from the embedding site, as they could cause clickjacking-type side effects.

Even better to add it in both places, so it works for non-WP sites. Will whip up a new patch. Anything else?

#7 @pento
3 years ago

That's all off the top of my head, but it's Friday evening, I'm all #yolofriday-ed out. :-)

@swissspidy
3 years ago

#8 @swissspidy
3 years ago

34451.3.diff is an updated patch as per the above comment. Slightly changed the JS in wp-embed.js to correctly remove the security attribute when in Internet Explorer. Tested with IE10+.

Last edited 3 years ago by swissspidy (previous) (diff)

@swissspidy
3 years ago

#9 @swissspidy
3 years ago

Seems like the reason for the broken editor preview was that the secret wasn't being added to new iframes (since it only does so in onLoad.

34451.4.diff fixes this by not removing that part from wp_filter_oembed_result() to make the experience for both WP sites and non-WP sites the same, without breaking already existing embeds.

The only concern now would be that getSecret() could lead to an infinite loop when there's never a secret added to the iframe.

#10 @swissspidy
3 years ago

Just saw that _oembed_filter_feed_content was recently added. That string replacement needs to be changed too (and tested!) because of the added class name.

@swissspidy
3 years ago

#11 @swissspidy
3 years ago

  • Keywords needs-testing removed
  • Owner set to pento
  • Status changed from new to assigned

34451.5.diff is an updated patch that now also fixes _oembed_filter_feed_content().

@pento Can you do a final review?

#12 @pento
3 years ago

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

In 35577:

Embeds: Fix support for embedding in non-WordPress sites.

This moves the last of the iframe message code from PHP to JavaScript, so it can be included in any site, without needing to rely on any of WordPress' internal behaviour.

Props swissspidy.

Fixes #34451.

Note: See TracTickets for help on using tickets.