Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#42609 closed defect (bug) (fixed)

Regression: WordPress 4.9 theme editor cannot edit files when running on a Windows based server

Reported by: otto42's profile Otto42 Owned by: dd32's profile dd32
Milestone: 4.9.1 Priority: normal
Severity: normal Version: 4.9
Component: Editor Keywords:
Focuses: Cc:

Description

The new ajax based editor in WordPress 4.9 calls the new wp_edit_theme_plugin_file function to perform the file edit.

On Windows based servers, this function fails in the call to validate_file, because Windows based machines will have filenames like the following sent to it:

C:\xampp\htdocs\wptrunk/wp-content/themes/twentyseventeen/style.css

The validate file function returns 2 because of the colon in the second character slot.

This worked in 4.8.3 because the editing code did not call validate_file(), it called validate_file_to_edit(), which in turn calls validate_file() but then ignores the "2" response from it.

Both the plugin and theme editor interfaces call the validate_file_to_edit() function, it's simply the new ajax code that is calling validate_file() directly.

Replacing validate_file with validate_file_to_edit in wp_edit_theme_plugin_file() should fix the issue.

Attachments (2)

42609.1.patch (822 bytes) - added by Otto42 7 years ago.
Immediate fix, but not the best solution
42609.diff (1.4 KB) - added by dd32 7 years ago.

Download all attachments as: .zip

Change History (37)

#1 @Otto42
7 years ago

#42576 was marked as a duplicate.

#2 @westonruter
7 years ago

  • Milestone changed from Awaiting Review to 4.9.1

#3 @Otto42
7 years ago

The specific issue is here:

https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/file.php#L388

https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/file.php#L448

The validate_file function will return 2 for Windows paths. The validate_file_to_edit ignores this case and returns null, which is not ideal. The validate_file_to_edit function seems strangely written and probably needs refactoring overall.

@Otto42
7 years ago

Immediate fix, but not the best solution

#4 @Otto42
7 years ago

  • Keywords has-patch 2nd-opinion good-first-bug added; needs-patch removed

42609.1.patch will fix the problem for those who need an immediate fix, but it's not the best solution and probably needs refactoring overall.

#5 follow-ups: @247pinger
7 years ago

I just wanted to add that this is not Windows specific... I am unable to edit new files added to my theme. But I am able to edit files that were a part of my theme prior to upgrading to 4.9.

I just created a new file called "page-giftcards.php" using the touch("page-giftcards.php") php code in one of my other theme files (which worked until updating to 4.9).

And not only does the file not show up in the list of theme files as seen through the dashboard (I have verified it's existence by logging into the server via SSH)... but when I hack the URL of my theme editor to try and edit page-giftcards.php specifically...

I am told "Sorry, that file cannot be edited."

And yes I have checked all of the permissions. The file permissions for the new page-giftcards.php is exactly the same as every other file in the theme folder (that is editable).

Did wordpress security implement a new method of verifying theme files or something? And it won't let me edit a file that it does not consider part of it's theme?

I am very stumped over this.

Running Debian 8.6 and NGINX.

#6 follow-up: @Otto42
7 years ago

@247pinger Thank you for your comment, however, this issue is Windows specific. Your issue is a different issue likely caused by permissions or file ownership.

You should post in the support forums about it.

#7 @birgire
7 years ago

#42638 was marked as a duplicate.

This ticket was mentioned in Slack in #core-committers by johnbillion. View the logs.


7 years ago

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


7 years ago

#10 in reply to: ↑ 5 @platinumshore
7 years ago

Confirmed.. have same problem on cloudlinux. but we seem to be getting ignored by the community :-( And temporary patch supplied didn't fix the problem either.

Replying to 247pinger:

I just wanted to add that this is not Windows specific... I am unable to edit new files added to my theme. But I am able to edit files that were a part of my theme prior to upgrading to 4.9.

I just created a new file called "page-giftcards.php" using the touch("page-giftcards.php") php code in one of my other theme files (which worked until updating to 4.9).

And not only does the file not show up in the list of theme files as seen through the dashboard (I have verified it's existence by logging into the server via SSH)... but when I hack the URL of my theme editor to try and edit page-giftcards.php specifically...

I am told "Sorry, that file cannot be edited."

And yes I have checked all of the permissions. The file permissions for the new page-giftcards.php is exactly the same as every other file in the theme folder (that is editable).

Did wordpress security implement a new method of verifying theme files or something? And it won't let me edit a file that it does not consider part of it's theme?

I am very stumped over this.

Running Debian 8.6 and NGINX.

#11 in reply to: ↑ 6 @platinumshore
7 years ago

Sorry but 247pinger is correct. I have tested a fresh install of wordpress 4.9 on cloudlinux and again unable to edit themes.. nothing to do with access rights.

Replying to Otto42:

@247pinger Thank you for your comment, however, this issue is Windows specific. Your issue is a different issue likely caused by permissions or file ownership.

You should post in the support forums about it.

#12 @Otto42
7 years ago

@platinumshore I understand that you have the same symptoms, but it is not the same problem.

This ticket is specific to the issue where Windows servers cannot edit files. The patch will only work for those systems having this specific problem.

There are other similar issues that can cause the same results on other platforms, and when we discover the causes of those, new tickets will be made for those issues.

For example, we have identified a number of plugins that will cause the file editors not to work because the plugins contain "session_start()" in them. Other known issues include the loopback checks on some hosts not working. These are all different problems that lead to the same end result.

So, we are not ignoring your problem, we are actively searching for those issues and attempting to fix them. But the same results do not indicate that you have the same problem.

In the meantime, please post on the support forums and not on the Trac. Trac is for monitoring and fixing known issues, the support forums are where we discover issues and find out their causes.

#13 @jharri92
7 years ago

Just wanted to add that the patch has not worked for me; I still get the same error when trying to update the style.css file through the Editor.

#14 @Clorith
7 years ago

#42676 was marked as a duplicate.

#15 @dd32
7 years ago

Hey @Otto42
Could you please post some extra details on how to reproduce this?

I'm unable to duplicate it on my windows hosting, it seems to act the same as what it does on a linux host.
The POST vars sent along look correct:

nonce: 1234568790
_wp_http_referer: /wordpress/wp-admin/plugin-editor.php?plugin=hello.php&Submit=Select
newcontent: <?php // testing
action: edit-theme-plugin-file
file: hello.php
plugin: hello.php
docs-list:

Based on your description, it sounds like a full filesystem path is being passed through the file param, is that correct? Can you see any reason why that's happening / trace down what causes that to be the case?
IIRC we fixed the bug where the full windows path would be shown in the file editor params, although I'm not sure if that was supposed to be fixed in 4.9 or earlier than that.

Thanks!

#16 follow-up: @Otto42
7 years ago

@dd32 Windows 10 system, XAMMP, basic default install. Fails by default. Nothing special, it's the validate_file call getting a path with a colon as the second character.

#17 in reply to: ↑ 16 @dd32
7 years ago

Replying to Otto42:

@dd32 Windows 10 system, XAMMP, basic default install. Fails by default. Nothing special, it's the validate_file call getting a path with a colon as the second character.

I tested with a Windows XP system, XAMMP, basic default install. Works by default. Nothing special.

I understand that it's the vaildate_file(), but I'm suggesting that's not the bug - the bug is earlier, whatever is causing the full filesystem path to be in the POST vars in the first place. They're not supposed to be there, and that's what I can't duplicate.

What's the URL to the plugin editor when you're editing a file, is it plugin-editor.php?file=hello.php&plugin=hello.php? or does it have the full path there? What about in the page source, what's the value of the <input type="hidden" name="file" fields? What about the header of the page? is it Editing hello.php or does it have the full path there too?

#18 @dd32
7 years ago

  • Summary changed from Regression: WordPress 4.9 plugin/theme editor cannot edit files when running on a Windows based server to Regression: WordPress 4.9 theme editor cannot edit files when running on a Windows based server

No sooner than I post that, I've duplicated it - It only affects the theme editor, plugins are not affected.

The issue in the Theme editor is that we're not actually comparing the no-path variants which are used in the source, it's that we're using validate_file( $real_file, $allowed_files ) which is using full paths, not relative ones, so even though validate_file() passes earlier on likes 357/404 it'll fail when it gets to the "Check that this file exists within the theme" check.

#19 @Otto42
7 years ago

No, this has nothing to do with the URL or post parameters. The path passed to validate_file is a full path, not a partial one.

I will dig further and test with XP if needed, but the problem is real and the (terrible) patch works. The 2 case from validate_file needs to be ignored for a reason. Look deeper into why validate_file_to_edit did it to find out why.

Edit: ah, themes makes sense. Seems like this should be made saner overall then.

Last edited 7 years ago by Otto42 (previous) (diff)

@dd32
7 years ago

#20 @dd32
7 years ago

42609.diff makes the check for themes the same as for plugins, instead of passing the full filesystem path, it simply passes the relative path to the stylesheet directory.

#21 @dd32
7 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

In 42244:

Theme Editor: Validate files are editable based on their relative filenames, rather than full file path.

This fixes theme editing on Windows platforms where validate_file() will return 2 on a full file path.

Fixes #42609.

#22 @dd32
7 years ago

In 42245:

Theme Editor: Validate files are editable based on their relative filenames, rather than full file path.

This fixes theme editing on Windows platforms where validate_file() will return 2 on a full file path.

Merges [42244] to the 4.9 branch.
Fixes #42609.

#23 @dd32
7 years ago

I went with 42609.diff as we should ideally remove the absolute paths from this section of code.

The final component which uses the full path rather than the relative path is the nonces, which I'm going to switch over in the next commit with a new ticket for tracking in 5.0.

#24 @dd32
7 years ago

In 42246:

Theme Editior: Base the nonce on a simpler combination of fields, for easier debugging & reading.

See #42609.
Fixes #42705.

#25 follow-up: @highpec
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi,

I have tried all the patches in this thread, and every one causes PHP/MySQL to crash completely. I understand code and am certain I'm adding the changes to the proper locations.

Specs:
Windows 7 Professional x64
WAMP x64 3.0.6
Apache 2.4.23
PHP 5.6.25
MySQL 5.7.14

Crashes PHP/MySQL (unsure which one, but 1/2 crashes) entirely:
https://core.trac.wordpress.org/attachment/ticket/42609/42609.diff

Has no impact at all:
https://core.trac.wordpress.org/changeset/42246

I love the new Wordpress theme / script editor and want to upgrade my localhost installs, but I've tried everything and it does not work.

I just tested updating a plugin, as a previous response here said that plugins were not affected. I reversed all changes back to default 4.9, and editing a plugin again completely crashes PHP/MySQL on WAMP.

I understand why in theory the patch would work -- because you're mimicking what the plugins are supposedly doing when editing, and applying it to themes, however, for me the plugin editor crashes WAMP by default.

Any idea why the patch doesn't work for me -- any idea when this issue will be fixed / rolled out in an update?

Thanks!

Last edited 7 years ago by highpec (previous) (diff)

#26 @dd32
7 years ago

  • Keywords has-patch 2nd-opinion good-first-bug removed

Crashes PHP/MySQL (unsure which one, but 1/2 crashes) entirely:

Can you expand on that? What exactly happens? Do you get a PHP fatal error? anything in logs?

Can you try installing http://wordpress.org/nightly-builds/wordpress-4.9-latest.zip over the top of your install to see if that helps?

#27 in reply to: ↑ 25 @dd32
7 years ago

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

Replying to highpec:

I just tested updating a plugin, as a previous response here said that plugins were not affected. I reversed all changes back to default 4.9, and editing a plugin again completely crashes PHP/MySQL on WAMP.

This sounds like something completely unrelated to the editor and probably unrelated to WordPress completely to be honest.

I'm going to remark this ticket as fixed for now, but if you can post any information which indicates that it's not, I'll re-open it.

#28 @highpec
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi,

I've used every previous version of Wordpress on my WAMP install and have never had a problem... so, it must a problem with Wordpress.

I've honed it more on the exact problem.

Note: This is after updating to the version you linked in your response, the same issue is happening.

Saving a CSS file works properly, but ANY php file crashes (rather, forces a timeout and restarts) WAMP/Apache/MySQL. Whatever is different between saving a CSS file and saving a PHP file, is what's causing the issue. I also tried multiple PHP files, footer.php, header.php, all crash in the same way. All CSS files however, saved properly.

It seems to be this file that's timing out / crashing...

admin-ajax.php

Here are more details from the Chrome network section:

https://i.imgur.com/fjw61EO.jpg

I have also looked into the Apache/PHP/MySQL logs and don't see anything besides the server / process forcing itself to restart. There doesn't seem to be any indication of what is actually causing it to restart.

Access logs show up to trying to update, then just ends.

127.0.0.1 - - [28/Nov/2017:22:19:28 -0500] "GET /wp-admin/theme-editor.php HTTP/1.1" 200 54652
127.0.0.1 - - [28/Nov/2017:22:19:31 -0500] "GET /wp-admin/load-styles.php?c=1&amp;dir=ltr&amp;load%5B%5D=dashicons,admin-bar,code-editor,common,forms,admin-menu,dashboard,list-tables,edit,revisions,media,themes,about,nav-menus,wp-poi&amp;load%5B%5D=nter,widgets,site-icon,l10n,buttons,wp-auth-check&amp;ver=4.9.1-beta1-42254 HTTP/1.1" 200 2
127.0.0.1 - - [28/Nov/2017:22:19:30 -0500] "GET /wp-admin/theme-editor.php?file=comments.php&theme=imnews HTTP/1.1" 200 55228

Downloaded a few different themes, all the same result.

Last edited 7 years ago by highpec (previous) (diff)

#29 @dd32
7 years ago

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

@highpec Thanks, that confirms that the issue you're experiencing is unrelated to this ticket. This ticket is specifically related to the case where editing a file on a windows platform returns Sorry, that file cannot be edited..

It sounds like you may have an issue with the added features of validating that the PHP files parse after an update. That was also added in #21622

Unfortunately it's impossible for me to diagnose that remotely, and you'll need to debug/diagnose the issue yourself.
Once you've done so and able to pinpoint what the issue is, you can create your own Trac ticket detailing the issue and steps to reproduce / what line of code is failing and/or causing issue.

The area of code you'll need to investigate is most likely (but may be something else entirely) this: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/file.php?marks=491,494-601#L485
If I had to guess, it's probably related to the opcache_invalidate() calls with a specific version of PHP/extension combinations that fails - but you'll need to debug that yourself and open a new ticket if that's the case.

#30 @highpec
7 years ago

I don't understand how it's unrelated to this ticket. I was having the original issue "Sorry, that file cannot be edited."

I am on a Windows 7 machine running WAMP x64.

I installed the patches on this thread, which caused this timeout/crash issue. It seems you just integrated the patches into the latest WP version, which also causes the issue.

It seems the "fix" you've created treats CSS and PHP files differently when editing, and that's what's causing the issue...

How is a novice coder like myself supposed to debug a core Wordpress issue? The "fix" you added is what's causing the issue, and IMO should be of concern because it will likely affect thousands of other Windows users.

Last edited 7 years ago by highpec (previous) (diff)

#31 @highpec
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#32 @dd32
7 years ago

@highpec Unfortunately there's nothing I can do to help unless you can provide further details.

Please create your own ticket if you're experiencing related/similar issues after this patch with details and some diagnosis. This ticket is fixed and will be closed on the 4.9.1 milestone, any further issues will be tasked to 4.9.2 or later.

This is my last response on your specific issue, unless you can provide further details that helps narrow down your issue in a new ticket.

#33 @Otto42
7 years ago

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

@highpec This is not the same issue. You are having a different problem. This problem is very specific and fixed.

You should start a support forum thread about your specific problem, and give details there so that we can attempt to solve it. You will not get a solution to your problem in this ticket.

#34 in reply to: ↑ 5 @SergeyBiryukov
7 years ago

Replying to 247pinger:

I just created a new file called "page-giftcards.php" using the touch("page-giftcards.php") php code in one of my other theme files (which worked until updating to 4.9).

And not only does the file not show up in the list of theme files as seen through the dashboard (I have verified it's existence by logging into the server via SSH)... but when I hack the URL of my theme editor to try and edit page-giftcards.php specifically... I am told "Sorry, that file cannot be edited."

This sounds more like #42573 (also fixed in 4.9.1).

#35 @pento
5 years ago

#42569 was marked as a duplicate.

Note: See TracTickets for help on using tickets.