File Permissions on Creation (Plugin but might be Theme as well)

[hakre]

While installing a plugin from the wordpress repository directly within the admin/backend, wordpress creates directories and files.

It looks like that directories are created with certain settings/modes. For example in my case this is rwx------ for directories and rw-r--r-- for files.

It assume this depends on server configuration. It would be an improvement to have a setting (server-wide possible?) that specifies automatically chmod for directories (e.g. rwxr-xr-x) and files (can stay like it is with my example).

[Denis-de-Bernardy]

I suspect that this is what FS_CHMOD_FILE and FS_CHMOD_DIR do. Invalid no?

[dd32]

Yes, Thats what those constants are for, Cases where the current default settings do not work.

However.. That folder creation seems wrong, By default, its 755 for folders, and 644 for files..

[dd32]

Closing as worksforme due to the constants default settings being set to 755/644 already. Constants should allow you to change it if the defaults do not work under your setup.

If you've got any details why the files may not be being chmod'd correctly, please re-open with details..

[hakre]

I could analyze it now and found the cause: it's the umask. As said, 0755 is the default FS_CHMOD_DIR. That is perfecly well. WP_Filesystem_Direct->mkdir() does use that value then as parameter of the PHP mkdir() function. This is where the (design) error resides. Because mkdir() will apply the umask (see the PHP umask()-function) to the mode while creating the directory. In my case the umask is 77 and therefore the directory is created with the 0700 mode.

Conclusion: WP_Filesystem_Direct does not reflect umask settings while creating a new directory.

Fix: chmod the path after mkdir:

		if ( ! @mkdir($path, $chmod) )
			return false;
		chmod($path, $chmod);

I already tested that fragment and will provide a patch against current trunk in a minute.

[hakre]

Chmod after mkdir to ensure correct mode on newly created directory.

[dd32]

Looks good to me

[hakre]

Media upload does use the wp_mkdir_p() function. It is taking care of chmodding the direcotry to the parents directory mask after creating it with mkdir(). Pretty crushing the routine anyway but chmod is in there.

[azaozz]

Looks like the patch may cause problems in some cases as the function uses $this->permission which is set to umask() by default and is usually 22 (0022). This actually doesn't seem right.

[dd32]

Looks like the patch may cause problems in some cases as the function uses $this->permission which is set to umask() by default and is usually 22 (0022). This actually doesn't seem right.

$this->permission should really be set to 0777 & !umask() i think..

Should just strip the permission var out of the class, we're relying upon FS_CHMOD_(FILE|DIR) now anyway..

[hakre]

@azaozz: moved umask out there, this is defenetly wrong.

@dd32: using umask for the default permission (even if used mathematically as intended) does not fit here because to that value umask() will be automatically applied later on -again- with mkdir() and the like.

---

the one ->permission value can not reflect both: FILE, DIR. therefore that value should become obsolete in favor of the contants -or- the class itself relfects both settings: file and dir (->permission_file, ->permission_dir or by array.).

technically it is not needed, because those can be replaced by constants. this would be easier as well. practically it makes sense not to do so because otherwise the class would be directly connected with global contants which does not help to keep things apart from each ohter.

I added some more docblocks from the top of the file but could not finish. Next function is group() to contiue with. Maybe DD32?

[hakre]

Extended Patch.

[dd32]

IMO, Theres no need to add verbose comments like that. A simple "Octal representation of a unix permission" would fit better.

I'm adding a patch which strips out the permission property, since its useless, And makes the permission a required arguement of ::chmod() (I mean, You call it to be very specific, chmod to THIS permission set). The method setDefaultPermission() has never really been used by WP, nor is it actually useable.. So.. removed that as well.

My patch needs testing, As i've not actually tested it.

[dd32]

I'd like to see hakre's first patch commited for 2.8.1, And mine for trunk.

Some documentation for the functions would be nice, But IMO, should be in a different ticket.

[hakre]

Replying to dd32:

IMO, Theres no need to add verbose comments like that. A simple "Octal representation of a unix permission" would fit better.

Jup, that was copied over, I saw that after the upload of the patch, that's too much of it. Sorry for that. It's way much too big at all.

I'm adding a patch which strips out the permission property, since its useless, And makes the permission a required arguement of ::chmod() (I mean, You call it to be very specific, chmod to THIS permission set). The method setDefaultPermission() has never really been used by WP, nor is it actually useable.. So.. removed that as well.

You know that better then me. If it is not used, remove it.

My patch needs testing, As i've not actually tested it.

Could only review it:

• function mkdir(): if( ! $chmod ) with default of $chmod = false is unable for a $chmod = 0000; (int 0 as octal representation). that is a valid permission in unix, right? so better to comapre against NULL again (=== null)
• TYPO: return false; Prevent tihs function looping again. in line 213.

I have your patch on my list for testing.

[dd32]

function mkdir(): if( ! $chmod ) with default of $chmod = false is unable for a $chmod = 0000; (int 0 as octal representation). that is a valid permission in unix, right? so better to comapre against NULL again (=== null)

True. Can do a false === then.. But seriously, Who'd be setting it to 0000 in their right mind? :) I realise its valid and all, But theres little need for it to be set to that, let alone by wordpress.

TYPO: return false; Prevent tihs function looping again. in line 213.

Argh. I wish my editor had a spell checking function for comments.. Would've stoped me making that typo.. oh.. how many months ago?

[azaozz]

Perhaps we shouldn't be passing the second arg to mkdir(). Don't think it's needed when we do chmod($path, $chmod); straight after creating a directory. Doing mkdir($path, FS_CHMOD_DIR) would always make a dir with the wrong permissions when umask is not zero.

[azaozz]

Perhaps we can remove the default permission for 2.8.1. The attached patch still checks for it in case a plugin is using it.

Replying to hakre:

• function mkdir(): if( ! $chmod ) with default of $chmod = false is unable for a $chmod = 0000; (int 0 as octal representation). that is a valid permission in unix, right? so better to comapre against NULL again (=== null)

Not allowing permissions of 0000 is a feature not a bug. As DD32 says there's no need for unusable files or dirs.

[hakre]

Replying to azaozz:

Not allowing permissions of 0000 is a feature not a bug. As DD32 says there's no need for unusable files or dirs.

True. It was only a quick review from my side and it caught my attention.

---

For testing: I could only test the filesystem direct class for changes on that system. It worked out well so far for 10170.2.diff against current trunk.

[dd32]

+1 for azaozz's patch for 2.8 then.

Not sure of the need to check $this->permission though, Nothing uses it that i'm aware of.. Core certainly doesnt (It always passes the value anyway)

[dd32]

Cloased #10299 as duplicate of this.

[Ovidiu]

Wow. great. that was me with the duplicate bug report. to be honest these problems have existed for a long time, I just got along now to report it as I lsot ssh access from work, so this bug now really hindered my work :-)

how can we find out when this bug has been fixed?

[hakre]

@ovidiu: Hopefully with 2.8.1. You can apply the patch on your sites directly as well.

---

+1 for azaozz's patch as well. nice one.

[dd32]

how can we find out when this bug has been fixed?

When this ticket is closed.

If you've added an email address to your preferences, You should receive updates on this ticket. - Preferences: ​http://core.trac.wordpress.org/prefs

[Ovidiu]

great, my first experience with trac :-) updated my informatio with my email address.

[ryan]

(In [11667]) Proper permissions for newly created files. Props azaozz. fixes #10170 for trunk

[ryan]

(In [11668]) Proper permissions for newly created files. Props azaozz. fixes #10170 for 2.8.1