Make WordPress Core

Opened 10 years ago

Closed 9 years ago

#39373 closed defect (bug) (fixed)

url_to_postid() returning post ID for external URL

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

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 10 years ago.
Sample iframe returned for Soundcloud oEmbed
39373.patch (1.9 KB ) - added by ivankristianto 9 years ago.
Check if the $url is from current domain
39373.diff (1.5 KB ) - added by swissspidy 9 years ago.

Download all attachments as: .zip

Change History (16)

@troutacular
10 years ago

Sample iframe returned for Soundcloud oEmbed

#1 @troutacular
10 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
10 years ago

  • Version 4.74.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
10 years ago

  • Keywords dev-feedback needs-patch added
  • Milestone Awaiting ReviewFuture Release
  • Summary url_to_postid() returning postid for external urlurl_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
9 years ago

#40368 was marked as a duplicate.

#5 @swissspidy
9 years ago

#40372 was marked as a duplicate.

#6 @swissspidy
9 years ago

  • Milestone Future Release4.8

#7 @swissspidy
9 years ago

  • Milestone 4.84.8.1
  • Owner set to swissspidy
  • Status newassigned

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


9 years ago

#9 @jbpaul17
9 years ago

  • Milestone 4.8.14.9

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

@ivankristianto
9 years ago

Check if the $url is from current domain

#10 @ivankristianto
9 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
9 years ago

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

@swissspidy
9 years ago

#12 @swissspidy
9 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
9 years ago

  • Resolutionfixed
  • Status assignedclosed

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.