Wrong Inconsistent Permissions Check in Filesystems

[SirLouen]

This line is checking with only two parameters: $source and $destination:

​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/update-core.php#L1083

But if it happens to reach this function (copy):

Filesystem Direct: ​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/class-wp-filesystem-direct.php#L305
Filesystem FTPEXT: ​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/class-wp-filesystem-ftpext.php#L346
FTP Socket: ​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/class-wp-filesystem-ftpsockets.php#L357

It will always return false, hence an error.

Simple solution, added in the following patch (sending true to the function)

Not much to comment here that I have not commented in the ticket OP

Trac ticket: https://core.trac.wordpress.org/ticket/63187#ticket

[SirLouen]

@audrasjb I'm going to ping you because you are a maintainer of this Upgrade component and this is a pretty obvious issue (conditional always returning false)

It's been a blocker, under certain conditions with ftpext filesystem I have detected that code will reach this point and triggered halting the core update with such error.

Maybe it can get into 6.8 given how simple it is and how much of a blocker it can be if this conditions are triggered?

[audrasjb]

Hello and thank you for the bug report and the pull request,

Looks like this was introduced ages ago in [22227].
Therefore, I think this is definitely not something we can realistically fix during WP 6.8 RC cycle. I'm moving it to 6.9 with the early workflow keyword :)

[audrasjb]

By the way, you're right when saying that the patch is super simple. I just feel concerned with introducing a change unrelated to 6.8 this late in the release cycle. I'll reconsider this during the next bug scrub so hopefully we can have more thoughts on this bugfix.

[SirLouen]

Replying to audrasjb:

By the way, you're right when saying that the patch is super simple. I just feel concerned with introducing a change unrelated to 6.8 this late in the release cycle. I'll reconsider this during the next bug scrub so hopefully we can have more thoughts on this bugfix.

A little more info about this:

It's very rare that it's triggered because generally version-current.php should be deleted during any upgrade here:
​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/update-core.php#L1103-L1104

But if it happens not to be deleted for whatever issue during the update, future updates will permanently fail, returning a weird error that is impossible to debug for the average user (Inconsistent permissions for a file that doesn't even correlate with the thing that is actually causing the error)

On the other hand, overwriting that file is innocuous because it's part of the process. In fact, the error is provoked because technically the copy (and potentially an overwrite) function are failing (always failing because of the perma-false).

TL;TR: A very edge case that could be potentially frustrating.

[siliconforks]

Replying to SirLouen:

But if it happens not to be deleted for whatever issue during the update, future updates will permanently fail

This should not cause future updates to fail. Before update_core() is called, the unpack_package() method is called:

​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/class-core-upgrader.php#L156
​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/class-core-upgrader.php#L178

The unpack_package() method is supposed to delete everything in the upgrade directory:

​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/class-wp-upgrader.php#L367

On the other hand, overwriting that file is innocuous

I don't think it's necessarily innocuous. That file is not supposed to be there. If that file is appearing there for some reason, I think it would be best to find the root cause of why that is happening instead of just overwriting it.

[SirLouen]

Replying to siliconforks:

The unpack_package() method is supposed to delete everything in the upgrade directory:

​https://github.com/WordPress/wordpress-develop/blob/baab2b8f2273319817edede1b3360f212cf42241/src/wp-admin/includes/class-wp-upgrader.php#L367

Great point, this is accurately why it hasn't historically caused troubles.

But if you read the error text:

The update cannot be installed because some files could not be copied. This is usually due to inconsistent file permissions

It's completely misdirecting. In fact, files can/could perfectly be copied. The exact problem is that there is an existence of this file version-current.php that was not deleted ​by this section

And, paradoxically, could not be a permissions issue (this is what I'm exactly researching now inside the ftpext Filesystem class).

Maybe it's not that urgent after all given the fact, as you say, that there is a good preventive operation that should save all users from this error.

But definitely the error is wrong, and the check is even more wrong.

PS: Again, it's 100% innocuous, if you are having a delete issue, you are going to receive a couple other hundred errors. But this can become a stone in the way of debugging this, after breaking my head for why I could not copy the file, to discover that the file was perfectly copied but not deleted.

Maybe the message should also be edited in case of real copy fail:

<?php
return new WP_Error(
        'copy_failed_for_version_file',
        __( 'The update cannot be installed because this file cannot be overwritten' ),
        $versions_file
);