FIX Ensure the setAllowedMaxFileSize wont allow config values to exce…

…ed PHP limits (#469)

Fixes an issue where you could specify a upload validation size larger 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

src/Upload_Validator.php

@@ -122,7 +122,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
@@ -131,9 +130,7 @@ public function getAllowedMaxFileSize($ext = null)
                 $this->setAllowedMaxFileSize($fileSize);
             } else {
                 // When no default is present, use maximum set by PHP
-                $maxUpload = Convert::memstring2bytes(ini_get('upload_max_filesize'));
-                $maxPost = Convert::memstring2bytes(ini_get('post_max_size'));
-                $this->setAllowedMaxFileSize(min($maxUpload, $maxPost));
+                $this->setAllowedMaxFileSize($this->maxUploadSizeFromPHPIni());
             }
         }
 
@@ -174,25 +171,49 @@ public function setAllowedMaxFileSize($rules)
             $finalRules = [];
 
             foreach ($rules as $rule => $value) {
-                if (is_numeric($value)) {
-                    $tmpSize = $value;
-                } else {
-                    $tmpSize = Convert::memstring2bytes($value);
-                }
-
-                $finalRules[$rule] = (int)$tmpSize;
+                $finalRules[$rule] = $this->parseAndVerifyAllowedSize($value);
             }
 
             $this->allowedMaxFileSize = $finalRules;
-        } elseif (is_string($rules)) {
-            $this->allowedMaxFileSize['*'] = Convert::memstring2bytes($rules);
-        } elseif ((int)$rules > 0) {
-            $this->allowedMaxFileSize['*'] = (int)$rules;
+        } else {
+            $this->allowedMaxFileSize['*'] = $this->parseAndVerifyAllowedSize($rules);
         }
 
         return $this;
     }
 
+    /**
+     * 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
+     */
+    private function parseAndVerifyAllowedSize($value): int
+    {
+        $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
+     */
+    private function maxUploadSizeFromPHPIni(): int
+    {
+        $maxUpload = Convert::memstring2bytes(ini_get('upload_max_filesize'));
+        $maxPost = Convert::memstring2bytes(ini_get('post_max_size'));
+        return min($maxUpload, $maxPost);
+    }
+
     /**
      * @return array
      */

tests/php/UploadTest.php

@@ -2,6 +2,8 @@
 
 namespace SilverStripe\Assets\Tests;
 
+use ReflectionMethod;
+use SilverStripe\Core\Convert;
 use Silverstripe\Assets\Dev\TestAssetStore;
 use SilverStripe\Assets\File;
 use SilverStripe\Assets\Folder;
@@ -181,6 +183,65 @@ public function testAllowedFilesize()
         $this->assertFalse($result, 'Load failed because size was too big');
     }
 
+    public function testAllowedSizeWontExceedPHPConfig()
+    {
+        $v = new Upload_Validator();
+        $reflectionMethod = new ReflectionMethod($v, 'maxUploadSizeFromPHPIni');
+        $reflectionMethod->setAccessible(true);
+        $maxUploadSize = $reflectionMethod->invoke($v);
+
+        // 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();
+        $reflectionMethod = new ReflectionMethod($v, 'maxUploadSizeFromPHPIni');
+        $reflectionMethod->setAccessible(true);
+        $maxUploadSize = $reflectionMethod->invoke($v);
+
+        $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();
+        $reflectionMethod = new ReflectionMethod($v, 'maxUploadSizeFromPHPIni');
+        $reflectionMethod->setAccessible(true);
+        $maxUploadSize = $reflectionMethod->invoke($v);
+
+        /*
+         The test for the zip extension should be enabled once
+         the Convert::memstring2bytes if fixed to handle empty strings.
+         */
+        $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'];