FTP automatic updates are not RFC 959 compliant for NLST command

[giox069]

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/

[desrosj]

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]

exists improved, will fallback to ftp_size and Fixes #28013

[mkox]

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.

[SergeyBiryukov]

#55307 was marked as a duplicate.

[costdev]

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

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

[afragen]

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.

[afragen]

Related #53318

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

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

[afragen]

#53318 was marked as a duplicate.

Might also fix #28013

[costdev]

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.

[pbiron]

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.

[costdev]

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?

[pbiron]

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".

[afragen]

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.

[audrasjb]

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

[costdev]

​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]

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.

[costdev]

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.

[chaion07]

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!

[audrasjb]

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

[audrasjb]

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

[peterwilsoncc]

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.

[afragen]

Related #39781

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

[peterwilsoncc]

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.

[audrasjb]

Self assigning for commit.

[audrasjb]

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.

Committed in https://core.trac.wordpress.org/changeset/53860

[SergeyBiryukov]

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.

[SergeyBiryukov]

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​.

[audrasjb]

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

[SergeyBiryukov]

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.

[desrosj]

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.

[desrosj]

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.

[SergeyBiryukov]

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.