Make WordPress Core

Opened 12 years ago

Closed 7 years ago

Last modified 7 years ago

#21622 closed enhancement (fixed)

Validate or sandbox theme file edits before saving them (as is done for plugins)

Reported by: eschwartz93's profile eschwartz93 Owned by: westonruter's profile westonruter
Milestone: 4.9 Priority: high
Severity: normal Version: 2.7.1
Component: Themes Keywords: has-patch needs-testing commit
Focuses: administration Cc:

Description

While using the academica theme, I accidentally missed an ending bracket while editing functions/wpzoom-seo.php in the theme editor. This is used as part of a set of pages creating a theme options page on the dashboard. When the function went bad, it caused my entire installation to be unaccessible.

It was necesssary to ftp to fix the error, because even the dashboard was blanked out by a fatal error. Messing something up in the theme should not cut off access to the dashboard.

Attachments (3)

wpzoom-seo.php (3.8 KB) - added by eschwartz93 12 years ago.
missing bracket on line 15
edit-plugin-fatal-error-scraped.png (161.5 KB) - added by westonruter 7 years ago.
21622.0.diff (48.8 KB) - added by westonruter 7 years ago.
https://github.com/xwp/wordpress-develop/pull/272

Download all attachments as: .zip

Change History (47)

@eschwartz93
12 years ago

missing bracket on line 15

#1 @Viper007Bond
12 years ago

  • Component changed from General to Administration
  • Summary changed from error in theme broke wordpress to Validate or sandbox file edits before saving them
  • Type changed from defect (bug) to enhancement

#2 follow-up: @markoheijnen
12 years ago

I'm curious if WordPress should do this kind of checking. If someone breaks something when using the editor they ask for it.

Also when changing functions.php and you make a mistake their it would always break wp-admin since functions.php their can be used their. To make it saver for yourself you can create load a file inside functions.php that you only use for the frontend.

#3 in reply to: ↑ 2 @Viper007Bond
12 years ago

We test plugins before we activate them to prevent you from breaking your site by activating a plugin. I don't see the difference with this.

#4 @markoheijnen
12 years ago

Thats true but we don't check if we change a plugin. The way of doing No image "so" attached to Ticket #21622 is different since you want to check something that is loaded by default.

#5 @Viper007Bond
12 years ago

Yep, the implementation would certainly be different, I just meant that we already do a bit of hand-holding so this functionality wouldn't be that big of a departure.

#6 @c3mdigital
12 years ago

  • Component changed from Administration to Editor
  • Keywords needs-patch added
  • Version changed from 3.4.1 to 2.7.1

#7 @Viper007Bond
12 years ago

  • Component changed from Editor to Administration

To the best of my knowledge, the "Editor" component refers to the post editor not the file editor. :)

#8 @nacin
11 years ago

  • Component changed from Administration to Themes
  • Focuses administration added

#9 @chriscct7
9 years ago

  • Severity changed from minor to normal

#10 @westonruter
7 years ago

  • Summary changed from Validate or sandbox file edits before saving them to Validate or sandbox theme file edits before saving them (as is done for plugins)

As noted, the plugin editor already has sandboxed edits (though it is broken, see #39766). As such, this ticket should be about sandboxing editors to themes specifically.

For the theme editor, it's not so easy to use the sandboxing approach for plugins because we can't temporarily deactivate a theme while checking if the theme still runs, because if the theme doesn't run then what theme is then left to be activated?

One idea that has been raised is to actually create a copy of a given theme or plugin and temporarily activate it to see if there are errors. If there are, then the sandbox copy should be deactivated and the old theme/plugin should be re-activated. If there are not errors, then the changes can be copied from the sandbox to the main plugin and the sandbox can be removed. There is a good discussion about this here: https://wordpress.slack.com/archives/C0381N237/p1499707763283370

#11 follow-up: @Clorith
7 years ago

My thought is to create a tentative copy of the file being edited (or keep the old content in memory), after writing changes we can then check the site status and replace the file (overwrite it with the old content) within the same running process if there is a failure.

This would prevent us from needing to copy the entire theme, and would possibly be a better approach to the plugin editor sandboxing as well (as deactivating the plugin may be detrimental to the site experience, but rapidly restoring is a cleaner resolution).

#12 @westonruter
7 years ago

In 41560:

Plugin Editor: Improve reliability of detecting PHP fatal errors when editing an active plugin.

  • Invalidate PHP opcache after file is updated to ensure include will include the written changes.
  • Define WP_ADMIN when activating plugin in sandbox so plugin code targeting admin will be loaded.
  • Do actions that get triggered when loading the admin to ensure plugin code runs that could cause errors on plugin editor screen (and lock out access).
  • Fix ability to re-activate a plugin after editing a PHP file other than the main plugin file, and ensure PHP fatal error will be displayed in such cases.
  • Consolidate duplicated code into plugin_sandbox_scrape() and re-use in activate_plugin().
  • Show an error notice instead of a success notice when a file is updated but a plugin was deactivated due to a fatal error.
  • Update style of warning when editing an active plugin to be styled as an actual warning notice.

See #12423, #21622.
Fixes #39766.

#13 in reply to: ↑ 11 ; follow-up: @westonruter
7 years ago

  • Milestone changed from Awaiting Review to 4.9
  • Priority changed from normal to high

@Clorith That might just work! What it would depend on is being able to run two separate PHP processes concurrently, which in theory WordPress can do since it's how it normally spawns WP Cron.

Details for how this could be implemented:

  1. User submits theme editor with change to a PHP file.
  2. WordPress grabs a copy of the file's contents and stores them in memory as a backup.
  3. WordPress writes the submitted changes to that file and (invalidates the opcache).
  4. WordPress issues an HTTP request back to itself as the authenticated user to see if the theme/plugin editor screen continues to load successfully. That's key, to be able to make an authenticated request back to the admin because that is where the user will have to access if they find they broke something somewhere else on the site. It wouldn't be possible to construct a self-request that would touch the specific PHP code that you just modified.
  5. If WordPress receives back an error from that self-request, then it writes the backed up changes back to the file (and invalidates the file's opcache) and then redirects back to file editor with an parameter indicating there was an error. Any error message scraped from the HTTP response could be stuffed into a transient for displaying in the error notice. 🚫
  6. Otherwise, if the HTTP response came back successfully, then WordPress can redirect to the theme editor with a success parameter to then show the updated message. ✅

The key part in all of this is being able to issue the self-request as the user. If we include all of the cookies in the request it may just work.

#14 @westonruter
7 years ago

One additional consideration is that the changes submitted by the user should get re-populated back into the form in the error scenario, or else all of the changes they had submitted could be lost as the file would have been reverted on disk.

#15 @westonruter
7 years ago

  • Keywords dev-feedback added

#16 @Clorith
7 years ago

Yes, the changes from the user should re-populate the field, luckily that should be easy as it should still be within the POST object from the request that triggered it all.

#17 in reply to: ↑ 13 @azaozz
7 years ago

Replying to westonruter:

This sounds pretty good, we've chatted a few times about the best ways to test for PHP fatals.

WordPress issues an HTTP request back to itself as the authenticated user to see if the theme/plugin editor screen continues to load successfully. That's key, to be able to make an authenticated request back to the admin...

Right. As the original (main) request was done with the proper credentials (cookies), they can be passed to this request as well. If this comes through without errors, it will have to be repeated simulating a front-end request (can be without credentials) to fully test "everything". If a theme template was edited it's preferably to be tested too.

There are also some possible variations. One would be to temporarily set the theme to the fallback theme only for the authenticated user while accessing wp-admin and saving an edited file. Then the changes can be test-loaded without breaking wp-admin access. The same could work for plugins too: while an edited plugin file is being saved, the plugin can be temporarily disabled only for the authenticated user and only when accessing wp-admin.

#18 follow-up: @Clorith
7 years ago

Let's not only do an authenticated hit to the admin though, as a fatal could be introduced in a template file which wouldn't be loaded until you visit the front end.

We obviously can't guarantee we'll hit the file if it's a page template or an archive template etc, but we can at least make sure the front page still loads to mitigate some forms of breakage is my thinking.

#19 in reply to: ↑ 18 @azaozz
7 years ago

Replying to Clorith:

Right, as per my previous comment:

it will have to be repeated simulating a front-end request (can be without credentials)

It could also be possible to load a specific template file after it was edited. Will probably need to do some sort of a "dry run" through the template hierarchy to determine which file(s) are used in which cases.

#20 @jdgrimes
7 years ago

It would be even better though if we didn't have to potentially break the site at all for other users, even if only for a few moments. That would be the benefit of copying the entire theme: then we could selectively load the edited copy only for those specific requests to check for errors; anybody else visiting the site while that was happening would get the backup copy of the theme.

This would have its own downsides though, like when the theme directory name is hard-coded in any part of the theme code—a backup copy might still utilize the edited file, even though it was loaded from a different directory.

#21 @Clorith
7 years ago

My main concern with copying the whole theme is that it starts becoming a huge process for a single edit, one that might not scale well with shared hosts for example.

#22 @westonruter
7 years ago

  • Keywords dev-feedback removed

In the case of editing a theme, I like the idea of identifying if a PHP template file is being edited and then issuing a loopback request for a URL that would load that template. It would be difficult to reliably reverse the template hierarchy to figure out a URL that would result in a given PHP template to be included, however. So I'd consider that an enhancement. The minimum would be to do the loopback request onto the file editor in the admin to make sure the user doesn't whitescreen themselves out of being able to fix any mistakes they make.

#23 @westonruter
7 years ago

  • Owner set to westonruter
  • Status changed from new to accepted

I'm working on this.

#25 @westonruter
7 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

@Clorith @jdgrimes @jjj @azaozz Here we go!!

Please test and review these changes: https://github.com/xwp/wordpress-develop/pull/272

  • Refactor themes and plugin editors to submit file changes via Ajax instead of doing full page refreshes. Moves file modifications from theme-editor.php and plugin-editor.php to common Ajax handler.
  • Do sandboxed updates of PHP files for both active themes and plugins, with rollbacks in the case of a loopback request results in a fatal error. This supersedes changes in #39766 (Plugin does not gracefully fail when editing active plugin causes fatal error).

The improvements to the user experience and the robustness of the file editors are huge.

#26 follow-up: @johnbillion
7 years ago

This looks really good. A few points from me:

  • The full file path shouldn't be exposed in the error message. It should show the path relative to ABSPATH, for example: str_replace( ABSPATH, '', $error_output ).
  • Every call to opcache_invalidate() needs a function_exists() check because it's PHP >= 5.5 only.
  • It looks like the error notice is displayed as HTML instead of plain text, which is not ideal for security hardening purposes. The error message should be run through wp_strip_all_tags() and displayed as text instead of HTML.
  • Use wp_json_encode() instead of json_encode() in wp_finalize_scraping_edited_file_errors().
  • Unrelated change in src/wp-includes/js/wp-a11y.js.
  • Should wp_start_scraping_edited_file_errors() return instead of dieing if the nonce is invalid?

#27 follow-up: @johnbillion
7 years ago

Also the editors need to be tested with JS disabled just to ensure the existing functionality continues to work as expected if there's a JS error.

#28 @johnbillion
7 years ago

Another one: wp_start_scraping_edited_file_errors() needs to forcibly enable display_errors, and probably set error_reporting to only show fatal errors.

#29 @johnbillion
7 years ago

Actually that last one might not be correct due to the use of error_get_last() in wp_finalize_scraping_edited_file_errors(). Needs some testing I guess :-)

#30 in reply to: ↑ 26 ; follow-up: @westonruter
7 years ago

Replying to johnbillion:

This looks really good. A few points from me:

Thanks a lot for the feedback.

  • The full file path shouldn't be exposed in the error message. It should show the path relative to ABSPATH, for example: str_replace( ABSPATH, '', $error_output ).

I did think about this, but it would be new behavior. There isn't any such path scrubbing in plugin_sandbox_scrape() previously for when a plugin was edited as I could see. See edit-plugin-fatal-error-scraped.png for existing behavior. In any case, if you're able to edit PHP files a user could just echo ABSPATH when editing a PHP file and access it just without causing an error.

  • Every call to opcache_invalidate() needs a function_exists() check because it's PHP >= 5.5 only.

You're right. I missed that. Fixed in c9766fe5f1.

  • It looks like the error notice is displayed as HTML instead of plain text, which is not ideal for security hardening purposes. The error message should be run through wp_strip_all_tags() and displayed as text instead of HTML.

The actual user-generated PHP error message will get escaped as it is getting printed in <pre>{{ data.message }}</pre>. The other messages are getting printed unescaped in <p class="notification-message">{{{ data.message || data.code }}}</p> because some (one) of the messages for the file_not_writable error code has markup in it, the link to the codex article.

  • Use wp_json_encode() instead of json_encode() in wp_finalize_scraping_edited_file_errors().

Done in 624c53de38. I hesitated to use this because it's PHP error outout and I didn't check if the function was available this early in the WordPress bootstrapping. But it is fine.

  • Unrelated change in src/wp-includes/js/wp-a11y.js.

It is related actually. In theme-plugin-editor.js there is a call to wp.a11y.speak() but static analysis was complaining about a missing function arg. But the arg is actually optional. So this just updates the jsdoc to make it explicit.

  • Should wp_start_scraping_edited_file_errors() return instead of dieing if the nonce is invalid?

Actually, it should be going beyond a silent die and it should be outputting an error which can be scraped. Otherwise, the file change would be blindly accepted even though it could be causing an error. Fixed in 9189511190.

Also the editors need to be tested with JS disabled just to ensure the existing functionality continues to work as expected if there's a JS error.

That's something I didn't explicitly mention about the changes here. The refactor means that JavaScript is required to use the editor. In order to be able to submit the change to a PHP file and have that change tested with a rollback but keeping the user's submitted change retained in the editor, this requires an Ajax submission process unless a big effort is put in to support browsers without JavaScript enabled. Given that it is 2017 and how more and more of WordPress fundamentally requires JS (Customizer, Gutenberg, etc), I don't want to make no-JS a requirement for the file editors either. We should probably add a noscript tag to inform users of this, however. I've done this in 8abef679ed.

Another one: wp_start_scraping_edited_file_errors() needs to forcibly enable display_errors, and probably set error_reporting to only show fatal errors. Actually that last one might not be correct due to the use of error_get_last() in wp_finalize_scraping_edited_file_errors(). Needs some testing I guess :-)

Exactly. error_get_last() is used here to grab the fatal error and print it out, with a preceding boundary marker, for us to reliably scrape the error even when display_errors is turned off.

Full delta diff of new changes: https://github.com/xwp/wordpress-develop/pull/272/files/540ee24..8abef67

#31 @johnbillion
7 years ago

The no-JS issue isn't about browsers not supporting JS, or users disabling JS, it's about basic functionality being able to work when JS has uncontrollably broken. Functionality provided by the Customizer is available elsewhere without JS, and the basic post editor still works without JS.

Particularly for the file editors, if a user introduces a syntax error in a JS file while editing it, it may cause them to be unable to use the file editor to correct it if it breaks the file editing screen. The basic functionality that exists currently (without error checking) should be retained for use in cases where JS is broken.

#32 in reply to: ↑ 30 ; follow-up: @johnbillion
7 years ago

Replying to westonruter:

  • The full file path shouldn't be exposed in the error message. It should show the path relative to ABSPATH, for example: str_replace( ABSPATH, '', $error_output ).

There isn't any such path scrubbing in plugin_sandbox_scrape() previously for when a plugin was edited as I could see.

Good point. I wonder if it should be introduced there too. At the least, it shortens the error message and makes it more readable.

The actual user-generated PHP error message will get escaped as it is getting printed in <pre>{{ data.message }}</pre>. The other messages are getting printed unescaped in <p class="notification-message">{{{ data.message || data.code }}}</p> because some (one) of the messages for the file_not_writable error code has markup in it, the link to the codex article.

I think we should try to avoid this as a matter of best practice. Inserting untrusted HTML into the DOM isn't a great idea. The Codex link could be moved into the strings available on the front and and appended depending on the error code. Not high priority by any means, but we can do better.

  • Unrelated change in src/wp-includes/js/wp-a11y.js.

It is related actually. In theme-plugin-editor.js there is a call to wp.a11y.speak() but static analysis was complaining about a missing function arg. But the arg is actually optional. So this just updates the jsdoc to make it explicit.

Ah yes. Thanks.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


7 years ago

#34 in reply to: ↑ 32 @westonruter
7 years ago

Replying to johnbillion:

Good point. I wonder if it should be introduced there too. At the least, it shortens the error message and makes it more readable.

Fixed in 2b2af81.

I think we should try to avoid this as a matter of best practice. Inserting untrusted HTML into the DOM isn't a great idea. The Codex link could be moved into the strings available on the front and and appended depending on the error code. Not high priority by any means, but we can do better.

Fixed in 455ea66.

Full list of new changes: https://github.com/xwp/wordpress-develop/pull/272/files/8abef67..cdc6ddc

#35 @jdgrimes
7 years ago

Looking over the changes, I don't see any issues. I haven't tested it though.

I do think that @johnbillion raises a good point about it being possible to break the editor via JS changes.

#36 in reply to: ↑ 27 @westonruter
7 years ago

Replying to johnbillion:

Also the editors need to be tested with JS disabled just to ensure the existing functionality continues to work as expected if there's a JS error.

Replying to jdgrimes:

Looking over the changes, I don't see any issues. I haven't tested it though.

I do think that @johnbillion raises a good point about it being possible to break the editor via JS changes.

Fallback support for editing when JS is disabled or broken has been implemented here: https://github.com/xwp/wordpress-develop/pull/272/files/cdc6ddc..5265a9d

#37 @westonruter
7 years ago

  • Keywords commit added

#38 @westonruter
7 years ago

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

In 41721:

File Editors: Introduce sandboxed live editing of PHP files with rollbacks for both themes and plugins.

  • Edits to active plugins which cause PHP fatal errors will no longer auto-deactivate the plugin. Supersedes #39766.
  • Introduce sandboxed PHP file edits for active themes, preventing accidental whitescreening of a user's site when introducing a fatal error.
  • After writing a change to a PHP file for an active theme or plugin, perform loopback requests on the file editor admin screens and the homepage to check for fatal errors. If a fatal error is encountered, roll back the edited file and display the error to the user to fix and try again.
  • Introduce a secure way to scrape PHP fatal errors from a site via wp_start_scraping_edited_file_errors() and wp_finalize_scraping_edited_file_errors().
  • Moves file modifications from theme-editor.php and plugin-editor.php to common wp_edit_theme_plugin_file() function.
  • Refactor themes and plugin editors to submit file changes via Ajax instead of doing full page refreshes when JS is available.
  • Use get method for theme/plugin dropdowns.
  • Improve styling of plugin editors, including width of plugin/theme dropdowns.
  • Improve notices API for theme/plugin editor JS component.
  • Strip common base directory from plugin file list. See #24048.
  • Factor out functions to list editable file types in wp_get_theme_file_editable_extensions() and wp_get_plugin_file_editable_extensions().
  • Scroll to line in editor that has linting error when attempting to save. See #41886.
  • Add checkbox to dismiss lint errors to proceed with saving. See #41887.
  • Only style the Update File button as disabled instead of actually disabling it for accessibility reasons.
  • Ensure that value from CodeMirror is used instead of textarea when CodeMirror is present.
  • Add "Are you sure?" check when leaving editor when there are unsaved changes.

Supersedes [41560].
See #39766, #24048, #41886.
Props westonruter, Clorith, melchoyce, johnbillion, jjj, jdgrimes, azaozz.
Fixes #21622, #41887.

#39 @adamsilverstein
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@westonruter seems like wp_ajax_edit_theme_plugin_file should have a nonce and user_can capabilities check? Am I missing something?

#40 follow-up: @westonruter
7 years ago

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

@adamsilverstein The nonce and cap checks are handled inside of wp_edit_theme_plugin_file(). Per the comment on this line:

// Validation of args is done in wp_edit_theme_plugin_file().

See wp_edit_theme_plugin_file() for the nonce and cap checks: https://github.com/WordPress/wordpress-develop/blob/f23be16bd24e18b82fa5955baacc49d107372d7f/src/wp-admin/includes/file.php#L313-L544

#41 in reply to: ↑ 40 @adamsilverstein
7 years ago

@adamsilverstein The nonce and cap checks are handled inside of wp_edit_theme_plugin_file(). Per the comment

@westonruter Ah thanks, I see that now, I was expecting the typical pattern of seeing these checks at the beginning of the callback and missed the comment line.

#42 @westonruter
7 years ago

@adamsilverstein It's a good question and yes, normally you're right. The thing is that the logic has to be invoked over Ajax as via traditional form POST, when JS isn't available: https://github.com/WordPress/wordpress-develop/blob/b7eac77/src/wp-admin/theme-editor.php#L102-L123

So instead of duplicating the nonce and cap checks in both places, the checks are centralized in a common wp_edit_theme_plugin_file(). By all means if there is a way to make this more explicit for future reviewers who will have the same reaction as you, I'm happy to make any changes you recommend to the comments.

#43 @westonruter
7 years ago

In 41805:

File Editor: Increase robustness of fatal error checking when saving PHP file edits.

  • Increase PHP execution time limit prior to issuing loopback requests where are themselves given timeouts to ensure PHP file can be reverted.
  • Output scrape messages on success and failure so that absence of either can also be flagged as an error condition.
  • Forward browser's HTTP Basic Auth credentials in loopback requests to admin and home URL.
  • Display more helpful message when loopback request fails.

Amends [41721].
See #21622.
Fixes #42102.

#44 @westonruter
7 years ago

In 41819:

Plugin Editor: Revert superseded changes to activate_plugin() and plugin_sandbox_scrape().

Partially revert [41671], [41561], [41560].
See #21622.
Fixes #39766.

Note: See TracTickets for help on using tickets.