Merge pull request #5601 from BookStackApp/file_permissions

Images: Changed how new image permissions are set

Author: ssddanbrown

app/Config/filesystems.php

@@ -32,7 +32,6 @@
         'local' => [
             'driver'     => 'local',
             'root'       => public_path(),
-            'visibility' => 'public',
             'serve'      => false,
             'throw'      => true,
         ],
@@ -47,7 +46,6 @@
         'local_secure_images' => [
             'driver'     => 'local',
             'root'       => storage_path('uploads/images/'),
-            'visibility' => 'public',
             'serve'      => false,
             'throw'      => true,
         ],

app/Uploads/ImageStorageDisk.php

@@ -5,6 +5,8 @@
 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 @@ public function put(string $path, string $data, bool $makePublic = false): void
         $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}");
+            }
         }
     }