Add sniff to check that WP is killed after wp_(safe_)redirect

[jrfnl]

Add sniff to check that any call to wp_redirect() and wp_safe_redirect() is followed by a call to die(), exit or wp_die().

The call is allowed to be conditional.

wp_redirect( $url );
exit;

if ( wp_redirect( $url ) ) {
    exit;
}

Refs:

• https://developer.wordpress.org/reference/functions/wp_redirect/
• https://developer.wordpress.org/reference/functions/wp_safe_redirect/

Reminder: if/when this sniff is pulled, the error message for WordPress.VIP.RestrictedFunctions.wp_redirect_wp_redirect can be shortened to avoid message duplication.

Suggestions for additional functions which should be checked for this welcome!

[johnbillion]

Some thoughts:

The call to exit or die() doesn't have to immediately follow the call to wp_redirect(). Example:

if ( $redirect_to ) {
	wp_safe_redirect( add_query_arg( $args, $redirect_to ) );
} elseif ( ! current_user_can( 'read' ) ) {
	wp_safe_redirect( add_query_arg( $args, home_url() ) );
} else {
	wp_safe_redirect( add_query_arg( $args, admin_url() ) );
}
exit;

I don't think there's any reason to allow wp_die() in addition to exit and die(). It's technically ok to use in Ajax calls, but in non-Ajax calls it outputs HTML which usually isn't desired after a location header.

[westonruter]

I think wp_die() should be always used instead of exit()/die() because the former can be unit-tested whereas the latter cannot.

[johnbillion]

Interesting point!

With that in mind, are redirects always guaranteed to work if there's HTML output after the location header? I guess they are, but I'm not sure.

[westonruter]

@johnbillion Yes, per https://webmasters.stackexchange.com/a/77189

[johnbillion]

Using wp_die() after wp_redirect() is a little more complicated than it initially looks. The correct HTTP response status code needs to be passed to the call, otherwise you'll end up with a redirect header and a 500 status.

Calling wp_die( '', 301 ); works, but then this bypasses the wp_redirect_status filter inside wp_redirect() which could potentially change the status code to something else (usually 302). It's not possible to pass a false-y status code to wp_die() to avoid setting the status header.

[lkraav]

phpmd flags exit expressions, but I guess there's currently no available "clean" resolution to this, other than silencing the warning in docblock, huh?

[jrfnl]

@lkraav This is the repo for the WordPress Coding Standards. Not for phpmd.

[lkraav]

phpmd was only mentioned as the trigger that led me here, other than that it indeed has nothing to do with this issue (aside from possibly leading more googlers here, and finding their peace of mind).

@johnbillion's Nov 2017 analysis of the situation #1205 (comment) might be the most authoritative piece on the subject right now, though, and was super useful.

It's 18 months later now, so my comment's idea was to ping the crew here if perhaps new knowledge has emerged in the meanwhile, but simply hasn't been written up.

[BrianHenryIE]

I was looking to write a sniff to enforce the conditional wrapping of die() or exit.

The wp_redirect filter allows returning false to prevent the redirect.

In cases where exit always runs, returning false here, as catered for in the WordPress code, will result in a white screen.

The other suggested way to structure wp_redirect()+exit with the conditional wrapping wp_redirect()'s return value should be enforced across all WordPress code.

My particular use case is on plugins.php I find it difficult to bulk activate plugins. Often plugins will use wp_redirect() on activation to bring admins to a settings screen. During bulk activation this halts the activation of all plugins alphabetically following the inconsiderate plugin, meaning using select-all, bulk actions, activate does not actually activate all plugins. Where I attempted to use the wp_redirect filter as designed, this didn't achieve what I intended because the plugins were not using the conditional. e.g. my code inside the wp_redirect filter:

if ( 'plugins.php' === $pagenow && false === stristr( $location, 'plugins.php' ) ) { return false; }

This code in pluggable.php:1349 cannot be relied upon to work without a sniff enforcing the conditional style:

if ( ! $location ) {
	return false;
}