Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Right now Drupal is unusable with filesystem (POSIX) ACLs. It's been broken for years. This was previously #944582: ./sites/default/files directory permission check is incorrect during install AND status report (affects Docker on Windows) and #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs which are/were doomed to failure.
Steps to reproduce in Drupal 7 (change apache to your web server user):
- Build a clean Drupal 7 development environment
- In /sites/default:
- mkdir files
- chmod -R 700 files
- setfacl -R -m user:apache:rwx files/
- setfacl -Rd -m user:apache:rwx files/
- Install Drupal
- Do anything that calls
file_prepare_directory('public://', FILE_CREATE_DIRECTORY)
. For instance, install the IMCE module and open its file browser from your "My Account" page.
Anything that calls file_prepare_directory('public://', FILE_CREATE_DIRECTORY)
will fail, claiming that the directory (sites/default/files) is not writable.
Comments
Comment #1
star-szrThanks @mjorlitzky, at a glance this looks pretty good. This needs to be fixed in 8.x first then backported to 7.x per the backport policy.
The 8.x patch from #944582-56: ./sites/default/files directory permission check is incorrect during install AND status report (affects Docker on Windows) will need to be rerolled against 8.x minus the installer changes.
Instructions for rerolling patches.
Comment #2
star-szrTagging, this would still need a forward port/reroll to apply to 8.x.
Comment #3
joates CreditAttribution: joates commentedi will reroll this patch against 8.x
Comment #4
juan.brein CreditAttribution: juan.brein commentedWe worked on this in the Drupal Spring London with Pasive and joates. We basically took the patch provided for D7 in this issue summary and ported it to D8 up to date. Thanks to Joates to guide us through the process!
Comment #5
Cameron Tod CreditAttribution: Cameron Tod commentedCode looks great!
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedMany things look wrong in there.
First, the original post mentions that Drupal is broken when ACLs are in use, but doesn't mention why checking the execute bit (which has nothing to do with ACLs) would fix anything. Other then parsing the ACLs manually, the only way to check if you can write a file in a given directory is to try to write it, and that's what #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs actually tried to do.
Second, using
drupal_realpath()
without checking the return value is *really wrong*. Remote stream wrappers do not implement->realpath()
, so this is going to return FALSE.Third, I don't see at all why this is necessary, because both
is_executable()
andis_writable()
are basically wrappers aroundstat()
and as a consequence they *actually work* with stream wrappers (as long as they implement->stat()
).Finally, I doubt
is_executable()
on a directory returns what you expect it to return on Windows.Comment #7
joates CreditAttribution: joates commentedty for the feedback.
possible alternative to drupal_realpath() (deprecated) is discussed here..
#1201024: drupal_realpath() should describe when it should be used
[edit] in the case of a remote resource ... [/edit]
returning FALSE is certainly not useful, should probably at least test for this & log an error when it occurs.
Comment #8
juan.brein CreditAttribution: juan.brein commentedHi Damien and Joates. Thank you guys for the feedback. As far as I can tell at this point this issue should have never existed? Would you suggest another alterantive or we'll just rely on the fact that drupal does not support ACLs full stop.
Please let me know as I'm new at drupal issues so sincerely I don't know how to proceed from here.
Cheers!
Juan B
Comment #9
BerdirFrom reading Damien's response, I think the first thing to do here is to provide detailed, reproducable steps on how to get Drupal to fail when it should work.
Comment #10
orlitzky CreditAttribution: orlitzky commentedSteps to reproduce in drupal-7.20 (swap apache with your website user):
IMCE will fail, claiming that the directory (sites/default/files) is not writable. This is due to
file_prepare_directory('public://', FILE_CREATE_DIRECTORY)
failing, and is fixed by the patch which modifiesfile_prepare_directory()
.Comment #11
juan.brein CreditAttribution: juan.brein commentedThanks, I'll try this on D8 and get back with the results here.
Comment #12
joates CreditAttribution: joates commentedin response to #10 Posted by mjorlitzky..
it would be more constructive if we can all agree that this is an 8.x issue now,
there is no 8.x release of IMCE
Notes:
[edit]
// 28-Feb-2013
Modified the instructions to implement full excluded file access to the entire files/ sub-tree (user = nobody)
and added recursive ACL permissions as Drupal needs to store file uploads in several sub-folder locations.
[/edit]
Comment #13
orlitzky CreditAttribution: orlitzky commentedYou don't really need IMCE to reproduce the issue in 8.x (if it exists). Just call
file_prepare_directory('public://', FILE_CREATE_DIRECTORY)
somewhere and see if it works. IMCE is just the reason why this is a complete show-stopper for us.Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, I see what's happening. PHP has a special case for local files in which it uses access(2) or the equivalent Windows call. This supports ACLs. If called on a stream-wrapper, it defaults to a (very complicated) process based on the stat(2) structure returned by the stream-wrapper implementation.
More precisely it checks part of the
.mode
structure depending on:getuid(2)
) is the same as the.uid
in the stat, it uses the "user" part of the mode;getgid(2)
) or the supplementary groups (as returned bygetgroups(2)
is the same as the.gid
in the stat, it uses the "group" part of the mode;So, one way of moving forward here, would be to modify the stat implementation in our local streamwrapper to fake the mode by calling
is_readable()
,is_writable()
andis_executable()
manually, and return the proper structure.Comment #15
clemens.tolboomIs this related to #1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination?
Comment #16
joates CreditAttribution: joates commentedRe #14: thanks Damien for locating what seems to be the root cause of this..
from core/lib/Drupal/Core/StreamWrapper/LocalStream.php, line 472
Comment #17
joates CreditAttribution: joates commentedin response to #9 and #12..
after setting
chmod 700 files
you can clearly see from the terminal that my normal user (joates) is group member but cannot gain access.
getfacl shows the ACL permissions (as described in #12)
(screenshot attachment not working?)
on a fresh install of D8 using default configuration (public:// streamwrapper) i am able to upload an image without error.
Comment #18
joates CreditAttribution: joates commentedtry again with the screenshot..
link: http://i.imgur.com/G0s2i99.png
afterwards i realised that having www-data as the directory owner is not too helpful :-p
so i did a
chown -R joates:joates files
cleared cache and tried again to upload the image, and it did indeed FAIL to upload the file(which sort of feels like causing it to fail when it *should* fail, but maybe i don't understand ACLs correctly --
does?)setfacl
in the top folder apply to every childUpdate: even with recursive ACL the file upload still fails.
link: http://i.imgur.com/FesrnJV.png
Comment #19
joates CreditAttribution: joates commentedcan someone with superior ACL sysadmin skills to me please confirm that it seems that Drupal is not providing support for ACLs ?
(perhaps due to the situation Damien describes in #14)
and maybe change the title of this issue if that is the case :)
Comment #20
star-szr@joates - Thanks for all your work on this issue. Could you please update the issue summary with the steps to reproduce, that way others can update them too and you don't have to keep editing your comments :)
Comment #20.0
joates CreditAttribution: joates commentedUpdated issue summary.
Comment #21
joates CreditAttribution: joates commentedok
Comment #22
orlitzky CreditAttribution: orlitzky commentedThe current steps to reproduce probably won't work I'm afraid. Uploading files—in and of itself—has always worked. The filesystem ACLs do really allow you to write the file, so if you try to do it, it will work.
The problem is when Drupal does its heuristic to determine, "will I be able to write this file if the user tries to do it?" That test is currently broken, so the answer is "no" even though it should be "yes" (thanks to the ACL).
Comment #23
orlitzky CreditAttribution: orlitzky commentedUpdated patch for 7.22.
Comment #25
orlitzky CreditAttribution: orlitzky commentedUpdated patch for 7.23.
Comment #25.0
orlitzky CreditAttribution: orlitzky commentedUpdated issue summary.
Comment #26
orlitzky CreditAttribution: orlitzky commentedUpdated patch for 7.24.
Comment #27
orlitzky CreditAttribution: orlitzky commentedUpdated patch for 7.25.
Comment #28
orlitzky CreditAttribution: orlitzky commentedThe patch for 7.25 applies cleanly to 7.26.
Comment #29
orlitzky CreditAttribution: orlitzky commentedAnd to 7.27.
Comment #30
orlitzky CreditAttribution: orlitzky commentedComment #31
YesCT CreditAttribution: YesCT commentedComment #32
orlitzky CreditAttribution: orlitzky commentedComment #33
orlitzky CreditAttribution: orlitzky commentedComment #34
gold.eagle CreditAttribution: gold.eagle commentedmjorlitzky: Just wanted to say "Thank you". We download this patch regularly, whenever we update Core.
Comment #35
orlitzky CreditAttribution: orlitzky commentedgold.eagle: glad someone else is making use of it!
Comment #36
orlitzky CreditAttribution: orlitzky commentedComment #37
YesCT CreditAttribution: YesCT commentedto run the tests ... change the version. (you can change it back)
Comment #38
YesCT CreditAttribution: YesCT commentedComment #39
YesCT CreditAttribution: YesCT commentedback to 8.0.x which needs a patch to work on that.
Comment #40
orlitzky CreditAttribution: orlitzky commentedComment #41
orlitzky CreditAttribution: orlitzky commentedComment #42
orlitzky CreditAttribution: orlitzky commentedComment #43
dooug CreditAttribution: dooug at Promet Source commentedRTBC++
This patch worked for me against the latest 7.38. It solved the problem I reported here: #2509188: The directory public://pictures does not exist or is not writable.
Is this ticket a backport of the mess in this ticket: #944582: ./sites/default/files directory permission check is incorrect during install AND status report (affects Docker on Windows)?
Comment #44
orlitzky CreditAttribution: orlitzky commentedIt's not a backport, just a cleanup. The patches on that issue tried to solve two problems at once, and people were arguing about the half that I don't care about. So, I split off the part that fixes the permission checks and posted the patch here.
But then someone told me I had to fix it in drupal-8.x before we could apply the patch to drupal-7.x where it's actually broken. And this was back when drupal-8.x was completely broken and couldn't be installed. Then the issue got updated with an incorrect description, and incorrect steps to reproduce. I don't have time to deal with that -- I'm just going to keep updating the patch for drupal-7.x for the rest of my life.
Comment #45
dooug CreditAttribution: dooug at Promet Source commentedWell, Thank you, @orlitzky! Thanks for the quick response and providing a solution amidst that chaotic ticket. And thank you in advance for your life-long service of re-rolling this patch ;)
Comment #47
orlitzky CreditAttribution: orlitzky commentedComment #49
orlitzky CreditAttribution: orlitzky commentedComment #51
orlitzky CreditAttribution: orlitzky commentedComment #52
orlitzky CreditAttribution: orlitzky commentedComment #53
jstollerGiven that no one has successfully reproduced this in Drupal 8 in the last four-and-a-half years, or indicated that it is still a problem in that version, I'm going to go out on a limb and bump this back down to Drupal 7. I've updated the issue description to reflect that.
Personally, I've experienced these errors on my local dev environment and the patch fixes it.
@orlitzky keep fighting the good fight! :-)
Comment #55
jstollerRe-uploading @orlitzky's patch from #52, so it'll test with the correct version of Drupal.
Comment #56
jstollerThat's better! Since this has been thoroughly tested by multiple people for several years now, I'm marking it RTBC.
Comment #57
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis almost certainly does affect Drupal 8, since the code in https://api.drupal.org/api/drupal/includes%21file.inc/function/file_prep... and https://api.drupal.org/api/drupal/core%21includes%21file.inc/function/fi... (as well as other places that call
is_writable()
/is_executable()
) is virtually the same. If this isn't an issue in Drupal 8, someone would need to explain specifically why.I also don't understand why this was split off from #944582: ./sites/default/files directory permission check is incorrect during install AND status report (affects Docker on Windows). Comparing the patches in the two issues, they are almost exactly the same - the primary difference is that the other issue changes the writable/executable checks to use the new system in more places than this one does. I do not understand the rationale for fixing this in only some places but not others, and especially don't see why there should be two open issues that are duplicating each other's work. So I think this could be closed as a duplicate of the issue. (Or alternatively, it could be repurposed as the Drupal 7 backport of that issue, and postponed on getting it fixed in Drupal 8 first.)
Comment #58
orlitzky CreditAttribution: orlitzky commentedThis is a seven-year-old "Drupal doesn't work at all" bug -- if you think closing it as a duplicate of another *unfixed* bug will help, then go for it. I'll keep posting patches until I don't have to any more.
Comment #60
ramzypro CreditAttribution: ramzypro commentedVerified that the patch for 5.6 still works with 5.7.
The difference between this ticket and the other is that this one regularly has patches that keep our site up and running. Myself and my colleagues (who run on AFS servers using ACLs) RELY on orlitzky's patches to keep our sites up and running. The other thread seems so caught up with posts that do not understand the issue, and patches that do nothing. Someone running with an ACL-enabled system needs to be actively involved in constructing solutions to this problem.
Also - thank you orlitzky for your tireless efforts, we would be writing our own patches every Drupal release if not for your work.
Comment #61
orlitzky CreditAttribution: orlitzky commentedIt really hasn't been that much work, but I appreciate the shout-out. I think I'm in the same boat as everyone else. We use ACLs on our web servers, and without this patch, file uploads simply don't work. This patch fixes it and has been tested on hundreds of sites, for how many years now? I'm not even sure what problem is being fixed on the other ticket these days.
Comment #62
slydevil CreditAttribution: slydevil commentedComment #63
MurzThanks, provided patch solves the problem with file permission check in Drupal. So will this patch be applied to Drupal 8.x core? Can anybody else review it and provide for applying?
Comment #64
orlitzky CreditAttribution: orlitzky commentedComment #65
ramzypro CreditAttribution: ramzypro commentedIt appears that the patch for 7.61 no longer applies for 7.63. Any chance this will be updated soon? Appreciated!EDIT: The patch for 7.61 does work!
Comment #66
orlitzky CreditAttribution: orlitzky commentedComment #67
orlitzky CreditAttribution: orlitzky as a volunteer commentedComment #68
orlitzky CreditAttribution: orlitzky as a volunteer commentedComment #69
magapov CreditAttribution: magapov commentedI am so happy to find the patch for 7.68, it also works for 7.69 flawlessly. I think we need to buy @orlitzky coffee and cake.
Comment #70
jstollerLet’s try this one more time. Marking RTBC.
Comment #71
orlitzky CreditAttribution: orlitzky as a volunteer commentedComment #72
jstoller@orlitzky your previous patch still applied for me. Are there any changes in this one that I should be aware of?
Comment #73
orlitzky CreditAttribution: orlitzky commentedNothing important, I just rebased the patch to apply cleanly without unexpected offsets or fuzz. When the patch applies but not cleanly, it leaves around backups in the form of
.orig
files that you don't want.Comment #74
orlitzky CreditAttribution: orlitzky commentedComment #75
orlitzky CreditAttribution: orlitzky commentedComment #76
orlitzky CreditAttribution: orlitzky commentedComment #77
izmeez CreditAttribution: izmeez commentedThis was added to #3192080: [meta] Priorities for 2021-04-07 release of Drupal 7 that will be copied into the meta issue for the next release. Not sure if it will help moving toward a commit.
Comment #78
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedUnfortunately per the backport-policy, this needs to go into D9 first:
See: https://www.drupal.org/project/drupal/issues/944582
And the patch there is identical to this one.
Comment #79
izmeez CreditAttribution: izmeez commentedThis is an example of when the policy makes no sense. As the D9 issue summary says:
So, why let it persist for the over 160,000 D7 sites?
Anyway, the patch is here for those that want to use it. Hopefully, it will remain on the priority list for the future when D9 catches up.
Comment #80
orlitzky CreditAttribution: orlitzky commentedComment #81
orlitzky CreditAttribution: orlitzky commentedComment #82
orlitzky CreditAttribution: orlitzky commentedComment #83
orlitzky CreditAttribution: orlitzky commentedComment #84
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented@orlitzky Thanks for the reroll, but I think that it was not needed, because the patch in #82 still applies. It would be best to concentrate our efforts on the D10 patch so that we can get back to this.
Comment #85
orlitzky CreditAttribution: orlitzky commented@poker10 the reason I reroll them is because, with GNU patch at least, any unexpected offsets or "fuzz" will leave
*.orig
files laying around in your public drupal directory. It's sloppy, but more importantly, if you're using diff/patch to update drupal, runs the risk of accidentally copying those*.orig
files into all of your sites.Finally, if there's a little but of fuzz between version A and version B, the patch will still apply. But if there's also fuzz between version B and C, the patch for version A will stop working. By constantly updating the patch, I ensure that there's always a minimal amount of fuzz so that I can at least apply the thing with
patch
instead of having to re-edit the source files manually. In other words, the only reason the latest patch still applies is because I "pointlessly" rerolled earlier ones :)Comment #86
orlitzky CreditAttribution: orlitzky commentedComment #87
orlitzky CreditAttribution: orlitzky commentedComment #88
orlitzky CreditAttribution: orlitzky commented