WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 6 months ago

Last modified 6 months ago

#23149 closed enhancement (fixed)

YouTube Embedding is incorrect for https:// URLs

Reported by: Otto42 Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.5
Component: Embeds Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Reference: #18719, #20102, Conversation from 9-19-2012:
https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2012-09-19&sort=asc#m459455

This is incorrect and a bug. If the user has posted an https link, then we should send the correct parameters to YouTube to get an https value in the resulting HTML returned.

At present, there is no way within WordPress to get the proper https code in a YouTube oEmbed iframe when https is actually desired.

Attachments (8)

23149.diff (1.3 KB) - added by Otto42 18 months ago.
23149a.diff (1.6 KB) - added by JayCC 14 months ago.
modified patch to work on youtu.be links
23149b.diff (1.6 KB) - added by JayCC 11 months ago.
Updating patch to work with wordpress 3.6
23149c.diff (1.6 KB) - added by JayCC 9 months ago.
Updated patch for Wordpress 3.7
23149.2.diff (1.4 KB) - added by adamsilverstein 6 months ago.
refresh
23149.3.diff (1.2 KB) - added by Otto42 6 months ago.
Patch to always use scheme https, regardless of URL
23149-test.diff (1.0 KB) - added by adamsilverstein 6 months ago.
unit test for #23149
23149-test.2.diff (892 bytes) - added by adamsilverstein 6 months ago.
just look for expected https url somewhere in assertion

Download all attachments as: .zip

Change History (37)

Otto4218 months ago

comment:1 nacin18 months ago

  • Type changed from defect (bug) to enhancement

Incorrect? Eye of the beholder. A bug? No, this looks like an enhancement.

  1. The oEmbed spec is oblivious when it comes to https content.
  1. Mixed content would still occur when someone uses an https link but doesn't actually want or expect https content. This, I imagine, is far more common than someone with frontend https who does want https content.
  1. Core does not currently have any frontend https logic baked in.

comment:2 Otto4218 months ago

I disagree. One would expect an obvious behavior, the code does not have that expected behavior. Therefore, it's a bug.

  1. I agree that the spec sucks. YouTube has specifically created a scheme parameter workaround because of this. There is no reason to not to use it.
  1. It is perfectly acceptable to put https content into an http page, and this is not typically considered to be "mixed content". The reverse of putting http into https pages, however, generates mixed content warnings, and that is indeed a problem. So basically, you're saying here that we should not support the case which is an actual problem because the totally-not-a-real-problem case is more common?
  1. The core does not need any https front end logic baked in for it to do the right thing for oembed when it is explicitly given an https URL.

comment:3 TobiasBg18 months ago

I'm with Otto here.

This fix does prevent annoying mixed content warnings on https pages, that basically make the YouTube oEmbed unusable on https sites at this time.

There's no problem to embed a video as https on an http page, so this doesn't do harm. No one will notice. Someone who enters a https URL but then doesn't want or expect https to be embedded is doing it wrong.

I don't care, if it's labeled bug or enhancement, but I'd also like to see this fixed.

comment:4 Asif2BD16 months ago

I also need https return if I am using https link. I have posted about this in a support thread.

Either fix it in core, or give us a function or switch, so we could force WordPress to return with https, maybe via condition in function.php

comment:5 JayCC15 months ago

FWIW, we've tested this patch in our development environment with much success, and will be deploying it to production soon. +1 to have it added to core (don't care much if it's classified as a bug or feature).

comment:6 JayCC15 months ago

  • Cc JayCC added

comment:7 rmccue15 months ago

+1 on this in core, and +1 on it being a bug. It doesn't matter much whether it's extra on top of the oEmbed protocol, since it causes broken behaviour on the frontend regardless. Given how trivial the patch is, it's irrelevant anyway.

JayCC14 months ago

modified patch to work on youtu.be links

comment:8 JayCC14 months ago

We've been using this patch in production for a couple of months now, without issue, however we had some users add youtube videos using the secure version of the shortened URL (https://youtu.be/xxxxxxx) which this patch didn't catch (so the videos were served to visitors on non-secure, causing mixed content warnings). I attached a modified version of the patch that addresses this.

JayCC11 months ago

Updating patch to work with wordpress 3.6

comment:9 markus.magnuson10 months ago

  • Cc markus.magnuson@… added

JayCC9 months ago

Updated patch for Wordpress 3.7

comment:10 martyn_7 months ago

Also hitting this on my https only site.

I disagree with this

The core does not need any https front end logic baked in for it to do the right thing for oembed when it is explicitly given an https URL.

though. If the site is https, then even if the user enters a http link, as is extremely common because they've just copy pasta from another window without any second thought, then it should be converted to https. Why not just have a protocol-relative URL ( //youtube.com/ ) generated, which will then follow the current protocol of the site?

Last edited 7 months ago by SergeyBiryukov (previous) (diff)

comment:11 Otto427 months ago

YouTube's oEmbed doesn't do protocol relative URLs. You have to explicitly tell it the scheme to use, with scheme=https for it to use https links in the returned HTML from their embed endpoint.

comment:12 dbarlett7 months ago

  • Cc dylan.barlett@… added

comment:13 rachelbaker7 months ago

  • Cc rachel@… added

adamsilverstein6 months ago

refresh

comment:14 adamsilverstein6 months ago

23149.2.diff: Refresh against trunk

comment:15 adamsilverstein6 months ago

  • Keywords has-patch dev-feedback added

comment:16 follow-up: nacin6 months ago

  • Milestone changed from Awaiting Review to 3.9

Okay, let's do it. Maybe we should always do scheme=https, regardless of video.

Otto426 months ago

Patch to always use scheme https, regardless of URL

comment:17 follow-up: Otto426 months ago

23149.3.diff is a patch to always use the https scheme, regardless of the URL given.

Whether this is desirable or not is up for debate.

Last edited 6 months ago by nacin (previous) (diff)

comment:18 in reply to: ↑ 16 adamsilverstein6 months ago

Replying to nacin:

Okay, let's do it. Maybe we should always do scheme=https, regardless of video.

Yea, lets do it - keep running into this issue over and over again! I'm not sure about the scheme, or whether https for non https pages would be detrimental in any way.

I would appreciate your tips re: unit testing. I tried to write a unit test for this issue and failed. I tried do_shortcode and also global $wp_embed; = $wp_embed->run_shortcode( '[embed]http://youtube.com/watch?v=AbcDeFg123[/embed]' ); all return null or empty strings.

Any tips on getting embeds to fire in tests? I scoured existing tests for an example without luck. Point me in the right direction and I will whip up a test... thanks

Thanks!

comment:19 follow-up: Otto426 months ago

@adamsilverstein: The shortcode function won't work for testing here because you're not in a normal Loop and so there is no global $post context setup. The shortcode expects there to be a $post because it uses the post_meta to do caching of the oembed result. This is to prevent it from retrieving the info from the remote URL every time. Additionally, you have to use a real working YouTube URL for it to actually get anything back, a fake URL won't have valid response data.

echo wp_oembed_get( 'http://www.youtube.com/watch?v=oHg5SJYRHA0' );

returns:

<iframe width="500" height="375" src="http://www.youtube.com/embed/oHg5SJYRHA0?feature=oembed" frameborder="0" allowfullscreen></iframe>

Last edited 6 months ago by Otto42 (previous) (diff)

comment:20 in reply to: ↑ 19 adamsilverstein6 months ago

Replying to Otto42:

@adamsilverstein: The shortcode function won't work for testing here because you're not in a normal Loop and so there is no global $post context setup. The shortcode expects there to be a $post because it uses the post_meta to do caching of the oembed result. This is to prevent it from retrieving the info from the remote URL every time. Additionally, you have to use a real working YouTube URL for it to actually get anything back, a fake URL won't have valid response data.

echo wp_oembed_get( 'http://www.youtube.com/watch?v=oHg5SJYRHA0' );

returns:

<iframe width="500" height="375" src="http://www.youtube.com/embed/oHg5SJYRHA0?feature=oembed" frameborder="0" allowfullscreen></iframe>

Great! Thank you for the detailed response. I think I tried wp_oembed_get along the way, maybe missed the piece about requiring a valid URL. I will give it another shot...

comment:21 in reply to: ↑ 17 ; follow-up: adamsilverstein6 months ago

Replying to Otto42:

23149.3.diff is a patch to always use the https scheme, regardless of the URL given.

Whether this is desirable or not is up for debate.

The arguments I would have against going all https are: 1. some networks may block https and that would mean users wouldn't see videos and 2. its unexpected behavior - do we do that anywhere else? (serve up https when http has been embedded)? and 3. it's a waste of resources (serving over https vs. http)

adamsilverstein6 months ago

unit test for #23149

comment:22 adamsilverstein6 months ago

23149-test.diff​: a simple unit test verifying that youtube embeds using https (either youtube.com or youtu.be urls) result in an https embedded element. Passes after applying 23149.2.diff​.

comment:23 follow-up: Otto426 months ago

You can't put in the width and height and such, because the return values there will depend on what theme you're running. Check for the url in the iframe only.

adamsilverstein6 months ago

just look for expected https url somewhere in assertion

comment:24 in reply to: ↑ 23 adamsilverstein6 months ago

Replying to Otto42:

You can't put in the width and height and such, because the return values there will depend on what theme you're running. Check for the url in the iframe only.

Doh! Fixed in 23149-test.2.diff​, hope that looks better :)

comment:25 ircbot6 months ago

This ticket was mentioned in IRC in #wordpress-dev by adamsilverstein. View the logs.

comment:26 ircbot6 months ago

This ticket was mentioned in IRC in #wordpress-dev by adamsilverstein. View the logs.

comment:27 nacin6 months ago

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

In 26978:

Ensure that SSL YouTube URLs receive SSL embeds.

props adamsilverstein, Otto42, JayCC.
fixes #23149.

comment:28 in reply to: ↑ 21 westonruter6 months ago

Replying to adamsilverstein:

Replying to Otto42:

23149.3.diff is a patch to always use the https scheme, regardless of the URL given.

Whether this is desirable or not is up for debate.

The arguments I would have against going all https are: 1. some networks may block https and that would mean users wouldn't see videos and 2. its unexpected behavior - do we do that anywhere else? (serve up https when http has been embedded)? and 3. it's a waste of resources (serving over https vs. http)

I just ran into a problem here with a site that is served entirely over HTTPS. If a user uses the URL that YouTube provides, this URL uses HTTP. If you then embed this URL into a site that is served over HTTPS, then it will fail to embed (at least in Chrome) and give the error:

[blocked] The page at 'https://example.com/' was loaded over HTTPS, but ran insecure content from 'http://www.youtube.com/embed/DYu_bGbZiiQ?feature=oembed': this content should also be loaded over HTTPS.

So if we can't always default to using HTTPS, then what about forcing all URLs to be HTTPS if it is determined that the siteurl is HTTPS? Otherwise, I suppose a content filter can be added which checks if is_ssl and replaces all HTTP oEmbeds with HTTPS ones. This would address the situation where some pages of the site are served over HTTP whereas others over HTTPS.

Should this be re-opened or should a new ticket be made?

comment:29 Otto426 months ago

That would be a new ticket, however I think Nacin's original reply holds here for that:

Core does not currently have any frontend https logic baked in.

This is still the case, more or less, because the logic on the https embed here is based on the URL given, not based on whether or not the page is_ssl or not.

If you embed an image with http in a post and view the page over https, then you'll have the same basic error.

Content is kind of the user's domain. Until we have a more generalized approach for munging stuff into https for the general case, there's not really any reason to munge it specifically for the embed case.

Note: See TracTickets for help on using tickets.