Merge pull request #5302 from kenjis/fix-UploadedFile-hasMoved

fix: UploadedFile::move() may return incorrect value
codeigniter4 · Nov 23, 2021 · d002547 · d002547
2 parents 6ba7f27 + c0a8005
commit d002547
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 27 deletions.
diff --git a/system/HTTP/Files/UploadedFile.php b/system/HTTP/Files/UploadedFile.php
@@ -141,20 +141,25 @@ public function move(string $targetPath, ?string $name = null, bool $overwrite =
         $destination = $overwrite ? $targetPath . $name : $this->getDestination($targetPath . $name);
 
         try {
-            move_uploaded_file($this->path, $destination);
+            $this->hasMoved = move_uploaded_file($this->path, $destination);
         } catch (Exception $e) {
             $error   = error_get_last();
-            $message = isset($error['message']) ? strip_tags($error['message']) : '';
+            $message = strip_tags($error['message'] ?? '');
+
+            throw HTTPException::forMoveFailed(basename($this->path), $targetPath, $message);
+        }
+
+        if ($this->hasMoved === false) {
+            $message = 'move_uploaded_file() returned false';
 
             throw HTTPException::forMoveFailed(basename($this->path), $targetPath, $message);
         }
 
         @chmod($targetPath, 0777 & ~umask());
 
         // Success, so store our new information
-        $this->path     = $targetPath;
-        $this->name     = basename($destination);
-        $this->hasMoved = true;
+        $this->path = $targetPath;
+        $this->name = basename($destination);
 
         return true;
     }

diff --git a/tests/system/HTTP/Files/FileMovingTest.php b/tests/system/HTTP/Files/FileMovingTest.php
@@ -35,6 +35,9 @@ protected function setUp(): void
         }
 
         $_FILES = [];
+
+        // Set the mock's return value to true
+        move_uploaded_file('', '', true);
     }
 
     protected function tearDown(): void
@@ -52,8 +55,7 @@ protected function tearDown(): void
     public function testMove()
     {
         $finalFilename = 'fileA';
-
-        $_FILES = [
+        $_FILES        = [
             'userfile1' => [
                 'name'     => $finalFilename . '.txt',
                 'type'     => 'text/plain',
@@ -76,7 +78,6 @@ public function testMove()
         $this->assertTrue($collection->hasFile('userfile2'));
 
         $destination = $this->destination;
-
         // Create the destination if not exists
         if (! is_dir($destination)) {
             mkdir($destination, 0777, true);
@@ -94,8 +95,7 @@ public function testMove()
     public function testMoveOverwriting()
     {
         $finalFilename = 'file_with_delimiters_underscore';
-
-        $_FILES = [
+        $_FILES        = [
             'userfile1' => [
                 'name'     => $finalFilename . '.txt',
                 'type'     => 'text/plain',
@@ -126,7 +126,6 @@ public function testMoveOverwriting()
         $this->assertTrue($collection->hasFile('userfile3'));
 
         $destination = $this->destination;
-
         // Create the destination if not exists
         if (! is_dir($destination)) {
             mkdir($destination, 0777, true);
@@ -146,8 +145,7 @@ public function testMoveOverwriting()
     public function testMoved()
     {
         $finalFilename = 'fileA';
-
-        $_FILES = [
+        $_FILES        = [
             'userfile1' => [
                 'name'     => $finalFilename . '.txt',
                 'type'     => 'text/plain',
@@ -162,7 +160,6 @@ public function testMoved()
         $this->assertTrue($collection->hasFile('userfile1'));
 
         $destination = $this->destination;
-
         // Create the destination if not exists
         if (! is_dir($destination)) {
             mkdir($destination, 0777, true);
@@ -172,15 +169,16 @@ public function testMoved()
 
         $this->assertInstanceOf(UploadedFile::class, $file);
         $this->assertFalse($file->hasMoved());
+
         $file->move($destination, $file->getName(), false);
+
         $this->assertTrue($file->hasMoved());
     }
 
     public function testStore()
     {
         $finalFilename = 'fileA';
-
-        $_FILES = [
+        $_FILES        = [
             'userfile1' => [
                 'name'     => $finalFilename . '.txt',
                 'type'     => 'text/plain',
@@ -195,7 +193,6 @@ public function testStore()
         $this->assertTrue($collection->hasFile('userfile1'));
 
         $destination = $this->destination;
-
         // Create the destination if not exists
         if (! is_dir($destination)) {
             mkdir($destination, 0777, true);
@@ -204,15 +201,16 @@ public function testStore()
         $file = $collection->getFile('userfile1');
 
         $this->assertInstanceOf(UploadedFile::class, $file);
+
         $path = $file->store($destination, $file->getName());
+
         $this->assertSame($destination . '/fileA.txt', $path);
     }
 
     public function testAlreadyMoved()
     {
         $finalFilename = 'fileA';
-
-        $_FILES = [
+        $_FILES        = [
             'userfile1' => [
                 'name'     => $finalFilename . '.txt',
                 'type'     => 'text/plain',
@@ -227,7 +225,6 @@ public function testAlreadyMoved()
         $this->assertTrue($collection->hasFile('userfile1'));
 
         $destination = $this->destination;
-
         // Create the destination if not exists
         if (! is_dir($destination)) {
             mkdir($destination, 0777, true);
@@ -254,15 +251,15 @@ public function testInvalidFile()
         ];
 
         $destination = $this->destination;
-
-        $collection = new FileCollection();
-        $file       = $collection->getFile('userfile');
+        $collection  = new FileCollection();
 
         $this->expectException(HTTPException::class);
+
+        $file = $collection->getFile('userfile');
         $file->move($destination, $file->getName(), false);
     }
 
-    public function testFailedMove()
+    public function testFailedMoveBecauseOfWarning()
     {
         $_FILES = [
             'userfile' => [
@@ -275,16 +272,47 @@ public function testFailedMove()
         ];
 
         $destination = $this->destination;
-
         // Create the destination and make it read only
         if (! is_dir($destination)) {
             mkdir($destination, 0400, true);
         }
 
         $collection = new FileCollection();
-        $file       = $collection->getFile('userfile');
 
         $this->expectException(HTTPException::class);
+
+        $file = $collection->getFile('userfile');
+        $file->move($destination, $file->getName(), false);
+    }
+
+    public function testFailedMoveBecauseOfFalseReturned()
+    {
+        $_FILES = [
+            'userfile1' => [
+                'name'     => 'fileA.txt',
+                'type'     => 'text/plain',
+                'size'     => 124,
+                'tmp_name' => '/tmp/fileA.txt',
+                'error'    => 0,
+            ],
+        ];
+
+        $collection = new FileCollection();
+
+        $this->assertTrue($collection->hasFile('userfile1'));
+
+        $destination = $this->destination;
+        // Create the destination if not exists
+        if (! is_dir($destination)) {
+            mkdir($destination, 0777, true);
+        }
+        // Set the mock's return value to false
+        move_uploaded_file('', '', false);
+
+        $this->expectException(HTTPException::class);
+        $this->expectExceptionMessage('move_uploaded_file() returned false');
+
+        $file = $collection->getFile('userfile1');
         $file->move($destination, $file->getName(), false);
     }
 }
@@ -311,10 +339,20 @@ function is_uploaded_file($filename)
  * This overwrite is for testing the move operation.
  */
 
-function move_uploaded_file($filename, $destination)
+function move_uploaded_file($filename, $destination, ?bool $setReturnValue = null)
 {
+    static $return = true;
+
+    if ($setReturnValue !== null) {
+        $return = $setReturnValue;
+
+        return true;
+    }
+
     copy($filename, $destination);
     unlink($filename);
+
+    return $return;
 }
 
 function rrmdir($src)