WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#8794 closed enhancement (wontfix)

Allow Automatical upgrade to use direct method when files are group writable

Reported by: vilhelmk Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.8
Component: Upgrade/Install Keywords: has-patch needs-testing
Focuses: Cc:

Description

Even though the files wordpress creates have g+w permissions and the web-server runs under the correct gid wordpress won't do automatical upgrade using direct file-system method.

Attachments (1)

wordpress-automatical-upgrade-does-not-choose-direct-method.patch (453 bytes) - added by vilhelmk 7 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 follow-up: @DD327 years ago

  • Component changed from General to Upgrade
  • Keywords needs-testing added
  • Owner anonymous deleted
  • Summary changed from Automatical upgrade does not choose direct method to Allow Automatical upgrade to use direct method when files are group writable
  • Type changed from defect (bug) to enhancement
  • Version set to 2.8

Would it be required for WordPress to change the owner of the files to the users username as well? (ie. in this case, the files would get written as www-data:www-data assuming the web servers username/groupname is that, instead of dd32:www-data)

Theres been a few tickets where this idea has been closed off due to the above concern, but since you've submitted a patch, worth looking at.

I guess: fileperms($temp_file) & 0x0010 is checking if its group-writable? (Or have i misunderstood g+w as group writable instead of globally-writable? if the latter, i'd seriously highly not suggest it)

comment:2 in reply to: ↑ 1 @vilhelmk7 years ago

Replying to DD32:

Would it be required for WordPress to change the owner of the files to the users username as well? (ie. in this case, the files would get written as www-data:www-data assuming the web servers username/groupname is that, instead of dd32:www-data)

In my use-case it would *not* be required for wordpress to change any ownership of the files (or more specifically group ownership).

To clarify what the patch fixes, here's the use-case where I experienced this bug and therefore fixed it:

  • All directories in the wordpress installation has the g+ws flags, where the "s" means "sticky", which again means that the permissions (group writable) will follow on new files, including who owns the files.
  • The web-server runs each virtualhost (or wordpress installation) under different uid/gid's specified by the apache2-mpm-itk-module in apache, making it easier for multiple unix users to have access to all files (including the ones wordpress creates) on the same virtualhost/installation, by putting them all in the same unix group and setting all files to g+ws.

I guess that would make it easier to test :-).


Theres been a few tickets where this idea has been closed off due to the above concern, but since you've submitted a patch, worth looking at.

I guess: fileperms($temp_file) & 0x0010 is checking if its group-writable? (Or have i misunderstood g+w as group writable instead of globally-writable? if the latter, i'd seriously highly not suggest it)

Yes, 0x0010 is group-writable (also see php.net/fileperms).

comment:3 @DD327 years ago

mmm Ok, That makes sense.

Now, Under your setup it works correctly due to the sticky flag (ahhh goes my mind, THAT's what you've been forgetting to set), but that doesnt affect the owner of the files? as in, the uid will still get set to the user apache is running under, which in your case, will work fine, however, In my case, It will not (Group writable files, however apache and the owner of the files are different), Correct?

So what i'm saying, Is whilst the group settings may be correct, the file owner may change, and in that case, under some setups (which is the exact reason for allowing the use of FTP, to avoid this), would cause the user to no longer be able to manage their files correctly via FTP/other access means?

comment:4 @vilhelmk7 years ago

I had to do some more testing and reading up on the subject since my mind isn't wrapped around this either (and it is a while since I looked into this, and fixed it manually - but the changes was lost in version 2.7, obviously).

So, to correct myself (sorry about any misinformation in my previous comment):

The g+s only ensures that the GID of the new files is inherited by the parent catalogue, meaning that the g+w does *not* get inherited. To solve this have umask 002 in apache's envvars (/etc/apache2/envvars on debian/ubuntu), and by doing this all files created by apache gets the g+w flag (+s won't solve this alone).

So, if the web-server is using umask 002 and does not setgid to the correct group the user won't have access to the files by ftp/cli.

I assume that having umask 002 in apache's envvars is a pretty rare setting, and that people probably knows what they are doing when using it. The default is usually umask 022 (meaning that files only get u+w).

So yes, if the user of the files changes and the user isn't in the correct group it will break, if the web-server is using umask 002.

Hope this clarifies more.

comment:5 @vilhelmk7 years ago

  • Cc vilhelm@… added

comment:6 @Otto427 years ago

-100. This patch can result in a situation where a user does an update and then no longer owns his files, and thus is unable to delete or modify them manually, via FTP.

Remember, you are not the only use case. The upgrade must take all possible configurations into account, and most of these configurations are not using "group" access for this sort of thing.

comment:7 @westi7 years ago

I don't like this patch either.

I think we should just let a plugin filter the method and then this corner-case can be handled by a simple plugin which forced direct mode?

comment:8 @DD327 years ago

  • Milestone 2.8 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Forcing direct mode would not be enough to make this happen either, The files would have to be chown'd to the users account name too i think.. well.. for compatibility with other systems at least, eg quota management.. and possibly FTP too.

I'm going with the "use a plugin" group.

Theres a filter filesystem_method available, Return 'direct' on that filter to force the Direct class to be used.

Note: See TracTickets for help on using tickets.