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

[gradyetc]

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.

[nacin]

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.

[gradyetc]

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

[gradyetc]

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...

[gradyetc]

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

[gradyetc]

Added test to expose bug for non-multisite installation

[nacin]

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).

[wonderboymusic]

Refreshed and added the bit in Nacin's last comment

[gcorne]

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.

[wonderboymusic]

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.