Make WordPress Core

Opened 8 years ago

Last modified 5 years ago

#34028 reopened defect (bug)

wp_safe_redirect can return admin_url() when get_admin_url() is used

Reported by: layotte's profile layotte Owned by: aaroncampbell's profile aaroncampbell
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: needs-patch needs-unit-tests
Focuses: Cc:

Description

Setup your site like this:

WordPress Address (URL): http://yourdomain.tld/ (without www)
Site Address (URL): http://www.yourdomain.tld/ (with www)

Example code (yes I know it's stupid code, but it's a working proof-of-concept):

<?php
function unsafe_safe_redirect() {
	$pagenow   = empty( $GLOBALS['pagenow'] ) ? false : $GLOBALS['pagenow'];

	if ( empty( $pagenow ) || 'post-new.php' != $pagenow )
		return;

	// Redirect for add new screen
	if ( 'post-new.php' == $pagenow ) {
		wp_safe_redirect( get_admin_url() . 'plugins.php' );
		die();
	}
}
add_action( 'admin_init', 'unsafe_safe_redirect' );

Visit: http://yourdomain.tld/wp-admin/post-new.php
It will redirect you to: http://yourdomain.tld/wp-admin/
But it should have redirected you to: http://yourdomain.tld/wp-admin/plugins.php

The problem is that wp_validate_redirect() uses home_url() which can be different from site_url() which is used by get_admin_url().

I propose that we use both in wp_validate_redirect(). Diff attached.

Attachments (3)

34028.diff (2.4 KB) - added by layotte 8 years ago.
34028.2.diff (1.2 KB) - added by layotte 8 years ago.
Patch to only modify the lines related to this ticket
34028.3.diff (755 bytes) - added by mdawaffe 8 years ago.
Correct logic

Download all attachments as: .zip

Change History (12)

@layotte
8 years ago

#1 @layotte
8 years ago

  • Keywords has-patch needs-testing dev-feedback added

@layotte
8 years ago

Patch to only modify the lines related to this ticket

#2 @obenland
8 years ago

  • Version trunk deleted

#3 @aaroncampbell
8 years ago

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

In 35792:

Consider both home and site domains to be valid in wp_validate_redirect().

Props layotte.
Fixes #34028.

#4 @netweb
8 years ago

  • Milestone changed from Awaiting Review to 4.5

@mdawaffe
8 years ago

Correct logic

#5 @mdawaffe
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I believe the logic in [35792] is incorrect and causes a regression for #5114. Can someone please double check?

If

  1. your home and site URLs contain uppercase characters, and
  2. your home and site URLs are different (as in the www. example in this ticket), and
  3. the (valid) URL passed to wp_validate_redirect() is all lowercased, then

the redirect will fail. This is because one of the strtolower( $site['host'] ) or strtolower( $wpp['host'] ) checks will always fail (since the domains are different) and the two are separated by an ||. We're only interested if they both fail: they should be separated by an &&.

To test:

  1. Set WordPress Address (URL): http://yourDOMAIN.tld/ (mixed case and without www)
  2. Set Site Address (URL): http://www.yourDOMAIN.tld/ (mixed case and with www)
  3. Use the provided test script, but modify the redirect to use strtolower():
    wp_safe_redirect( strtolower( get_admin_url() ) . 'plugins.php' );
    

Updated in attachment:34028.3.diff

By the way, it seems like we should also call strtolower() on the input URL's host ($lp['host']) and the $allowed_hosts array if we're going to do this at all. @markjaquith, do you recall why you went with [6219] over attachment:pluggable.php.diff:ticket:5114?

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


8 years ago

#7 @jorbin
8 years ago

In 36445:

Revert [35792]

This causes a regression and causes redirects to potentially fail.

See #5114 #34028
props ocean90

#8 @ocean90
8 years ago

  • Keywords needs-patch added; has-patch removed

Note that in multisite there is filter on allowed_redirect_hosts named redirect_this_site() which only returns the site URL.

#9 @ocean90
8 years ago

  • Keywords needs-unit-tests added; needs-testing dev-feedback removed
  • Milestone changed from 4.5 to Future Release
Note: See TracTickets for help on using tickets.