WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 15 months ago

Last modified 12 months ago

#45940 closed defect (bug) (invalid)

WSOD protection should disable plugins in fewer situations

Reported by: markjaquith Owned by: TimothyBlynJacobs
Milestone: Priority: normal
Severity: critical Version: 5.1
Component: Site Health Keywords: needs-patch servehappy
Focuses: Cc:

Description (last modified by markjaquith)

[44524] implemented WSOD protection, for #44458. I have concerns about how aggressively it pauses plugins.

It is possible to get a plugin paused in the admin by triggering a fatal error on the front of the site. Furthermore, that fatal error could be triggered by specific user input (maliciously, even). If this is a plugin that implements 2FA or SAML or admin event logging or permissions filtering or login attempt limiting or any other security- or authentication-related function, this could have dire effects on the site. Bad actors could have a way of targeting and disabling critical site functionality.

I think the scenarios where this pausing occurs should be narrowed. To start, I propose that pausing not happen for:

  1. POST requests (on the front end).
  2. Requests with a query string other than ?p=X (or a few other standard WordPress query strings we can whitelist).

For example, if a contact form submission, in certain circumstances, can result in a fatal error, this should not pause that plugin. If some ?obscure-query-arg=BAD_INPUT URL on a site can result in a fatal error, this should not pause the plugin.

If a plugin is taking down the entire site, or preventing a user from logging in, that is the dead end scenario we'd like to prevent, and those are the situations in which it's an acceptable trade-off to pause that functionality so that the site owner can take steps to recover more gracefully than they could in the past.

See also #45888 which proposes that critical plugins (where the plugin itself knows that it's better to take the site down than have its functionality disabled) have a way of opting out of WSOD protection.

Attachments (1)

45940.diff (2.1 KB) - added by flixos90 17 months ago.

Download all attachments as: .zip

Change History (24)

#1 @markjaquith
17 months ago

  • Description modified (diff)

#2 @TimothyBlynJacobs
17 months ago

I think we can also limit pausing extensions to only errors that occurred on pages where is_protected_endpoint() is true. We could continue to log these errors but display them differently in the admin.

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


17 months ago

#4 @flixos90
17 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.1
  • Owner set to flixos90
  • Status changed from new to assigned

@flixos90
17 months ago

#5 @flixos90
17 months ago

  • Keywords has-patch added; needs-patch removed

45940.diff separates the processes of detecting an error to handle and actually storing it (and by that pausing the extension). This provides us with the baseline to pause extensions more granularly. I also included the suggestion by @TimothyBlynJacobs already since that enhancement makes sense and is rather trivial to implement.

I will commit this now to have it in the next Beta, and we can continue working from there, also considering "irregular" requests for not pausing extensions, as suggested by @markjaquith.

#6 @flixos90
17 months ago

In 44623:

Bootstrap/Load: Only pause extensions when they cause a crash on a protected endpoint.

This is a first step on pausing extensions less aggressively. If a plugin or theme only causes a crash in the frontend, there is no point in pausing it in the admin backend.

See #45940, #44458.

#7 @flixos90
17 months ago

  • Keywords needs-patch added; has-patch removed

Flagging this for a follow-up patch to implement the initial concerns outlined in the ticket description.

#8 @flixos90
17 months ago

  • Keywords servehappy added

#9 @flixos90
17 months ago

Note that by not always pausing extensions when an error occurs, there currently is no longer any awareness of the error either. I wonder whether we should decouple pausing extensions from still storing the error in other cases so that the site owner can be informed by it.

Due to the time constraints, this specific part could be an enhancement for 5.2 (or possibly even a 5.1.x) though, as the other fixes are more critical.

#10 @flixos90
17 months ago

  • Component changed from General to Bootstrap/Load

#11 @pento
17 months ago

  • Version set to trunk

#12 @schlessera
17 months ago

I wonder whether we should decouple pausing extensions from still storing the error in other cases so that the site owner can be informed by it.

Isn't this already handled by WP_DEBUG_LOG? I don't think we should build a complete developer/debugger tool here, just make sure people can run updates/removals on their plugins/themes.

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


17 months ago

#14 @flixos90
17 months ago

  • Owner changed from flixos90 to TimothyBlynJacobs

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


17 months ago

#16 @schlessera
17 months ago

Added a mechanism to only paused plugins/themes after an initial redirect, to make sure the error is persistent: https://core.trac.wordpress.org/ticket/46066

This ticket was mentioned in Slack in #core by timothybjacobs. View the logs.


16 months ago

#18 @flixos90
16 months ago

In 44717:

Bootstrap/Load: Revert fatal error recovery mechanism from 5.1 to polish for 5.2.

Due to the high number of follow-up tickets and associated security concerns, it was decided to reschedule the fatal error recovery feature for WordPress 5.2, in order to address these issues properly. The feature will continue to be developed, with iterations being merged into trunk early in the 5.2 release cycle.

Fixes #46141. See #44458, #45932, #45940, #46038, #46047, #46068.

#19 @flixos90
16 months ago

  • Milestone changed from 5.1 to 5.2

#20 @flixos90
16 months ago

  • Priority changed from high to normal

#21 @flixos90
15 months ago

  • Milestone changed from 5.2 to 5.3

#22 @flixos90
15 months ago

  • Milestone 5.3 deleted
  • Resolution set to invalid
  • Status changed from assigned to closed

This ticket is based on the old fatal error recovery mode implementation and will be covered as part of #46130.

#23 @spacedmonkey
12 months ago

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