Images: Changed how new image permissions are set

Removed default public visibility for images at the driver level,
leaving only doing this as a specific action in the logic.
Added try/catch around permission setting so that
permission-incompatible environments won't fatally fail, but instead
log a warning.

Tested via a google cloud storage bucket FUSE mount, mounted under another
user but with open 777 permissions.

Related to #5269

diff --git a/app/Config/filesystems.php b/app/Config/filesystems.php
index 08ae7b047769d13c0c55184b0b6b5a6241c1935a..ab507a2f8c6fae9f923d85624c6956dd589ffdef 100644 (file)
--- a/app/Config/filesystems.php
+++ b/app/Config/filesystems.php
@@ -32,7 +32,6 @@ return [
         'local' => [
             'driver'     => 'local',
             'root'       => public_path(),
-            'visibility' => 'public',
             'serve'      => false,
             'throw'      => true,
         ],
@@ -47,7 +46,6 @@ return [
         'local_secure_images' => [
             'driver'     => 'local',
             'root'       => storage_path('uploads/images/'),
-            'visibility' => 'public',
             'serve'      => false,
             'throw'      => true,
         ],

diff --git a/app/Uploads/ImageStorageDisk.php b/app/Uploads/ImageStorageDisk.php
index da8bacb3447d3ee82df5ab7d0c40ffce9adacb60..8e364831f75ede661cf7798f45c96f49a1b9ad57 100644 (file)
--- a/app/Uploads/ImageStorageDisk.php
+++ b/app/Uploads/ImageStorageDisk.php
@@ -5,6 +5,8 @@ namespace BookStack\Uploads;
 use BookStack\Util\FilePathNormalizer;
 use Illuminate\Contracts\Filesystem\Filesystem;
 use Illuminate\Filesystem\FilesystemAdapter;
+use Illuminate\Support\Facades\Log;
+use League\Flysystem\UnableToSetVisibility;
 use Symfony\Component\HttpFoundation\StreamedResponse;
 
 class ImageStorageDisk
@@ -74,12 +76,19 @@ class ImageStorageDisk
         $path = $this->adjustPathForDisk($path);
         $this->filesystem->put($path, $data);
 
-        // Set visibility when a non-AWS-s3, s3-like storage option is in use.
-        // Done since this call can break s3-like services but desired for other image stores.
-        // Attempting to set ACL during above put request requires different permissions
-        // hence would technically be a breaking change for actual s3 usage.
+        // Set public visibility to ensure public access on S3, or that the file is accessible
+        // to other processes (like web-servers) for local file storage options.
+        // We avoid attempting this for (non-AWS) s3-like systems (even in a try-catch) as
+        // we've always avoided setting permissions for s3-like due to potential issues,
+        // with docs advising setting pre-configured permissions instead.
+        // We also don't do this as the default filesystem/driver level as that can technically
+        // require different ACLs for S3, and this provides us more logical control.
         if ($makePublic && !$this->isS3Like()) {
-            $this->filesystem->setVisibility($path, 'public');
+            try {
+                $this->filesystem->setVisibility($path, 'public');
+            } catch (UnableToSetVisibility $e) {
+                Log::warning("Unable to set visibility for image upload with relative path: {$path}");
+            }
         }
     }