Make WordPress Core

Opened 15 years ago

Closed 15 years ago

Last modified 15 years ago

#10170 closed defect (bug) (fixed)

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

Reported by: hakre's profile hakre Owned by: dd32's profile 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 15 years ago.
Chmod after mkdir to ensure correct mode on newly created directory.
10170.2.patch (6.1 KB) - added by hakre 15 years ago.
Extended Patch.
10170.diff (6.7 KB) - added by dd32 15 years ago.
10170.2.diff (8.2 KB) - added by dd32 15 years ago.
10170-wp2.8.patch (1.4 KB) - added by azaozz 15 years ago.

Download all attachments as: .zip

Change History (33)

#1 @Denis-de-Bernardy
15 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?

#2 @dd32
15 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..

#3 @Denis-de-Bernardy
15 years ago

  • Keywords close removed

#4 @dd32
15 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..

#5 @hakre
15 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.

@hakre
15 years ago

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

#6 @hakre
15 years ago

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

#7 @dd32
15 years ago

  • Type changed from enhancement to defect (bug)

Looks good to me

#8 @hakre
15 years ago

  • Keywords commit added

#9 @hakre
15 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.

#10 @azaozz
15 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.

#11 @dd32
15 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..

#12 @hakre
15 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?

@hakre
15 years ago

Extended Patch.

#13 follow-up: @dd32
15 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.

@dd32
15 years ago

@dd32
15 years ago

#14 @dd32
15 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.

#15 in reply to: ↑ 13 ; follow-up: @hakre
15 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.

#16 @dd32
15 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?

#17 @azaozz
15 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.

@azaozz
15 years ago

#18 in reply to: ↑ 15 ; follow-up: @azaozz
15 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.

#19 in reply to: ↑ 18 @hakre
15 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.

#20 @dd32
15 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)

#21 @dd32
15 years ago

Cloased #10299 as duplicate of this.

#22 @Ovidiu
15 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?

#23 @hakre
15 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.

#24 @dd32
15 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

#25 @Ovidiu
15 years ago

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

#26 @Denis-de-Bernardy
15 years ago

  • Keywords blocker added

#27 @ryan
15 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

#28 @ryan
15 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.