FS_Method with ftpext and Core Updates

[SirLouen]

This is one of the strangest bugs I've found so far.

There is a weird behaving section in the code:
​https://github.com/WordPress/wordpress-develop/blob/b943118a5b1967591fa1d15ee1ee87f93b7c0147/src/wp-admin/includes/update-core.php#L1057-L1073

When using FS_METHOD ftpext, it behaves in the following way:

• During a core update, it checks for files in the update package:

1. It checks for readme.html and it finds it
2. It checks for version.php and it doesn't find it.

But both do exist.

So it ends in this error: The update could not be unpacked

Given how generic everything is, this doesn't make sense. FTP Methods for FS_METHOD have been my nemesis for a decade.

I've pushed my current FTP setup in case anyone wants to test this issue on top of this:
​https://github.com/WordPress/wordpress-develop/pull/8589

Once set-up, for wp-config.php we need:

define( 'FS_METHOD', 'ftpext' );
define( 'FTP_USER', 'admin' );
define( 'FTP_PASS', 'password' );
define( 'FTP_HOST', 'ftp' );
define( 'FTP_SSL', false );

I'm going to add some screenshots to show the behaviour with more detail

[SirLouen]

Time for Screenshots and debugging:

1st checkpoint: ​https://i.imgur.com/wVuHBvg.png
$from = /wp-content/upgrade/wordpress-6.8-latest-sjCZm4
$root = /wordpress/
So the full URL will be /wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/readme.html

2nd checkpoint: ​https://i.imgur.com/cE7zaYy.png
As expected, $path = /wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/readme.html

So far, so good.

3rd checkpoint: ​https://i.imgur.com/XLVnrdi.png
We can see that the $list has one item, hence return true

So we can confirm that the first part is TRUE

Now lets go with the next file:

4th checkpoint: ​https://i.imgur.com/6UX1v0N.png
The $path looks decent: /wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/wp-includes/version.php

And finally, the last part:

5th checkpoint: ​https://i.imgur.com/IXZ1eWg.png

$list = array(0), hence the result is FALSE

Ok, wait, probably is a problem with permissions. Lets check the FTP:

$ docker exec wordpress-develop-ftp-1 ls -l /home/admin/wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/readme.html
-rw-r--r--    1 admin    admin         7425 Mar 25 18:13 /home/admin/wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/readme.html

readme.html has 644, pretty common

$ docker exec wordpress-develop-ftp-1 ls -ld /home/admin/wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/
drwxr-xr-x    5 admin    admin         4096 Mar 25 18:13 /home/admin/wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/

The parent directory of readme.html, has 755, everything is fine so far

$ docker exec wordpress-develop-ftp-1 ls -l /home/admin/wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/wp-includes/version.php
-rw-r--r--    1 admin    admin         1098 Mar 25 18:13 /home/admin/wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/wp-includes/version.php

644 for version.php

And finally... the parent directory of version.php:

$ docker exec wordpress-develop-ftp-1 ls -ld /home/admin/wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/wp-includes/
drwxr-xr-x   30 admin    admin        16384 Mar 25 18:13 /home/admin/wp-content/upgrade/wordpress-6.8-latest-sjCZm4/wordpress/wp-includes/

755! What kind of unexplained wizardry is this?

This patch is very tricky to test, here I'm going to provide some info and instructions.

The error comes when the FTP path is extremely long. This happens for example on core updates (with nightly versions), it creates a folder with a very long name, and then a full wordpress structure inside wp-content/upgrades

The simplest way to test this is the following:

1. Set an FTP in the server
2. Set up the FS_METHOD to ftpext (wp-config.php) with the FTP credentials
3. Install a WordPress nightly version but not the latest (go with the previous one so it will trigger an update)

You can also do these steps: https://core.trac.wordpress.org/ticket/62718#comment:20 to setup your own docker environment using the wordpress-develop env

Once you have everything set-up:

4. Go to the Admin area, then Dashboard > Updates
5. Update to latest nightly button
6. Without the patch it should throw an error: The update could not be unpacked
7. With the patch it might throw other errors unrelated to this particular report, but it will end updating.

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

[SirLouen]

I've been researching, and it seems that ftp_nlist can be faulty when working with numerous files or longer paths
​https://stackoverflow.com/questions/66416395/php-ftp-nlist-returning-false-when-retrieving-large-directory, specially if not using directory navigation to short distances with ftp_chdir

The better alternative is to switch to ftp_rawlist which can handle paths more conveniently.

Luckily, it seems that ftp_nlist is only used there.
​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/class-wp-filesystem-ftpext.php#L427-L445

I've provided a patch for this. For performance purposes, we could still use ftp_nlist under 2 directories, but thereafter, we should be using ftp_rawlist for accuracy. Maybe even 3 directories.

I'm not sure how we could introduce some unit-tests apart from hard-coding some mocks for the PHP ftp functions.

[SirLouen]

Just for more context, I want to provide just one example of what's going on (and this problem is scaling big): ​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/update-core.php#L1296-L1297

In a ftpext, this is the delete operation: ​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/class-wp-filesystem-ftpext.php#L399-L400

And in direct: ​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/class-wp-filesystem-direct.php#L373-L391

.maintenance is a file. But the deletion of .maintenance is being handled as a directory (in fact, since it's not a directory and the function is not called as recursive, it will always be jumping to a recursive delete mode because this line is not specifying that it's a file with the 'f' type)

Yes, everything is working because ultimately this recursive function will end deleting most files. But as soon as we play with a more sensible filesystem (like in the sample I'm analyzing, a very specific FTP with ftpext), everything falls apart. And this is affecting a nice amount of deletes, file creations as mkdirs, etc…

And this is why this report became a blocker to me (truly irrelevant blocker by the way because the warning was still there and the reason of why I started debugging all this)

So probably for the average user, this is completely irrelevant. But, as I say, this must be fixed taking all this context in mind. An innocuous patch that brings more good than evil.

[SirLouen]

I'm having serious issues with ftpext and I'm trying to debug everything from scratch

For now, I'm 100% confident that ftp_nlist is problematic, and I've been able to reproduce this consistently with this test file:

​https://gist.github.com/SirLouen/ac3de9972a6b4c6d904f014921ab1a44

The algorithm I've proposed can be improved, I've going to submit a new commit with a better version. Basically, Ì've found that nlist should be completely wiped, and not even be used for shorter paths.

[SirLouen]

Finally, I've been able to do a full core update with the environment proposed.

I've located further issues in this function, I've repaired:

1. It was not accounting for .invisible files, so I needed to add the -a to the command
2. There is an ultra-tricky scenario, that might only happen in special file systems like FTP: when you point straight to root /, this is impossible that happens in other scenarios like direct because no one is going to mount their WP in their root directory for massive security concerns.

With this, I think that the function is completely ready for production, pinging Filesystem maintainer @costdev

I'm still wondering how people have been able to update core with ftpext, maybe some ftp servers are very permissive with functions or something?