Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#44317 closed enhancement (fixed)

wp_safe_redirect() and wp_redirect() shouldn't allow non-3xx status codes

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile johnbillion
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: General Keywords: good-first-bug has-patch needs-testing has-unit-tests
Focuses: Cc:

Description

It might not be immediately obvious that the below code has a serious bug in it:

wp_safe_redirect( $url, 404 );
exit;

The wp_safe_redirect() and wp_redirect() functions should trigger an error if an HTTP status code is passed in that isn't in the 3xx range. The code above can cause much head scratching when you're presented with a 404 with no output.

I think it would make sense to trigger a wp_die() error message in this situation, to ensure maximum chance of visibility to the developer.

Attachments (3)

44317.diff (3.0 KB) - added by mjnewman 5 years ago.
fix(wp_redirect)__Adding_validation_of_status_code___wp_redirect()_will_now_exit_if_status.patch (585 bytes) - added by spenserhale 5 years ago.
44317.patch (2.4 KB) - added by spenserhale 5 years ago.

Download all attachments as: .zip

Change History (14)

#1 @johnbillion
5 years ago

  • Keywords good-first-bug added

@mjnewman
5 years ago

#2 @mjnewman
5 years ago

  • Keywords has-patch added; needs-patch removed

Added check_http_status_code() function to check if an HTTP status code is within a specific range, such as 3XX, or a multiple ranges, such as 2XX and 3XX. Includes option to wp_die(). Returns the HTTP status code if it's within the specified range.

Added check_http_status_code() to wp_redirect() after the 'wp_redirect_status' filter is applied.

#3 @mjnewman
5 years ago

  • Keywords needs-testing added

#4 @mjnewman
5 years ago

Hi @johnbillion wanted to check-in and see if my proposed patch was along the lines of what you were looking for. Happy to make any adjustments or updates as necessary. Thanks!

#5 @spenserhale
5 years ago

Hello @mjnewman and @johnbillion,

I added a pull request for a simpler fix to the problem that doesn't change as much.

https://github.com/WordPress/wordpress-develop/pull/112

Version 0, edited 5 years ago by spenserhale (next)

#6 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#7 @johnbillion
5 years ago

  • Owner changed from SergeyBiryukov to johnbillion

@spenserhale
5 years ago

#8 @spenserhale
5 years ago

  • Keywords has-unit-tests added

Hey @johnbillion, I added unit tests to test common usages and edge cases.

#9 @jorbin
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 46641:

General: wp_safe_redirect() and wp_redirect() shouldn't allow non-3xx status codes

Redirects should use redirect status codes and if you attempt to call wp_safe_redirect or wp_redirect with a non redirect status it can lead to undesired behavior and head scratching.

Fixes #44317.
Props spenserhale, johnbillion, mjnewman for initial patch.

#10 @jorbin
5 years ago

In 46645:

Remove Failing Tests added in r46641

The tests for good redirects send headers that we can't handle in the test suite, so let's just remove them.

Unprops Jorbin.
Fixes #44317.

#11 @SergeyBiryukov
5 years ago

In 46649:

Build/Test Tools: Adjust the test for wp_redirect() status codes added in [46641] per the documentation and coding standards.

Move the test to a more appropriate place for consistency with wp_sanitize_redirect() and wp_validate_redirect() tests.

See #44317.

Note: See TracTickets for help on using tickets.