[wp-trac] [WordPress Trac] #56713: Check ACL permission before upgrading

[WordPress Trac]

#56713: Check ACL permission before upgrading
--------------------------------------------+------------------------------
 Reporter:  Cartman34                       |       Owner:  (none)
     Type:  defect (bug)                    |      Status:  new
 Priority:  normal                          |   Milestone:  Awaiting Review
Component:  Upgrade/Install                 |     Version:
 Severity:  normal                          |  Resolution:
 Keywords:  reporter-feedback dev-feedback  |     Focuses:
--------------------------------------------+------------------------------
Changes (by costdev):

 * keywords:  reporter-feedback => reporter-feedback dev-feedback


Comment:

 == Notes
 - [https://github.com/wordpress/wordpress-
 develop/blob/49da4afcf03f6eea5aba20b42abc0130fc9e2749/src/wp-
 admin/includes/class-core-upgrader.php#L162 This is the chmod() call] that
 leads to the first warning. There are others later in the trace.
 - In `WP_Filesystem_Direct::chmod()`, [https://github.com/wordpress
 /wordpress-develop/blob/49da4afcf03f6eea5aba20b42abc0130fc9e2749/src/wp-
 admin/includes/class-wp-filesystem-direct.php#L173 this is the line] that
 actually triggers the first warning.
 - `Site Health > Info > Filesystem Permissions` shows all directories as
 writable.
 - Changing file ownership via `chown -R www-data:www-data <directory>`
 does resolve the issue, but defeats the purpose of adding `u:www-data:rwx`
 to the (F)ACL.
 - `chmod()` can only be used by the file owner, or `root`.

 -----

 == When Testing

 For my tests, I used the following commands:

 {{{
 # Set the owner to root.
 sudo chown root:root <directory>

 # Give www-data permissions via (F)ACL.
 setfacl -Rm u:www-data:rwx <directory>
 }}}
 When trying to update through `Dashboard > Updates`, `Plugins > Installed
 Plugins > Update now`, or `Themes > Update now`, the "Connection
 Information" dialog to enter FTP credentials may be displayed.

 This is caused by a mismatch in file ownership vs temp file ownership in
 [https://github.com/wordpress/wordpress-
 develop/blob/49da4afcf03f6eea5aba20b42abc0130fc9e2749/src/wp-
 admin/includes/file.php#L2091 this condition].

 `define( 'FS_METHOD', 'direct' );` in `wp-config.php` resolves this.

 -----

 == Findings

 This seems to be an issue with calling `$wp_filesystem->chmod()` when the
 file is not owned by `www-data`. The files are writable, but `chmod()` can
 only be used by the actual file owner.

 Unfortunately, I don't believe we can rely on `fileowner()` or
 `$wp_filesystem->owner()` checks, as `www-data` is a default username and
 will be different based on environment or customizations.

 Therefore, wrapping each of these calls to `$wp_filesystem->chmod()` with
 an `! $wp_filesystem->is_writable()` check should make sure that `chmod()`
 is only run when the path is not writable. This is done in most other
 places in Core, but not all.

 Wrapping various `chmod()` calls with `if ( $wp_filesystem->is_writable()
 ) {}` and defining `FS_METHOD` as `direct` seems to update just fine for
 me.

 The `chmod()` calls I found to be relevant to this issue are:
 - [https://github.com/wordpress/wordpress-
 develop/blob/49da4afcf03f6eea5aba20b42abc0130fc9e2749/src/wp-
 admin/includes/class-core-upgrader.php#L162 wp-admin/includes/class-core-
 upgrader.php].
 - [https://github.com/wordpress/wordpress-
 develop/blob/49da4afcf03f6eea5aba20b42abc0130fc9e2749/src/wp-
 admin/includes/class-wp-filesystem-direct.php#L312 wp-admin/includes
 /class-wp-filesystem-direct.php] - Problem 1.
 - [https://github.com/wordpress/wordpress-
 develop/blob/49da4afcf03f6eea5aba20b42abc0130fc9e2749/src/wp-
 admin/includes/update-core.php#L1213 wp-admin/includes/update-core.php] -
 Problem 2.

 **Problem 1**
 This change may (read: ''will'') have unintended consequences.

 **Problem 2**
 This file is loaded from ''the files of the WordPress version being
 installed'', not the files of the current WordPress version.

 This means any patch will not be fully functional unless:

 1. We backport the `is_writable()` checks to older branches (not
 desirable), OR
 2. Users with such a setup continue to manually install until upgrading to
 the version ''after'' the one that patches this issue (a minor hassle, but
 not asking anything extra of users). That is, if WordPress 6.2 patches
 this issue, the patch won't be fully functional until WordPress 6.3 is
 released.

 -----

 == Conclusion

 This may be resolvable in Core, but needs feedback and discussion on the
 above before it can move forward.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/56713#comment:5>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform