[wp-trac] [WordPress Trac] #21622: Validate or sandbox theme file edits before saving them (as is done for plugins)

[WordPress Trac]

#21622: Validate or sandbox theme file edits before saving them (as is done for
plugins)
-------------------------------------+-----------------------------
 Reporter:  eschwartz93              |       Owner:  westonruter
     Type:  enhancement              |      Status:  accepted
 Priority:  high                     |   Milestone:  4.9
Component:  Themes                   |     Version:  2.7.1
 Severity:  normal                   |  Resolution:
 Keywords:  has-patch needs-testing  |     Focuses:  administration
-------------------------------------+-----------------------------

Comment (by westonruter):

 Replying to [comment:26 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 [attachment: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 [https://github.com/xwp/wordpress-
 develop/pull/272/commits/c9766fe5f1 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 [https://github.com/xwp/wordpress-
 develop/pull/272/commits/624c53de38 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
 [https://github.com/xwp/wordpress-develop/pull/272/commits/9189511190
 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 [https://github.com/xwp
 /wordpress-develop/pull/272/commits/8abef679ed 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

--
Ticket URL: <https://core.trac.wordpress.org/ticket/21622#comment:30>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform