Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#46045 closed defect (bug) (fixed)

Sandboxed live editing of PHP files doesn't work together with WSOD protection

Reported by: ocean90's profile ocean90 Owned by: flixos90's profile flixos90
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: servehappy has-patch
Focuses: Cc:

Description (last modified by ocean90)

To reproduce:

  • Open the plugin editor
  • Select an active plugin
  • Do a change which will cause a fatal error
  • Click "Update File"
  • You should now get the loopback_request_failed message (Unable to communicate back with site…)
  • Go to wp-admin/plugins.php and notice that the plugin has been paused which means the changes were actually saved because the WSOD protection did kick in even the changes were rolled back by wp_edit_theme_plugin_file().

Attachments (2)

46045.diff (1.9 KB) - added by flixos90 5 years ago.
46045.2.diff (2.5 KB) - added by flixos90 5 years ago.

Download all attachments as: .zip

Change History (22)

#1 @ocean90
5 years ago

  • Description modified (diff)

#2 @ocean90
5 years ago

  • Keywords servehappy added

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

5 years ago

#4 @flixos90
5 years ago

  • Milestone changed from 5.1 to 5.2

#5 @flixos90
5 years ago

  • Milestone changed from 5.2 to 5.3

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

5 years ago

#7 @flixos90
5 years ago

  • Milestone changed from 5.3 to 5.2
  • Version changed from 5.1 to trunk

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

5 years ago

#9 @ohiosierra
5 years ago

Am I missing a step? I don't have this issue. The plugin is paused for me, presumably in time to prevent the loopback request from failing.

On a fresh install of 5.2-beta1-20190404.183103:

  1. Created an empty plugin.
Plugin Name: Fatal Error plugin
Version: .1
Description: This plugin starts blank, but you can fix...errr, break...that.
Text Domain: fatal-error
  1. Activated the plugin.
  2. Loaded Plugins->Plugin Editor
  3. Modified the plugin to induce a fatal error
Plugin Name: Fatal Error plugin
Version: .1
Description: This plugin starts blank, but you can fix...errr, break...that.
Text Domain: fatal-error

trigger_error("Fatal error", E_USER_ERROR);
  1. Hit "Update File"

Result: The message, "File edited successfully." is shown. On the next page load, WordPress triggers WSOD protection and generates the recovery mode email. If I enter Recovery Mode, the plugin has been paused. The error has been saved in the file and not rolled back.

Last edited 5 years ago by ohiosierra (previous) (diff)

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

5 years ago

#11 @xkon
5 years ago

If it's of any help:

While using an Apache/Mysql setup I was able to replicate @ocean90 report.

While using VVV everything is as @ohiosierra mentions.

In both cases ( same php, same options, fresh trunk pulls, clean installs etc ).

#12 @ohiosierra
5 years ago

I've identified the difference for when it occurs and doesn't -> WP_DEBUG.

When WP_DEBUG is enabled, the "doing_it_wrong" function outputs a user notice. Specifically, it outputs the doing it wrong notice 3 times during the wp_die() function.

One of these notices becomes the error_get_last().

When wp_finalize_scraping_edited_file_errors() tries to determine if the change caused a fatal error, it specifically looks at the last error only and sees if it is one of E_CORE_ERROR, E_COMPILE_ERROR, E_ERROR, E_PARSE, E_USER_ERROR, or E_RECOVERABLE_ERROR.

Since doing_it_wrong is only a user notice, it fails that check. wp_finalize_scraping_edited_file_errors() returns a "true" (success), the loopback completes, and no changes are rolled back.

Of course, there *is* a fatal error, so the next load triggers the WSOD protection.

This might be worth a separate ticket to improve the wp_finalize_scraping_edited_file_errors function to look at all errors and see if any are fatal. However, for the purposes of this ticket, the only note is for reproducing the error and confirming resolution, make sure WP_DEBUG is off.

5 years ago

#13 @flixos90
5 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to flixos90
  • Status changed from new to assigned

This is caused because the fatal error handler treats such errors, although in this case it shouldn't.

We actually noticed this is also affecting other areas:

  • When you resume a plugin/theme that is still broken, you will end up in a redirect loop.
  • When you activate a plugin/theme that is still stored as paused/broken, you will end up in a redirect loop too.

46045.diff fixes this:

  • It introduces a WP_SANDBOX_SCRAPING constant and sets it whenever it is loading plugin/theme code in some "sandbox" mode.
  • The fatal error handler bails if that constant is set and true so that it no longer conflicts.

#14 @ohiosierra
5 years ago

Tested @flixos90 's patch.

It does fix the redirect loop for resuming a paused plugin with a fatal error. I do still get the redirect loop when re-activating a plugin that was paused with a fatal error and then deactivated.

As for this specific ticket, I was still not seeing the issue @ocean90 specified even before the patch. It was rolling back my fatal error change with the appropriate message and not triggering WSOD protection. I believe @xkon had the same experience based on our conversation on Slack, but he would have to confirm.

5 years ago

#15 @flixos90
5 years ago

Good catch @ohiosierra! 46045.2.diff adds the missing occurrence where that constant needs to be set too.

#16 @flixos90
5 years ago

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

In 45114:

Bootstrap/Load: Ensure WP_Fatal_Error_Handler does not conflict with existing mechanisms treating fatal errors.

Prior to this change, resuming or activating a plugin or theme that is still broken would result in a redirect loop if in recovery mode. If outside recovery mode, it would cause the error template to be displayed.

Furthermore this applies to breaking a plugin or theme when editing from the backend.

Props aandrewdixon, azaozz, dhanukanuwan, henrywright, ocean90, ohiosierra, PandelisZ, xkon.
Fixes #46045, #46751.

#17 @flixos90
5 years ago

In 45116:

Bootstrap/Load: Do not redefine constants because tests don't like it.

See #46045.

#18 @tmdesigned
5 years ago

@flixos90 that latest patch took care of the loop on activating a broken plugin, thanks. (this is @ohiosierra, sorry for using 2 usernames)

There is a slight difference in terms of display for un-pausing a broken plugin vs activating a broken plugin, but not a very significant one.

Un-pausing a broken plugin keeps it paused, showing the code problem, whereas activating a broken plugin keeps it deactivated, so there is no error message shown via the pause functionality. This seems natural to me since it's not paused, although maybe in the future plugins that fail to activate could do something similar.

#19 @xkon
5 years ago

In case this is helpful as well even though this was closed as I kept seeing the same message:

When testing with VVV or any other local setup but going via https ( with the generated certs ) it will continue to show the Unable to communicate back with site to check for fatal errors... message, it works via http though (there's no need to actually change anything in the file as well, just hitting the Update File without any changes).

On the other hand testing the same build on a live host with a proper ssl cert I'm getting the Your PHP code changes were rolled back due to an error....

#20 @spacedmonkey
5 years ago

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