[wp-trac] [WordPress Trac] #30245: Background updates should work with Group/World Writable

[WordPress Trac]

#30245: Background updates should work with Group/World Writable
-----------------------------+--------------------
 Reporter:  dd32             |       Owner:
     Type:  enhancement      |      Status:  new
 Priority:  normal           |   Milestone:  4.1
Component:  Upgrade/Install  |     Version:  trunk
 Severity:  normal           |  Resolution:
 Keywords:  has-patch        |     Focuses:
-----------------------------+--------------------
Changes (by dd32):

 * keywords:   => has-patch


Comment:

 Attached are working attempts at this for Background Core updates,
 translations not implemented.

 Enabling this conditionally requires significant parameter passing, as the
 filesystem checks are called in many different locations.

 [attachment:30245.diff 30245.diff] initially went with subclassing
 `WP_Filesystem_Direct` to `WP_Filesystem_Direct_GroupWritable` as a way of
 flagging that `FS_CHMOD_DIR` / `FS_CHMOD_FILE` should be initialized with
 a different default.
 However, the problem here, is that we might fall through to the
 WorldWritable case and set world-writable, when in actual fact, it should
 only be owner/group writable.
 Consider a scenario where the file owner check functions are unavailable
 or broken, but we were still able to write the files, this would result in
 it thinking it was world-writable.. and chmoding things to `0666` when it
 should only be `0644`.

 [attachment:30245.2.diff 30245.2.diff] works around this by not setting a
 higher default value for `FS_CHMOD_FILE`/`FS_CHMOD_DIR` and relying on
 `fileperms()` returning `0664` or `0666` as appropriate for the
 configuration.
 The worst case scenario here, is that WordPress might do a background
 update with files owned as `0666` and alter the permissions of some files
 to `0644`, I think this is a far better scenario than the alternative of
 accidentally making the files world writable, and the patch is far smaller
 in addition.

 One other potential problem we've got is that we use PHP's `copy()` method
 to copy a file (from the extracted zip) owned by the PHP user over the top
 of a file owned by the user. PHP achieves this by simply opening both
 files, [https://github.com/php/php-
 src/blob/958c6d1c5deb4cc16ee54a9fbd9e93958e086a79/ext/standard/file.c#L1687-L1777
 and then performing a stream copy] which means file ownership is kept
 '''if the file exists''', if the destination file doesn't exist, it's
 created as being owned by PHP instead of being owned by the user.

 [attachment:30245.3.diff 30245.3.diff] attempts to work around this by
 setting a flag on `WP_Upgrader` when relaxed file ownership is in use, and
 triggers pre-flight `file_exists()` checks in `Core_Upgrader`, this
 appears to work as expected in the limited testing I've done.

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