fix: UploadedFile::hasMoved() may return incorrect value

Author: kenjis

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']) : '';
 
             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;
     }

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
@@ -262,7 +265,7 @@ public function testInvalidFile()
         $file->move($destination, $file->getName(), false);
     }
 
-    public function testFailedMove()
+    public function testFailedMoveBecauseOfWarning()
     {
         $_FILES = [
             'userfile' => [
@@ -287,6 +290,41 @@ public function testFailedMove()
         $this->expectException(HTTPException::class);
         $file->move($destination, $file->getName(), false);
     }
+
+    public function testFailedMoveBecauseOfFalseReturned()
+    {
+        $finalFilename = 'fileA';
+
+        $_FILES = [
+            'userfile1' => [
+                'name'     => $finalFilename . '.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);
+        }
+
+        $file = $collection->getFile('userfile1');
+
+        // Set the mock's return value to false
+        move_uploaded_file('', '', false);
+
+        $this->expectException(HTTPException::class);
+
+        $file->move($destination, $file->getName(), false);
+    }
 }
 
 /*
@@ -311,10 +349,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)