WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 5 months ago

#34335 reviewing defect (bug)

URL shorteners ( redirects) should to be supported by the embed handling for couple of hops

Reported by: pbearne Owned by: swissspidy
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Embeds Keywords: has-unit-tests needs-patch
Focuses: Cc:
PR Number:

Description (last modified by DrewAPicture)

It should be possible to use URL shorteners ( redirects) to load embeds.

I have been testing the new embeds feature and have found that a Bit.ly link doesn't work, but the resolved link does

I believe we should support at least one hop if not two

Attachments (2)

34335.diff (3.0 KB) - added by stephenharris 3 years ago.
34335.2.diff (3.4 KB) - added by swissspidy 3 years ago.

Download all attachments as: .zip

Change History (16)

#1 @SergeyBiryukov
4 years ago

  • Component changed from Rewrite Rules to Embeds

#2 @swissspidy
4 years ago

  • Keywords 2nd-opinion added

Thanks for creating the ticket after our recent chat.

Currently, when pasting a URL like http://bit.ly/1Muwesb (a YouTube video), http://www.youtube.com/oembed?url=http://bit.ly/1Muwesb&format=json gets called in the end, which of course doesn't work. That's because WordPress follows the redirect to YouTube, but passes the original URL to the endpoint instead of the final redirect URL.

To make this work, we'd need to store the final redirect URL, which is currently not possible with the HTTP API. This means there needs to be an equivalent toWP_Http::handle_redirects that doesn't just redirect, but returns the final URL.

This would certainly make things more complex and I'm not sure if it's worth the effort and even the desired behaviour.

#3 @pbearne
4 years ago

I can see the problem.

How is the embed code going to know that http://bit.ly/1Muwesb is a youtube video short of having a white list of shorteners and then calling the URL in order to get the end URL when we when calling for the embed at that point. This would be a lot of traffic and we would have to do some caching to manage the load.

This would be nice maybe the answer would be to only follow URL's that are wrapped in an [embed] shortcode

I can not see this request going anywhere soon

Paul

#4 @johnbillion
4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

Closing based on the above. It'll be a lot of effort for little gain.

#5 @stephenharris
3 years ago

@swissspidy I've come across this issue in another context. Do we actually need to store the final URL?

In the YouTube example above, and with WordPress, the discovered endpoint URL includes the ?url part, but WordPress over-writes it with the URL originally provided: https://github.com/WordPress/WordPress/blob/55c3fa0cb0637c85f89b225b3fde8d0256a9e463/wp-includes/class-oembed.php#L460

Is that line necessary? The oembed discovery documentation (http://oembed.com/#section4) states that the provider is responsible for providing a full embed endpoint (including URL).

Removing that line will fix the issue for embedding redirected urls, and shouldn't impact other cases.

#6 @swissspidy
3 years ago

  • Keywords 2nd-opinion removed
  • Milestone set to Awaiting Review
  • Resolution maybelater deleted
  • Status changed from closed to reopened

@stephenharris Good catch. However, the URL part is only there if it was found during oEmbed discovery, but not when wp_oembed_add_provider() is used for registering a provider. So yes, that line is necessary.

But we could try to extract any url param from the $provider variable and only use add_query_arg() if the url param isn't already there.

@stephenharris
3 years ago

#7 @stephenharris
3 years ago

  • Keywords has-patch has-unit-tests added

Ah, of course. Checking for an existing url query variable seems the best way forward. I've attached a patch with tests. Any thoughts?

#8 @swissspidy
3 years ago

  • Milestone changed from Awaiting Review to 4.7
  • Owner set to swissspidy
  • Status changed from reopened to reviewing

Thanks, I'll have a look shortly. Looks good at first glance, but could use a few improvements.

#9 @jorbin
3 years ago

@swissspidy what improvements would you like to see on this patch?

@swissspidy
3 years ago

#10 @swissspidy
3 years ago

In 34335.2.diff:

  • Correct @ticket annotation
  • Use parse_url() with the PHP_URL_QUERY argument.
  • Correctly declare $_provider_url in tests.
  • Improve docs a bit.

#11 @swissspidy
3 years ago

  • Keywords needs-patch added; has-patch removed

One big problem with the current approach is is that when pasting the shortened URL to a YouTube video, WordPress will eventually embed it, but not detect that it's coming from a trusted provider. That means the embedded YouTube video won't work because of the security restrictions WordPress puts on the iframe.

That's because the URL passed to wp_filter_oembed_result() is still the shortened URL. The resolved URL gets lost in WP_oEmbed::fetch().

#12 @swissspidy
3 years ago

  • Version changed from 4.4 to 2.9

#13 @swissspidy
3 years ago

  • Milestone changed from 4.7 to Awaiting Review

#14 @DrewAPicture
3 years ago

  • Description modified (diff)
  • Summary changed from URL shorteners ( redirects) should to be supported by the embed handling for couple of hopes to URL shorteners ( redirects) should to be supported by the embed handling for couple of hops
Note: See TracTickets for help on using tickets.