fix: validate icons to be images (#1173)

app/Helper.php

@@ -14,15 +14,15 @@ function format_bytes($bytes, bool $is_drive_size = true, string $beforeunit = '
     $btype = ($is_drive_size === true) ? 1000 : 1024;
     $labels = ['B', 'KB', 'MB', 'GB', 'TB'];
     // use 1000 rather than 1024 to simulate HD size not real size
-    for ($x = 0; $bytes >= $btype && $x < (count($labels) - 1); $bytes /= $btype, $x++);
+    for ($x = 0; $bytes >= $btype && $x < (count($labels) - 1); $bytes /= $btype, $x++) ;
     if ($labels[$x] == 'TB') {
-        return round($bytes, 3).$beforeunit.$labels[$x].$afterunit;
+        return round($bytes, 3) . $beforeunit . $labels[$x] . $afterunit;
     } elseif ($labels[$x] == 'GB') {
-        return round($bytes, 2).$beforeunit.$labels[$x].$afterunit;
+        return round($bytes, 2) . $beforeunit . $labels[$x] . $afterunit;
     } elseif ($labels[$x] == 'MB') {
-        return round($bytes, 2).$beforeunit.$labels[$x].$afterunit;
+        return round($bytes, 2) . $beforeunit . $labels[$x] . $afterunit;
     } else {
-        return round($bytes, 0).$beforeunit.$labels[$x].$afterunit;
+        return round($bytes, 0) . $beforeunit . $labels[$x] . $afterunit;
     }
 }
 
@@ -37,11 +37,11 @@ function str_slug($title, string $separator = '-', string $language = 'en'): str
     return Str::slug($title, $separator, $language);
 }
 
-if (! function_exists('str_is')) {
+if (!function_exists('str_is')) {
     /**
      * Determine if a given string matches a given pattern.
      *
-     * @param  string|array  $pattern
+     * @param string|array $pattern
      * @param string $value
      * @return bool
      *
@@ -64,7 +64,7 @@ function get_brightness($hex)
     // $hex = str_replace('#', '', $hex);
     $hex = preg_replace("/[^0-9A-Fa-f]/", '', $hex);
     if (strlen($hex) == 3) {
-        $hex = $hex[0].$hex[0].$hex[1].$hex[1].$hex[2].$hex[2];
+        $hex = $hex[0] . $hex[0] . $hex[1] . $hex[1] . $hex[2] . $hex[2];
     }
 
     $c_r = hexdec(substr($hex, 0, 2));
@@ -97,7 +97,7 @@ function getLinkTargetAttribute(): string
     if ($target === 'current') {
         return '';
     } else {
-        return ' target="'.$target.'"';
+        return ' target="' . $target . '"';
     }
 }
 
@@ -112,10 +112,17 @@ function className($name)
 
 /**
  * @param string $file
+ * @param string $extension
  * @return bool
  */
-function isImage(string $file):bool
+function isImage(string $file, string $extension): bool
 {
+    $allowedExtensions = ['jpg', 'jpeg', 'png', 'bmp', 'gif', 'svg', 'webp'];
+
+    if (!in_array($extension, $allowedExtensions)) {
+        return false;
+    }
+
     $tempFileName = tempnam("/tmp", "image-check-");
     $handle = fopen($tempFileName, "w");
 

app/Http/Controllers/ItemController.php

@@ -141,7 +141,7 @@ class ItemController extends Controller
      */
     public function index(Request $request): View
     {
-        $trash = (bool) $request->input('trash');
+        $trash = (bool)$request->input('trash');
 
         $data['apps'] = Item::ofType('item')->orderBy('title', 'asc')->get();
         $data['trash'] = Item::ofType('item')->onlyTrashed()->get();
@@ -197,6 +197,7 @@ class ItemController extends Controller
      * @param Request $request
      * @param null $id
      * @return Item
+     * @throws ValidationException
      */
     public static function storelogic(Request $request, $id = null): Item
     {
@@ -219,21 +220,18 @@ class ItemController extends Controller
                     "verify_peer_name" => false,
                 ),
             );
+
+            $file = $request->input('icon');
+            $path_parts = pathinfo($file);
+            $extension = $path_parts['extension'];
+
             $contents = file_get_contents($request->input('icon'), false, stream_context_create($options));
 
-            if (!isImage($contents)) {
+            if (!isImage($contents, $extension)) {
                 throw ValidationException::withMessages(['file' => 'Icon must be an image.']);
             }
 
-            if ($application) {
+            $path = 'icons/' . ($application ? $application->icon : md5($contents) . '.' . $extension);
-                $icon = $application->icon;
-            } else {
-                $file = $request->input('icon');
-                $path_parts = pathinfo($file);
-                $icon = md5($contents);
-                $icon .= '.' . $path_parts['extension'];
-            }
-            $path = 'icons/' . $icon;
 
             // Private apps could have here duplicated icons folder
             if (strpos($path, 'icons/icons/') !== false) {
@@ -340,7 +338,7 @@ class ItemController extends Controller
     public function destroy(Request $request, int $id): RedirectResponse
     {
         //
-        $force = (bool) $request->input('force');
+        $force = (bool)$request->input('force');
         if ($force) {
             Item::withTrashed()
                 ->where('id', $id)
@@ -394,11 +392,11 @@ class ItemController extends Controller
         $output['custom'] = null;
 
         $app = Application::single($appid);
-        $output = (array) $app;
+        $output = (array)$app;
 
         $appdetails = Application::getApp($appid);
 
-        if ((bool) $app->enhanced === true) {
+        if ((bool)$app->enhanced === true) {
             // if(!isset($app->config)) { // class based config
             $output['custom'] = className($appdetails->name) . '.config';
             // }
@@ -444,7 +442,7 @@ class ItemController extends Controller
         }
 
         $app_details = new $app();
-        $app_details->config = (object) $data;
+        $app_details->config = (object)$data;
         $app_details->test();
     }
 

tests/Unit/helpers/IsImageTest.php

@@ -9,9 +9,21 @@ class IsImageTest extends TestCase
     /**
      * @return void
      */
-    public function test_isImage_returns_false_when_file_is_not_image()
+    public function test_returns_true_when_file_is_image()
     {
-        $actual = isImage("<?php ?>");
+        $file = file_get_contents(__DIR__ . '/fixtures/heimdall-icon-small.png');
+
+        $actual = isImage($file, 'png');
+
+        $this->assertTrue($actual);
+    }
+
+    /**
+     * @return void
+     */
+    public function test_returns_false_when_file_extension_is_image_but_content_is_not()
+    {
+        $actual = isImage("<?php ?>", "png");
 
         $this->assertFalse($actual);
     }
@@ -19,24 +31,12 @@ class IsImageTest extends TestCase
     /**
      * @return void
      */
-    public function test_isImage_returns_true_when_file_is_image()
+    public function test_returns_false_when_file_extension_is_not_image_but_content_is()
     {
         $file = file_get_contents(__DIR__ . '/fixtures/heimdall-icon-small.png');
 
-        $actual = isImage($file);
+        $actual = isImage($file, 'php');
 
-        $this->assertTrue($actual);
+        $this->assertFalse($actual);
-    }
-
-    /**
-     * @return void
-     */
-    public function test_isImage_returns_false_when_file_is_php_but_png()
-    {
-        $file = file_get_contents(__DIR__ . '/fixtures/heimdall-icon-small-php.php');
-
-        $actual = isImage($file);
-
-        $this->assertTrue($actual);
     }
 }