From f9e210cc7bbaa52adc2a6f695cc343854fa43df6 Mon Sep 17 00:00:00 2001 From: Christian Flothmann Date: Fri, 1 Sep 2017 11:39:51 +0200 Subject: [PATCH] ensure that submitted data are uploaded files --- UPGRADE-2.7.md | 2 + src/Symfony/Component/Form/CHANGELOG.md | 5 + .../Form/Extension/Core/Type/FileType.php | 31 +++-- .../HttpFoundationRequestHandler.php | 9 +- .../Component/Form/NativeRequestHandler.php | 21 +-- .../Form/RequestHandlerInterface.php | 7 + .../Form/Tests/AbstractRequestHandlerTest.php | 12 ++ .../Extension/Core/Type/FileTypeTest.php | 121 ++++++++++++------ .../HttpFoundationRequestHandlerTest.php | 5 + .../Form/Tests/NativeRequestHandlerTest.php | 11 ++ 10 files changed, 168 insertions(+), 56 deletions(-) diff --git a/UPGRADE-2.7.md b/UPGRADE-2.7.md index 1cf3d3df7f4dc..533068b970b5e 100644 --- a/UPGRADE-2.7.md +++ b/UPGRADE-2.7.md @@ -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. diff --git a/src/Symfony/Component/Form/CHANGELOG.md b/src/Symfony/Component/Form/CHANGELOG.md index 58b0f3862b23f..6b574ee4c6a92 100644 --- a/src/Symfony/Component/Form/CHANGELOG.md +++ b/src/Symfony/Component/Form/CHANGELOG.md @@ -1,6 +1,11 @@ CHANGELOG ========= +2.7.38 +------ + + * [BC BREAK] the `isFileUpload()` method was added to the `RequestHandlerInterface` + 2.7.0 ----- diff --git a/src/Symfony/Component/Form/Extension/Core/Type/FileType.php b/src/Symfony/Component/Form/Extension/Core/Type/FileType.php index 914c9402fc8e4..fc5b1aae68329 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/FileType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/FileType.php @@ -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); + } + }); } /** diff --git a/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php b/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php index f65ee33f87a0c..368a5a80a1481 100644 --- a/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php +++ b/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php @@ -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; + } } diff --git a/src/Symfony/Component/Form/NativeRequestHandler.php b/src/Symfony/Component/Form/NativeRequestHandler.php index e28c2b4b102cf..f68efe25cfe54 100644 --- a/src/Symfony/Component/Form/NativeRequestHandler.php +++ b/src/Symfony/Component/Form/NativeRequestHandler.php @@ -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. * diff --git a/src/Symfony/Component/Form/RequestHandlerInterface.php b/src/Symfony/Component/Form/RequestHandlerInterface.php index 598a7bb17438c..e6360e449889f 100644 --- a/src/Symfony/Component/Form/RequestHandlerInterface.php +++ b/src/Symfony/Component/Form/RequestHandlerInterface.php @@ -25,4 +25,11 @@ interface RequestHandlerInterface * @param mixed $request The current request */ public function handleRequest(FormInterface $form, $request = null); + + /** + * @param mixed $data + * + * @return bool + */ + public function isFileUpload($data); } diff --git a/src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php b/src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php index 0ef713da1745b..46350659b16a8 100644 --- a/src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php +++ b/src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php @@ -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(); diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php index 98c0a8c304782..5d6fe1e20d668 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php @@ -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, + ); } } diff --git a/src/Symfony/Component/Form/Tests/Extension/HttpFoundation/HttpFoundationRequestHandlerTest.php b/src/Symfony/Component/Form/Tests/Extension/HttpFoundation/HttpFoundationRequestHandlerTest.php index e71a1fdfd8058..9fbe12258311a 100644 --- a/src/Symfony/Component/Form/Tests/Extension/HttpFoundation/HttpFoundationRequestHandlerTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/HttpFoundation/HttpFoundationRequestHandlerTest.php @@ -51,4 +51,9 @@ protected function getMockFile($suffix = '') { return new UploadedFile(__DIR__.'/../../Fixtures/foo'.$suffix, 'foo'.$suffix); } + + protected function getInvalidFile() + { + return 'file:///etc/passwd'; + } } diff --git a/src/Symfony/Component/Form/Tests/NativeRequestHandlerTest.php b/src/Symfony/Component/Form/Tests/NativeRequestHandlerTest.php index 38580063e7195..aba417288a948 100644 --- a/src/Symfony/Component/Form/Tests/NativeRequestHandlerTest.php +++ b/src/Symfony/Component/Form/Tests/NativeRequestHandlerTest.php @@ -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', + ); + } }