Skip to content

Commit

Permalink
Merge pull request #162 from sminnee/invalid-upload-better-error
Browse files Browse the repository at this point in the history
FIX: Better error message for invalid upload
  • Loading branch information
robbieaverill authored Oct 4, 2018
2 parents e7c45d9 + 40c7a0a commit 787a48b
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 82 deletions.
2 changes: 1 addition & 1 deletion lang/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
6 changes: 5 additions & 1 deletion src/Storage/DBFile.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
41 changes: 24 additions & 17 deletions src/Upload_Validator.php
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
Expand Down Expand Up @@ -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);
}

Expand All @@ -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 '';
}

/**
Expand Down Expand Up @@ -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',
Expand All @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/php/FileTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
43 changes: 25 additions & 18 deletions tests/php/UploadTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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(
Expand All @@ -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]');
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -434,15 +434,22 @@ public function testUploadDeniesNoExtensionFilesIfNoEmptyStringSetForValidatorEx
'error' => UPLOAD_ERR_OK,
);

$v = new UploadTest\Validator();
// 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()
Expand Down Expand Up @@ -533,7 +540,7 @@ public function testUploadFileWithNoExtensionTwiceAppendsNumber()
'error' => UPLOAD_ERR_OK,
);

$v = new UploadTest\Validator();
$v = new Upload_Validator();
$v->setAllowedExtensions(array(''));

// test upload into default folder
Expand Down Expand Up @@ -591,7 +598,7 @@ public function testReplaceFile()
'error' => UPLOAD_ERR_OK,
);

$v = new UploadTest\Validator();
$v = new Upload_Validator();
$v->setAllowedExtensions(array(''));

// test upload into default folder
Expand Down Expand Up @@ -650,7 +657,7 @@ public function testReplaceFileWithLoadIntoFile()
'error' => UPLOAD_ERR_OK,
);

$v = new UploadTest\Validator();
$v = new Upload_Validator();

// test upload into default folder
$u = new Upload();
Expand Down Expand Up @@ -731,7 +738,7 @@ public function testDeleteResampledImagesOnUpload()
'error' => UPLOAD_ERR_OK,
);

$v = new UploadTest\Validator();
$v = new Upload_Validator();

// test upload into default folder
$u = new Upload();
Expand Down Expand Up @@ -772,7 +779,7 @@ public function testFileVersioningWithAnExistingFile()
'error' => UPLOAD_ERR_OK,
);

$v = new UploadTest\Validator();
$v = new Upload_Validator();

// test upload into default folder
$u = new Upload();
Expand Down
44 changes: 0 additions & 44 deletions tests/php/UploadTest/Validator.php

This file was deleted.

0 comments on commit 787a48b

Please sign in to comment.