From 9a9cd97bc36a55b30798bfa7c9fecbe5ae6ccf89 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 22 Sep 2016 14:03:09 +1200 Subject: [PATCH] BUG Fix invalid file uploads not being validated --- Assets/Upload_Validator.php | 52 ++++++++++++++++++++--- tests/filesystem/UploadTest.php | 74 +++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 6 deletions(-) diff --git a/Assets/Upload_Validator.php b/Assets/Upload_Validator.php index 86967a8f6..bc83bbd89 100644 --- a/Assets/Upload_Validator.php +++ b/Assets/Upload_Validator.php @@ -89,7 +89,7 @@ class Upload_Validator if (empty($this->allowedMaxFileSize)) { // Set default max file sizes if there isn't $fileSize = Config::inst()->get('SilverStripe\\Assets\\Upload_Validator', 'default_max_file_size'); - if (isset($fileSize)) { + if ($fileSize) { $this->setAllowedMaxFileSize($fileSize); } else { // When no default is present, use maximum set by PHP @@ -191,12 +191,31 @@ class Upload_Validator */ public function isValidSize() { + // If file was blocked via PHP for being excessive size, shortcut here + switch ($this->tmpFile['error']) { + case UPLOAD_ERR_INI_SIZE: + case UPLOAD_ERR_FORM_SIZE: + return false; + } $pathInfo = pathinfo($this->tmpFile['name']); $extension = isset($pathInfo['extension']) ? strtolower($pathInfo['extension']) : null; $maxSize = $this->getAllowedMaxFileSize($extension); return (!$this->tmpFile['size'] || !$maxSize || (int)$this->tmpFile['size'] < $maxSize); } + /** + * Determine if this file is valid but empty + * + * @return bool + */ + public function isFileEmpty() { + // Don't check file size for errors + if ($this->tmpFile['error'] !== UPLOAD_ERR_OK) { + return false; + } + return empty($this->tmpFile['size']); + } + /** * Determines if the temporary file has a valid extension * An empty string in the validation map indicates files without an extension. @@ -225,25 +244,25 @@ class Upload_Validator public function validate() { // we don't validate for empty upload fields yet - if (empty($this->tmpFile['name']) || empty($this->tmpFile['tmp_name'])) { + if (empty($this->tmpFile['name'])) { return true; } - $isRunningTests = (class_exists('SilverStripe\\Dev\\SapphireTest', false) && SapphireTest::is_running_test()); - if (isset($this->tmpFile['tmp_name']) && !is_uploaded_file($this->tmpFile['tmp_name']) && !$isRunningTests) { + // Check file upload + if (!$this->isValidUpload()) { $this->errors[] = _t('File.NOVALIDUPLOAD', 'File is not a valid upload'); return false; } // Check file isn't empty - if (empty($this->tmpFile['size']) || !filesize($this->tmpFile['tmp_name'])) { + if ($this->isFileEmpty()) { $this->errors[] = _t('File.NOFILESIZE', 'Filesize is zero bytes.'); return false; } - $pathInfo = pathinfo($this->tmpFile['name']); // filesize validation if (!$this->isValidSize()) { + $pathInfo = pathinfo($this->tmpFile['name']); $ext = (isset($pathInfo['extension'])) ? $pathInfo['extension'] : ''; $arg = File::format_size($this->getAllowedMaxFileSize($ext)); $this->errors[] = _t( @@ -269,4 +288,25 @@ class Upload_Validator return true; } + /** + * Check that a valid file was given for upload (ignores file size) + * + * @return bool + */ + public function isValidUpload() { + // Check file upload + if ($this->tmpFile['error'] === UPLOAD_ERR_NO_FILE) { + return false; + } + + // Check if file is valid uploaded (with exception for unit testing) + // Note that some "max file size" errors leave "temp_name" empty, so don't fail on this. + $isRunningTests = (class_exists('SilverStripe\\Dev\\SapphireTest', false) && SapphireTest::is_running_test()); + if (!empty($this->tmpFile['tmp_name']) && !is_uploaded_file($this->tmpFile['tmp_name']) && !$isRunningTests) { + return false; + } + + return true; + } + } diff --git a/tests/filesystem/UploadTest.php b/tests/filesystem/UploadTest.php index 6f91bdf8b..76ae6f2d8 100644 --- a/tests/filesystem/UploadTest.php +++ b/tests/filesystem/UploadTest.php @@ -154,6 +154,80 @@ class UploadTest extends SapphireTest { $this->assertFalse($result, 'Load failed because size was too big'); } + public function testPHPUploadErrors() { + $configMaxFileSizes = ['*' => '1k']; + Config::inst()->update( + 'SilverStripe\\Assets\\Upload_Validator', + 'default_max_file_size', + $configMaxFileSizes + ); + // create tmp file + $tmpFileName = 'myfile.jpg'; + $tmpFilePath = TEMP_FOLDER . '/' . $tmpFileName; + $tmpFileContent = ''; + for($i=0; $i<100; $i++) $tmpFileContent .= '0'; + file_put_contents($tmpFilePath, $tmpFileContent); + + // Build file + $upload = new Upload(); + $tmpFile = array( + 'name' => $tmpFileName, + 'type' => '', + 'tmp_name' => $tmpFilePath, + 'size' => filesize($tmpFilePath), + 'error' => UPLOAD_ERR_OK, + ); + + // Test ok + $this->assertTrue($upload->validate($tmpFile)); + + // Test zero size file + $upload->clearErrors(); + $tmpFile['size'] = 0; + $this->assertFalse($upload->validate($tmpFile)); + $this->assertContains( + _t('File.NOFILESIZE', 'Filesize is zero bytes.'), + $upload->getErrors() + ); + + // Test file too large + $upload->clearErrors(); + $tmpFile['error'] = UPLOAD_ERR_INI_SIZE; + $this->assertFalse($upload->validate($tmpFile)); + $this->assertContains( + _t( + 'File.TOOLARGE', + 'Filesize is too large, maximum {size} allowed', + 'Argument 1: Filesize (e.g. 1MB)', + array('size' => '1 KB') + ), + $upload->getErrors() + ); + + // Test form size + $upload->clearErrors(); + $tmpFile['error'] = UPLOAD_ERR_FORM_SIZE; + $this->assertFalse($upload->validate($tmpFile)); + $this->assertContains( + _t( + 'File.TOOLARGE', + 'Filesize is too large, maximum {size} allowed', + 'Argument 1: Filesize (e.g. 1MB)', + array('size' => '1 KB') + ), + $upload->getErrors() + ); + + // Test no file + $upload->clearErrors(); + $tmpFile['error'] = UPLOAD_ERR_NO_FILE; + $this->assertFalse($upload->validate($tmpFile)); + $this->assertContains( + _t('File.NOVALIDUPLOAD', 'File is not a valid upload'), + $upload->getErrors() + ); + } + public function testGetAllowedMaxFileSize() { Config::nest();