Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#39373 closed defect (bug) (fixed)

url_to_postid() returning post ID for external URL

Reported by: jkhongusc's profile jkhongusc Owned by: swissspidy's profile swissspidy
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.5.3
Component: Embeds Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

One our site has a post that has a SoundCloud embed in it. The WP embed is embedding the post instead of the SoundCloud content. See bug#37359 as another person reported but flagged the wrong function.

Unfortunately I dont have a fix. This is what I see, the post (https://bedrosian.usc.edu/podcast/drown/) embed eventually makes this call:

url_to_postid ('https://soundcloud.com/uscbedrosian/drown')

This is obviously (to human eyes) an external url but url_to_postid() returns the postid of the post itself. This site uses rewrites, I see url_to_postid() make this database call:

SELECT wp_334_posts.* FROM wp_334_posts WHERE 1=1 AND wp_334_posts.post_name = 'drown' AND wp_334_posts.post_type = 'post' ORDER BY wp_334_posts.post_date DESC " posts?

Due to rewrites, all it has to match is 'drown' to be flagged as an internal url... and returns the original post. Happens to all SoundCloud embeds on the same site.

Attachments (3)

post_to_id-embed-sample.png (272.7 KB) - added by troutacular 7 years ago.
Sample iframe returned for Soundcloud oEmbed
39373.patch (1.9 KB) - added by ivankristianto 7 years ago.
Check if the $url is from current domain
39373.diff (1.5 KB) - added by swissspidy 6 years ago.

Download all attachments as: .zip

Change History (16)

@troutacular
7 years ago

Sample iframe returned for Soundcloud oEmbed

#1 @troutacular
7 years ago

As jkhongusc described, the iframe being returned was being altered to being the post itself. Due to this site being a production site, we have applied a filter patch as noted in 37359:

remove_filter( 'pre_oembed_result', 'wp_filter_pre_oembed_result' );

The iframe source being returned before this patch was https://bedrosian.usc.edu/podcast/drown/embed/#?secret=z5fm4h1xpp

<iframe sandbox="allow-scripts" security="restricted" src="https://bedrosian.usc.edu/podcast/drown/embed/#?secret=z5fm4h1xpp" width="200" height="339" title="“Drown” — Bedrosian Center" frameborder="0" marginwidth="0" marginheight="0" scrolling="no" class="wp-embedded-content" data-secret="z5fm4h1xpp"></iframe>

#2 @swissspidy
7 years ago

  • Version changed from 4.7 to 4.5.3

Hey there,

Thanks for your report. This seems like a duplicate of #37359, but yet I haven't been able to reproduce this in a unit test.

However, I can see that url_to_postid() doesn't really check if the domain matches, so there is a potential for false positives. Such a check might need to be added to wp_filter_pre_oembed_result() or url_to_postid() directly.

#3 @swissspidy
7 years ago

  • Keywords dev-feedback needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Summary changed from url_to_postid() returning postid for external url to url_to_postid() returning post ID for external URL

As per my previous comment, I'm not sure yet what's the best approach.

#4 @swissspidy
7 years ago

#40368 was marked as a duplicate.

#5 @swissspidy
7 years ago

#40372 was marked as a duplicate.

#6 @swissspidy
7 years ago

  • Milestone changed from Future Release to 4.8

#7 @swissspidy
7 years ago

  • Milestone changed from 4.8 to 4.8.1
  • Owner set to swissspidy
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#9 @jbpaul17
7 years ago

  • Milestone changed from 4.8.1 to 4.9

Per today's bug scrub and resulting discussion in a thread in #core, we're punting this to 4.9.

@ivankristianto
7 years ago

Check if the $url is from current domain

#10 @ivankristianto
7 years ago

I agree to do direct check in url_to_postid so it won't cause any future false positive anymore.

I attached my patch above.

#11 @welcher
7 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

@swissspidy
6 years ago

#12 @swissspidy
6 years ago

  • Keywords dev-feedback removed

Simplified the patch a bit in 39373.diff. Would be great to have this in trunk as soon as possible for testing.

#13 @SergeyBiryukov
6 years ago

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

In 41786:

Rewrite: In url_to_postid(), bail early if the URL does not belong to the site.

Props ivankristianto, swissspidy, jkhongusc, SergeyBiryukov.
Fixes #39373.

Note: See TracTickets for help on using tickets.