"Cannot Remove Old Plugin" Obliterates files!

[johnstonphilip]

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?

[dd32]

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.

[johnstonphilip]

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]

Image preview of issue

[dd32]

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.

[valendesigns]

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. ;)

[johnstonphilip]

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.

[johnstonphilip]

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)

[johnstonphilip]

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.

[jorbin]

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

[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?

[dd32]

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]

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.

[programmin]

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 )?

[dd32]

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]

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

[johnstonphilip]

@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 :)

[dd32]

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.

[dd32]

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

[Otto42]

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

[dd32]

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