Make WordPress Core

Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#29610 closed defect (bug) (maybelater)

FTPext Filesystem class can't detect isWritable, isReadable correctly

Reported by: programmin's profile programmin Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.5
Component: Filesystem API Keywords:
Focuses: Cc:

Description

The wp-admin/includes/class-wp-filesystem-ftpext.php doesn't actually detect if a file is readable or writable:

	public function is_readable($file) {
		return true;
	}

	public function is_writable($file) {
		return true;
	}

even though it's parsing permissions ('perms') in another part of that file.

The lack of this feature is also likely the main reason it's not actually checking file permissions are such that a wp-update can be copied? In wp-admin/includes/update-core.php:

// If we're using the direct method, we can predict write failures that are due to permissions.
	if ( $check_is_writable && 'direct' === $wp_filesystem->method ) {
        .... checks permissions won't make upgrade choke ...

Change History (14)

#1 @SergeyBiryukov
10 years ago

  • Component changed from Upgrade/Install to Filesystem API
  • Version changed from 4.0 to 2.5

Introduced in [6779].

#2 follow-up: @programmin
9 years ago

As a side note, it looks like it never checks permissions when upgrading WP plugin. You can reliably test this by doing sudo chmod -R 544 ./nextgen-gallery/products/ for example.

If there is a permissions problem (or, out-of-memory or any other fatal error when upgrading) the plugin will be incomplete - for example:

Installing the latest version…

Removing the old version of the plugin…

Could not remove the old plugin.

Plugin update failed.

Worse yet, there is now no way to re-install the plugin short of manual FTP, even if permissions are corrected or the source of the fatal is resolved:

Unpacking the package…

Installing the plugin…

Destination folder already exists. (directory)

Plugin install failed.

#3 in reply to: ↑ 2 ; follow-up: @dd32
9 years ago

Replying to programmin:

it looks like it never checks permissions when upgrading WP plugin.

Correct, we never check writability for FTP, and the only write checks are done to determine if it should use FTP or direct file operations.

Realistically, None of the functions mentioned here should've been implemented for either the FTP or Direct filesystem wrappers.

#4 in reply to: ↑ 3 @programmin
9 years ago

Replying to dd32:

Replying to programmin:

it looks like it never checks permissions when upgrading WP plugin.

Correct, we never check writability for FTP, and the only write checks are done to determine if it should use FTP or direct file operations.

Realistically, None of the functions mentioned here should've been implemented for either the FTP or Direct filesystem wrappers.

Why not, can you please clarify? Btw I was using direct on a testing system that had not had FTP mode set up, when I got the plugin to break as described above. If even one of the folders of a plugin is not writable, it deletes the rest and stays that way. (same thing happens if it fails to copy fully for whatever reason).

I'm wondering why it does not check these permissions, and also does not allow removal of broken plugin-directory when installing the plugin again? In Plugin_Upgrader class:

                $this->run( array(
			'package' => $package,
			'destination' => WP_PLUGIN_DIR,
			'clear_destination' => false, // Do not overwrite files.
			'clear_working' => true,
			'hook_extra' => array(
				'type' => 'plugin',
				'action' => 'install',
			)
		) );

Why not overwrite the files?

#5 follow-up: @dd32
9 years ago

Why not, can you please clarify?

Quite simply because we assume that FTP access has the ability to modify the files. Short of attempting to write to each file, there isn't an easy way to cover every system.
If a file is chmod to 400, we'll alter it to 644 and attempt again, if it's 777 and a linux/windows ACL prevents write, we can't "see" that ACL or write to the file.

So for the vast majority of sites, we can simply assume that FTP has write access to the system.
If a system has mixed permissions on files or various ACL's on specific locations, then it's a system we don't fully support, WordPress does it's best, but it may fail, this is usually only seen when someone leaves some files owned by root.

also does not allow removal of broken plugin-directory when installing the plugin again

This is for technical & legacy reasons, preventing overwriting of a different plugin, and a few other obscure reasons I can't quite remember. This scenario is basically one that should never be hit on any server where everything is working correctly, call it a failsafe, manual interaction is often required to determine what (if anything) caused it in the first place.

I'm fairly certain there's another ticket somewhere to alter that check to ignore the flag if the directory is empty.

#6 in reply to: ↑ 5 @programmin
9 years ago

Replying to dd32:

Why not, can you please clarify?

Quite simply because we assume that FTP access has the ability to modify the files. Short of attempting to write to each file, there isn't an easy way to cover every system.
If a file is chmod to 400, we'll alter it to 644 and attempt again, if it's 777 and a linux/windows ACL prevents write, we can't "see" that ACL or write to the file.

This is not done automatically unfortunately - edit a plugin's main file's version line to be an old version, and give it some wrong folder permissions, and update breaks it.

The wp update is doing file by file check, but ideally it could try chmod-ing each file to correct mode, an operation both FTP and Direct do. I think upgrading plugin should do the same if there isn't a way to have is_writable() function.

I tried a couple filters to see where I could check for existence of incomplete folder while installing, but it seems there isn't a filter for that.

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


9 years ago

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


9 years ago

#9 @ericlewis
9 years ago

programmin said

The wp update is doing file by file check, but ideally it could try chmod-ing each file to correct mode, an operation both FTP and Direct do. I think upgrading plugin should do the same if there isn't a way to have is_writable() function.

If you could make a proof-of-concept of this approach (with a clear description of how to test), that would be the way forward here.

Otherwise, this seems like a good amount of dev lift for an edge case.

#10 @programmin
9 years ago

How to test this bug-

1) Replace a plugin's files with older version from the archives
2) Mess with the permissions as I described.
3) (Attempt to) Update the plugin.

Making a recursive function to call the fs chmod, returning a failure when failing, and going through all in a certain folder, is fairly easy and would remove a common cause of breakage.

#11 @dd32
9 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

I don't personally think parsing the permissions returned by the FTP server is reliable enough, my reasoning being that it only gives a "maybe" answer - the FTP server can still block the operation, and it may still accept it based on other ACLs.

FTP servers don't offer a method of testing if something can be done, they operate on a "try and see" method.
The only way to tell if a file is writable is to attempt writing to it.

Unfortunately I'm closing this as maybelater until a time where a patch can be offered that actually achieves the goal.

#12 @programmin
9 years ago

That makes sense. So you're saying there could be a FTP server that would let you rewrite files, yet not chmod those same files? Or, that would allow you to chmod files yet not remove/write them?

I see your point but I would be curious to know which FTP servers/configurations would have that behavior - that is, how often would chmod writability check not work?

#13 @dd32
9 years ago

So you're saying there could be a FTP server that would let you rewrite files, yet not chmod those same files?

Yes, that.

But also that it may allow writing even though the chmod values returned don't indicate writability.

For example:

  • FTP user is www
  • Files permissions are 664
  • File is owned by apache:apache

Looking at that we can gather:

  • user www != apache, therefor can't write
  • Don't know our groups, therefor can't check that.
  • global has read-only permissions, therefor can't write
  • In reality: www was in the apache group and could modify the file.

Then take:

  • FTP user is www
  • Files permissions are 777
  • Server denies file modifications

We can assume from that:

  • File should be writable.
  • Upon writing, we find the server still denies.

Then another case

  • FTP user is www
  • Files permissions are 644
  • Files are owned by www:www

Then

  • FTP user === file owner, ftp file permissions say we can write
  • Upon write, FTP server or filesystem ACLs deny the action, file is unwritable.
Last edited 9 years ago by dd32 (previous) (diff)

#14 @dd32
9 years ago

Additionally to complicate it further, the chmod values for a individual file do not indicate if the file can be deleted, only if it can be written to. To delete the file you have to look at the directories permissions instead.

Note: See TracTickets for help on using tickets.