WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#20060 new enhancement

wp_redirect() doesn't exit

Reported by: iandunn Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Security Keywords: dev-feedback has-patch
Focuses: Cc:

Description

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.

Attachments (2)

20060.diff (1.0 KB) - added by kurtpayne 2 years ago.
wp_redirect_and_exit with a hook for testability
20060.2.diff (1.3 KB) - added by iandunn 8 weeks ago.
refresh of 20060.diff

Download all attachments as: .zip

Change History (13)

comment:1 iandunn2 years ago

  • Cc ian_dunn@… added

comment:2 nacin2 years ago

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.

comment:3 follow-up: nacin2 years ago

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

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

comment:4 follow-up: johnbillion2 years ago

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()?

comment:5 iandunn2 years ago

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.

comment:6 follow-up: hakre2 years ago

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.

comment:7 in reply to: ↑ 4 ericlewis2 years ago

  • Cc eric.andrew.lewis@… added

comment:8 in reply to: ↑ 6 johnbillion2 years ago

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.

comment:9 iandunn2 years ago

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.

comment:10 in reply to: ↑ 3 kurtpayne2 years ago

  • Cc kpayne@… added

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).

kurtpayne2 years ago

wp_redirect_and_exit with a hook for testability

comment:11 kurtpayne2 years ago

  • Keywords has-patch added; needs-patch removed

iandunn8 weeks ago

refresh of 20060.diff

Note: See TracTickets for help on using tickets.