WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#30921 closed defect (bug) (fixed)

"Cannot Remove Old Plugin" Obliterates files!

Reported by: johnstonphilip Owned by: dd32
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.1
Component: Upgrade/Install Keywords: has-patch needs-testing
Focuses: Cc:

Description

The problem: If your file permissions are 'owned' by a specific user (sudo chown -R www-data:username /var/www) but is thrown off somehow, you end up with a scenario where WP can delete files but not folders. Thus, when you do an auto-update, it HALF-DELETES all of the plugins you are updating.

Imagine you are updating 15-20 plugins and the WP Filesystem removed the "parent" plugin file but no sub-directories within those plugins. The update can't finish and gives the error "Cannot Remove Old Plugin". Your WordPress becomes pretty un-usable at this point as you need to go through each plugin directory with a fine-toothed comb to see which plugins were half-deleted.

The solution seems fairly simple: before doing an update, create an arbitrary folder and delete it using the WP Filesystem. If it comes back with errors, we know the permissions are not set right and we can stop trying to update.

This way, we can prevent obliterating peoples files by half-deleting them - and only THEN telling them the permissions are wrong.

Background: I recently started hosting my own WP installs on Digital Ocean. I am by NO means a "professional" server administrator and definitely have issues with permissions. However, I feel this issue is something that WordPress could "see coming" and at least prevent your entire file-system from getting utterly destroyed when doing auto-updates.

With the popularity of Digital Ocean and self-managed systems increasing, I feel this issue requires immediate attention. I'm just not sure of the exact right place to do a check that like.

Any thoughts?


Attachments (4)

Could-not-delete-old-plugin.jpg (98.2 KB) - added by johnstonphilip 5 years ago.
Image preview of issue
30921.diff (2.9 KB) - added by dd32 5 years ago.
Screen Shot 2015-06-16 at 10.39.17 AM.png (151.4 KB) - added by johnstonphilip 5 years ago.
Yes! Plugin updates with root ownership failing without deleting any files
30921.2.diff (7.8 KB) - added by dd32 5 years ago.

Download all attachments as: .zip

Change History (23)

#1 @dd32
6 years ago

  • Component changed from Filesystem API to Upgrade/Install

For Plugin and Theme updates, it would make sense for us to perform an is_writable() check on each file within the plugin/theme folder.

The problem with that is some servers will report that a file is writable, but it won't be deletable or modifiable due to filesystem ACL's that PHP simply doesn't understand (linux acl's and all flavours of Windows), so it won't fix it 100%, but will protect against cases where it's simply a file ownership issue.

#2 @johnstonphilip
5 years ago

Yeah thats a good idea. It may not prevent ALL cases - but perhaps it would for most. In my experience with clients doing their own server administration (and going into their FTP after all their plugins have been half-deleted) is that most often, WP has permission to delete "files" but not "folders".

Can we do an is_writable check on both files and folders? Are both considered "files" in these types of instances?

@johnstonphilip
5 years ago

Image preview of issue

#3 follow-up: @dd32
5 years ago

Can we do an is_writable check on both files and folders? Are both considered "files" in these types of instances?

My assumption was yes, it'd be applied to both files and folders.

However, I've just realised that FTP doesn't really allow us to verify if a folder is writable/deletable without actually trying. It's made harder by the fact that FTP exposes permissions differently than the underlying filesystem, so it's a potentially difficult situation. We could certainly perform it for requests using the direct transport (direct file operations) though for sure.

#4 in reply to: ↑ 3 ; follow-up: @valendesigns
5 years ago

Replying to dd32:

However, I've just realised that FTP doesn't really allow us to verify if a folder is writable/deletable without actually trying. It's made harder by the fact that FTP exposes permissions differently than the underlying filesystem, so it's a potentially difficult situation. We could certainly perform it for requests using the direct transport (direct file operations) though for sure.

In FTP mode is there an equivalent action, in terms of permissions, that could be performed on the directory to see if it's writable before trying to delete it? For example, moving it or something along those line. Or possibly, we could move the files into a temp directory until we know if the directory can be deleted and either delete on success or move back on failure. This way the files are not lost and craziness never ensues. ;)

#5 in reply to: ↑ 4 @johnstonphilip
5 years ago

Replying to valendesigns:
That's the type of check I was thinking would be valuable as well. I'm not sure if there's a better way or not.

#6 @johnstonphilip
5 years ago

I'm not totally sure of how WordPress checks if a folder/directory is writable, but I do know that the owner has to be www-data in order for WordPress to write to it.

Could we potentially use php's filegroup function to check the owner is www-data prior to doing the upgrade?
http://php.net/manual/en/function.filegroup.php

(I wish I had more time to delve into the code on this, but alas I do not at this exact moment in time)

#7 @johnstonphilip
5 years ago

Just got the "WordPress white screen of death". After a bit of investigating I realized it was because of this problem when it tried to auto update to 4.2.1 and didn't have the ability to write to the server any longer because the permissions had been temporarily changed.

It would be soo great if WordPress could check before it tries to delete stuff.

#8 @jorbin
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

If this is causing wsod, it seems like something that should be fixed.

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


5 years ago

#10 follow-up: @jorbin
5 years ago

@dd32 do you think there is a benefit in adding this in for the transports that can support it? Do we want to go with a progressive enhancement approach to the transports so that ones which can easily support solutions to issues like this get them, while ones like FTP in this situation miss out?

#11 in reply to: ↑ 10 @dd32
5 years ago

Replying to jorbin:

@dd32 do you think there is a benefit in adding this in for the transports that can support it? Do we want to go with a progressive enhancement approach to the transports so that ones which can easily support solutions to issues like this get them, while ones like FTP in this situation miss out?

Depending on what "it" is, then yes.

The Core Upgrade already performs a lot of extra checks that Plugin & Themes don't, if we can bring any of those over we should.

Simply running a is_writable loop over all of the items being touched could work, as that's a no-op function for FTP, it'll be a progressive enhancement.

@dd32
5 years ago

#12 @dd32
5 years ago

  • Milestone changed from Future Release to 4.3

Lets do something here in 4.3.

30921.diff performs a $wp_filesystem->is_writable() check on all files before it attempts to run a delete. This applies for Theme/Plugin updates.

I've tested sparingly by altering a plugin file to be owned by root and attempting to update.

#13 @programmin
5 years ago

That would be great if this could be in upcoming 4.3! While you're on this I wonder it would be possible to add the isWritable check on FTP, so it can't seriously break plugins ( see #29610 )?

#14 @dd32
5 years ago

This patch doesn't deal with the fact that we might be able to alter the permissions to allow the write to occur, so we may need to add a chmod() call in there in the event the check fails.

While you're on this I wonder it would be possible to add the isWritable check on FTP, so it can't seriously break plugins ( see #29610 )?

We can keep discussion of that in the respective ticket.

@johnstonphilip
5 years ago

Yes! Plugin updates with root ownership failing without deleting any files

#15 @johnstonphilip
5 years ago

@dd32 This is beautiful! I just ran a test where I leave the permissions as root and ran an update on both a custom repo plugin (one of my own) and a WP Repo plugin (WooCommerce) - and both gracefully failed. That is truly a thing of beauty right there :)

(see screenshot above)

Following that, I reset the permissions back to www-data and the updates went through perfectly :)

Last edited 5 years ago by johnstonphilip (previous) (diff)

@dd32
5 years ago

#16 @dd32
5 years ago

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

30921.2.diff covers attempting to alter the file permissions when it hits an unwritable file.

For deletion, changing the permissions isn't entirely required, if we can change the permissions we can delete the file (even if we can't modify it). Changing permissions to enable writing however is a good way to test it's deletable without actually performing the deletion.

The error message now copies the one from the core update:

The update cannot be installed because we will be unable to copy some files. This is usually due to inconsistent file permissions.

If anyone else would like to give it further testing, that is the likely commit candidate pending no issues.

#17 @dd32
5 years ago

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

In 32854:

When updating plugins/themes verify that the files to be deleted can be modified before starting the deletion process.
This will avoid partially deleting an item during update which has inconsistent permissions.
This change only affects those using the direct & ssh transports as FTP's is_writable() currently always returns true.
Fixes #30921

#18 @Otto42
5 years ago

Post 4.3 release:

We're starting to get reports from assorted SSH users that plugin updates cannot be performed at all because of this. Examination of the permissions shows no reason for this. Suspect that the SSH checks here are bugged in some way.

https://wordpress.org/support/topic/after-43-update-im-unable-to-update-plugins-or-themes

#19 @dd32
5 years ago

We'll track the SSH2-related failures in #33480

Note: See TracTickets for help on using tickets.