Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#47479 closed enhancement (fixed)

Do not return 5xx for invalid/expired recovery mode cookies

Reported by: davidbinda's profile david.binda Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: servehappy has-patch
Focuses: Cc:

Description

The WP_Recovery_Mode class dies in certain situations where returning a 5xx status code does not feel appropriate, as the request did not produce a server error, but rather the authentication failed. In such situations, it might be more appropriate to return a 4xx error (presumably 403). The situations in mind here are the following:

  1. when the recovery mode cookie is expired
  2. when the recovery mode cookie is invalid
  3. when the exit recovery mode nonce check failed

As those failures also unset related cookies, the 5xx status may result in an improper handling on certain server configurations (eg.: overriding 5xx responses with a custom response which is not properly passing the cookie headers).

I'm attaching a patch which changes the response codes from default 500 to 403 in the cases mentioned above.

Attachments (2)

47479.diff (983 bytes) - added by david.binda 5 years ago.
47479-2.diff (1.0 KB) - added by david.binda 5 years ago.

Download all attachments as: .zip

Change History (13)

@david.binda
5 years ago

#1 @spacedmonkey
5 years ago

  • Keywords needs-patch servehappy added
  • Owner set to spacedmonkey
  • Status changed from new to assigned

#2 @SergeyBiryukov
5 years ago

  • Component changed from General to Bootstrap/Load

#3 @spacedmonkey
5 years ago

@davidbinda This looks good.

For calls to wp_die that pass a WP_Error object. Please add the status code like this.

@david.binda
5 years ago

#4 @david.binda
5 years ago

Thanks for the feedback @spacedmonkey ! I've updated the patch accordingly, please let me know if it works for you :)

#5 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.3

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


5 years ago

#7 @spacedmonkey
5 years ago

  • Owner changed from spacedmonkey to SergeyBiryukov

Assigning to @SergeyBiryukov to merge.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


5 years ago

#9 @SergeyBiryukov
5 years ago

  • Keywords has-patch added; needs-patch removed

#10 @SergeyBiryukov
5 years ago

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

In 45544:

Bootstrap/Load: Return a 403 error code when the recovery mode cookie is invalid or expired, or the exit recovery mode nonce check failed.

Props david.binda, spacedmonkey.
Fixes #47479.

#11 @spacedmonkey
5 years ago

  • Component changed from Bootstrap/Load to Site Health
Note: See TracTickets for help on using tickets.