[D7 backport] drupal_mkdir does not set permissions to directories it created recursively [#2789723]

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	7.x: PHP 7.4 & MySQL 5.7 2,110 pass PHP 5.3 & MySQL 5.5 2,110 pass PHP 5.6 & MySQL 5.5 2,110 pass PHP 7.2 & MySQL 5.6 2,110 pass PHP 7.3 & MySQL 5.7 2,110 pass PHP 7.4 & MySQL 8 2,110 pass PHP 7.4 & SQLite 3.27 2,106 pass PHP 8.0 & MySQL 5.7 2,110 pass PHP 8.1 & sqlite-3.27 2,106 pass
#24	interdiff-2789723-1-24.txt	2.36 KB	mcdruid
#24	2789723-1_reroll.patch	6.68 KB	mcdruid
#24
#2	2789723-1.patch	6.7 KB	pcambra
#2	7.x: PHP 5.3 & MySQL 5.5 2,032 pass PHP 7 & MySQL 5.5 2,108 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

25 August 2016 at 02:52

pcambra created an issue. See original summary.

Comment #2

pcambra

he/him

Spanish

Asturies

CreditAttribution: pcambra at Cambrico commented 25 August 2016 at 02:54

File	Size
2789723-1.patch	6.7 KB
7.x: PHP 5.3 & MySQL 5.5 2,032 pass PHP 7 & MySQL 5.5 2,108 pass

Patch 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 #3

25 August 2016 at 03:06

Status:

Needs review

» Needs work

The last submitted patch, 2: 2789723-1.patch, failed testing.

Comment #4

pcambra

he/him

Spanish

Asturies

CreditAttribution: pcambra at Cambrico commented 30 August 2016 at 00:17

Status:

Needs work

» Needs review

Test pass

Comment #5

JonMcL CreditAttribution: JonMcL commented 9 September 2016 at 02:11

Works well for me. Just deployed to a Rackspace Cloud environment.

Comment #6

yash_khandelwal

English

CreditAttribution: yash_khandelwal as a volunteer and commented 9 September 2016 at 05:03

By 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

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 9 September 2016 at 11:11

@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 commented 25 January 2017 at 22:09

Status:

Needs review

» Reviewed & tested by the community

Tested 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 commented 6 June 2017 at 04:01

Issue tags:

+Drupal 7.60 target

Comment #10

joseph.olstad

French

CreditAttribution: joseph.olstad commented 2 September 2017 at 14:14

@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.olstad

French

CreditAttribution: joseph.olstad commented 2 September 2017 at 14:23

Issue summary:

View changes

updated issue summary marking all remaining tasks as completed

Comment #12

joseph.olstad

French

CreditAttribution: joseph.olstad commented 18 September 2017 at 15:47

This 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 commented 21 February 2018 at 19:47

I'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 commented 21 February 2018 at 19:48

Status:

Reviewed & tested by the community

» Needs work

setting to "needs work" due to my last comment. Might just be me.

Comment #15

afinnarn CreditAttribution: afinnarn at University of Colorado Boulder commented 21 February 2018 at 19:52

Status:

Needs work

» Reviewed & tested by the community

nevermind...it was me.

Comment #16

acolden CreditAttribution: acolden commented 1 March 2018 at 16:42

Out 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 commented 1 March 2018 at 17:46

The 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 commented 2 March 2018 at 10:52

Thanks for the info!

Comment #19

joseph.olstad

French

CreditAttribution: joseph.olstad commented 9 April 2018 at 19:11

I'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.olstad

French

CreditAttribution: joseph.olstad commented 21 October 2018 at 16:51

Issue tags:

-Drupal 7.60 target

+Drupal 7.70 target

Bumping to 7.70. This didn't make it into 7.60

Comment #21

joseph.olstad

French

CreditAttribution: joseph.olstad commented 30 October 2020 at 02:44

Issue tags:

-Drupal 7.70 target

+Drupal 7.74 target

Has tests, D8 already has this change

Comment #22

joseph.olstad

French

CreditAttribution: joseph.olstad commented 29 November 2020 at 03:52

Issue tags:

-Drupal 7.74 target

+Drupal 7.76 target

Comment #23

joseph.olstad

French

CreditAttribution: joseph.olstad commented 29 November 2020 at 03:55

Issue tags:

+Drupal 7 bugfix target

Justification:
1) Fix is already in D8
2) Patch has tests and proves need for fix
3) is RTBC

patch still applies cleanly (with fuzz)

j @ hostname ~/drupal(7.x)
└─ $ ▶ patch -p1 < 2789723-1.patch 
patching file includes/file.inc
Hunk #1 succeeded at 439 (offset 2 lines).
Hunk #2 succeeded at 2481 (offset 100 lines).
Hunk #3 succeeded at 2511 (offset 100 lines).
patching file includes/stream_wrappers.inc
Hunk #1 succeeded at 784 (offset 148 lines).
patching file modules/simpletest/tests/file.test
Hunk #3 succeeded at 1058 (offset 162 lines).

Comment #24

mcdruid

🇬🇧🇪🇺

CreditAttribution: mcdruid at Acquia commented 23 November 2021 at 21:32

Issue tags:

-Drupal 7.76 target

File	Size
2789723-1_reroll.patch	6.68 KB

interdiff-2789723-1-24.txt	2.36 KB
2789723-24.patch	7.58 KB
7.x: PHP 7.4 & MySQL 5.7 2,110 pass PHP 5.3 & MySQL 5.5 2,110 pass PHP 5.6 & MySQL 5.5 2,110 pass PHP 7.2 & MySQL 5.6 2,110 pass PHP 7.3 & MySQL 5.7 2,110 pass PHP 7.4 & MySQL 8 2,110 pass PHP 7.4 & SQLite 3.27 2,106 pass PHP 8.0 & MySQL 5.7 2,110 pass PHP 8.1 & sqlite-3.27 2,106 pass

I compared 2789723-1.patch with what's actually in D9 now, particularly:

core/lib/Drupal/Core/File/FileSystemInterface.php
core/lib/Drupal/Core/File/FileSystem.php

...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 be drupal_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

mcdruid

🇬🇧🇪🇺

CreditAttribution: mcdruid at Acquia commented 24 November 2021 at 10:14

Issue tags:

+needs manual testing on Windows

Ideally 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

mcdruid

🇬🇧🇪🇺

CreditAttribution: mcdruid at Acquia commented 24 November 2021 at 15:04

Issue tags:

-Drupal 7 bugfix target, -needs manual testing on Windows

I'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 #27

24 November 2021 at 15:23

mcdruid committed 6c6b0dc on 7.x

Issue #2789723 by mcdruid, pcambra: drupal_mkdir does not set...

Comment #28

mcdruid

🇬🇧🇪🇺

CreditAttribution: mcdruid at Acquia commented 24 November 2021 at 15:25

Status:

Reviewed & tested by the community

» Fixed

Thanks everyone!

Comment #29

pcambra

he/him

Spanish

Asturies

CreditAttribution: pcambra at Cambrico commented 24 November 2021 at 17:00

@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