merged branch bamarni/http-uploaded-file (PR #7201)
This PR was merged into the master branch.
Discussion
----------
[2.3] moved a security check in HttpUploadedFile
closes #6802
- [x] fix the testsuite, I've only run the component suite, but it needs to be updated in other places too (according to travis)
Commits
-------
5bb44f5
[HttpFoundation] UploadedFile - moved a security check
This commit is contained in:
commit
2b4cfbdbb7
@ -179,13 +179,15 @@ class UploadedFile extends File
|
||||
/**
|
||||
* Returns whether the file was uploaded successfully.
|
||||
*
|
||||
* @return Boolean True if no error occurred during uploading
|
||||
* @return Boolean True if the file has been uploaded with HTTP and no error occurred.
|
||||
*
|
||||
* @api
|
||||
*/
|
||||
public function isValid()
|
||||
{
|
||||
return $this->error === UPLOAD_ERR_OK;
|
||||
$isOk = $this->error === UPLOAD_ERR_OK;
|
||||
|
||||
return $this->test ? $isOk : $isOk && is_uploaded_file($this->getPathname());
|
||||
}
|
||||
|
||||
/**
|
||||
@ -196,7 +198,7 @@ class UploadedFile extends File
|
||||
*
|
||||
* @return File A File object representing the new file
|
||||
*
|
||||
* @throws FileException if the file has not been uploaded via Http
|
||||
* @throws FileException if, for any reason, the file could not have been moved
|
||||
*
|
||||
* @api
|
||||
*/
|
||||
@ -205,21 +207,21 @@ class UploadedFile extends File
|
||||
if ($this->isValid()) {
|
||||
if ($this->test) {
|
||||
return parent::move($directory, $name);
|
||||
} elseif (is_uploaded_file($this->getPathname())) {
|
||||
$target = $this->getTargetFile($directory, $name);
|
||||
|
||||
if (!@move_uploaded_file($this->getPathname(), $target)) {
|
||||
$error = error_get_last();
|
||||
throw new FileException(sprintf('Could not move the file "%s" to "%s" (%s)', $this->getPathname(), $target, strip_tags($error['message'])));
|
||||
}
|
||||
|
||||
@chmod($target, 0666 & ~umask());
|
||||
|
||||
return $target;
|
||||
}
|
||||
|
||||
$target = $this->getTargetFile($directory, $name);
|
||||
|
||||
if (!@move_uploaded_file($this->getPathname(), $target)) {
|
||||
$error = error_get_last();
|
||||
throw new FileException(sprintf('Could not move the file "%s" to "%s" (%s)', $this->getPathname(), $target, strip_tags($error['message'])));
|
||||
}
|
||||
|
||||
@chmod($target, 0666 & ~umask());
|
||||
|
||||
return $target;
|
||||
}
|
||||
|
||||
throw new FileException(sprintf('The file "%s" has not been uploaded via Http', $this->getPathname()));
|
||||
throw new FileException(sprintf('The file "%s" is not valid', $this->getPathname()));
|
||||
}
|
||||
|
||||
/**
|
||||
|
@ -197,7 +197,8 @@ class UploadedFileTest extends \PHPUnit_Framework_TestCase
|
||||
'original.gif',
|
||||
null,
|
||||
filesize(__DIR__.'/Fixtures/test.gif'),
|
||||
UPLOAD_ERR_OK
|
||||
UPLOAD_ERR_OK,
|
||||
true
|
||||
);
|
||||
|
||||
$this->assertTrue($file->isValid());
|
||||
@ -229,4 +230,17 @@ class UploadedFileTest extends \PHPUnit_Framework_TestCase
|
||||
array(UPLOAD_ERR_EXTENSION),
|
||||
);
|
||||
}
|
||||
|
||||
public function testIsInvalidIfNotHttpUpload()
|
||||
{
|
||||
$file = new UploadedFile(
|
||||
__DIR__.'/Fixtures/test.gif',
|
||||
'original.gif',
|
||||
null,
|
||||
filesize(__DIR__.'/Fixtures/test.gif'),
|
||||
UPLOAD_ERR_OK
|
||||
);
|
||||
|
||||
$this->assertFalse($file->isValid());
|
||||
}
|
||||
}
|
||||
|
@ -114,7 +114,7 @@ class ClientTest extends \PHPUnit_Framework_TestCase
|
||||
|
||||
$files = array(
|
||||
array('tmp_name' => $source, 'name' => 'original', 'type' => 'mime/original', 'size' => 123, 'error' => UPLOAD_ERR_OK),
|
||||
new UploadedFile($source, 'original', 'mime/original', 123, UPLOAD_ERR_OK),
|
||||
new UploadedFile($source, 'original', 'mime/original', 123, UPLOAD_ERR_OK, true),
|
||||
);
|
||||
|
||||
foreach ($files as $file) {
|
||||
@ -147,7 +147,7 @@ class ClientTest extends \PHPUnit_Framework_TestCase
|
||||
|
||||
$file = $this
|
||||
->getMockBuilder('Symfony\Component\HttpFoundation\File\UploadedFile')
|
||||
->setConstructorArgs(array($source, 'original', 'mime/original', 123, UPLOAD_ERR_OK))
|
||||
->setConstructorArgs(array($source, 'original', 'mime/original', 123, UPLOAD_ERR_OK, true))
|
||||
->setMethods(array('getSize'))
|
||||
->getMock()
|
||||
;
|
||||
|
@ -82,7 +82,7 @@ abstract class FileValidatorTest extends \PHPUnit_Framework_TestCase
|
||||
$this->context->expects($this->never())
|
||||
->method('addViolation');
|
||||
|
||||
$file = new UploadedFile($this->path, 'originalName');
|
||||
$file = new UploadedFile($this->path, 'originalName', null, null, null, true);
|
||||
$this->validator->validate($file, new File());
|
||||
}
|
||||
|
||||
|
Reference in New Issue
Block a user