change get_filesystem_method()'s code for direct to reflect actual purpose

[dd32]

due to tickets such as #10205 and #10423 I propose we add some documentation, or change the code a bit to reflect to others the intended purpose of the code.

Eg, something such as this could potentially work:

if ( file_owner($temp_file) !== false && file_owner(__FILE__) === file_owner($temp_file) ) {
//use direct
}

Too many people assume its a typo, and just a complex is_writable() call..

[sivel]

I'm thinking that perhaps we should also do an is_writable check as well. This may actually make the check more restrictive but possibly more accurate.

if ( file_owner($temp_file) !== false && file_owner(__FILE__) === file_owner($temp_file) && is_writable($temp_file) && is_writable(__FILE__) ) {
//use direct
}

This could help us avoid issues where the default umask doesn't give write permissions to the owner, which I have seen in cases where the file is created by the web server user on more restrictive setups.

This would also help to explain via code that we are checking more than just if the file is writable. Comments would still need to be included to describe the check more thoroughly and referring the user to the filter where they could define their own check and the constant to avoid the check all together.

[dd32]

The creation of the temporary file should fail if the folder is not writable i'd have thought..

[dd32]

But its not the folder you're talking about, its the actual file.. Shouldnt it fail if we open it with 'w' mask set? Or do we actually need to test writing something to the file as well..

[sivel]

Checking the file owner of the temporary file is not needed. I was thinking it would help avoid issues in the future but as I said really isn't needed.

[dd32]

Checking the file owner of the temporary file is not needed.

Except it is. And i'm not going to explain in this comment today the reasonings of it.

[filosofo]

Replying to dd32:

Checking the file owner of the temporary file is not needed.

Except it is. And i'm not going to explain in this comment today the reasonings of it.

Could you link to an explanation that provides the reasoning? Because I've searched for it and at least skimmed over all the tickets ​your search links to. The best I could turn up is that it "creates problems," but I haven't yet found what those problems are.

[dd32]

Could you link to an explanation that provides the reasoning?

I dont have the explanation handy, However, I will explain it:

• Under most shared hosting environments apache will be running as apache:apache (user:group)
• Under most shared hosting environments, The users files will be owned by <username> and have a group of either <username> or <apache> (To allow apache access to the files)
• Under *some* shared hosting environments, apache will be able to read/write the users files, even when the Apache Process is not running PHP as the same user that owns the files.

• If apache creates a file in a users folder (is_writable()) it'll often be owned by apache:apache, This is the reason that a lot of users will have to chmod their uploads folder to 777 or 775
• If the folder is NOT chmod'd to 777/775 AND PHP/Apache is executing the code as a user other than who owns the files, Often results in files being created in the users folder which are not owned, or modifyable by the user via FTP
  • This often results in files which are undeletable, other than via Cpanel or directly by the host
  • Also results in the files not counting towards the users quota in some cases
• Having the folders chmod'd to 777/775 allows for the user to be able to modify the files in that folder via FTP. (It also allows any other user on the box(777) or any other user in that group(775) to modify the files in that folder)
  • Yes, This is an unsecure environment for a hoster to be running, However it is often common.
    • Quite often, Apache(and PHP) can read ANY file in other users folders due to this, The setup is a bad one, but is much more common amongst el-cheapo hosts around the world than one would prefer.
  • Some european countries seem to have much less of this type of setup however, And seem to have configurations under which the direct access method would work, however is currently failing.

However:

• If apache is running as apache:apache
• The files are owned by <username>:<username>
• Apache executes PHP as the owner of the files (suPHP / ExecPHP) as <username>:<username>

THEN

• PHP can read/write the files in the users folder as if they were its own (As they are)
• The user accessing via FTP can read/write the files as if they are their own (As they are)

That is the entire reason for FTP, In the event that writing files directly to the hard drive via PHP causes the files not to be owned by the user, or potentially not modifyable by the user, there is a fallback option available.

I am not 100% sure why i originally used getmyuid() & family, Perhaps it was because it worked, It might not have been the best choice however.

See also: Ticket #12201 getmyuid() is disabled - workaround

[dd32]

I think i've discovered the reason i originally didnt use fileowner().

fileowner() may return (int)0 as the fileowner on windows systems. getuid() may return a different value however (I believe).

I'm attaching a patch which I believe should work under most situations.

The addition of the is_writable() check should prevent the (int)0 ownership issues I hope, PHP should be able to determine it cant write to the target path even in that case.

[imme-emosol]

On ubuntu my wordpress files are owned by root:www-data
and therefore fileowner() returns (int)0 as well, because of root user.
Your patch did not work as I expected,
because php seems to writes files as www-data:www-data.

So, in an effort to recap you, I think this should :

• check the owner of the wp-content(/plugins) folder
• choose the option that will hopefully create new files with same permissions as wp-content folder

So I added some checks to the script that do a strict group && user check
and I changed it from FILE to $context

[imme-emosol]

Diff for wp-admin/includes/file.php WordPress version 2.8.4-1ubuntu1

[dd32]

Not going to change this late in the dev cycle.

[dd32]

#10205 was marked as a duplicate.

[dd32]

In 30384:

Background Updates: Introduce support to take advantage of Group Writable (or World Writable) to Core Background updates.
This is only enabled when new files will not be installed during the update (as indicated by the WordPress.org API), and does not apply to Plugin/Theme/Translation Background Updates.

Additionally, the code to determine if the 'direct' filesystem transport should be used has been tweaked for wider support (where getmyuid() was unavailalbe) which fixes #10424

See #10205, #30245