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.
silverstripe · Oct 3, 2018 · 40c7a0a · 40c7a0a
1 parent fbd8843
commit 40c7a0a
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 24 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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()