Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#25822 closed defect (bug) (fixed)

Upload folder permissions

Reported by: petyo1313's profile petyo1313 Owned by: dd32's profile dd32
Milestone: 3.7.2 Priority: normal
Severity: critical Version: 3.7
Component: Media Keywords: has-patch fixed-major
Focuses: Cc:

Description

After upgrading to 3.7.1 I noticed that all new items uploaded to media folder cannot be opened with "Forbidden 403 error". I believe the newly created folders "11" are not open for reading by everyone.Those issues do not exist for anything uploaded prior October, 2013

Attachments (1)

25822.diff (983 bytes) - added by dd32 10 years ago.

Download all attachments as: .zip

Change History (15)

#1 @SergeyBiryukov
10 years ago

  • Milestone changed from Awaiting Review to 3.7.2
  • Version set to 3.7

The only related changeset appears to be [25047]. Moving for investigation.

#2 @dd32
10 years ago

More information regarding the permissions of the existing folders (wp-content/upload) and the other previously created folders would be good here.

The changeset didn't appear to change any logic - although it may be that PHP isn't applying the $mode to recursively created directories in some cases.

#3 @SergeyBiryukov
10 years ago

  • Keywords reporter-feedback added

#4 @dd32
10 years ago

  • Keywords has-patch added; needs-patch removed

The only thing I've been able to find is that mkdir()'s $mode is affected by umask(), but chmod() isn't, that would be a behaviour change between 3.6 and 3.7.

umask basically sets the defaults for created files, by removing permissions from created files/directories.

This means that we cannot use the $recursive option of mkdir() AND use the $mode parameter reliably without a work around.

25822.diff attempts to work around this by in the event that it's detected that the umask altered the permissions it'd have created. In addition, dirname() doesn't return false, and also catches a case where stat() returns false resulting in the default value of 0777 not being used.

@dd32
10 years ago

#5 @dd32
10 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

In 26449:

Fix a regression in wp_mkdir_p() where the $mode of the parent folder is not correctly applied to all created paths. Fixes #25822 for trunk

#6 @dd32
10 years ago

  • Keywords fixed-major added; reporter-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#7 @fboender
10 years ago

There's a bug in the patch (and thus now in the live code) which causes the permission-fixing done by wp_mkdir_p to fail. This line:

1381:                if ( $dir_perms != $dir_perms & ~umask() ) {

Shoud be:

1381:                if ( $dir_perms != ($dir_perms & ~umask()) ) {

Right now, the condition is not met properly due to operator precedence. This causes the code to re-set the $dir_perms correctly to not be run.

I can attach a patch if desired.

#8 @schamane
10 years ago

  • Cc as2008+wordpress@… added

Thanks a lot fboender! I'd like to confirm the bug and your patch for 3.8.

#9 @markoheijnen
10 years ago

I think this ticket need to be closed since it was done for 3.8.

@fboender: Could you create a new ticket and add a patch to that one?

#10 @fboender
10 years ago

I've created a new ticket (#26781) for this bug. I've also added proof, steps to reproduce and a patch to fix the problem to the ticket.

#11 @markoheijnen
10 years ago

Thank you for doing that.

This ticket was mentioned in IRC in #wordpress-dev by humungulous. View the logs.


10 years ago

#13 @dd32
10 years ago

In 26927:

Uploads: Fix the Order of Operations for wp_mkdir_p() which caused this branch to never be hit. Props fboender. Fixes #26781 for 3.8.1. See #25822

#14 @nacin
10 years ago

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

In 27887:

Fix a regression in wp_mkdir_p() where the $mode of the parent folder is not correctly applied to all created paths.

Merges [26449] and [26927] from 3.8.x to the 3.7 branch.

props dd32.
fixes #25822.

Note: See TracTickets for help on using tickets.