WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#10170 closed defect (bug) (fixed)

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

Reported by: hakre Owned by: dd32
Milestone: 2.8.1 Priority: normal
Severity: normal Version: 2.8
Component: Filesystem API Keywords: has-patch tested commit blocker
Focuses: Cc:

Description

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).

Attachments (5)

10170.patch (487 bytes) - added by hakre 6 years ago.
Chmod after mkdir to ensure correct mode on newly created directory.
10170.2.patch (6.1 KB) - added by hakre 6 years ago.
Extended Patch.
10170.diff (6.7 KB) - added by dd32 6 years ago.
10170.2.diff (8.2 KB) - added by dd32 6 years ago.
10170-wp2.8.patch (1.4 KB) - added by azaozz 6 years ago.

Download all attachments as: .zip

Change History (33)

comment:1 @Denis-de-Bernardy6 years ago

  • Component changed from General to Filesystem
  • Keywords close added; developer-feedback removed
  • Milestone changed from Unassigned to 2.9
  • Owner set to dd32

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

comment:2 @dd326 years ago

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..

comment:3 @Denis-de-Bernardy6 years ago

  • Keywords close removed

comment:4 @dd326 years ago

  • Milestone 2.9 deleted
  • Resolution set to worksforme
  • Status changed from new to closed

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..

comment:5 @hakre6 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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.

@hakre6 years ago

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

comment:6 @hakre6 years ago

  • Keywords has-patch tested added
  • Milestone set to 2.8.1

comment:7 @dd326 years ago

  • Type changed from enhancement to defect (bug)

Looks good to me

comment:8 @hakre6 years ago

  • Keywords commit added

comment:9 @hakre6 years ago

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.

comment:10 @azaozz6 years ago

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.

comment:11 @dd326 years ago

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..

comment:12 @hakre6 years ago

@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?

@hakre6 years ago

Extended Patch.

comment:13 follow-up: @dd326 years ago

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.

@dd326 years ago

@dd326 years ago

comment:14 @dd326 years ago

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.

comment:15 in reply to: ↑ 13 ; follow-up: @hakre6 years ago

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.

comment:16 @dd326 years ago

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?

comment:17 @azaozz6 years ago

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.

@azaozz6 years ago

comment:18 in reply to: ↑ 15 ; follow-up: @azaozz6 years ago

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.

comment:19 in reply to: ↑ 18 @hakre6 years ago

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.

comment:20 @dd326 years ago

+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)

comment:21 @dd326 years ago

Cloased #10299 as duplicate of this.

comment:22 @Ovidiu6 years ago

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?

comment:23 @hakre6 years ago

@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.

comment:24 @dd326 years ago

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

comment:25 @Ovidiu6 years ago

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

comment:26 @Denis-de-Bernardy6 years ago

  • Keywords blocker added

comment:27 @ryan6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

comment:28 @ryan6 years ago

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

Note: See TracTickets for help on using tickets.