diff --git a/system/Files/File.php b/system/Files/File.php index feda0cd5780d..2cf394355185 100644 --- a/system/Files/File.php +++ b/system/Files/File.php @@ -170,7 +170,7 @@ public function move(string $targetPath, ?string $name = null, bool $overwrite = throw FileException::forUnableToMove($this->getBasename(), $targetPath, strip_tags($error['message'])); } - @chmod($destination, 0777 & ~umask()); + @chmod($destination, 0666 & ~umask()); return new self($destination); } diff --git a/system/HTTP/Files/UploadedFile.php b/system/HTTP/Files/UploadedFile.php index bf594acddc38..f469e3e7023a 100644 --- a/system/HTTP/Files/UploadedFile.php +++ b/system/HTTP/Files/UploadedFile.php @@ -159,7 +159,7 @@ public function move(string $targetPath, ?string $name = null, bool $overwrite = throw HTTPException::forMoveFailed(basename($this->path), $targetPath, $message); } - @chmod($targetPath, 0777 & ~umask()); + @chmod($destination, 0666 & ~umask()); // Success, so store our new information $this->path = $targetPath; diff --git a/tests/system/Files/FileWithVfsTest.php b/tests/system/Files/FileWithVfsTest.php index a6c26383d187..e0065591963d 100644 --- a/tests/system/Files/FileWithVfsTest.php +++ b/tests/system/Files/FileWithVfsTest.php @@ -150,4 +150,13 @@ public function testMoveReturnsNewInstance(): void $this->assertInstanceOf(File::class, $file); $this->assertSame($destination . '/apple.php', $file->getPathname()); } + + public function testMovePermissions(): void + { + $destination = $this->start . 'baker'; + $this->file->move($destination); + + $expectedPerms = 0666 & ~umask(); + $this->assertSame($expectedPerms, $this->root->getChild('baker/apple.php')->getPermissions()); + } } diff --git a/tests/system/HTTP/Files/FileMovingTest.php b/tests/system/HTTP/Files/FileMovingTest.php index bb4b637fdc47..fb07d10ed74b 100644 --- a/tests/system/HTTP/Files/FileMovingTest.php +++ b/tests/system/HTTP/Files/FileMovingTest.php @@ -98,6 +98,7 @@ public function testMove(): void $this->assertTrue($this->root->hasChild('destination/' . $finalFilename . '.txt')); $this->assertTrue($this->root->hasChild('destination/' . $finalFilename . '_1.txt')); + $this->assertSame(0666 & ~umask(), $this->root->getChild('destination/' . $finalFilename . '.txt')->getPermissions()); } public function testMoveSanitizesClientNameByDefault(): void diff --git a/user_guide_src/source/changelogs/v4.7.5.rst b/user_guide_src/source/changelogs/v4.7.5.rst index 47d447dfbbc3..6a5e9210265b 100644 --- a/user_guide_src/source/changelogs/v4.7.5.rst +++ b/user_guide_src/source/changelogs/v4.7.5.rst @@ -36,6 +36,7 @@ Bugs Fixed - **CLIRequest:** Fixed a bug where ``parseCommand()`` could throw a TypeError when ``argv`` is missing. - **Content Security Policy:** Fixed a bug where empty ``Content-Security-Policy``, ``Content-Security-Policy-Report-Only``, and ``Reporting-Endpoints`` response headers were generated when no corresponding values existed. +- **Files:** Fixed a bug where ``File::move()`` and ``UploadedFile::move()`` set executable and overly permissive file permissions (``0777 & ~umask()`` instead of ``0666 & ~umask()``), and ``UploadedFile::move()`` targeted the parent directory instead of the destination file for ``chmod()``. - **Helpers:** Fixed a bug where ``get_dir_file_info()`` returned incomplete entries for subdirectories and missing files instead of omitting them. - **Honeypot:** Fixed a bug where bot detection returned an HTTP 500 response instead of 403 (Forbidden). - **Logger:** Fixed a bug where interpolating a log message with array or non-stringable context values could raise PHP warnings or errors.