Opened 9 years ago
Closed 8 years ago
#30245 closed task (blessed) (fixed)
Background updates should work with Group/World Writable
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.1 | Priority: | normal |
Severity: | normal | Version: | 4.1 |
Component: | Upgrade/Install | Keywords: | |
Focuses: | Cc: |
Description
At present we apply a strict owner check to created files before using the 'direct' file operations (ie. not FTP) for updates, this is due to many hosts FTP access not allowing access to non-owned files.
Background Updates, by design, do not create new files (at least in-branch core updates & translations), and only alter existing WordPress files. As a result of this, we can relax the strict checks for these updates, allowing security releases to be installed automatically in more cases.
The 1.7 version-check API returns a flag on autoupdate responses new_files
which specifies if the update from the current version to that package will mean new files (ie. 3.9 -> 4.0.1 is true, yet 4.0 -> 4.0.1 is false), we can leverage that flag to enable group-writable.
Please keep this ticket on track and lets not debate Group/World Writable for plugins/themes/others here, that's been done over in #10205 & #8794
Attachments (4)
Change History (17)
#2
@
9 years ago
Can we use this for nightly builds after a beta is released too? If so, it should be in beta 2 IMO.
#4
@
9 years ago
Can we use this for nightly builds after a beta is released too?
Yes, we'll tweak the WordPress.org API to understand that 4.1-beta builds don't have any new files (when updating from another install >= 4.1-beta1)
In 30384
The changes in 30245.3.diff to pre-check that the files exist has been dropped in this commit, after discussions we decided that having a incorrectly owned file was a far better user experience than having a file missing altogether. This scenario is highly unlikely to be hit however, as only in-branch releases will trigger this check, and usually with a partial build (And partial builds usually only modify core files, which if one is missing there'll generally be fatal errors anyway, unlike say a missing readme.html or tinymce image).
#5
@
9 years ago
- Owner set to dd32
- Status changed from new to assigned
- Type changed from enhancement to task (blessed)
#7
@
8 years ago
- Resolution set to fixed
- Status changed from assigned to closed
Please open new tickets for any issues regarding this. Closing as fixed for 4.1
#9
@
8 years ago
Re-opening for stats. In order to track success and error rates we need to pass the status of what method WordPress is using, 30245.stats.diff simply uses a global, it's pretty hacky, but it does what's needed and is actually pretty clean solution compared to the alternatives.
[30860] adds this to trunk, needs merging to branch.
Can we use this for nightly builds after a beta is released too?
Yes, we'll tweak the WordPress.org API to understand that 4.1-beta builds don't have any new files (when updating from another install >= 4.1-beta1)
It looks like this never happened unfortunately, as I've only just realised, so noting it here.
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.
30245.diff initially went with subclassing
WP_Filesystem_Direct
toWP_Filesystem_Direct_GroupWritable
as a way of flagging thatFS_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 be0644
.30245.2.diff works around this by not setting a higher default value for
FS_CHMOD_FILE
/FS_CHMOD_DIR
and relying onfileperms()
returning0664
or0666
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 to0644
, 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, 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.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-flightfile_exists()
checks inCore_Upgrader
, this appears to work as expected in the limited testing I've done.