WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 12 months ago

#21602 new defect (bug)

redirect_canonical can lead to infinite loop on index navigation if site url is not all lower case

Reported by: sreedoap Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Canonical Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

The function redirect_canonical in wp-includes/canonical.php (WordPress 3.4.1) on line 406 and 422 makes the following check:

if ( !$redirect_url || $redirect_url == $requested_url )
		return false;

This ensures that it does not attempt to redirect you to the page you requested in the first place. However this function is not case sensitive so if the redirect URL is in a different case than the requested URL then the user can enter an infinite redirect loop. (For example if the Site Address (URL) of the site is set to be in all upper case.)

This function should do a case-insensitive string comparison since domain names are case-insensitive.

The issue only appears to happen with certain plugins installed (ShareThis and PilotPress both led to this issue,) I haven't figured out yet why it's only an issue with certain plugins but it should still be fixed in WordPress to make the proper string comparison.

Attachments (2)

canonical.php.patch (959 bytes) - added by sreedoap 20 months ago.
21602.patch (1.5 KB) - added by SergeyBiryukov 20 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 sreedoap20 months ago

Correction: The "Theme My Login" plugin is one that will lead to this issue.

sreedoap20 months ago

comment:2 sreedoap20 months ago

  • Keywords has-patch added

comment:3 SergeyBiryukov20 months ago

  • Component changed from General to Canonical

comment:4 sreedoap20 months ago

Narrowed this down a bit.. this will not happen with the default permalink setting but will happen with all the other ones (Post name, Numeric, etc.)

comment:5 sreedoap20 months ago

Alright.. I've compiled below a complete list of steps to reproduce. Turns out this only will happen if there is a plugin activated that uses the is_front_page() function.

1) Change the URL of the site address/wordpress address setting to have uppercase characters (for example testsite.example.com to TESTSITE.example.com)

2) Change the permalinks setting to an option besides the default (for example post name)

3) In settings > reading change the "front page displays" setting to a static page for the front page.

3) Install the following plugin: https://gist.github.com/9649e00c6acec8805f39

4) Attempt to access the sites homepage, a infinite redirect loop will occur.

Last edited 20 months ago by sreedoap (previous) (diff)

comment:6 sreedoap20 months ago

Sorry, forgot to include one step to reproduce.. in settings > reading change the "front page displays" setting to a static page for the front page.

Last edited 20 months ago by sreedoap (previous) (diff)

comment:7 follow-up: nacin20 months ago

Confirmed.

All that is necessary is this:

add_action( 'wp', function() {
   is_front_page();
});

This is a tricky one.

I haven't tracked down what exactly fails to trigger an infinite redirect, but I do know why is_front_page() is the trigger. In canonical, there is this conditional:

} elseif ( is_page() && !is_feed() && isset($wp_query->queried_object) && 'page' == get_option('show_on_front') && $wp_query->queried_object->ID == get_option('page_on_front') && ! $redirect_url ) {
			$redirect_url = home_url('/');

This is designed to redirect a /front-page/ page that is assigned to the page_on_front to home_url(). But, since it checks if $wp_query->queried_object is set, it doesn't actually work. is_front_page() calls is_page() with the right arguments to trigger a proper get_queried_object(). This then causes the conditional to run.

As to why it spirals into a redirect loop, home_url() returns something different than what was submitted, and all that jazz.

I see the need for multiple unit tests here, both to cover front-page -> / redirect failures, and the domain issue.

SergeyBiryukov20 months ago

comment:8 in reply to: ↑ 7 SergeyBiryukov20 months ago

Replying to nacin:

I haven't tracked down what exactly fails to trigger an infinite redirect

home_url('/') returns http://trunk.WordPress/, wp_redirect() fires, browser converts that to http://trunk.wordpress/, and the loop starts again.

There's a block in line 375 (introduced in [6097] for #4773) to prevent exactly this situation:
http://core.trac.wordpress.org/browser/tags/3.4.1/wp-includes/canonical.php#L375

However, it doesn't seem to work as intended. As far as I can see, the purpose of the block is to discard the differences in host capitalization, so there shouldn't be a redirect in this case.

What happens instead is $compare_original and $compare_redirect end up being the same, $redirect_url is not rewritten, and the redirect proceeds.

21602.patch moves the block lower, after $compare_original and $compare_redirect are filled.

comment:9 SergeyBiryukov20 months ago

  • Keywords needs-unit-tests added

comment:10 meloniq12 months ago

  • Cc meloniq@… added
Note: See TracTickets for help on using tickets.