Follow-up to #1068266: drupal_mkdir does not set permissions to directories it created recursively
Problem/Motivation
PHP mkdir() does not set file permissions as expected when we create directories recursively (See http://us.php.net/manual/en/function.mkdir.php#100106 for more information.) As a result:
- file_prepare_directory does not set the permissions to folders it has created recursively
- Users on shared hosting cannot delete the directories created by image module. (#1196382: styles subfolders get wrong folder permissions (0755))
Proposed resolution
The original patches addressed the issue at the drupal_mkdir() level, but it must also be addressed at the LocalStreamWrapper level. Also we do not want to repeat the code for the recursive bits in both locations, so the best way to address it is with a new function drupal_mkdir_local() to also recursively set the permissions.
Here's the IRC discussion. Let's presume you have an URI like s3://styles/large/s3/foo.jpg (coming from image_style_path). This does not mean that s3://styles/large has any meaning at all. Core does not use it, so why should the stream wrapper handle it? Accessing it might throw an exception for all you know. (see also http://www.advomatic.com/blogs/aaron-winborn/stream-wrappers-and-you ). So what we need to do is:
Remaining tasks
- Introduce drupal_mkdir_local().This handles file paths like sites/default/files/styles/large/public/foo.jpg and NOT schemed URIs. Can be refactored from http://drupalbin.com/21585
- drupal_mkdir checks for a scheme, if it's not there, it's a local path so it calls drupal_mkdir_local(). http://privatepaste.com/fda114cd01
- The local stream wrapper already has a local path so it calls drupal_mkdir_local(). http://privatepaste.com/7a5832a7fd
- Write a simpletest to check a new recursively created local directory for correct file system permissions.
- Backport the above to D7 s#DrupalLocalStreamWrapper::mkdir#LocalStreamWrapper::mkdir#
Original report by [weboide]
// Text of original report here.
file_prepare_directory does not seem to set the permissions to folders it has created recursively.
For example:
In settings.php:
$conf['file_chmod_directory'] = 02775; // even tried with 0775
And then run:
$f = 'public://test1/test2/test3/'; file_prepare_directory(\$f, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
would create
`-- [drwxr-sr-x] test1
`-- [drwxr-sr-x] test2
`-- [drwxrwxr-x] test3
I was expecting to see test1 and test2 with group write permission.
Comment | File | Size | Author |
---|---|---|---|
#24 | 2789723-24.patch | 7.58 KB | mcdruid |
#24 | interdiff-2789723-1-24.txt | 2.36 KB | mcdruid |
#2 | 2789723-1.patch | 6.7 KB | pcambra |
Comments
Comment #2
pcambraPatch from #1068266-107: drupal_mkdir does not set permissions to directories it created recursively
Credits to jbrown, chx, scottrigby, andyceo, Mile23, pcambra, linclark, ñull, wesleydv, vijaycs85 so far.
Comment #4
pcambraTest pass
Comment #5
JonMcL CreditAttribution: JonMcL commentedWorks well for me. Just deployed to a Rackspace Cloud environment.
Comment #6
yash_khandelwalBy giving 777 to files folder worked for me.
Another solution is, I solved this by changing the path to my temporary directory to "sites/default/files/temp".
Comment #7
DamienMcKenna@Yash Khandelwal: Setting the file permissions to 777 can be a security issue, you shouldn't do that.
Comment #8
fullerja CreditAttribution: fullerja at University of Colorado Boulder commentedTested on latest dev of d7. Works for me, RTBC based on this and #5. Patch also passed automated testing again.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedComment #10
joseph.olstad@David_Rothstein, looks like the above patch is ready for Drupal 7.60 , until then I'm going to use it in place of the previous patch I was using.
flagging the duplicate issue which I've closed as a duplicate.
Please lets commit this, it's ready for D7, the fix for D8 was committed 4 years ago.
Comment #11
joseph.olstadupdated issue summary marking all remaining tasks as completed
Comment #12
joseph.olstadThis is ready to go for the next Drupal 7.x core release, easy commit for the win. has tests
Comment #13
afinnarn CreditAttribution: afinnarn at University of Colorado Boulder commentedI'm getting "error: corrupt patch at line 178" when trying to apply to Drupal 7.57. I will attempt to re-roll and find out more.
Comment #14
afinnarn CreditAttribution: afinnarn at University of Colorado Boulder commentedsetting to "needs work" due to my last comment. Might just be me.
Comment #15
afinnarn CreditAttribution: afinnarn at University of Colorado Boulder commentednevermind...it was me.
Comment #16
acolden CreditAttribution: acolden commentedOut of curiosity - was there a specific reason this wasn't in 7.57?
And does the issue tag "Drupal 7.60 target" mean that it might not be in 7.58 or 7.59?
Comment #17
oadaeh CreditAttribution: oadaeh at Hook 42 commentedThe reason it wasn't in 7.57 is because that was a security release only, and they do not include non-security related changes (like this ticket) in those releases. This ticket will only be included in a bug fix release. Which bug fix release it will actually be included in remains to be seen.
Just because it is tagged "Drupal 7.60 target" does not mean that it will not be in 7.58 or 7.59.
Comment #18
acolden CreditAttribution: acolden commentedThanks for the info!
Comment #19
joseph.olstadI'm not sure if this issue is related or not but perhaps this core fix will resolve the issue.
I've just noticed a recent issue with drupal_basename
In february 2018 file_entity automated tests for php 5.3 started failing but the code in question had not changed. It was code conditional on the php version, basename function was used by all versions newer than 5.3 , only php 5.3 used the drupal_basename function. (and it should work for the other versions too, it is core) I tried forcing newer versions of php to use the drupal_basename but it failed like the php 5.3. Switching back to basename for all versions now all versions pass.
please see:
#2953708-11: fix php 5.3 compatibility regression issue, drupal_basename vs php basename
this patch
simply swapping out drupal_basename for basename fixes the issue, however the code in question for file_entity hasn't changed in years. Yet the test results changed in february 2018.
Comment #20
joseph.olstadBumping to 7.70. This didn't make it into 7.60
Comment #21
joseph.olstadHas tests, D8 already has this change
Comment #22
joseph.olstadComment #23
joseph.olstadJustification:
1) Fix is already in D8
2) Patch has tests and proves need for fix
3) is RTBC
patch still applies cleanly (with fuzz)
Comment #24
mcdruidI compared 2789723-1.patch with what's actually in D9 now, particularly:
...and it's pretty similar, but there have been a couple more changes upstream so I've updated the backport.
There was also one nit in the comments; they referred to
mkdir()
(which in D8/9 means\Drupal\Core\File\FileSystem::mkdir
) but should bedrupal_mkdir()
in D7.The interdiff was a mess initially as things have moved around, so I've also rerolled patch 1, and the interdiff here is against that.
Comment #25
mcdruidIdeally we'd do a little manual testing of this on Windows; I'll see if I can dig up a suitable VM unless anyone beats me to it.
IIUC a good manual test is to set up and use some image styles and check all of the directories are created as expected without errors.
Comment #26
mcdruidI've done some basic manual testing on Windows/IIS.
It looks like once the basic permissions are set up for the public files directory, Drupal is able to create dirs for e.g. image styles recursively and AFAICS the permissions look okay and there are no errors.
That - coupled with the fact that we're looking at a pretty straight backport of fairly mature code from D8/9 - gives me enough confidence in this patch to commit it.
Comment #28
mcdruidThanks everyone!
Comment #29
pcambra@mcdruid wondering if you could add credits to "Credits to jbrown, chx, scottrigby, andyceo, Mile23, pcambra, linclark, ñull, wesleydv, vijaycs85 so far." as per #2
Comment #39
mcdruidOhhh sorry, I saw that initially but it slipped my mind once I got into reviewing the patch.
Thank you for the reminder.
Comment #40
pcambraThanks :)