[HttpFoundation] UploadedFile - moved a security check

Squashed commit of the following: commit b03b32ecc985c4a4f9dc7df2d3336a4cd75aae30 Merge: fb7004b fc70e13 Author: Bilal Amarni <bilal.amarni@gmail.com> Date: Wed Feb 27 11:33:37 2013 +0100 [HttpFoundation] UploadedFile - moved a security check commit fc70e13 Author: Bilal Amarni <bilal.amarni@gmail.com> Date: Thu Jan 24 11:07:29 2013 +0100 explicitly passed UPLOAD_ERR_OK constant in a test commit dda03a2 Author: Bilal Amarni <bilal.amarni@gmail.com> Date: Fri Jan 18 17:24:06 2013 +0100 [HttpFoundation] UploadedFile - moved a security check from move() to isValid()
symfony · Mar 23, 2013 · 5bb44f5 · 5bb44f5
1 parent 69dbbdd
commit 5bb44f5
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 17 deletions.
diff --git a/src/Symfony/Component/HttpFoundation/File/UploadedFile.php b/src/Symfony/Component/HttpFoundation/File/UploadedFile.php
@@ -179,13 +179,15 @@ public function getError()
     /**
      * 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 @@ public function isValid()
      *
      * @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 @@ public function move($directory, $name = null)
         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());
+            $target = $this->getTargetFile($directory, $name);
 
-                return $target;
+            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()));
     }
 
     /**

diff --git a/src/Symfony/Component/HttpFoundation/Tests/File/UploadedFileTest.php b/src/Symfony/Component/HttpFoundation/Tests/File/UploadedFileTest.php
@@ -197,7 +197,8 @@ public function testIsValid()
             'original.gif',
             null,
             filesize(__DIR__.'/Fixtures/test.gif'),
-            UPLOAD_ERR_OK
+            UPLOAD_ERR_OK,
+                true
         );
 
         $this->assertTrue($file->isValid());
@@ -229,4 +230,17 @@ public function uploadedFileErrorProvider()
             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());
+    }
 }
diff --git a/src/Symfony/Component/HttpKernel/Tests/ClientTest.php b/src/Symfony/Component/HttpKernel/Tests/ClientTest.php
@@ -114,7 +114,7 @@ public function testUploadedFile()
 
         $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 @@ public function testUploadedFileWhenSizeExceedsUploadMaxFileSize()
 
         $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()
         ;

diff --git a/src/Symfony/Component/Validator/Tests/Constraints/FileValidatorTest.php b/src/Symfony/Component/Validator/Tests/Constraints/FileValidatorTest.php
@@ -82,7 +82,7 @@ public function testValidUploadedfile()
         $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());
     }