Make WordPress Core

Opened 12 months ago

Last modified 4 months ago

#60208 new enhancement

Prevent redirect loops

Reported by: kkmuffme's profile kkmuffme Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: has-patch dev-feedback has-unit-tests
Focuses: Cc:

Description

There are all kinds of issues open that deal with redirect loops and their patches try to resolve the symptom.

I propose to instead fix the root cause by checking in wp_redirect/wp_safe_redirect if the $location is equal to the current host/request uri and if that's the case to not execute the redirect but wp_die - just like it's done in those function if response code is not in the 3xx range.

If a rogue plugin causes this, this will at least give you a hint of what was happening/where to start, when your WP page suddenly starts to reload indefinitely.

Change History (12)

This ticket was mentioned in PR #6017 on WordPress/wordpress-develop by @kkmuffme.


12 months ago
#1

  • Keywords has-patch added

Prevent redirect loops when trying to redirect to the current URL, except when changing http to https

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


11 months ago

#3 @swissspidy
11 months ago

  • Keywords needs-unit-tests needs-testing added

WordPress server setups can be vastly different (think reverse proxies, containers, headless setups, etc.). This seems like a test that requires a good amount of testing to ensure this won't break anything.

#4 @kkmuffme
11 months ago

Testing for what specifically? The $_SERVER superglobals?

If they aren't set, then the code that was added by the PR will not do anything and it will behave as before. It will only change the behavior if they're set.

#5 @swissspidy
11 months ago

  • Keywords dev-feedback added

We would want to verify that this code does what it promises to do, right, so this can be tested with unit tests but also with manual testing.

#6 @kkmuffme
11 months ago

so this can be tested with unit tests

Not possible, since unit tests run with PHP-CLI which does not populate INPUT_SERVER (see PHP core for why)

#7 @kkmuffme
11 months ago

Which is the reason why wp_redirect does not have any unit tests in the first place - impossible to test a redirect in CLI, isn't it?

#8 @kkmuffme
11 months ago

I will separate it out into a function and modify it, so it will just use the superglobals if sapi === CLI
filter_input function doesn't exist, so we'll have a unit test.

Will take care of that tomorrow.

#9 @kkmuffme
10 months ago

  • Keywords has-unit-tests added; needs-unit-tests needs-testing removed

Found a way to test it (or at least the error case where it should not redirect), and removed FILTER_INPUT since that is moved to a separate ticket, since it affects lots of places anyway.

Failing tests are unrelated. It's ready for a review and merge.

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


10 months ago

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


8 months ago

#12 @kkmuffme
4 months ago

A common case I found is where the url is an admin URL (e.g. /wp-admin/) but the request came in via a frontend PHP handler, e.g. DOCUMENT_URI="/index.php"

The add_action( 'template_redirect', 'wp_redirect_admin_locations', 1000 ); callback will create an infinite redirect loop via this particular part of the code in that callback

`
if ( in_array( untrailingslashit( $_SERVERREQUEST_URI? ), $admins, true ) ) {

wp_redirect( admin_url() );
exit;

}
`

Note: See TracTickets for help on using tickets.