#21622 closed enhancement (fixed)
Validate or sandbox theme file edits before saving them (as is done for plugins)
Reported by: | eschwartz93 | Owned by: | 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)
Change History (47)
#1
@
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:
↓ 3
@
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
@
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
@
12 years ago
Thats true but we don't check if we change a plugin. The way of doing is different since you want to check something that is loaded by default.
#5
@
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
@
12 years ago
- Component changed from Administration to Editor
- Keywords needs-patch added
- Version changed from 3.4.1 to 2.7.1
#7
@
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. :)
#10
@
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:
↓ 13
@
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).
#13
in reply to:
↑ 11
;
follow-up:
↓ 17
@
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:
- User submits theme editor with change to a PHP file.
- WordPress grabs a copy of the file's contents and stores them in memory as a backup.
- WordPress writes the submitted changes to that file and (invalidates the opcache).
- 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.
- 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. 🚫
- 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
@
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.
#16
@
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
@
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:
↓ 19
@
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
@
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
@
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
@
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
@
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.
#24
@
7 years ago
Demo of what I have working so far: https://wordpress.slack.com/files/U02RR79S2/F7C97BS0J/plugin-live-editing-via-ajax-and-without-deactivation.mov
It's working well!
#25
@
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:
↓ 30
@
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 afunction_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 ofjson_encode()
inwp_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:
↓ 36
@
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
@
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
@
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:
↓ 32
@
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 afunction_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 ofjson_encode()
inwp_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
@
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:
↓ 34
@
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 thefile_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 towp.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
@
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
@
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
@
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
#39
@
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:
↓ 41
@
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
@
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
@
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.
missing bracket on line 15