Merge pull request #6056 from open-sausages/pulls/4.0/fix-file-validation

BUG Fix invalid file uploads not being validated

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());
+		// Check file upload
-		if (isset($this->tmpFile['tmp_name']) && !is_uploaded_file($this->tmpFile['tmp_name']) && !$isRunningTests) {
+		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;
+	}
+
 }

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();