From fbd88434cfc89eac7d75e34cdcc48f97821198ff Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Tue, 4 Sep 2018 16:42:26 +1200 Subject: [PATCH 1/2] FIX: Remove unnecessary UploadTest\Validator Its use-case had been previously replaced by the use_is_uploaded_file config option --- tests/php/UploadTest.php | 28 +++++++++---------- tests/php/UploadTest/Validator.php | 44 ------------------------------ 2 files changed, 14 insertions(+), 58 deletions(-) delete mode 100644 tests/php/UploadTest/Validator.php diff --git a/tests/php/UploadTest.php b/tests/php/UploadTest.php index bd11ef9d..288b4a12 100644 --- a/tests/php/UploadTest.php +++ b/tests/php/UploadTest.php @@ -74,7 +74,7 @@ public function testUpload() 'error' => UPLOAD_ERR_OK, ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); // test upload into default folder $u1 = new Upload(); @@ -134,7 +134,7 @@ public function testAllowedFilesize() // test upload into default folder $u1 = new Upload(); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); $v->setAllowedMaxFileSize(array('txt' => 10)); $u1->setValidator($v); @@ -261,7 +261,7 @@ public function testGetAllowedMaxFileSize() 'txt' => 1000 ); Upload_Validator::config()->set('default_max_file_size', $configMaxFileSizes); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); $retrievedSize = $v->getAllowedMaxFileSize('[image]'); $this->assertEquals( @@ -282,7 +282,7 @@ public function testGetAllowedMaxFileSize() '[document]' => 2000, 'txt' => '4k' ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); $v->setAllowedMaxFileSize($maxFileSizes); $retrievedSize = $v->getAllowedMaxFileSize('[document]'); @@ -313,7 +313,7 @@ public function testGetAllowedMaxFileSize() ); // Check a wildcard max file size against a file with an extension - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); $v->setAllowedMaxFileSize(2000); $retrievedSize = $v->getAllowedMaxFileSize('.jpg'); @@ -344,7 +344,7 @@ public function testAllowedSizeOnFileWithNoExtension() 'error' => UPLOAD_ERR_OK, ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); $v->setAllowedMaxFileSize(array('' => 10)); // test upload into default folder @@ -373,7 +373,7 @@ public function testUploadDoesNotAllowUnknownExtension() 'error' => UPLOAD_ERR_OK, ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); $v->setAllowedExtensions(array('txt')); // test upload into default folder @@ -402,7 +402,7 @@ public function testUploadAcceptsAllowedExtension() 'error' => UPLOAD_ERR_OK, ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); $v->setAllowedExtensions(array('txt')); // test upload into default folder @@ -434,7 +434,7 @@ public function testUploadDeniesNoExtensionFilesIfNoEmptyStringSetForValidatorEx 'error' => UPLOAD_ERR_OK, ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); $v->setAllowedExtensions(array('txt')); // test upload into default folder @@ -533,7 +533,7 @@ public function testUploadFileWithNoExtensionTwiceAppendsNumber() 'error' => UPLOAD_ERR_OK, ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); $v->setAllowedExtensions(array('')); // test upload into default folder @@ -591,7 +591,7 @@ public function testReplaceFile() 'error' => UPLOAD_ERR_OK, ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); $v->setAllowedExtensions(array('')); // test upload into default folder @@ -650,7 +650,7 @@ public function testReplaceFileWithLoadIntoFile() 'error' => UPLOAD_ERR_OK, ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); // test upload into default folder $u = new Upload(); @@ -731,7 +731,7 @@ public function testDeleteResampledImagesOnUpload() 'error' => UPLOAD_ERR_OK, ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); // test upload into default folder $u = new Upload(); @@ -772,7 +772,7 @@ public function testFileVersioningWithAnExistingFile() 'error' => UPLOAD_ERR_OK, ); - $v = new UploadTest\Validator(); + $v = new Upload_Validator(); // test upload into default folder $u = new Upload(); diff --git a/tests/php/UploadTest/Validator.php b/tests/php/UploadTest/Validator.php deleted file mode 100644 index 7c04dbeb..00000000 --- a/tests/php/UploadTest/Validator.php +++ /dev/null @@ -1,44 +0,0 @@ -tmpFile['name']); - // filesize validation - - if (!$this->isValidSize()) { - $ext = (isset($pathInfo['extension'])) ? $pathInfo['extension'] : ''; - $arg = File::format_size($this->getAllowedMaxFileSize($ext)); - $this->errors[] = _t( - 'SilverStripe\\Assets\\File.TOOLARGE', - 'File size is too large, maximum {size} allowed', - 'Argument 1: File size (e.g. 1MB)', - array('size' => $arg) - ); - return false; - } - - // extension validation - if (!$this->isValidExtension()) { - $this->errors[] = _t('SilverStripe\\Assets\\File.INVALIDEXTENSION_SHORT', 'Extension is not allowed'); - return false; - } - - return true; - } -} From 40c7a0aac6390237515cd30d9b23de8e7ad0f5ba Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Tue, 4 Sep 2018 14:26:05 +1200 Subject: [PATCH 2/2] FIX: Better error message for invalid upload FIX: By default, files without extensions are allowed. The invalid-extension upload error message now lists the extension of the file uploaded. Previously, there was a bug that if no allowed-extensions were specified then any file _except_ a no-extension file was allowed. --- lang/en.yml | 2 +- src/Storage/DBFile.php | 6 +++++- src/Upload_Validator.php | 41 +++++++++++++++++++++++----------------- tests/php/FileTest.php | 2 +- tests/php/UploadTest.php | 15 +++++++++++---- 5 files changed, 42 insertions(+), 24 deletions(-) diff --git a/lang/en.yml b/lang/en.yml index 7d3206fb..675c2ac7 100644 --- a/lang/en.yml +++ b/lang/en.yml @@ -11,7 +11,7 @@ en: GifType: 'GIF image - good for diagrams' GzType: 'GZIP compressed file' HtmlType: 'HTML file' - INVALIDEXTENSION_SHORT: 'Extension is not allowed' + INVALIDEXTENSION_SHORT_EXT: 'Extension ''{extension}'' is not allowed' IcoType: 'Icon image' JpgType: 'JPEG image - good for photos' JsType: 'Javascript file' diff --git a/src/Storage/DBFile.php b/src/Storage/DBFile.php index f3d593da..07b34cdc 100644 --- a/src/Storage/DBFile.php +++ b/src/Storage/DBFile.php @@ -506,7 +506,11 @@ public function validate(ValidationResult $result, $filename = null) return true; } - $message = _t('SilverStripe\\Assets\\File.INVALIDEXTENSION_SHORT', 'Extension is not allowed'); + $message = _t( + 'SilverStripe\\Assets\\File.INVALIDEXTENSION_SHORT_EXT', + 'Extension \'{extension}\' is not allowed', + [ 'extension' => strtolower(File::get_file_extension($filename)) ] + ); $result->addError($message); return false; } diff --git a/src/Upload_Validator.php b/src/Upload_Validator.php index 83847f6b..f73933c9 100644 --- a/src/Upload_Validator.php +++ b/src/Upload_Validator.php @@ -112,8 +112,8 @@ public function getAllowedMaxFileSize($ext = null) } } - $ext = strtolower($ext); - if ($ext) { + if ($ext !== null) { + $ext = strtolower($ext); if (isset($this->allowedMaxFileSize[$ext])) { return $this->allowedMaxFileSize[$ext]; } @@ -210,9 +210,7 @@ public function isValidSize() case UPLOAD_ERR_FORM_SIZE: return false; } - $pathInfo = pathinfo($this->tmpFile['name']); - $extension = isset($pathInfo['extension']) ? strtolower($pathInfo['extension']) : null; - $maxSize = $this->getAllowedMaxFileSize($extension); + $maxSize = $this->getAllowedMaxFileSize($this->getFileExtension()); return (!$this->tmpFile['size'] || !$maxSize || (int)$this->tmpFile['size'] < $maxSize); } @@ -237,15 +235,25 @@ public function isFileEmpty() */ public function isValidExtension() { - $pathInfo = pathinfo($this->tmpFile['name']); + return !count($this->allowedExtensions) + || in_array($this->getFileExtension(), $this->allowedExtensions, true); + } - // Special case for filenames without an extension - if (!isset($pathInfo['extension'])) { - return in_array('', $this->allowedExtensions, true); - } else { - return (!count($this->allowedExtensions) - || in_array(strtolower($pathInfo['extension']), $this->allowedExtensions)); + /** + * Return the extension of the uploaded file, in lowercase + * Returns an empty string for files without an extension + * + * @return string + */ + public function getFileExtension() + { + $pathInfo = pathinfo($this->tmpFile['name']); + if (isset($pathInfo['extension'])) { + return strtolower($pathInfo['extension']); } + + // Special case for files without extensions + return ''; } /** @@ -276,9 +284,7 @@ public function validate() // filesize validation if (!$this->isValidSize()) { - $pathInfo = pathinfo($this->tmpFile['name']); - $ext = (isset($pathInfo['extension'])) ? $pathInfo['extension'] : ''; - $arg = File::format_size($this->getAllowedMaxFileSize($ext)); + $arg = File::format_size($this->getAllowedMaxFileSize($this->getFileExtension())); $this->errors[] = _t( 'SilverStripe\\Assets\\File.TOOLARGE', 'Filesize is too large, maximum {size} allowed', @@ -291,8 +297,9 @@ public function validate() // extension validation if (!$this->isValidExtension()) { $this->errors[] = _t( - 'SilverStripe\\Assets\\File.INVALIDEXTENSION_SHORT', - 'Extension is not allowed' + 'SilverStripe\\Assets\\File.INVALIDEXTENSION_SHORT_EXT', + 'Extension \'{extension}\' is not allowed', + [ 'extension' => $this->getFileExtension() ] ); return false; } diff --git a/tests/php/FileTest.php b/tests/php/FileTest.php index 93783a54..e293bedd 100644 --- a/tests/php/FileTest.php +++ b/tests/php/FileTest.php @@ -141,7 +141,7 @@ public function testValidateExtension() $this->assertFalse($result->isValid()); $messages = $result->getMessages(); $this->assertEquals(1, count($messages)); - $this->assertEquals('Extension is not allowed', $messages[0]['message']); + $this->assertEquals('Extension \'php\' is not allowed', $messages[0]['message']); // Valid ext $file->Name = 'asdf.txt'; diff --git a/tests/php/UploadTest.php b/tests/php/UploadTest.php index 288b4a12..104aff2b 100644 --- a/tests/php/UploadTest.php +++ b/tests/php/UploadTest.php @@ -434,15 +434,22 @@ public function testUploadDeniesNoExtensionFilesIfNoEmptyStringSetForValidatorEx 'error' => UPLOAD_ERR_OK, ); + // Upload will work if no special validator is set + $u1 = new Upload(); + $result1 = $u1->loadIntoFile($tmpFile); + $this->assertTrue($result1, 'Load failed because extension was not accepted'); + + // If a validator limiting extensions is applied, then no-extension files are no longer allowed $v = new Upload_Validator(); $v->setAllowedExtensions(array('txt')); // test upload into default folder - $u = new Upload(); - $result = $u->loadIntoFile($tmpFile); + $u2 = new Upload(); + $u2->setValidator($v); + $result2 = $u2->loadIntoFile($tmpFile); - $this->assertFalse($result, 'Load failed because extension was not accepted'); - $this->assertEquals(1, count($u->getErrors()), 'There is a single error of the file extension'); + $this->assertFalse($result2, 'Load failed because extension was not accepted'); + $this->assertEquals(1, count($u2->getErrors()), 'There is a single error of the file extension'); } public function testUploadTarGzFileTwiceAppendsNumber()