Make WordPress Core

Opened 2 years ago

Closed 8 months ago

#20560 closed defect (bug) (fixed)

url_to_postid() string matching is not strict enough, can lead to incorrect results

Reported by: gradyetc Owned by: wonderboymusic
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.0
Component: Query Keywords: has-patch needs-testing
Focuses: Cc:


I have been working on a plugin that uses url_to_postid() to automatically translate user generated links to internal links utilizing post ID in order to protect against potential permalink changes.

A bug was reported that involved a link to an external location being translated to an internal link (with post ID) by the plugin.

The current site: http://www.example.com/test

The attempted link: http://www.example.com/test-research

Where test-research is an entirely different site on our multi-site install.

Upon investigation, the source of the problem was an overly generous strpos() check in url_to_postid(), starting at line 322:

if ( false !== strpos($url, home_url()) ) {
	// Chop off http://domain.com
	$url = str_replace(home_url(), '', $url);
} else {
	// Chop off /path/to/blog
	$home_path = parse_url(home_url());
	$home_path = isset( $home_path['path'] ) ? $home_path['path'] : '' ;
	$url = str_replace($home_path, '', $url);

As home_url() does not append a trailing slash, the link passed that check and was incorrectly processed. The logic inside that if block resulted in -research being passed as a pagename to WP_Query, which happened to turn up a matching post ID on this particular site.

I have a proposed patch, which I will attach to this ticket asap.

Attachments (6)

20560.diff (1.2 KB) - added by gradyetc 2 years ago.
20876.diff (1.2 KB) - added by gradyetc 2 years ago.
Updated patch -- replaces trailingslashit() with home_url('/')
test_includes_rewrite.diff (2.6 KB) - added by gradyetc 21 months ago.
Added test to expose bug for non-multisite installation
20560.2.diff (1.4 KB) - added by wonderboymusic 8 months ago.
20560.3.patch (932 bytes) - added by gcorne 8 months ago.
rewrite.php (3.7 KB) - added by gcorne 8 months ago.

Download all attachments as: .zip

Change History (18)

gradyetc2 years ago

comment:1 SergeyBiryukov2 years ago

  • Keywords has-patch added

comment:2 nacin2 years ago

  • Keywords needs-unit-tests added
  • Version changed from 3.4 to 3.0

Definitely not new - home_url() was I think 3.0, so we can go with that.

home_url( '/' ) can be used as an alternative to trailingslashit().

Looks good. Unit tests would be nice.

gradyetc2 years ago

Updated patch -- replaces trailingslashit() with home_url('/')

comment:3 gradyetc2 years ago

  • Keywords needs-unit-tests removed

Thanks for the response -- I have updated my patch to use home_url('/') instead of trailingslashit() per your recommendation.

I have also attached a new unit test case file for url_to_postid(). AFAICT no file existed to test anything in wp-includes/rewrite.php, so I made a new one and put in a few tests for url_to_postid().

The exact nature of this bug made it a bit tricky to test against, as it takes somewhat of a perfect storm to occur (non-root/subdirectory site url path, specific post names existing, etc.). As a result, the test against this bug requires the -m (multisite) switch to be on, though the bug itself is not limited to multi-site installs.

I am open to suggestions on how to improve that, but at least for now I think it's better than what existed before...

comment:4 gcorne21 months ago

  • Cc gcorne@… added

comment:5 gradyetc21 months ago

I have updated the previously attached unit tests patch to work with the recently updated test framework. My comment above comment above remains relevant.

Version 0, edited 21 months ago by gradyetc (next)

comment:6 SergeyBiryukov21 months ago

  • Milestone changed from Awaiting Review to 3.5

gradyetc21 months ago

Added test to expose bug for non-multisite installation

comment:7 ryan18 months ago

  • Keywords commit added

comment:8 nacin18 months ago

I think the only change that actually needs to occur is ( false !== strpos( $url, home_url() ) ). Should this become ( false !== strpos( trailingslashit( $url ), home_url( '/' ) ) )? We don't want to avoid matching a URL that is the same as the home_url() but minus the trailing slash (as the examples in the ticket description, ironically).

comment:9 nacin18 months ago

  • Keywords commit removed
  • Milestone changed from 3.5 to Future Release

wonderboymusic8 months ago

comment:10 wonderboymusic8 months ago

  • Keywords needs-testing added
  • Milestone changed from Future Release to 3.7

Refreshed and added the bit in Nacin's last comment

gcorne8 months ago

gcorne8 months ago

comment:11 gcorne8 months ago

Nacin's suggestion that only the if ( false !== strpos($url, home_url()) )} condition needed to be modified to address the issue was correct, but as I was looking more closely at the logic, I noticed that there were a couple related problems with how a server-relative url is handled. The additional cases are illustrated in the refreshed set of unit tests in rewrite.php and addressed in 20560.3.patch.

I also am pretty sure that the condition if ( !empty($url) && ($url != $request) && (strpos($match, $url) === 0) ) later on will never return true so it and the subsequent statement is unnecessary.

comment:12 wonderboymusic8 months ago

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

In 25258:

Avoids incorrect results when url_to_postid() checking is not strict enough. Adds a bunch of Unit Tests for `url_to_postid()', which is currently only tested lightly in some XML-RPC tests.

Props gcorne, gradyetc.
Fixes #20560.

Note: See TracTickets for help on using tickets.