wp_redirect() doesn't exit

[iandunn]

As discussed in #15518, not exit()'ing after a redirect can be a security vulnerability and also lead to unexpected behavior. I think that most developers assume that the API would take care of that for them, since that would be the best practice, so they don't do it in their own code.

There are some cases where features need to redirect without exiting, though, so the API needs to support both cases. Ideally wp_redirect() should exit() by default, but that would cause too many backwards-compatibility issues. So, I'd propose making these changes:

• Add a new boolean parameter to wp_redirect() that determines if it should exit() or not. It's false by default. If it's passed in as true, then exit() is called at the end of the function. Having it set to false by default avoids the backwards-compatibility issues.
• Add the new boolean parameter to wp_safe_redirect() also.
• Create new wp_redirect_exit() function that is a wrapper for a wp_redirect() and passes in a true value for the new parameter. Then, promote this new function on the Codex and other places to inform developers that they should be using it unless they actually need to execute code after the redirect.
• Also create wp_safe_redirect_exit() in the same way.

Another problem here is that wp_redirect() is a pluggable function, which means a plugin can override it. That means there's no guarantee that it will support a new argument when we call it. It'd be nice to un-plug most or all of said functions, but we can't very easily.

Thus, rather than a parameter, it would need to be:

function wp_redirect_and_exit( $location, $status = 302 ) {
    wp_redirect( $location, $status );
    exit;
}

wp_redirect_and_exit() is a pretty terrible function name. Not to mention it's longer than typing out the two calls separately. How about wp_do_redirect()?

The problem with wp_do_redirect() is that it's not self-documenting or intuitive. I think the function name should tell you how it differs from wp_redirect(). I think it'd be fine to shorten wp_redirect_and_exit() to wp_redirect_exit(), though.

I must admit that I don't really understand the security issue.

What's wrong with writing:

wp_redirect( $location, $status );
exit;

(apart from the fact that you're using exit; which is a code-smell)?

There is no security issue I can see with it on such a generic level. The ​blog post you gave in #15518 is not specifically wordpress related but just highlights a problem what could happen if you don't know what a HTTP response and specifically a header is and your own programming logic does not take care.

My 2 cents, I just have a problem to see an actual issue here that could be patched out globally. Probably a first step would be to leave a note in codex that users who don't want the program to continue after they used wp_redirect should call exit or die.

Replying to hakre:

I must admit that I don't really understand the security issue.

What's wrong with writing:

wp_redirect( $location, $status );
exit;

There's nothing wrong with writing that. The problem comes when someone uses wp_redirect() without exit()ing afterwards. Subsequent code will be executed and subsequent output returned to the client, which can be a security issue. This ticket is trying to address that by exit()ing after the redirect header is sent.

hakre, I think the API should exit after redirecting for the same reason the API sanitizes input before storing it in the database. Sure, I could do that myself, but it's extra work, and many developers aren't willing or able to do it themselves. Things like this are one of the main advantages of having an API in the first place.

I also think that a large percentage of the developers who are aware of the exit issue just assume that the API is already doing it for them, which was my situation. I was very surprised to stumble upon this. If I'd known from the start that the API wasn't doing it I would have done it manually, but now I'm wondering how many of my past projects have a potential bug.

Replying to nacin:

Thus, rather than a parameter, it would need to be:

function wp_redirect_and_exit( $location, $status = 302 ) {
    wp_redirect( $location, $status );
    exit;
}

We should avoid writing raw exit or die statements in the code without a way to override or hijack the behavior. This kills unit tests. I'd prefer to see this implemented similar to wp_die (or use wp_die outright).

wp_redirect_and_exit with a hook for testability