WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 weeks ago

#31779 reviewing enhancement

Warn users before using a built-in file editor for the first time

Reported by: helen Owned by: helen
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Themes Keywords: good-first-bug has-patch
Focuses: ui, administration Cc:

Description

As a person who began tinkering with her WordPress site via the theme and plugin file editors, I am not keen to get rid of them entirely. However, I think we could also do some small amount of education for our users. To that end, I suggest that on a per-user basis, a prompt be shown warning them of the dangers and suggesting alternative methods / linking to more documentation, for both the theme and plugin file editors. If they choose to continue, that prompt should not be shown again for that given editor.

Attachments (5)

first-time-editor-notices.diff (2.3 KB) - added by toddnestor 20 months ago.
Patch that adds the first time user notices for both the plugin and theme editors.
Screen Shot 2016-08-24 at 9.34.25 PM.png (73.8 KB) - added by helen 8 months ago.
31779-33.diff (52.0 KB) - added by aryamaaru 6 months ago.
patch for #31779
Edit Reminder.png (17.0 KB) - added by lukecavanagh 7 weeks ago.
Edit Reminder Example
31779.diff (12.2 KB) - added by ZaneMatthew 7 weeks ago.

Download all attachments as: .zip

Change History (43)

#1 @helen
2 years ago

A related thought, would probably warrant its own ticket were we to do it: I don't want to add "make a child theme" buttons everywhere in core (plugins do that just fine), but it could be a nice touch to prompt to create one if somebody is headed into the theme file editor.

#2 @MikeHansenMe
2 years ago

We have actually implemented this on a few of our onboarding tests and had zero complaints.

#3 follow-up: @magicroundabout
2 years ago

Hi guys,

I don't know if this is appropriate here, but I came to trac to see if there was a discussion about the idea of making the plugin and theme editors off by default and I couldn't find one, but I did find this.

My opinion is that these shouldn't be removed entirely, but they should be disabled - in the config - by default. My reasoning for this is security. I've probably worked on about 100 WordPress sites in the last four years and had maybe 5 or 6 sites hacked. Every time analysis of the logs has shown that after an initial entry (either by brute force or by some unknown method) the hacker bots proceeded to inject code using the theme editors.

I now add

define('DISALLOW_FILE_EDIT', true);

to wp-config.php as a matter of course. It's like a nervous tick: install WordPress, disable editors.

YES, I am totally aware that this is not a security risk if other security is present and correct: brute force protection, strong passwords, etc. But I should add that most of these sites that I'm rescuing are not sites that I developed - they are places where I'm bailing out another developer or an un-knowing user.

Is there a good reason why this feature, which seems to me to be a huge security issue in WordPress, is not off by default? I would suggest that this config option should be ALLOW_FILE_EDIT and it should be false by default. I would be quite happy for this to be an option during the WordPress install (though I doubt that would be a good idea as the user probably wouldn't understand the option in many cases). It would need to be backwards-compatible somehow to prevent an update breaking users' installs.

I'm happy to spin off another ticket to discuss. But this seemed an appropriate place to raise it. I hope you don't mind.

#4 in reply to: ↑ 3 @helen
2 years ago

Replying to magicroundabout:

If somebody has the ability to edit their wp-config.php, they are already a little past the point of needing/using an in-browser editor. Per the ticket description:

As a person who began tinkering with her WordPress site via the theme and plugin file editors, I am not keen to get rid of them entirely.

"Entirely" could also be "by default". This has been discussed in other venues a number of times - I don't see us disabling/removing them by default.

#5 follow-up: @magicroundabout
2 years ago

Thanks Helen, I'm sure it has been discussed before, I just can't find anywhere in trac or Google. Google is mostly people like me telling others how to disable the editors. I was hoping to find a rationale for keeping these things active. Can someone sum up why it is felt that the editors shouldn't be removed? Are there some stats showing usage? Is there research showing that users need or want these things and that if we take them away they will complain?

The amount of pain that these features cause me, as a developer, and my clients and end users FAR outweighs any benefit that we get from them, and even this ticket suggests that they should be used with caution. I'd turn your argument about ability/access on its head: if someone wants to be editing themes and plugins then they should probably be using something more sophisticated than an in-browser editor.

Thanks

#6 in reply to: ↑ 5 @helen
2 years ago

Replying to magicroundabout:

I'd turn your argument about ability/access on its head: if someone wants to be editing themes and plugins then they should probably be using something more sophisticated than an in-browser editor.

Yes, users *should* be using something else. However, they may not know any better. I was one of those users: I knew what FTP was, but only in the framework of getting MP3s from Audiogalaxy sources. I did not know what it had to do with making my blog look the way I wanted it to - if WordPress (like Blogger) hadn't provided in-browser methods for tinkering with my sites, I would never have gotten into WP development, full stop. I'm not here for the nostalgia, though - I'm saying this is a good place for a little bit of "are you sure about this" as well as education, not wholesale preventing users from learning that this world of editing the web even exists.

#7 @magicroundabout
2 years ago

I'm all up for education, but I'm still not hearing a convincing argument for keeping these features. There are better ways of learning the web these days - CodeAcademy, etc. My opinion would be that editing a PHP file where an incorrectly-placed piece of syntax white-screens your site so that you need to learn to use FTP anyway isn't a great way to learn to code. It's actually pretty frustrating.

Blogger let you use a templating language with tags, from what I remember. So these things aren't equivalent. The problem with the WordPress editor is that it lets you edit and create PHP.

A learning to code feature would be better on WordPress.com, but WordPress.com doesn't let me edit PHP files does it? I've never gone premium, but from what I can tell it only lets users edit CSS, which I think is an awesome idea - we should allow that, and there are good plugins (including JetPack) that let us do it. I'd love to see a built-in CSS editor.

But for WordPress.org? I'd say that by the time you've bought a domain, got a hosting package, logged into a cPanel or whatever and installed WordPress, you may as well have learned how to use FTP as well. Given that if you're editing code you'll probably end up white-screening anyway and needing FTP to recover, is it not better to teach people the FTP/control panel skills up front so they're not learning it in "ARGH MY BLOG IS DOWN" crisis mode? You could even teach them FTP by instructing them in how to add the "ALLOW_FILE_EDITORS" constant to wp-config.php?

In fact, the installation instructions for WordPress say:

Before you begin the install, there are a few things you need to have and do. These are:
Access to your web server (via shell or FTP)
A text editor
An FTP Client
Your web browser of choice

And if you're not doing it that way then you're using an installer in a hosting control panel where you probably have a file manager anyway, which is going to be a better experience than the theme/plugin editors.

I get what you're saying, I really do. But I just feel like these editors are a hinderance, even to people who want to learn to code and tinker with their site.

I'd definitely not say "do away with all ability to customise your site", I'm sure there are creative solutions to allow people that facility and I'd love to see them discussed. Could we add a PHP linter? Or some means of validating/sanitizing code. But the ability to add PHP in an un-sanitized manner is dangerous and I'd like to see it removed.

#8 @georgestephanis
2 years ago

I know there's previously been discussion of adding 'reauth' sections in the admin -- things you need to enter your password again to access. This would be as good of a candidate as any I've seen, just due to the potential extra danger of a session stealer injecting naughty php code via wp.

#9 @brocheafoin
2 years ago

I'm with @magicroundabout. I have just created ticket #31952 on the subject. My impression is that the file editor is a throwback to the days of the hacks.php file, when there where no widgets, etc. I'm pretty sure the most heavy users of this feature are hacking bots.

#10 @Otto42
2 years ago

#31952 was marked as a duplicate.

#11 @magicroundabout
2 years ago

I should add that @nacin was involved in an exchange on Twitter on this topic.

His stance was that the plugin and theme upload capability posed as much of a threat as the editors. I said that that may be true, but the uploaders aren't (in my experience, at least) being exploited. To which his response was that it's an arms race. Which I take to mean that if we lock down the editors then they'll head to the uploaders for their exploits.

(I hope this is an accurate interpretation of that conversation - apologies to Andrew if I've misrepresented his view)

I don't really have a response to the arms war comment. I think he's right. But I also think @brocheafoin is right in his new-but-closed ticket: the editors aren't actually much help anyway and end up getting users into trouble.

I think what I'd like to see here is a more informed and creative conversation about both the editors and the uploaders. If these are frequently exploited (or exploitable) functions from a security point of view, and cause users pain as much as they help them, then let's find some better solutions, as I don't feel like education is enough. I think (but I have no proof) that if you put a "This thing is dangerous and here's why" warning on the page, people will ignore it, as much as they ignore terms and conditions.

Some of my previous questions stand:

  • what are the usage stats for the editors?
  • is there any evidence that users will miss them?
  • could we come up with some way of ensuring that code modified through the editors is not malicious or broken? e.g. If someone proposed including a linting process, or even syntax checking, before saving a PHP file, then that might help.

But also I wonder if there are better ways to secure both the editor and uploader functions, or at the very least to monitor activity through them. Has use of a captcha (or honeypot, or similar) ever been discussed? Or some kind of re-verification? Or a log of changes so that hacks can be detected? I'm not a security expert, so I don't know what the modern-day options are here for preventing unwanted changes.

There seems to be acceptance that there is an issue here. The proposed solution is education/notification. I'd like to see some more options. Happy to be referred to a previous debate/discussion somewhere if this has already done the rounds.

Thanks

#12 @iseulde
2 years ago

  • Component changed from Editor to Themes

#13 @helen
20 months ago

  • Keywords needs-patch good-first-bug added

I would still like to see an interstitial prompt, so if somebody would like to make a patch, we can start looking at getting it in. I think we would also need to review or possibly write a good reference page in the Codex. Do note that this is a single targeted enhancement to an existing feature. There are a lot of other great ideas wrapped up in comments above that can (and should!) be their own distinct tickets if they are not already, like sandboxing/error checking and such.

I agree that there are better learning materials out there - what I see is that the person needs to know to be looking for those in the first place. We can wish all we want that people come into WordPress with some amount of web development knowledge, but that's just not the case, especially with one-click installers (me, even now!). Sure, there are file managers somewhere in cPanel if you can find them, but that still assumes you know where to browse to find the file you're looking for, and if you're not familiar with WP, do you know where to look? Do you know to go into public_html and then wp_content and then themes and then twentyfifteen and then maybe which file you want? I know I wouldn't have.

I say this frequently in talks: when you're running at the scale WordPress is running on the internet, you have this incredible opportunity to show people that web development is a thing that exists. Not teaching how to do it, not showing off with our code (ha, ha) - just that it exists at all, and maybe how to learn about it. Intrepid learners can take it from there. It sounds so basic in our world, seeing as we're here on Trac, but you can't have options if you don't know what they are. I would like to see users helped along better and saved from hurting themselves as much as possible, so let's make this experience good enough to be safe but not so good it prevents users from ever using something else. :)

#14 @toddnestor
20 months ago

I would like to make a patch for this as my first patch I make for the Wordpress Core. I am going to work on it right now.

@toddnestor
20 months ago

Patch that adds the first time user notices for both the plugin and theme editors.

#15 @toddnestor
20 months ago

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

I added a patch to the attached files, I couldn't really find a good article about how to submit a patch (https://make.wordpress.org/core/handbook/tutorials/trac/submitting-a-patch/ only seems to tell you how to make one), so I am assuming that is how I do it.

#16 follow-up: @RSPublishing
20 months ago

Interesting!

That given 'editor' would only be either Super admin (mu) or admin (single), correct.?

While new to this, in my head:

1 > given a prompt urging to backup folders/files before continuing
2 > given another prompt warning him of potential dangers/alternatives
3 > given option to proceed (enter editor) / cancel (redirect to dashboard)

Wouldn't mind playing around with this.

#17 in reply to: ↑ 16 @toddnestor
20 months ago

I actually wrote a patch for this that you can see in the attachments. However I don't know how to submit the ticket or get it over to testing.

Replying to RSPublishing:

Interesting!

That given 'editor' would only be either Super admin (mu) or admin (single), correct.?

While new to this, in my head:

1 > given a prompt urging to backup folders/files before continuing
2 > given another prompt warning him of potential dangers/alternatives
3 > given option to proceed (enter editor) / cancel (redirect to dashboard)

Wouldn't mind playing around with this.

#18 @johnbillion
19 months ago

  • Owner set to helen
  • Status changed from new to reviewing

#19 @jb510
13 months ago

+1 on this ticket, but +100 on removing the editor from core all together, I've long felt it should be a plugin and prompted for installation similar to the WordPress Importer along with upgrading it to provide basic syntax highlighting and checking.

Still, love the idea requiring reauth for using it and a warning prompt. Following.

#20 @celloexpressions
13 months ago

Related: #35395. Seeks to provide a better alternative to the code editors for, well introducing users to code, particularly for those like me (and also helen) who started out be playing with stuff in the theme and plugin editors. Not directly tied to the future of the editors but would certainly offer some direction. That ticket is still awaiting constructive feedback from the security team on how we could make it work/what's required; any assistance would be greatly appreciated.

Also +1 on this ticket.

#21 @Presskopp
10 months ago

All discussion aside, I don't really like the phrasing

You are editing the actual theme files. Make sure you know what you're doing!

From a user's perspective:
"What am I doing? I don't care, I want to edit something, WP nags me with a dismissable "Are you sure"-crap - if I wasn't sure, I wouldn't have gone to editor, stupid WP-developers!" ;-)

So I propose something more like:
Please be aware by editing a file this way you can easily do damage to your site - better keep it backed up

I changed this sentence while typing 4-5 times, because my mind didn't stop throwing alternatives. So it's only one example.

I'm not a so called security-expert, but +1 for https://core.trac.wordpress.org/ticket/31779#comment:19

#22 @Presskopp
10 months ago

Just found this in the translation files (wp-admin/theme-editor.php:32):

<strong>Advice:</strong> think very carefully about your site crashing if you are live-editing the theme currently in use.

There is a connection.

#23 follow-up: @helen
8 months ago

Screen Shot 2016-08-24 at 9.34.25 PM.png is a quick mockup of what I was imagining, which is an interstitial rather than a notice. Clicking the button would take you to the editor as it currently exists and the interstitial for editing a theme would no longer show for that specific user. Plugin editor should get the same treatment with its own separate user meta.

The important things here are:

  • The right language in all of the messaging, including letting the user know that they won't see this message again should they continue on to the editor. My mockup is by no means a hard suggestion, although I think it's not so bad.
  • A resource (on .org somewhere? Codex?) that talks about custom CSS, child theming, FTP, and maybe even with a link off to something about version control. This would be the "learn more" link.
  • A parallel resource for plugins.
  • Visually emphasizing that this is very important to read, maintaining hierarchy and still looking good.

Each of these items can be worked on separately, and if one wants to take a stab at a patch for the functionality of it, that is also a good start (although there should be agreement that this is a good approach).

Reauth may be a nice add as well, but let's do this part first.

#24 follow-up: @brocheafoin
8 months ago

Does this mean this task is blessed for 4.7?

Can we work on the wording right here or do we have to submit patches?

#25 in reply to: ↑ 23 ; follow-up: @boogah
8 months ago

Replying to helen:

Clicking the button would take you to the editor as it currently exists and the interstitial for editing a theme would no longer show for that specific user.

I am incredibly excited about this! It'd be nice to have the ability to not hide the interstitial after it has been dismissed though. Via a filter, perhaps?

#26 in reply to: ↑ 24 @helen
8 months ago

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

Replying to brocheafoin:

Does this mean this task is blessed for 4.7?

Can we work on the wording right here or do we have to submit patches?

I generally don't move existing tickets into a milestone without a patch, but so long as there's general agreement that this is a good move and patches are made, I imagine it would go into 4.7.

Language work can go on right here on Trac, it can be annoying to do via constant repatching.

Replying to boogah:

It'd be nice to have the ability to not hide the interstitial after it has been dismissed though. Via a filter, perhaps?

I guess there are two potential methods here, one that is kind of hacky but not adding new hooks, and the other new hooks. I suppose a new hook makes sense in particular for managed instances and hosts - curious if in those cases you'd want to be linking off to your own resource instead of .org.

This ticket was mentioned in Slack in #design by hugobaeta. View the logs.


8 months ago

#28 in reply to: ↑ 25 ; follow-up: @brocheafoin
8 months ago

Replying to boogah:

It'd be nice to have the ability to not hide the interstitial after it has been dismissed though. Via a filter, perhaps?

I don't see the use case for this. If you're that worried about your users that you want them to see the warning all the time, you should probably just DISALLOW_FILE_EDIT.

Design-wise, I think the interstitial should be as in-your-face as Chrome or Firefox' malware/phishing warning. It really _is_ the most dangerous part of the WordPress admin. One false move could turn your whole site, including the admin, into a white screen of death. So here's my wording suggestion for the theme editor:


Danger Ahead!

This theme editor is provided as a convenience, but it is not the recommended way to modify a theme.

One false move can render your whole site unusable. That means you won't even be able to come back here to undo what caused the error. You would need to restore a backup using FTP. If you don't know what FTP is, you probably should not be using this editor.

Furthermore, even if everything goes well, as soon as the theme is updated, you will lose all the changes you have made.

If you want to make small style changes, consider intalling Jetpack and using its Custom CSS module. It provides a better editor, keeps track of your changes, can not break your site and will keep your modifications when the theme is updated.

If the changes you want to make are more substantial, the recommended approach is to set up a local development environment and create your own child theme. This will isolate your modifications and make sure they are retained when the original theme is updated.

Primary button label: Back to safety
Secondary button label: Just leave me alone, I know what to do


Reference for the secondary button label: https://www.youtube.com/watch?v=nlbjnZbxEcI

Last edited 8 months ago by brocheafoin (previous) (diff)

#29 follow-up: @celloexpressions
8 months ago

I generally like the text in 28, although we should not point them to Jetpack for custom CSS (it isn't in the customizer). Hopefully #35395 will be ready for 4.7, or if not there are numerous other, better plugins to suggest.

#30 in reply to: ↑ 29 @brocheafoin
8 months ago

Replying to celloexpressions:

we should not point them to Jetpack for custom CSS (it isn't in the customizer).

I put Jetpack in there because it's the one I use for quick CSS mods. I honestly thought it was available in the Customizer since that's the way it works on WP.com.

Personally, I hate typing CSS in the Customizer's narrow column but to each their own. I really don't care what it is, but we need to link to something allowing custom CSS.

Hopefully #35395 will be ready for 4.7

I did not know Custom CSS was considered as a future part of core. If it's to happen, obviously we should just point there.

there are numerous other, better plugins to suggest.

I am all ears. Maybe it would be better if the notice suggested a few plugins so it doesn't appear to favour one in particular?

#31 @DrewAPicture
8 months ago

I know it's been discussed previously, but it's also worth reconsidering merging the Code Revisions plugin that was developed as a WordPress GSOC project a couple of years ago. Perhaps not so much the revisioning aspect, but more-so the breaking changes safeguards that prevent actively breaking a site when making edits via the file editors.

#32 in reply to: ↑ 28 @voldemortensen
8 months ago

Replying to brocheafoin:

Replying to boogah:

It'd be nice to have the ability to not hide the interstitial after it has been dismissed though. Via a filter, perhaps?

I don't see the use case for this. If you're that worried about your users that you want them to see the warning all the time, you should probably just DISALLOW_FILE_EDIT.

Design-wise, I think the interstitial should be as in-your-face as Chrome or Firefox' malware/phishing warning. It really _is_ the most dangerous part of the WordPress admin. One false move could turn your whole site, including the admin, into a white screen of death.

I think you just provided a use case. If it is indeed the most dangerous part of the admin, it should have reminders. Chrome and Firefox don't just show you a phishing warning for one site and then never warn you again. They do allow you to click through details, and then visit the site. But I don't think there's a way to permanently dismiss a phishing warning for a site (other than doing something like --disable-web-security in chromium or similar for other browsers).

I think it should show every time a file editor is loaded by default, with a filter to only show it once or disable it.

@aryamaaru
6 months ago

patch for #31779

#33 @ZaneMatthew
7 weeks ago

Is the patch still valid?

When applying the patch via

grunt patch:31779

I'm getting the following;

Running "patch:31779" (patch) task
? Please select a patch to apply 31779-33.diff (52.0 KB) - added by aryamaaru 4 months ago.
patching file wp-admin/themes.php
Hunk #1 FAILED at 158.
1 out of 1 hunk FAILED -- saving rejects to file wp-admin/themes.php.rej
patching file wp-admin/theme-editor.php
patching file wp-admin/js/theme.js
Hunk #1 FAILED at 83.
1 out of 1 hunk FAILED -- saving rejects to file wp-admin/js/theme.js.rej
can't find file to patch at input line 58
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: wp-admin/js/theme.min.js
|===================================================================
|--- wp-admin/js/theme.min.js	(revision 38598)
|+++ wp-admin/js/theme.min.js	(working copy)
--------------------------
File to patch:
Skip this patch? [y] y
Skipping patch.
1 out of 1 hunk ignored

Done, without errors
Last edited 7 weeks ago by ZaneMatthew (previous) (diff)

#34 @ZaneMatthew
7 weeks ago

I was able to get the patch working, just a few things seemed odd. Currently working on adjusting it.

@lukecavanagh
7 weeks ago

Edit Reminder Example

@ZaneMatthew
7 weeks ago

#35 @ZaneMatthew
7 weeks ago

  • Keywords has-patch added; needs-patch removed

@lukecavanagh I just submitted a patch based the styles that are inline with core, see @helen mockups.


Aside from removing the "Editor" from core, I feel strongly that this is a good approach. Granted, there are helpful resources in that seldom-clicked-on-help-menu.

I've submitted a patch based on Helen's suggestion that does the following;

  • Given a users first time visiting the "Edit Themes" page a message is displayed. This message affords; a verbose or concise statement, resource URLs, and an action to be taken.
  • Once the user takes an action, their user meta is updated to no longer display this message.

A question for those following;

#36 follow-up: @lukecavanagh
7 weeks ago

@ZaneMatthew

I do not have a problem with keeping the editor in core, but a warning for end-users before editing plugins or theme files is a decent change.

It makes sense to show the warning to first time users on site before editing.

Last edited 7 weeks ago by lukecavanagh (previous) (diff)

#37 @ZaneMatthew
7 weeks ago

While working on another project I was greeted with a message that solves the issue we are discussing above.

Caution: You can break stuff here.

Its coarse, yet concise.

If the above patch has the desired work flow, we should focus next on content, including URLs to send visitors to for guidance. The next round of revisions on the patch should include URLs, and content (maybe the statement above ;)

#38 in reply to: ↑ 36 @ZaneMatthew
3 weeks ago

@lukecavanagh I agree as well, it is a "nice to have" feature/notice for first time users.

Replying to lukecavanagh:

@ZaneMatthew

I do not have a problem with keeping the editor in core, but a warning for end-users before editing plugins or theme files is a decent change.

It makes sense to show the warning to first time users on site before editing.

Note: See TracTickets for help on using tickets.