Make WordPress Core

Opened 4 years ago

Closed 20 months ago

Last modified 17 months ago

#51170 closed defect (bug) (duplicate)

FTP automatic updates are not RFC 959 compliant for NLST command

Reported by: giox069's profile giox069 Owned by: audrasjb's profile audrasjb
Milestone: Priority: normal
Severity: normal Version: 3.7
Component: Filesystem API Keywords: has-patch
Focuses: Cc:

Description

Wordpress uses FTP NLST command to check if a file exists:
https://developer.wordpress.org/reference/classes/wp_filesystem_ftpsockets/exists/

But according RFC 959, NLST is not intended to be applied to a file.

Many ftp servers are permissive, and allowed to execute NLST on files. But a recent update to pure-ftpd removed support for this wordpress non-rfc compliant behaviour.
https://github.com/jedisct1/pure-ftpd/commit/dc71ecfc39a6258d9e49b9918b600a9d46365358

Wordpress should adhere to RFC 959, and check FTP file existence in a different way than NLST command, for example using RFC 3659 commands, and then falling back to NLST if RFC 3659 commands are not available.

Wordpress should also warn the user and stop processing when NLST fails.

See my post here:
https://wordpress.org/support/topic/problems-installing-plugins-and-upgrading-with-newer-pure-ftpd/

Attachments (3)

51170.diff (1.6 KB) - added by mkox 2 years ago.
exists improved, will fallback to ftp_size and Fixes #28013
test-ftp.zip (2.4 KB) - added by costdev 2 years ago.
51170.2.diff (896 bytes) - added by SergeyBiryukov 21 months ago.

Download all attachments as: .zip

Change History (69)

#1 @SergeyBiryukov
4 years ago

  • Component changed from General to Filesystem API

#2 @desrosj
4 years ago

  • Version changed from trunk to 3.7

Looks like nlist() was added in [25274] as part of #14049.

There were attempts to change exists() up in in #28013 as part of WordPress 4.4, but it was reverted in 4.4.1 because of issues on various servers (see #34976 and #35026).

@mkox
2 years ago

exists improved, will fallback to ftp_size and Fixes #28013

#3 @mkox
2 years ago

  • Keywords reporter-feedback dev-feedback has-patch added

After digging into the exists topic and different rfcs I think this patch is a good solution.

It improves behavior by first checking for dirs.

Then it uses the approved working method via ftp_nlist and adds flags if someone is looking for a hidden file. #28013

If ftp_nlist will fail, it will now try to use the ftp_size method for the target file - so @giox069 with pure-ftp issues with pure ftp should be solved as well.

#4 @SergeyBiryukov
2 years ago

#55307 was marked as a duplicate.

#5 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.0

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


2 years ago

#7 @costdev
2 years ago

  • Keywords needs-testing added

Per the discussion in the bug scrub, I'm adding needs-testing.

Are there any testing instructions that can be provided to testers?

#8 @afragen
2 years ago

If the fallback is to check ftp_size, is there some reason to even try ftp_nlist?

Since the function only checks a single file or directory it seems less efficient to try and find all the files in a directory and then search for the file name.

Last edited 2 years ago by afragen (previous) (diff)

#9 @afragen
2 years ago

Related #53318

This ticket was mentioned in PR #2460 on WordPress/wordpress-develop by afragen.


2 years ago
#10

WP FTP file checking is currently base upon ftp_nlist. This function can be problematic at scale with a directory containing a large number of files. There also exists an issue using it with ftpsockets.

This PR rewrites the FTP exists() functions to utilize file size as a more efficient and stable check.

Trac ticket: https://core.trac.wordpress.org/ticket/51170

afragen commented on PR #2460:


2 years ago
#11

Thanks @costdev for all logic checking and rabbit hole digging.

#12 @afragen
2 years ago

#53318 was marked as a duplicate.

This ticket was mentioned in Slack in #hosting-community by costdev. View the logs.


2 years ago

#14 @afragen
2 years ago

  • Owner set to afragen
  • Status changed from new to assigned

#15 @afragen
2 years ago

  • Keywords reporter-feedback removed

afragen commented on PR #2460:


2 years ago
#16

Might also fix #28013

#17 @costdev
2 years ago

  • Keywords commit added; dev-feedback needs-testing removed

This ticket proposes making WP_Filesystem_FTPext::exists() and WP_Filesystem_ftpsockets::exists() compliant with RFC 959 by only using NLST on directories, rather than on both directories and files.


Using ftp_nlist() requires checking an array. As the number of files in the directory are unknown, this approach has inconsistent performance.

Using ftp_size() looks like a better option. While the PHP Manual says that it may not be available on all servers, I spoke to @rawrly in the #hosting-community channel who believes that this may be because the FTP SIZE command was introduced in 2002 and in the years that followed, this may not have been enabled by some. However, this is believed to be less and less likely as things move towards sFTP.


PR 2460 uses:

  • WP_Filesystem_FTPext::is_dir() and WP_Filesystem_FTPsockets::is_dir() to check for a directory.
  • WP_Filesystem_FTPext::size() (which uses ftp_size() internally) to check for a file.
  • WP_Filesystem_ftpsockets::size() (which uses ftp_base::filesize internally) to check for a file.

This approach seems sound to me, as the respective ::is_dir() and ::size() should be returning accurate results.


I've tested PR 2460 with FS_METHOD set to ftpext and ftpsockets.

In both cases, these were the results when using the respective ::exists() method:

Test Result
A directory that exists true
An empty hidden file true
A non-empty hidden file true
An empty file true
A non-empty file true
A directory that does not exist false
A file that does not exist false

In addition, the following Dashboard actions continue to work as expected for both ftpext and ftpsockets:

Plugins

  • Install plugin from dot org
  • Install plugin from .zip
  • Delete plugin via Dashboard

Themes

  • Install theme from dot org
  • Install theme from .zip
  • Delete theme via Dashboard

Media

  • Upload image via Media Library
  • Delete image via Media Library

Nominating PR 2460 as a commit candidate.

Last edited 2 years ago by costdev (previous) (diff)

#18 @pbiron
2 years ago

I've tested the PR with both ftpext and ftpsockets FS_METHOD and I get results consistent with @costdev (although I only tested in the context of the uses of ::exists() that are exercised by WP_Upgrader).

My only concern is the comment from @rawrly about SIZE not being implemented by some FTP servers. For reference, the SIZE command was introduced in RFC 3659 - Extensions to FTP.

#19 @costdev
2 years ago

As a safety, we could follow this flow:

  1. Check if it's a directory. If true, it exists. Return true.
  2. Check for the availability of the SIZE command.
    • As ftp_size() may still exist and be callable even when the SIZE command is not available, function_exists() or is_callable() wouldn't be effective here.
    • So how can we verify the SIZE command is available? What file do we know exists? Oh yeah, this one (__FILE__)
  3. If ftp_size() returns -1 for __FILE__, we can fall back to ftp_nlist().

Like this (untested):

// Use SIZE, if available.
if ( -1 !== $this->size( __FILE__ ) ) {
   return -1 !== $this->size( $file );
}

// Run NLST on the directory.
$dir   = dirname( $file );

// Run with '-a' flag to include hidden files.
$files = ftp_nlist( $this->link, '-a ' . dir );

return $files && in_array( $file, $files, true );

Thoughts on the approach?

Last edited 2 years ago by costdev (previous) (diff)

#20 @pbiron
2 years ago

I just found a StackOverflow reply where someone who claims to be the author of particular FTP server software mentions that his FTP server specifically returns an error if the SIZE command is issued when TYPE is not BINARY :-(

He quotes from the section about the SIZE command in the RFC:

4.2. Error Responses
Where the command is correctly parsed but the size is not available,
perhaps because the pathname identifies no existing entity or because
the entity named cannot be transferred in the current MODE and TYPE
(or at all), then a 550 reply should be sent.
....
The presence of the 550 error response to a SIZE
command MUST NOT be taken by the client as an indication that the
file cannot be transferred in the current MODE and TYPE. A server
may generate this error for other reasons -- for instance if the
processing overhead is considered too great.

note: the server responding with anything other than a 213 response code will result in ftp_size() returning -1...see the PHP source for ftp_size().

So, maybe using ftp_size() to check for file existence is not a viable approach? Of course, I'm not saying that we shouldn't use it...just try to make sure we don't introduce other problems with this "fix".

#21 @afragen
2 years ago

PR updated to use ftp_rawlist() after is_dir() check in FTPext. This avoids any potential incorrectly configured FTP servers. ftp_rawlist() uses the LIST command which should always be present.

Testing is all still passing.

Last edited 2 years ago by afragen (previous) (diff)

#22 @audrasjb
2 years ago

  • Keywords commit removed

commit removed, please feel free to re-add the keyword when the PR is ok :)

#23 @costdev
2 years ago

PR 2460 has now been tested using vsftpd, Filezilla server and PureFTP.

In all three cases, the PR returns the expected results:

Test Result
A directory that exists true
An empty hidden file true
A non-empty hidden file true
An empty file true
A non-empty file true
A directory that does not exist false
A file that does not exist false

If passed a directory path, the PR uses the relative ::is_dir() method to check for an existing directory.

  • If this passes, it returns true for an existing directory.
  • If it fails, it uses ftp_rawlist() to check for a file.

When using ftp_rawlist() on a path that doesn't exist, some FTP servers will return false, others will return an empty array. The PR uses a falsy check to cover both of these instances.


ftp_rawlist() uses the FTP "LIST" command. Per RFC 959:

This command causes a list to be sent from the server to the passive DTP. If the pathname specifies a directory or other group of files, the server should transfer a list of files in the specified directory. If the pathname specifies a file then the server should send current information on the file.

(my emphasis)

ftp_rawlist() is called without passing the -a flag, used to include hidden files, which had caused issues in attempts to patch this issue in previous tickets. In testing, it became apparent that Filezilla Server doesn't support adding the flag, and already shows hidden files.


After successful testing using three different pieces of FTP server software, on both the WP_Filesystem_FTPext and WP_Filesystem_ftpsockets filesystem methods, this looks ready for commit to me.

@costdev
2 years ago

#24 @costdev
2 years ago

For those who want to test the patch, you can download, install and activate the attached test-ftp plugin.

Instructions

It's recommended to set up a new user on your system for FTP.
Use a guide online that shows how to do this for your system.

Make sure you have an FTP server running.

For vsftpd:

sudo apt update -y
sudo apt install vsftpd -y
sudo service vsftpd start

For PureFTP:

sudo apt update -y
sudo apt install pure-ftpd -y
sudo service pure-ftpd start

If testing multiple FTP servers, make sure to run this between tests:

sudo service <name_of_unwanted_FTP_server> stop
sudo service <name_of_wanted_FTP_server> restart

Use this to verify which server is running ( "+" means running ):

sudo service --status-all

Add the following to wp-config.php:

define( 'WP_DEBUG', true );
define( 'WP_DEBUG_LOG', true );
define( 'FS_METHOD', 'ftpext' ); // Or 'ftpsockets'.
define( 'FTP_BASE', '/var/www/html/wordpress-develop/src/' ); // Or the root of your WordPress installation.
define( 'FTP_USER', 'FTP_USER' ); // Change to your ftp user's username.
define( 'FTP_PASS', 'password' ); // Change to your ftp user's password.
define( 'FTP_HOST', 'localhost' );
define( 'FTP_SSL', false );

The plugin will output the results to wp-content/debug.log.

Please report your results.

#25 @costdev
2 years ago

  • Keywords commit added
  • Milestone changed from 6.0 to 6.1

While there has been testing and the current PR looks good, this still needs more testing.

As it's 6.0 RC1 tomorrow, let's move this ticket to 6.1 and mark as early to get it the attention it needs.

#26 @afragen
23 months ago

  • Keywords early added

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


23 months ago

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


23 months ago

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


23 months ago

#30 @chaion07
23 months ago

Thanks @giox069 for reporting this. We recently reviewed this ticket during a bug-scrub session. Requesting attention from the Core Committers for this ticket. Cheers!

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


23 months ago

#32 @audrasjb
23 months ago

  • Keywords commit removed

Removing the commit workflow keyword since there's still discussions in the proposed PR

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


22 months ago

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


22 months ago

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


22 months ago

#36 @audrasjb
22 months ago

  • Keywords commit added

As per today's bugscrub, the above PR looks ready to ship.
Just wanted to ping @peterwilsoncc to review the last comments in the PR before commit :)

#37 @peterwilsoncc
22 months ago

  • Keywords commit removed

Given teh problems in the past, I'd prefer some more feedback from the #hosting-community channel. I know they've been asked in the past but the notes on the PR only list tests against a couple of server types.

This ticket was mentioned in Slack in #hosting-community by costdev. View the logs.


22 months ago

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


22 months ago

This ticket was mentioned in Slack in #hosting-community by chaion07. View the logs.


22 months ago

This ticket was mentioned in Slack in #hosting-community by jadonn. View the logs.


22 months ago

This ticket was mentioned in Slack in #hosting-community by pbiron. View the logs.


22 months ago

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


21 months ago

This ticket was mentioned in Slack in #hosting-community by amykamala. View the logs.


21 months ago

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


21 months ago

This ticket was mentioned in Slack in #core-auto-updates by sergey. View the logs.


21 months ago

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


21 months ago

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


21 months ago

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


21 months ago

This ticket was mentioned in Slack in #hosting-community by afragen. View the logs.


21 months ago

#52 follow-up: @audrasjb
21 months ago

  • Keywords commit added

This ticket was mentioned 7 times in #hosting-community in the last few weeks, without any feedback. Let's commit it so it could be tested early in the release process.

#53 in reply to: ↑ 52 @peterwilsoncc
21 months ago

Replying to audrasjb:

This ticket was mentioned 7 times in #hosting-community in the last few weeks, without any feedback. Let's commit it so it could be tested early in the release process.

Works for me.

#54 @audrasjb
21 months ago

  • Owner changed from afragen to audrasjb
  • Status changed from assigned to accepted

Self assigning for commit.

#55 @audrasjb
21 months ago

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

In 53860:

Filesystem: Rewrite FTP/FTP Sockets exists() methods to implement a more stable check.

WordPress FTP file checking was previously based upon ftp_nlist(). This function can be problematic at scale with a directory containing a large number of files. The same issue occurred using it with ftpsockets.

This changeset rewrites the FTP exists() functions to utilize a more efficient and stable check.

Props giox069, desrosj, mkox, afragen, costdev, pbiron, peterwilsoncc.
Fixes #51170.
See #53318, #39781.

#57 @SergeyBiryukov
21 months ago

In 53862:

Docs: Improve @since notes for some WP_Filesystem_* methods:

  • WP_Filesystem_FTPext::exists()
  • WP_Filesystem_FTPext::size()
  • WP_Filesystem_ftpsockets::exists()

The ::exists() methods were previously using the FTP NLST command, which works for directories, but is not intended to be applied to a file. This only worked most of the time due to many FTP servers being permissive and allowing to execute NLST on files, which cannot be guaranteed and appears to not be the case in newer versions of Pure-FTPd (1.0.48 or later).

With a recent change in [53860], both methods were updated for compatibility with RFC 959:

  • Both methods check if the path is a directory that can be changed into (and therefore exists).
  • WP_Filesystem_FTPext uses ftp_rawlist() (FTP LIST command) to check for file existence.
  • WP_Filesystem_ftpsockets uses file size to check for file existence.

Reference: RFC 959: File Transfer Protocol (FTP)

Follow-up to [6779], [11821], [25274], [33648], [34733], [35944], [35946], [53860].

See #51170.

#58 @SergeyBiryukov
21 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Good catch on correcting the return value for WP_Filesystem_FTPext::size() in [53860]!

However, that makes it inconsistent with all the other WP_Filesystem_*::size() methods:

  • WP_Filesystem_Base::size()
  • WP_Filesystem_Direct::size()
  • WP_Filesystem_ftpsockets::size()
  • WP_Filesystem_SSH2::size()
    @return int|false Size of the file in bytes on success, false on failure.
    

This means we'd have to check both for false and for -1 anywhere the ::size() method is used, which increases complexity without a strong reason.

As the purpose of the API is to provide a consistent interface for various filesystem implementations, I believe a better solution would be to update the return value to match the documented value instead. See 51170.2.diff.

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


20 months ago

#60 @audrasjb
20 months ago

  • Keywords early commit removed

Removing early keyword as we only need some follow-up patches now.

#61 @SergeyBiryukov
20 months ago

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

In 53898:

Upgrade/Install: Make WP_Filesystem_FTPext::size() return false on failure.

While WP_Filesystem_Base::size() is documented to return false on failure, ftp_size() returns -1, and the method documentation was recently updated to reflect that.

This commit restores the previous @return tag and corrects the actual return value instead, to bring consistency with all the other WP_Filesystem_*::size() methods:

  • WP_Filesystem_Base::size()
  • WP_Filesystem_Direct::size()
  • WP_Filesystem_ftpsockets::size()
  • WP_Filesystem_SSH2::size()
    @return int|false Size of the file in bytes on success, false on failure.
    

This better matches the purpose of the API to provide a consistent interface for various filesystem implementations.

Follow-up to [6779], [30678], [45226], [53860], [53862].

Fixes #51170.

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


17 months ago

#63 @desrosj
17 months ago

In 54815:

Filesystem: Return FTP/FTP Sockets exists() methods to a previous state.

This partially reverts [53860] and [53862], which refactored the exists() method to rely on ftp_rawlist() instead of ftp_nlist().

[53860] makes a similar attempt to the ones made in [33648] and [34733] (which were also reverted in [35944]). Being compliant with the specifications while continuing to work without issue for all FTP servers continues seem impossible. These little ghosts are the ones we’re scared of the most.

Props jsh4, afragen, costdev, pkolenbr, SergeyBiryukov, dd32, peterwilsoncc, gamecreature, desrosj.
Fixes #56966.
See #51170, #28013.

#64 @desrosj
17 months ago

In 54816:

Filesystem: Return FTP/FTP Sockets exists() methods to a previous state.

This partially reverts [53860] and [53862], which refactored the exists() method to rely on ftp_rawlist() instead of ftp_nlist().

[53860] makes a similar attempt to the ones made in [33648] and [34733] (which were also reverted in [35944]). Being compliant with the specifications while continuing to work without issue for all FTP servers continues seem impossible. These little ghosts are the ones we’re scared of the most.

Props jsh4, afragen, costdev, pkolenbr, SergeyBiryukov, dd32, peterwilsoncc, gamecreature, desrosj.
Merges [54815] to the 6.1 branch.
Fixes #56966.
See #51170, #28013.

#65 @SergeyBiryukov
17 months ago

  • Resolution changed from fixed to duplicate

As [53860] was reverted, this should either be closed as maybelater or reopened and moved to Future Release.

However, per comment:15:ticket:56966, this appears to be a duplicate of #28013. Let's continue the discussion there.

Last edited 17 months ago by SergeyBiryukov (previous) (diff)

#66 @SergeyBiryukov
17 months ago

  • Milestone 6.1 deleted
Note: See TracTickets for help on using tickets.