#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)
Change History (48)
#2
@
12 years ago
I disagree. One would expect an obvious behavior, the code does not have that expected behavior. Therefore, it's a bug.
- 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.
- 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?
- 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.
#3
@
12 years 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.
#4
@
12 years 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
#5
@
12 years 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).
#7
@
12 years 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.
#8
@
12 years 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.
#10
@
11 years 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?
#11
@
11 years 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.
#14
@
11 years ago
23149.2.diff: Refresh against trunk
#16
follow-up:
↓ 18
@
11 years ago
- Milestone changed from Awaiting Review to 3.9
Okay, let's do it. Maybe we should always do scheme=https, regardless of video.
#17
follow-up:
↓ 21
@
11 years 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.
#18
in reply to:
↑ 16
@
11 years 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!
#19
follow-up:
↓ 20
@
11 years 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>
#20
in reply to:
↑ 19
@
11 years 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...
#21
in reply to:
↑ 17
;
follow-up:
↓ 28
@
11 years 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)
#22
@
11 years 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.
#23
follow-up:
↓ 24
@
11 years 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.
#24
in reply to:
↑ 23
@
11 years 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 :)
This ticket was mentioned in IRC in #wordpress-dev by adamsilverstein. View the logs.
11 years ago
This ticket was mentioned in IRC in #wordpress-dev by adamsilverstein. View the logs.
11 years ago
#27
@
11 years ago
- Owner set to nacin
- Resolution set to fixed
- Status changed from new to closed
In 26978:
#28
in reply to:
↑ 21
@
11 years 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?
#29
@
11 years 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.
Incorrect? Eye of the beholder. A bug? No, this looks like an enhancement.