From e456de11b03f856599b9fc09c276fdf07ebb2541 Mon Sep 17 00:00:00 2001 From: Nick Date: Mon, 8 Jan 2024 15:49:41 +1300 Subject: [PATCH] Fix clobbering of the upload size validation (#10059) * Fix clobbering of the upload size validation When the validation is set here like this, it overrides validation which has already been setup with a simple '*' rule for the size based on PHP. If you've defined in the sites yml config something like SilverStripe\Assets\Upload_Validator: default_max_file_size: '[image]': '2m' '*' : '1m' then it will not be respected. If you review SilverStripe\Assets\Upload_Validator and check the getAllowedMaxFileSize method, you'll see the sizing will be populated (if it hasn't been done before). You can see it fail by; - Setup a new SilverStripe site. - Set your PHP to allow max post / max upload size of 10mb. - Add the above config to your sites yml file and flush. - In the CMS you'll be able to upload a 5MB file, when you shouldn't. * Test that FileField will use size validation if defined Couple of tests which prove a fix so the FileField and others will use the default_max_file_size setting * Fix variable name in last commit This is what happens when you refactor in the github window. Fix the variable names. This will get squashed once merged. * Updates the pr - white space and non deprecated method for byte conversion Remove extra white space to appease the CS. Use the non deprecated method for memstring2bytes * White space fixes for the phpcs White space fixes for the phpcs * Ensure that "memstring2bytes" can handle if an empty or value with no number is passed in * DEP Bump assets constraint to ensure that change is also pulled in --------- Co-authored-by: Guy Sartorelli --- composer.json | 2 +- src/Core/Convert.php | 4 ++++ src/Forms/UploadReceiver.php | 5 ---- tests/php/Core/ConvertTest.php | 6 ++++- tests/php/Forms/FileFieldTest.php | 38 +++++++++++++++++++++++++++++++ 5 files changed, 48 insertions(+), 7 deletions(-) diff --git a/composer.json b/composer.json index a74b1fe35e5..ba9b93f1b0b 100644 --- a/composer.json +++ b/composer.json @@ -36,7 +36,7 @@ "psr/http-message": "^1", "sebastian/diff": "^4.0", "silverstripe/config": "^2", - "silverstripe/assets": "^2", + "silverstripe/assets": "^2.2", "silverstripe/vendor-plugin": "^2", "sminnee/callbacklist": "^0.1.1", "symfony/cache": "^6.1", diff --git a/src/Core/Convert.php b/src/Core/Convert.php index 0a625bb8664..c4140ad863c 100644 --- a/src/Core/Convert.php +++ b/src/Core/Convert.php @@ -451,6 +451,10 @@ public static function memstring2bytes($memString) // Remove non-numeric characters from the size $size = preg_replace('/[^0-9\.\-]/', '', $memString ?? ''); + if (empty($size)) { + return 0; + } + if ($unit) { // Find the position of the unit in the ordered string which is the power // of magnitude to multiply a kilobyte by diff --git a/src/Forms/UploadReceiver.php b/src/Forms/UploadReceiver.php index 9d745b0d456..50eac94e305 100644 --- a/src/Forms/UploadReceiver.php +++ b/src/Forms/UploadReceiver.php @@ -46,11 +46,6 @@ protected function constructUploadReceiver() $this->getValidator()->setAllowedExtensions( array_filter(File::config()->allowed_extensions ?? []) ); - - // get the lower max size - $maxUpload = Convert::memstring2bytes(ini_get('upload_max_filesize')); - $maxPost = Convert::memstring2bytes(ini_get('post_max_size')); - $this->getValidator()->setAllowedMaxFileSize(min($maxUpload, $maxPost)); } /** diff --git a/tests/php/Core/ConvertTest.php b/tests/php/Core/ConvertTest.php index a4d0d73d055..0eef687a550 100644 --- a/tests/php/Core/ConvertTest.php +++ b/tests/php/Core/ConvertTest.php @@ -437,7 +437,10 @@ public function memString2BytesProvider() 'mbytes' => ['512 mbytes', 512 * 1024 * 1024], 'megabytes' => ['512 megabytes', 512 * 1024 * 1024], 'giga' => ['1024g', 1024 * 1024 * 1024 * 1024], - 'G' => ['1024G', 1024 * 1024 * 1024 * 1024] + 'G' => ['1024G', 1024 * 1024 * 1024 * 1024], + 'blank' => ['', 0], + 'null' => [null, 0], + 'no_numbers' => ['k', 0], ]; } @@ -459,6 +462,7 @@ public function testBytes2MemString($bytes, $expected) public function bytes2MemStringProvider() { return [ + [0, '0B'], [200, '200B'], [2 * 1024, '2K'], [512 * 1024 * 1024, '512M'], diff --git a/tests/php/Forms/FileFieldTest.php b/tests/php/Forms/FileFieldTest.php index f2f3a2c4f53..18e397d30f5 100644 --- a/tests/php/Forms/FileFieldTest.php +++ b/tests/php/Forms/FileFieldTest.php @@ -3,6 +3,7 @@ namespace SilverStripe\Forms\Tests; use ReflectionMethod; +use SilverStripe\Core\Convert; use SilverStripe\Assets\Upload_Validator; use SilverStripe\Dev\FunctionalTest; use SilverStripe\Control\Controller; @@ -162,4 +163,41 @@ public function testUploadMissingRequiredFile() 'A null value was passed as parameter for an uploaded file, but the validator returned true' ); } + + /** + * Test the file size validation will use the PHP max size setting if + * no config for the Upload_Validator::default_max_file_size has been defined + */ + public function testWeWillDefaultToPHPMaxUploadSizingForValidation() + { + // These 3 lines are how SilverStripe works out the default max upload size as defined in Upload_Validator + $phpMaxUpload = Convert::memstring2bytes(ini_get('upload_max_filesize')); + $maxPost = Convert::memstring2bytes(ini_get('post_max_size')); + $defaultUploadSize = min($phpMaxUpload, $maxPost); + + $fileField = new FileField('DemoField'); + + $this->assertEquals($defaultUploadSize, $fileField->getValidator()->getAllowedMaxFileSize('jpg')); + $this->assertEquals($defaultUploadSize, $fileField->getValidator()->getAllowedMaxFileSize('png')); + } + + /** + * Test the file size validation will use the default_max_file_size validation config if defined + */ + public function testWeUseConfigForSizingIfDefined() + { + $configMaxFileSizes = [ + 'jpg' => $jpgSize = '2m', + '*' => $defaultSize = '1m', + ]; + + Upload_Validator::config()->set('default_max_file_size', $configMaxFileSizes); + + $fileField = new FileField('DemoField'); + + $this->assertEquals(Convert::memstring2bytes($jpgSize), $fileField->getValidator()->getAllowedMaxFileSize('jpg')); + + // PNG is not explicitly defined in config, so would fall back to * + $this->assertEquals(Convert::memstring2bytes($defaultSize), $fileField->getValidator()->getAllowedMaxFileSize('png')); + } }