WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 4 years 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:

Description

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 6 years ago.
20876.diff (1.2 KB) - added by gradyetc 5 years ago.
Updated patch -- replaces trailingslashit() with home_url('/')
test_includes_rewrite.diff (2.6 KB) - added by gradyetc 5 years ago.
Added test to expose bug for non-multisite installation
20560.2.diff (1.4 KB) - added by wonderboymusic 4 years ago.
20560.3.patch (932 bytes) - added by gcorne 4 years ago.
rewrite.php (3.7 KB) - added by gcorne 4 years ago.

Download all attachments as: .zip

Change History (18)

@gradyetc
6 years ago

#1 @SergeyBiryukov
6 years ago

  • Keywords has-patch added

#2 @nacin
5 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.

@gradyetc
5 years ago

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

#3 @gradyetc
5 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...

#4 @gcorne
5 years ago

  • Cc gcorne@… added

#5 @gradyetc
5 years ago

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

Last edited 5 years ago by gradyetc (previous) (diff)

#6 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 3.5

@gradyetc
5 years ago

Added test to expose bug for non-multisite installation

#7 @ryan
5 years ago

  • Keywords commit added

#8 @nacin
5 years 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).

#9 @nacin
5 years ago

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

#10 @wonderboymusic
4 years ago

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

Refreshed and added the bit in Nacin's last comment

@gcorne
4 years ago

@gcorne
4 years ago

#11 @gcorne
4 years 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.

#12 @wonderboymusic
4 years 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.