Permalink
Browse files

security #24993 Ensure that submitted data are uploaded files (xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

Ensure that submitted data are uploaded files

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

<!--
- Bug fixes must be submitted against the lowest branch where they apply
  (lowest branches are regularly merged to upper ones so they get the fixes too).
- Features and deprecations must be submitted against the master branch.
- Please fill in this template according to the PR you're about to submit.
- Replace this comment by a description of what your PR is solving.
-->

Commits
-------

f9e210c ensure that submitted data are uploaded files
  • Loading branch information...
fabpot committed Nov 16, 2017
2 parents 07fc11c + f9e210c commit 0a1ea85998051518d4c8cb790d795b7293d0a630
View
@@ -39,6 +39,8 @@ Router
Form
----
* the `isFileUpload()` method was added to the `RequestHandlerInterface`
* In form types and extension overriding the "setDefaultOptions" of the
AbstractType or AbstractTypeExtension has been deprecated in favor of
overriding the new "configureOptions" method.
@@ -1,6 +1,11 @@
CHANGELOG
=========
2.7.38
------
* [BC BREAK] the `isFileUpload()` method was added to the `RequestHandlerInterface`
2.7.0
-----
@@ -27,20 +27,35 @@ class FileType extends AbstractType
*/
public function buildForm(FormBuilderInterface $builder, array $options)
{
if ($options['multiple']) {
$builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $event) {
$form = $event->getForm();
$data = $event->getData();
$builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $event) use ($options) {
$form = $event->getForm();
$requestHandler = $form->getConfig()->getRequestHandler();
$data = null;
if ($options['multiple']) {
$data = array();
foreach ($event->getData() as $file) {
if ($requestHandler->isFileUpload($file)) {
$data[] = $file;
}
}
// submitted data for an input file (not required) without choosing any file
if (array(null) === $data) {
if (array(null) === $data || array() === $data) {
$emptyData = $form->getConfig()->getEmptyData();
$data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData;
$event->setData($data);
}
});
}
$event->setData($data);
} elseif (!$requestHandler->isFileUpload($event->getData())) {
$emptyData = $form->getConfig()->getEmptyData();
$data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData;
$event->setData($data);
}
});
}
/**
@@ -16,6 +16,7 @@
use Symfony\Component\Form\FormInterface;
use Symfony\Component\Form\RequestHandlerInterface;
use Symfony\Component\Form\Util\ServerParams;
use Symfony\Component\HttpFoundation\File\File;
use Symfony\Component\HttpFoundation\Request;
/**
@@ -28,9 +29,6 @@ class HttpFoundationRequestHandler implements RequestHandlerInterface
{
private $serverParams;
/**
* {@inheritdoc}
*/
public function __construct(ServerParams $serverParams = null)
{
$this->serverParams = $serverParams ?: new ServerParams();
@@ -109,4 +107,9 @@ public function handleRequest(FormInterface $form, $request = null)
$form->submit($data, 'PATCH' !== $method);
}
public function isFileUpload($data)
{
return $data instanceof File;
}
}
@@ -23,14 +23,6 @@ class NativeRequestHandler implements RequestHandlerInterface
{
private $serverParams;
/**
* {@inheritdoc}
*/
public function __construct(ServerParams $params = null)
{
$this->serverParams = $params ?: new ServerParams();
}
/**
* The allowed keys of the $_FILES array.
*/
@@ -42,6 +34,11 @@ public function __construct(ServerParams $params = null)
'type',
);
public function __construct(ServerParams $params = null)
{
$this->serverParams = $params ?: new ServerParams();
}
/**
* {@inheritdoc}
*/
@@ -121,6 +118,14 @@ public function handleRequest(FormInterface $form, $request = null)
$form->submit($data, 'PATCH' !== $method);
}
public function isFileUpload($data)
{
// POST data will always be strings or arrays of strings. Thus, we can be sure
// that the submitted data is a file upload if the "error" value is an integer
// (this value must have been injected by PHP itself).
return is_array($data) && isset($data['error']) && is_int($data['error']);
}
/**
* Returns the method used to submit the request to the server.
*
@@ -25,4 +25,11 @@
* @param mixed $request The current request
*/
public function handleRequest(FormInterface $form, $request = null);
/**
* @param mixed $data
*
* @return bool
*/
public function isFileUpload($data);
}
@@ -353,12 +353,24 @@ public function getPostMaxSizeFixtures()
);
}
public function testUploadedFilesAreAccepted()
{
$this->assertTrue($this->requestHandler->isFileUpload($this->getMockFile()));
}
public function testInvalidFilesAreRejected()
{
$this->assertFalse($this->requestHandler->isFileUpload($this->getInvalidFile()));
}
abstract protected function setRequestData($method, $data, $files = array());
abstract protected function getRequestHandler();
abstract protected function getMockFile($suffix = '');
abstract protected function getInvalidFile();
protected function getMockForm($name, $method = null, $compound = true)
{
$config = $this->getMockBuilder('Symfony\Component\Form\FormConfigInterface')->getMock();
@@ -11,6 +11,11 @@
namespace Symfony\Component\Form\Tests\Extension\Core\Type;
use Symfony\Component\Form\Extension\HttpFoundation\HttpFoundationRequestHandler;
use Symfony\Component\Form\NativeRequestHandler;
use Symfony\Component\Form\RequestHandlerInterface;
use Symfony\Component\HttpFoundation\File\UploadedFile;
class FileTypeTest extends BaseTypeTest
{
const TESTED_TYPE = 'file';
@@ -28,40 +33,49 @@ public function testSetData()
$this->assertSame($data, $form->getData());
}
public function testSubmit()
/**
* @dataProvider requestHandlerProvider
*/
public function testSubmit(RequestHandlerInterface $requestHandler)
{
$form = $this->factory->createBuilder(static::TESTED_TYPE)->getForm();
$data = $this->createUploadedFileMock('abcdef', 'original.jpg', true);
$form = $this->factory->createBuilder(static::TESTED_TYPE)->setRequestHandler($requestHandler)->getForm();
$data = $this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo', 'foo.jpg');
$form->submit($data);
$this->assertSame($data, $form->getData());
}
public function testSetDataMultiple()
/**
* @dataProvider requestHandlerProvider
*/
public function testSetDataMultiple(RequestHandlerInterface $requestHandler)
{
$form = $this->factory->createBuilder(static::TESTED_TYPE, null, array(
'multiple' => true,
))->getForm();
))->setRequestHandler($requestHandler)->getForm();
$data = array(
$this->createUploadedFileMock('abcdef', 'first.jpg', true),
$this->createUploadedFileMock('zyxwvu', 'second.jpg', true),
$this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo', 'foo.jpg'),
$this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo2', 'foo2.jpg'),
);
$form->setData($data);
$this->assertSame($data, $form->getData());
}
public function testSubmitMultiple()
/**
* @dataProvider requestHandlerProvider
*/
public function testSubmitMultiple(RequestHandlerInterface $requestHandler)
{
$form = $this->factory->createBuilder(static::TESTED_TYPE, null, array(
'multiple' => true,
))->getForm();
))->setRequestHandler($requestHandler)->getForm();
$data = array(
$this->createUploadedFileMock('abcdef', 'first.jpg', true),
$this->createUploadedFileMock('zyxwvu', 'second.jpg', true),
$this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo', 'foo.jpg'),
$this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo2', 'foo2.jpg'),
);
$form->submit($data);
@@ -72,11 +86,14 @@ public function testSubmitMultiple()
$this->assertArrayHasKey('multiple', $view->vars['attr']);
}
public function testDontPassValueToView()
/**
* @dataProvider requestHandlerProvider
*/
public function testDontPassValueToView(RequestHandlerInterface $requestHandler)
{
$form = $this->factory->create(static::TESTED_TYPE);
$form = $this->factory->createBuilder(static::TESTED_TYPE)->setRequestHandler($requestHandler)->getForm();
$form->submit(array(
'file' => $this->createUploadedFileMock('abcdef', 'original.jpg', true),
'file' => $this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo', 'foo.jpg'),
));
$this->assertEquals('', $form->createView()->vars['value']);
@@ -108,29 +125,59 @@ public function testSubmitNullWhenMultiple()
$this->assertSame(array(), $form->getViewData());
}
private function createUploadedFileMock($name, $originalName, $valid)
/**
* @dataProvider requestHandlerProvider
*/
public function testSubmittedFilePathsAreDropped(RequestHandlerInterface $requestHandler)
{
$file = $this
->getMockBuilder('Symfony\Component\HttpFoundation\File\UploadedFile')
->setConstructorArgs(array(__DIR__.'/../../../Fixtures/foo', 'foo'))
->getMock()
;
$file
->expects($this->any())
->method('getBasename')
->will($this->returnValue($name))
;
$file
->expects($this->any())
->method('getClientOriginalName')
->will($this->returnValue($originalName))
;
$file
->expects($this->any())
->method('isValid')
->will($this->returnValue($valid))
;
return $file;
$form = $this->factory->createBuilder(static::TESTED_TYPE)->setRequestHandler($requestHandler)->getForm();
$form->submit('file:///etc/passwd');
$this->assertNull($form->getData());
$this->assertNull($form->getNormData());
$this->assertSame('', $form->getViewData());
}
/**
* @dataProvider requestHandlerProvider
*/
public function testMultipleSubmittedFilePathsAreDropped(RequestHandlerInterface $requestHandler)
{
$form = $this->factory
->createBuilder(static::TESTED_TYPE, null, array(
'multiple' => true,
))
->setRequestHandler($requestHandler)
->getForm();
$form->submit(array(
'file:///etc/passwd',
$this->createUploadedFileMock(new HttpFoundationRequestHandler(), __DIR__.'/../../../Fixtures/foo', 'foo.jpg'),
$this->createUploadedFileMock(new NativeRequestHandler(), __DIR__.'/../../../Fixtures/foo2', 'foo2.jpg'),
));
$this->assertCount(1, $form->getData());
}
public function requestHandlerProvider()
{
return array(
array(new HttpFoundationRequestHandler()),
array(new NativeRequestHandler()),
);
}
private function createUploadedFileMock(RequestHandlerInterface $requestHandler, $path, $originalName)
{
if ($requestHandler instanceof HttpFoundationRequestHandler) {
return new UploadedFile($path, $originalName, null, 10, null, true);
}
return array(
'name' => $originalName,
'error' => 0,
'type' => 'text/plain',
'tmp_name' => $path,
'size' => 10,
);
}
}
@@ -51,4 +51,9 @@ protected function getMockFile($suffix = '')
{
return new UploadedFile(__DIR__.'/../../Fixtures/foo'.$suffix, 'foo'.$suffix);
}
protected function getInvalidFile()
{
return 'file:///etc/passwd';
}
}
@@ -216,4 +216,15 @@ protected function getMockFile($suffix = '')
'size' => 100,
);
}
protected function getInvalidFile()
{
return array(
'name' => 'upload.txt',
'type' => 'text/plain',
'tmp_name' => 'owfdskjasdfsa',
'error' => '0',
'size' => '100',
);
}
}

0 comments on commit 0a1ea85

Please sign in to comment.