When the module creates a directory to export the views, this is the code (line 121):

@mkdir($target_path, 0777, TRUE);

This seems like a bad idea to me. The 'all' group should never have write permissions on any web server. I'd suggest a more restrictive default, 0775 at the least.

CommentFileSizeAuthor
#4 drush_views_743970-4.patch1.04 KBxjm

Comments

pescetti’s picture

pescetti commented
Title: Should directory really be 777? » Permissions on directory creation: avoid 777 (or hardcoded)
Category: support » bug

Well, whether 777 is acceptable or not depends of course on context, might be necessary in some (admittedly flawed) situations and the target dir is not necessarily under the web root.

What should be corrected is the fact that we hardcode permissions.

We could just create a dir having the same permissions as its deepest existing parent (which surely include, of course, write permissions for the current user or the export will fail anyway). Could this be a solution? This would respect whatever existing setup the user has.

xjm’s picture

xjm
she/her
Primary language English
commented

That sounds like a good idea to me. I'll come up with a patch.

xjm’s picture

xjm
she/her
Primary language English
commented

As I was testing my patch, I noticed that the directories are not, in fact, created with the 0777 permissions. Apparently the permissions argument is set against the current umask(), and Apache inherits the umask of the user that starts the process, typically 0022. So by default directories will be created as 0755 unless the user has already modified the environment. (This still might be more permissive than the user's chosen export directory, though.)

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new drush_views_743970-4.patch1.04 KB

Attached patch creates the directory with the permissions of the parent directory, if it exists, or of the Drupal directory if it does not.

xjm’s picture

xjm
she/her
Primary language English
commented

Tested the patch; it works properly on my system.

pescetti’s picture

pescetti commented
Assigned: Unassigned » pescetti
Status: Needs review » Fixed

Thanks, applied.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.