Patch confusion
This issue has patches for both 8 and 7. The 7 issue is created, and the goal remaining on this issue is to test the rerolled patch in #123 for 8.4.x.
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 |
---|---|---|---|
#123 | 1068266_rerolled_103-123.patch | 2.04 KB | georob |
#107 | interdiff-1068266-107.txt | 1.9 KB | pcambra |
#107 | 1068266-107.patch | 6.7 KB | pcambra |
#104 | 1068266_reroll_84.patch | 6.7 KB | Mile23 |
| |||
#103 | 1068266_103.patch | 2.01 KB | Mile23 |
Comments
Comment #1
weboide CreditAttribution: weboide commentedIt seems to come from mkdir itself.
http://us.php.net/manual/en/function.mkdir.php#100106
Comment #2
weboide CreditAttribution: weboide commentedCouldn't we use something like that in drupal_mkdir?:
From the link above:
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedMarked #1196382: styles subfolders get wrong folder permissions (0755) as a duplicate. I'm upping the priority because this affects users on shared hosting, who cannot delete the directories created by image module because of this.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch applies the solution proposed in #2. It gets the current umask, sets umask to 0, creates the directories, and then resets the umask to whatever it was.
You can test this by creating an Image Style. Before applying the patch, if you
ls -al sites/default/files/styles/
, you will see that the file perms on the new image style file are 755 (if they aren't, that means you have a different existing umask). After the patch, the file perms will be 775.Comment #5
grossmann CreditAttribution: grossmann commentedThanks for the patch. I will test it later.
grossmann-mcs
Comment #6
grossmann CreditAttribution: grossmann commentedDeleted for double post. Sorry.
grossmann-mcs
Comment #7
guile2912 CreditAttribution: guile2912 commentedConfirm that the #4 patch works fine under 7.12
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedThis bug affects users on shared hosting, who cannot delete the directories created by image module.
Because it's likely that these people won't know what the issue is or have the permissions to fix it even if they do, I'm going to bump this to major.
Comment #9
Volx CreditAttribution: Volx commentedI can also confirm that patch #4 works on 7.12. I would really like to see this commited, thanks :)
I'm wondering why this is behaving the way it is, the last directory having the correct permissions but not the parent directories. Is mkdir applying the umask only on the parent directories?
Comment #10
tim.plunkettThe PHP docs for umask warn:
I think this is something we have to worry about.
Also, needs tests.
Comment #11
catchTagging.
Comment #12
paolomainardi CreditAttribution: paolomainardi commentedHas been committed to D7 ?
Comment #13
tim.plunkett@paolomainardi no, hence the "needs backport to D7". It will be committed to D8 first, and then moved for backport, and marked "fixed" when it has been committed.
Comment #14
andyceo CreditAttribution: andyceo commentedHi all,
I changed title to be more suitable.
I maded a patch with comment #10 in mind.
Also, I provided for following situations:
Maybe it would be better to begin chmod from the end of directories: if we create
$f = 'public://test1/test2/test3/'; file_prepare_directory($f, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
, then we should chmod to 'public://test1/test2/test3/', then 'public://test1/test2/' and at last to 'public://test1/' - because of forgoing reasons. Currently we made it from the begining.I think there is can be a problems on Windows systems - because of using another directory delimiter.
Function was tested with $uri with schema ('public://test1/test2/') and absolute uri ('/var/www/drupal8-core/sites/default/files/test_a1/test_a2/test_a3'). Maybe there is problems with relative uri.
Test assertion plan:
Where shoud I place a file with test of that core functions? I looked for tests of drupal file subsystem in D7 and D8 without results! Where can I see an example of core api tests? Please point me right direction if you can.
Comment #16
andyceo CreditAttribution: andyceo commentedJust warning in non-recursive case in drupal_chmod(). Fixed.
Probably I found out where to put tests for drupal_chmod:
core/modules/system/lib/Drupal/system/Tests/System
or
core/modules/system/tests/file.test
Comment #17
andyceo CreditAttribution: andyceo commentedComment #19
andyceo CreditAttribution: andyceo commented#16: drupal-mkdir_recursive-1068266-16.patch queued for re-testing.
Comment #20
andyceo CreditAttribution: andyceo commented#16: drupal-mkdir_recursive-1068266-16.patch queued for re-testing.
Comment #21
scottrigbyWriting tests
Comment #22
scottrigbyThis will give a false positive if it fails.
Also added tests.
Including new patch plus interdiff.
Comment #23
scottrigbyComment #25
scottrigbyd'oh. No longer needs tests. But they do need to be reviewed.
Comment #26
andyceo CreditAttribution: andyceo commented#22: mkdir_recursive-1068266-21.patch queued for re-testing.
Comment #28
andyceo CreditAttribution: andyceo commentedThank you, scottrigby!
Made little fixes for #21.
Comment #29
andyceo CreditAttribution: andyceo commentedComment #30
tim.plunkettLost the tag in there somewhere
Comment #32
scottrigby@andyceo looks like my patch failed because i renamed the method, and also didn't break. I added your fix, and also removed the extra 'check' from `testFileCheckDirectoryHandling` and `checkFileCheckDirectoryHandling` because it seems redundant now.
I'm adding an interdiff against my patch in #22 only because #28 contains multiple commits.
edit: also cleaned up 1 line documentation to match standards.
Comment #34
chx CreditAttribution: chx commentedYou can not do this. You should patch the local stream only because you have no idea that if foo/bar/baz exists then foo/bar does as well. That's a presumption made on local filesystem grounds.
Comment #35
chx CreditAttribution: chx commentedSorry for not addign this is in #34 : thanks to everyone working on this patch.
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedAs I mentioned to chx in IRC, it is important to ensure whatever gets committed here fixes #1196382: styles subfolders get wrong folder permissions (0755) as well, or we need to reopen that issue.
I marked that issue as a duplicate of this one in October. The only reason that I moved this issue to major is because the other issue likely affects users who are unable to diagnose the problem for themselves.
Comment #37
chx CreditAttribution: chx commentedHere's the IRC discussion. Let's presume you have an URI like
s3://styles/large/s3/foo.jpg
(coming fromimage_style_path
). This does not mean thats3://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. So what we need to do isdrupal_mkdir_local()
.This handles file paths likesites/default/files/styles/large/public/foo.jpg
and NOT schemed URIs.drupal_mkdir
checks for a scheme, if it's not there, it's a local path so it callsdrupal_mkdir_local()
.drupal_mkdir_local()
.Comment #37.0
ZenDoodles CreditAttribution: ZenDoodles commentedUpdated issue summary.
Comment #38
scottrigbyNew patch for 1-3 in new issue summary. New tests need to be written.
Comment #40
scottrigbyHere's a new patch based on IRC with chx & zendoodles.
I left an {@inheritdoc} in there but I'm tired and need to upload this before I forget (doc can be updated if this is looking like the right direction) =)
Comment #42
chx CreditAttribution: chx commentedThis fails because
$directory_path .= DIRECTORY_SEPARATOR . $directory;
makes it start with the root. I have fixed that and tightened the code just a tiny little bit. Tiny bit: you can move the whole thing into onewhile
condition which we do not want to do because the readability of that is horrid. I also added / cleaned up the comments a tiny bit. I know it installs at least. I also renamed the new helpers to private.Comment #43
chx CreditAttribution: chx commentedRemoved one outdated comment.
Comment #45
chx CreditAttribution: chx commentedComment #47
jbrown CreditAttribution: jbrown commentedDirectories must be executable and writable for a directory to be created within them.
Not necessary to set $mode to file_chmod_directory variable if default.
Simplified _drupal_mkdir_local() a bit.
fileperms() returns directory bit so can't be used in the test.
Comment #48
jbrown CreditAttribution: jbrown commentedMaking some improvements.
Comment #49
jbrown CreditAttribution: jbrown commentedThis issue is not related to umask at all. The problem is simply that PHP's mkdir() only sets the mode on the top-level directory being created.
Overriding umask when creating directories would be a separate issue.
I was also able to simplify the code and only introduce one function instead of two.
Interdiff coming up.
Comment #50
jbrown CreditAttribution: jbrown commentedinterdiff #45 => #49
Comment #51
ZenDoodles CreditAttribution: ZenDoodles commented@jbrown Thank you for your effort on this, but I think maybe you misunderstand the issue. We've already looked at this approach (see the progression of patches starting from the one in comment #38) with a variety of roadblocks.
Also, the test changes no longer actually test the issue here. If you make changes to tests, please upload a tests only patch in addition to your complete patch to demonstrate failing tests without the fixes in the full patch.
(See also this duplicate issue #1196382: styles subfolders get wrong folder permissions (0755) which is broken with 0755 for directory permissions, the current behavior.)
(Adding the needs tests tag back based on scottrigby's comment in #38 and so we don't loose the ones we have based on the most recent patch.)
Comment #52
chx CreditAttribution: chx commentedThis is the test only from #47 and #49. We will see -- if either passes, the test is not good.
Comment #53
jbrown CreditAttribution: jbrown commentedI am reworking this patch.
Comment #54
jbrown CreditAttribution: jbrown commented@ZenDoodles you do not understand my patch - please read my patches more carefully before commenting.
My previous patch includes chx's request that we should not override the recursive functionality for uris with schemes - these should be handled by the scheme implementation:
and
The tests from #45 were totally broken for these reasons:
I have realised that I misunderstood the issue summary:
This implies that mkdir() does set the file permissions as expected when we do not create directories recursively. However, this is not correct. mkdir() is always influenced by the umask - it is just that in file_prepare_directory() we override this with chmod - but only for the top-level directory.
The solution is to define that drupal_mkdir() overrides umask for both the top-level directory created and any components of the directory path that get created due to $recursive being specified.
Comment #55
andyceo CreditAttribution: andyceo commented#54: 1068266.patch queued for re-testing.
Comment #56
jbrown CreditAttribution: jbrown commentedWow - it still applies!!
Comment #57
ñull CreditAttribution: ñull commentedWould this address #1791280: folders (auto) created with wrong permissions too? If so, then I can mark it as duplicate of this one. If not, would it be possible to take that issue on board? It is very similar, isn't it?
Comment #58
ñull CreditAttribution: ñull commentedI back-ported patch #54 to Drupal 7 and can confirm that it fixes #1791280: folders (auto) created with wrong permissions. I think it is safe to mark it as duplicate of this issue.
The changes in includes/file.inc are all the same. Changes of lib/Drupal/Core/StreamWrapper/LocalStream.php in D7 will need to be made in includes/stream_wrappers.inc instead.
Would be nice when the patch is submitted and back-ported soon, because it is really an annoying bug.
Comment #59
sun@chx is all over this issue, so would be good if he'd review and eventually sign off #54
Comment #60
ñull CreditAttribution: ñull commentedAttached the back-port to D7
Comment #62
Volx CreditAttribution: Volx commentedTest previous patch for 7.x.
Comment #63
Volx CreditAttribution: Volx commented#60: drupal-mkdir_recursive-1068266-60.patch queued for re-testing.
Comment #64
tim.plunkettI don't think this was committed to D8 yet, please don't move it to D7 until it is.
Comment #65
Volx CreditAttribution: Volx commentedJust moved it to test the D7 patch.
Comment #66
mjgruta CreditAttribution: mjgruta commentedThe patch #60 works with my Drupal 7.18 version on my local machine. The problem occurs when I create or edit image styles. The permission of the directory it creates or re-creates becomes 700 or drwx------. I dont know if this is related to this problem or it's a settings problem on my local machine.
I'm using CentOS 5.8 64bit, PHP 5.3.3, Apache 2.2.3
Comment #67
jbrown CreditAttribution: jbrown commented#54: 1068266-tests-only.patch queued for re-testing.
Comment #68
jbrown CreditAttribution: jbrown commented#54: 1068266.patch queued for re-testing.
Comment #69
jbrown CreditAttribution: jbrown commented#54: 1068266-tests-only.patch queued for re-testing.
Comment #70
jbrown CreditAttribution: jbrown commented#54: 1068266.patch queued for re-testing.
Comment #71
chx CreditAttribution: chx commentedYes #54 is good. I clicked an admin retest for it (boo abusing test admin powers), just to see it still applies and apparently it does, I have no doubts it'll pass again.
Comment #72
jbrown CreditAttribution: jbrown commented#54: 1068266.patch queued for re-testing.
Comment #73
webchickCommitted and pushed to 8.x. Thanks!
Marking back to 7.x, but AFAIK those URI -> uri changes in the docs look incorrect, so we should re-roll without those.
Comment #74
jbrown CreditAttribution: jbrown commentedD7 reroll.
Comment #75
vijaycs85I found a regression issue of this ticket which blocks drupal 8 installation on windows OS.
Step to reproduce the issue.
1. try to install drupal-8 and on step 3, you can see this screen:
2. var_dump($uri); var_dump(DIRECTORY_SEPARATOR); in drupal_mkdir() shows exactly why it doesn't work:
The main issue is explode() at https://gist.github.com/vijaycs85/4756532#file-file-php-L11
Comment #76
jbrown CreditAttribution: jbrown commentedThis should fix it on Windows - can someone with a Windows setup test it?
Comment #77
plach#76 fixes the issue on Windows XP, thanks!
Comment #78
vijaycs85#76 works for both install and simpletest. Thanks @jbrown.
Comment #79
vijaycs85removed tag.
Comment #80
dman CreditAttribution: dman commentedThanks guys.
I guess this now may fix the scary problem (pictured in #75) that killed all our Windows-based core contribution sprint beginners at the Sydney Drupalcon sprint.
It kept saying the problem was permissions, but nothing sane we could try would get past it.
Comment #81
jbrown CreditAttribution: jbrown commentedCan someone RTBC?
Comment #82
vijaycs85Comment #83
catchCommitted/pushed the followup, thanks!
Comment #84
jbrown CreditAttribution: jbrown commentedThanks catch!
Here's the D7 reroll.
Comment #85
vijaycs85#84: 1068266.patch queued for re-testing.
Comment #86
vijaycs85Good to go, if test is green.
Comment #87
David_Rothstein CreditAttribution: David_Rothstein commentedEr, this will pollute the actual site's files directory with something that remains after the test run and never gets cleaned up. It should be using the simpletest version of the public files directory instead.
Otherwise, this looked good to me (nice work!), but how much has the Drupal 7 patch been manually tested? A patch like this definitely requires manual testing in several different scenarios (especially if an earlier version already broke things on Windows).... I did some light testing on my own (including uploading/installing modules via the user interface which seemed like a code path that used drupal_mkdir() in interesting ways) and so far so good, but more people really need to try it out.
***
A couple other comments (less likely to be commit blockers though):
Seems odd to me that $uri is modified only in the if() statement, but then used either way later on. Either it needs to be modified or it doesn't, right? (However, I'm not sure if this matters in practice.)
Well, I guess... though drupal_chmod() also does a "@" to suppress errors on the chmod() call (not sure why), and other nice things like a watchdog() message on failure. Should we at least use the "@" here? I think I agree with not calling drupal_chmod() itself though, since it does some high-level stuff with stream wrappers that might actually fail in certain situations where drupal_mkdir() would have worked before.
Comment #88
jbrown CreditAttribution: jbrown commentedFixed the testing directory.
1. The only reason we need be using forward slashes is so the explode() on the next line works. It doesn't actually make sense to use DIRECTORY_SEPARATOR: http://alanhogan.com/tips/php/directory-separator-not-necessary - I've removed it. PHP handles it behind the scenes.
2. I think it is better to let chmod() just output it's own errors. This will automatically go into watchdog.
3. Fixed.
Comment #89
jbrown CreditAttribution: jbrown commented#88: 1068266-followup.patch queued for re-testing.
Comment #90
jbrown CreditAttribution: jbrown commented#88: 1068266-followup.patch queued for re-testing.
Comment #91
vijaycs85#88: 1068266-followup.patch queued for re-testing.
Comment #92
jbrown CreditAttribution: jbrown commented#88: 1068266-followup.patch queued for re-testing.
Comment #93
jbrown CreditAttribution: jbrown commented#88: 1068266-followup.patch queued for re-testing.
Comment #95
slashrsm CreditAttribution: slashrsm commented#88: 1068266-followup.patch queued for re-testing.
Comment #96
slashrsm CreditAttribution: slashrsm commentedI'd leave DIRECTORY_SEPARATOR anyway. It does no harm at all and it looks much nicer to use a constant instead of a hard-coded value.
Comment #96.0
slashrsm CreditAttribution: slashrsm commentedUpdated issue summary.
Comment #97
wesleydv CreditAttribution: wesleydv commentedRe-rolled #88 against head, not sure if we need to implement #96
Comment #98
sunNote that we recently implemented a smarter algorithm for this in
PhpStorage
:#2110863: Support open_basedir
Already in that issue, we discussed that we should probably change
drupal_mkdir()
to use the same algorithm.I'm not sure whether this is the right issue for doing that though, since this appears to be a bug in D7, too...
I'm confused —
The subsequently following code relies on DIRECTORY_SEPARATOR, but you're removing it?
Comment #101
jbrown CreditAttribution: jbrown commented$uri = str_replace('/', DIRECTORY_SEPARATOR, $uri);
ensures that the only separator in the string is DIRECTORY_SEPARATOR otherwise you can have both / and \ in the same path.Comment #102
jphelan CreditAttribution: jphelan commentedI realize not everyone has access to this on their server, thus the necessity for this patch but this issue was resolved for me by changing a line in the suphp.conf file. It sets umask to 0077 by default, changing it to 0022 made new child directories create with the correct permissions.
Comment #103
Mile23Here's a straight reroll of #88.
What that means is: NO modification to
drupal_mkdir()
(which is just a wrapper for a service now), and only changes toDirectoryTest
.The changes to the test only do two things:
public://
instead of assembling a path fromsite.path . '/files'
.t()
.DirectoryTest
passes locally after modification, so either the test is no good or subsequent work on Drupal addressed the issue of recursive permissions.If this is still an issue it could be addressed in
Drupal\Core\File\FileSystem::mkdir()
and then tested against.Really though it's probably still a D7 issue exclusively. Change the version once this test passes ::fingers crossed::
Comment #104
Mile23The D8 patch was committed in #83. Yay!
There was a backport patch in #84. Yay!
That patch was reviewed in #87. Yay!
The patch in #88 switched versions to D8, but incorporated some changes from the review in #87. Erm...
So future work on this issue should re-roll against Drupal 7.x on #84 and move forward with the review in #87.
Here's the reroll of #84.
Comment #106
cutesquirrel CreditAttribution: cutesquirrel commentedApplied on a drupal 7.x, worked well, many thanks !
Comment #107
pcambraA bit confused about this, but let's get the ball rolling again, started from #104 and added the feedback from #87, let's see what the testbot thinks.
Comment #110
pcambraIt finally passed, had some odd errors on the testbot.
Comment #113
pcambraDo we need to add a separate issue for the remaining 7.x patch?
Comment #114
stefan.r CreditAttribution: stefan.r commented@pcambra yes, if you want (see https://www.drupal.org/core/backport-policy)
Comment #115
izmeez CreditAttribution: izmeez commentedIn some ways drupal's policies regarding fixing and back porting is becoming unnecessarily complicated.
The policy itself says,
Personally, I would take issue with the merits of what is in brackets.
Why should an issue that began 5 years ago as a Drupal 7 issue, which has subsequently made way for Drupal 8 to take precedence, now need a separate issue?
Using the "needs backport to D7" tag makes far more sense, especially since the patch already exists in the issue thread.
Comment #116
pcambra@stefan.r, just created #2789723: [D7 backport] drupal_mkdir does not set permissions to directories it created recursively to follow up this.
Comment #117
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC for #103 for 8.x.
Comment #123
georob CreditAttribution: georob commentedHi, I went ahead and rerolled #103's patch. Here is the only conflict from the reroll.
I resolved it by adding public to the function from the patch. Patch now applies cleanly to 8.4.x-dev.
Comment #125
joseph.olstadThis was fixed in 8.x by webchick back in 2013, please do not re-open this issue. Also, see duplicate 8.x issue which needs to be closed as a duplicate (duplicate issue below)
#2211657: file_unmanaged_copy not working properly when the destination directory does not exist.
For proof of fix;
see commit:
1068266
Therefore marking this as fixed in order to allow 7.x backport to get committed.
see RTBC backport for 7.x
#2789723: [D7 backport] drupal_mkdir does not set permissions to directories it created recursively