Permissions of generated files don't work as expected when parent directory is using 771

[hostep]

Hi!

We use this library through a Magento 2 module to generate webp images for images on a Magento webshop system.

However, we seem to have noticed a problem with the file permissions of generated webp images where generated images can't be read by the webserver in our case.

We set the permissions of directories which contain images to 2771. We set the permissions of image files to 0664.

We use "1" on directories as a way to say to the webserver: you can access the directory but not display the contents of the directory.

This module seems to lose the read permissions for the "others" on generated webp image files and sets them to 0660 for some reason. Is there a way to have more control over the file permissions this module is using for generated images?

Thanks! 🙂

[hostep]

Finally found some time to look into this in more depth.

It appears this module is looking at the permissions of the parent directory to determine the file permissions of the converted webp file.

My question is: why make this so complicated? Shouldn't we just take over the permissions from the source file?

I propose to change the code like this (based on version 1.3.9):

diff --git a/src/Converters/Cwebp.php b/src/Converters/Cwebp.php
index 4a5525c..6ddb9e0 100644
--- a/src/Converters/Cwebp.php
+++ b/src/Converters/Cwebp.php
@@ -376,13 +376,12 @@ class Cwebp

         // cwebp sets file permissions to 664 but instead ..
-        // .. $destination's parent folder's permissions should be used (except executable bits)
+        // .. $source file permissions should be used
         if ($success) {
-            $destinationParent = dirname($destination);
-            $fileStatistics = @stat($destinationParent);
+            $fileStatistics = @stat($source);
             if ($fileStatistics !== false) {
-                // Apply same permissions as parent folder but strip off the executable bits
-                $permissions = $fileStatistics['mode'] & 0000666;
+                // Apply same permissions as source file
+                $permissions = $fileStatistics['mode'];
                 @chmod($destination, $permissions);
             }
         }

@rosell-dk: what do you think? Would this be a better solution for setting file permissions?

[hostep]

And here is the diff for the master version:

diff --git a/src/Convert/Converters/Cwebp.php b/src/Convert/Converters/Cwebp.php
index 7c85ecc..172191d 100644
--- a/src/Convert/Converters/Cwebp.php
+++ b/src/Convert/Converters/Cwebp.php
@@ -700,15 +700,13 @@ class Cwebp extends AbstractConverter
         }

         // cwebp sets file permissions to 664 but instead ..
-        // .. $destination's parent folder's permissions should be used (except executable bits)
-        // (or perhaps the current umask instead? https://www.php.net/umask)
+        // .. $this->source file permissions should be used

         if ($success) {
-            $destinationParent = dirname($this->destination);
-            $fileStatistics = stat($destinationParent);
+            $fileStatistics = stat($this->source);
             if ($fileStatistics !== false) {
-                // Apply same permissions as parent folder but strip off the executable bits
-                $permissions = $fileStatistics['mode'] & 0000666;
+                // Apply same permissions as source file
+                $permissions = $fileStatistics['mode'];
                 chmod($this->destination, $permissions);
             }
         } else {

I'm willing to create a pull request for this if we can agree that this is a better solution?

[rosell-dk]

Sounds reasonable. Sorry for not being reacting. Something came in the way, which required my full attention.

[hostep]

No worries 🙂 Thanks for the merge!

Any chance we can see a new version being released in the coming days with this change included? That would be awesome!

[rosell-dk]

No problem

[rosell-dk]

@hostep:I think we should strip off the executable bits, as was done before. Just to be safe. I shall make the change now

[rosell-dk]

@hostep I have just created a new release

[hostep]

Ok, that should be fine I think, thanks!