Fixes an issue where you could specify a upload validation size large…

…r than the php in allowed value. This will now check to ensure what is set on config wont exeed php limit. This also uses Convert since File::ini2bytes is depricated, added some tests
silverstripe · Sep 8, 2021 · 4445c3d · 4445c3d
1 parent 022f09c
commit 4445c3d
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 15 deletions.
diff --git a/src/Upload_Validator.php b/src/Upload_Validator.php
@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Assets;
 
+use SilverStripe\Core\Convert;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Injector\Injectable;
@@ -112,7 +113,6 @@ public function getLargestAllowedMaxFileSize()
      */
     public function getAllowedMaxFileSize($ext = null)
     {
-
         // Check if there is any defined instance max file sizes
         if (empty($this->allowedMaxFileSize)) {
             // Set default max file sizes if there isn't
@@ -121,9 +121,7 @@ public function getAllowedMaxFileSize($ext = null)
                 $this->setAllowedMaxFileSize($fileSize);
             } else {
                 // When no default is present, use maximum set by PHP
-                $maxUpload = File::ini2bytes(ini_get('upload_max_filesize'));
-                $maxPost = File::ini2bytes(ini_get('post_max_size'));
-                $this->setAllowedMaxFileSize(min($maxUpload, $maxPost));
+                $this->setAllowedMaxFileSize($this->maxUploadSizeFromPHPIni());
             }
         }
 
@@ -162,21 +160,48 @@ public function setAllowedMaxFileSize($rules)
             $finalRules = [];
 
             foreach ($rules as $rule => $value) {
-                if (is_numeric($value)) {
-                    $tmpSize = $value;
-                } else {
-                    $tmpSize = File::ini2bytes($value);
-                }
-
-                $finalRules[$rule] = (int)$tmpSize;
+                $finalRules[$rule] = $this->parseAndVerifyAllowedSize($value);
             }
 
             $this->allowedMaxFileSize = $finalRules;
-        } elseif (is_string($rules)) {
-            $this->allowedMaxFileSize['*'] = File::ini2bytes($rules);
-        } elseif ((int)$rules > 0) {
-            $this->allowedMaxFileSize['*'] = (int)$rules;
+        } else {
+            $this->allowedMaxFileSize['*'] = $this->parseAndVerifyAllowedSize($rules);
+        }
+    }
+
+    /**
+     * Converts values in bytes or INI format to bytes.
+     * Will fall back to the default PHP size if null|0|''
+     * Verify the parsed size will not exceed PHP's ini settings
+     * @param $value
+     * @return int
+     */
+    public function parseAndVerifyAllowedSize($value)
+    {
+        $maxPHPAllows = $this->maxUploadSizeFromPHPIni();
+
+        if (is_numeric($value)) {
+            $tmpSize = (int)$value;
+        } elseif(is_string($value)) {
+            $tmpSize = Convert::memstring2bytes($value);
         }
+
+        if(empty($tmpSize)) {
+            $tmpSize = $maxPHPAllows;
+        }
+
+        return min($maxPHPAllows, $tmpSize);
+    }
+
+    /**
+     * Checks the two ini settings to work out the max upload size php would allow
+     * @return int
+     */
+    public function maxUploadSizeFromPHPIni()
+    {
+        $maxUpload = Convert::memstring2bytes(ini_get('upload_max_filesize'));
+        $maxPost = Convert::memstring2bytes(ini_get('post_max_size'));
+        return min($maxUpload, $maxPost);
     }
 
     /**

diff --git a/tests/php/UploadTest.php b/tests/php/UploadTest.php
@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Assets\Tests;
 
+use SilverStripe\Core\Convert;
 use Silverstripe\Assets\Dev\TestAssetStore;
 use SilverStripe\Assets\File;
 use SilverStripe\Assets\Folder;
@@ -184,6 +185,56 @@ public function testAllowedFilesize()
         $this->assertFalse($result, 'Load failed because size was too big');
     }
 
+    public function testAllowedSizeWontExceedPHPConfig()
+    {
+        $v = new Upload_Validator();
+
+        $maxUploadSize = $v->maxUploadSizeFromPHPIni();
+
+        // Bump up the size by about 2MB.
+        // This would result in a hard PHP error if upload was attempted.
+        $exceedsMaxPHP = $maxUploadSize + 2000000;
+
+        $v->setAllowedMaxFileSize([
+            'jpg' => Convert::bytes2memstring($exceedsMaxPHP),
+            '*' => Convert::bytes2memstring($exceedsMaxPHP),
+        ]);
+
+        // Our config exceeded the ini settings for PHP, so fall back to that size
+        $this->assertEquals($maxUploadSize, $v->getAllowedMaxFileSize('jpg'));
+        $this->assertEquals($maxUploadSize, $v->getAllowedMaxFileSize('png'));
+    }
+
+    public function testWeFallBackToMaxPHPConfigIfEmptyArrayPassedForConfig()
+    {
+        $v = new Upload_Validator();
+        $maxUploadSize = $v->maxUploadSizeFromPHPIni();
+
+        $v->setAllowedMaxFileSize([]);
+
+        // We just fall back to default, which is the max php upload allowed
+        $this->assertEquals($maxUploadSize, $v->getAllowedMaxFileSize('jpg'));
+        $this->assertEquals($maxUploadSize, $v->getAllowedMaxFileSize('png'));
+    }
+
+    public function testWeHandleEmptyOrNullFileSizeValues()
+    {
+        $v = new Upload_Validator();
+        $maxUploadSize = $v->maxUploadSizeFromPHPIni();
+
+        $v->setAllowedMaxFileSize([
+            'jpg' => '50',
+            'png' => 0,
+            'txt' => null,
+            'zip' => '',
+        ]);
+
+        $this->assertEquals(50, $v->getAllowedMaxFileSize('jpg'));
+        $this->assertEquals($maxUploadSize, $v->getAllowedMaxFileSize('png'));
+        $this->assertEquals($maxUploadSize, $v->getAllowedMaxFileSize('txt'));
+        $this->assertEquals($maxUploadSize, $v->getAllowedMaxFileSize('zip'));
+    }
+
     public function testPHPUploadErrors()
     {
         $configMaxFileSizes = ['*' => '1k'];