Fix: image/svg can contain scripts

fisharebest · Oct 8, 2021 · 7729532 · 7729532
1 parent 505f23e
commit 7729532
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 40 deletions.
diff --git a/app/Factories/ImageFactory.php b/app/Factories/ImageFactory.php
@@ -45,9 +45,10 @@
 use function basename;
 use function extension_loaded;
 use function get_class;
+use function implode;
 use function pathinfo;
 use function response;
-use function strlen;
+use function str_contains;
 use function view;
 
 use const PATHINFO_EXTENSION;
@@ -88,26 +89,17 @@ class ImageFactory implements ImageFactoryInterface
     public function fileResponse(FilesystemOperator $filesystem, string $path, bool $download): ResponseInterface
     {
         try {
-            $data = $filesystem->read($path);
-
             try {
-                $content_type = $filesystem->mimeType($path);
+                $mime_type = $filesystem->mimeType($path);
             } catch (UnableToRetrieveMetadata $ex) {
-                $content_type = Mime::DEFAULT_TYPE;
+                $mime_type = Mime::DEFAULT_TYPE;
             }
 
-            $headers = [
-                'Content-Type' => $content_type,
-            ];
-
-            if ($download) {
-                $headers['Content-Disposition'] = 'attachment; filename="' . addcslashes(basename($path), '"');
-            }
+            $filename = $download ? addcslashes(basename($path), '"') : '';
 
-            return response($data, StatusCodeInterface::STATUS_OK, $headers);
-        } catch (FilesystemException | UnableToReadFile $ex) {
-            return $this->replacementImageResponse((string) StatusCodeInterface::STATUS_NOT_FOUND)
-                ->withHeader('X-Thumbnail-Exception', get_class($ex) . ': ' . $ex->getMessage());
+            return $this->imageResponse($filesystem->read($path), $mime_type, $filename);
+        } catch (FileNotFoundException $ex) {
+            return $this->replacementImageResponse((string) StatusCodeInterface::STATUS_NOT_FOUND);
         }
     }
 
@@ -163,29 +155,25 @@ public function thumbnailResponse(
     public function mediaFileResponse(MediaFile $media_file, bool $add_watermark, bool $download): ResponseInterface
     {
         $filesystem = Registry::filesystem()->media($media_file->media()->tree());
-        $filename   = $media_file->filename();
+        $path   = $media_file->filename();
 
         if (!$add_watermark || !$media_file->isImage()) {
-            return $this->fileResponse($filesystem, $filename, $download);
+            return $this->fileResponse($filesystem, $path, $download);
         }
 
         try {
-            $image = $this->imageManager()->make($filesystem->readStream($filename));
-            $image = $this->autorotateImage($image);
-
-            $watermark_image = $this->createWatermark($image->width(), $image->height(), $media_file);
-
-            $image = $this->addWatermark($image, $watermark_image);
-
-            $download_filename = $download ? basename($filename) : '';
-
-            $format  = static::INTERVENTION_FORMATS[$image->mime()] ?? 'jpg';
-            $quality = $this->extractImageQuality($image, static::GD_DEFAULT_IMAGE_QUALITY);
-            $data    = (string) $image->encode($format, $quality);
-
-            return $this->imageResponse($data, $image->mime(), $download_filename);
+            $image     = $this->imageManager()->make($filesystem->readStream($path));
+            $image     = $this->autorotateImage($image);
+            $watermark = $this->createWatermark($image->width(), $image->height(), $media_file);
+            $image     = $this->addWatermark($image, $watermark);
+            $filename  = $download ? basename($path) : '';
+            $format    = static::INTERVENTION_FORMATS[$image->mime()] ?? 'jpg';
+            $quality   = $this->extractImageQuality($image, static::GD_DEFAULT_IMAGE_QUALITY);
+            $data      = (string) $image->encode($format, $quality);
+
+            return $this->imageResponse($data, $image->mime(), $filename);
         } catch (NotReadableException $ex) {
-            return $this->replacementImageResponse(pathinfo($filename, PATHINFO_EXTENSION))
+            return $this->replacementImageResponse(pathinfo($path, PATHINFO_EXTENSION))
                 ->withHeader('X-Image-Exception', $ex->getMessage());
         } catch (FilesystemException | UnableToReadFile $ex) {
             return $this->replacementImageResponse((string) StatusCodeInterface::STATUS_NOT_FOUND);
@@ -337,7 +325,7 @@ public function replacementImageResponse(string $text): ResponseInterface
 
         // We can't send the actual status code, as browsers won't show images with 4xx/5xx.
         return response($svg, StatusCodeInterface::STATUS_OK, [
-            'Content-Type' => 'image/svg+xml',
+            'content-type' => 'image/svg+xml',
         ]);
     }
 
@@ -350,15 +338,20 @@ public function replacementImageResponse(string $text): ResponseInterface
      */
     protected function imageResponse(string $data, string $mime_type, string $filename): ResponseInterface
     {
-        $headers = [
-            'Content-Type' => $mime_type,
-        ];
+        if ($mime_type === 'image/svg+xml' && str_contains($data, '<script')) {
+            return $this->replacementImageResponse('XSS')
+                ->withHeader('X-Image-Exception', 'SVG image blocked due to XSS.');
+        }
+
+        $response = response($data)
+            ->withHeader('content-type', $mime_type);
 
-        if ($filename !== '') {
-            $headers['Content-Disposition'] = 'attachment; filename="' . addcslashes(basename($filename), '"');
+        if ($filename === '') {
+            return $response;
         }
 
-        return response($data, StatusCodeInterface::STATUS_OK, $headers);
+        return $response
+            ->withHeader('content-disposition', 'attachment; filename="' . addcslashes(basename($filename), '"'));
     }
 
     /**

diff --git a/app/Validator.php b/app/Validator.php
@@ -108,6 +108,10 @@ public function isBetween(int $minimum, int $maximum): self
     public function isLocalUrl(string $base_url): self
     {
         $this->rules[] = static function ($value) use ($base_url): ?string {
+            if ($value === null) {
+                return null;
+            }
+
             if (is_string($value)) {
                 $value_info    = parse_url($value);
                 $base_url_info = parse_url($base_url);