From f8b46c3241ebab58f562f29f7474b745a9ef8a02 Mon Sep 17 00:00:00 2001 From: Niels Braczek Date: Sat, 12 Feb 2022 17:09:40 +0100 Subject: [PATCH 1/4] Fix - Don't fail, if filenames are too long, shorten them instead --- Tests/FileTest.php | 103 ++++++++++++++++++++---------------- Tests/fixtures/tempFile.txt | 1 + src/File.php | 46 ++++++++++++++-- 3 files changed, 101 insertions(+), 49 deletions(-) create mode 100644 Tests/fixtures/tempFile.txt diff --git a/Tests/FileTest.php b/Tests/FileTest.php index b93d63c0..3556c9f7 100644 --- a/Tests/FileTest.php +++ b/Tests/FileTest.php @@ -44,7 +44,7 @@ public function dataTestStripExt(): \Generator * * @dataProvider dataTestStripExt */ - public function testStripExt($fileName, $nameWithoutExt) + public function testStripExt(string $fileName, string $nameWithoutExt): void { $this->assertEquals( File::stripExt($fileName), @@ -111,10 +111,10 @@ public function dataTestMakeSafe(): \Generator * @param string $expected The expected safe file name * @param string $message The message to show on failure of test * - * @covers Joomla\Filesystem\File::makeSafe + * @covers \Joomla\Filesystem\File::makeSafe * @dataProvider dataTestMakeSafe */ - public function testMakeSafe($name, $stripChars, $expected, $message) + public function testMakeSafe(string $name, array $stripChars, string $expected, string $message): void { $this->assertEquals(File::makeSafe($name, $stripChars), $expected, $message); } @@ -122,7 +122,7 @@ public function testMakeSafe($name, $stripChars, $expected, $message) /** * Test copy method. */ - public function testCopyWithPathArgPassed() + public function testCopyWithPathArgPassed(): void { $name = 'tempFile'; $copiedName = 'tempCopiedFileName'; @@ -148,7 +148,7 @@ public function testCopyWithPathArgPassed() /** * Test copy method. */ - public function testCopyWithoutPathArgPassed() + public function testCopyWithoutPathArgPassed(): void { $name = 'tempFile'; $copiedName = 'tempCopiedFileName'; @@ -174,7 +174,7 @@ public function testCopyWithoutPathArgPassed() /** * Test copy method using streams. */ - public function testCopyWithStreams() + public function testCopyWithStreams(): void { $name = 'tempFile'; $copiedName = 'tempCopiedFileName'; @@ -200,7 +200,7 @@ public function testCopyWithStreams() /** * Test makeCopy method for an exception */ - public function testCopySrcDontExist() + public function testCopySrcDontExist(): void { $this->expectException(\UnexpectedValueException::class); @@ -213,7 +213,7 @@ public function testCopySrcDontExist() /** * Test delete method. */ - public function testDeleteForSingleFile() + public function testDeleteForSingleFile(): void { $name = 'tempFile'; $data = 'Lorem ipsum dolor sit amet'; @@ -232,7 +232,7 @@ public function testDeleteForSingleFile() /** * Test delete method. */ - public function testDeleteForArrayOfFiles() + public function testDeleteForArrayOfFiles(): void { $name1 = 'tempFile1'; $name2 = 'tempFile2'; @@ -257,7 +257,7 @@ public function testDeleteForArrayOfFiles() /** * Tests the File::move method. */ - public function testMoveWithPathArgPassed() + public function testMoveWithPathArgPassed(): void { $name = 'tempFile'; $movedName = 'tempCopiedFileName'; @@ -277,7 +277,7 @@ public function testMoveWithPathArgPassed() /** * Tests the File::move method. */ - public function testMoveWithoutPathArgPassed() + public function testMoveWithoutPathArgPassed(): void { $name = 'tempFile'; $movedName = 'tempCopiedFileName'; @@ -297,7 +297,7 @@ public function testMoveWithoutPathArgPassed() /** * Tests the File::move method. */ - public function testMoveWithStreams() + public function testMoveWithStreams(): void { $name = 'tempFile'; $movedName = 'tempCopiedFileName'; @@ -314,11 +314,10 @@ public function testMoveWithStreams() ); } - /** * Test the File::move method where source file doesn't exist. */ - public function testMoveSrcDontExist() + public function testMoveSrcDontExist(): void { $name = 'tempFile'; $movedName = 'tempCopiedFileName'; @@ -332,7 +331,7 @@ public function testMoveSrcDontExist() /** * Test write method. */ - public function testWrite() + public function testWrite(): void { $name = 'tempFile'; $data = 'Lorem ipsum dolor sit amet'; @@ -352,10 +351,10 @@ public function testWrite() /** * Test write method when appending to a file. */ - public function testWriteWithAppend() + public function testWriteWithAppend(): void { - $name = 'tempFile.txt'; - $data = 'Lorem ipsum dolor sit amet'; + $name = 'tempFile.txt'; + $data = 'Lorem ipsum dolor sit amet'; $appendData = PHP_EOL . $data; $this->assertTrue( @@ -378,7 +377,7 @@ public function testWriteWithAppend() /** * Test write method. */ - public function testWriteCreatesMissingDirectory() + public function testWriteCreatesMissingDirectory(): void { $name = 'tempFile'; $data = 'Lorem ipsum dolor sit amet'; @@ -398,7 +397,7 @@ public function testWriteCreatesMissingDirectory() /** * Test write method. */ - public function testWriteWithStreams() + public function testWriteWithStreams(): void { $name = 'tempFile'; $data = 'Lorem ipsum dolor sit amet'; @@ -415,33 +414,50 @@ public function testWriteWithStreams() ); } + /** + * Provides the data to test the upload method. + * + * @return \Generator + */ + public function dataTestUpload(): \Generator + { + /* + * This case describes the happy path + */ + yield 'valid filename' => [ + 'uploadedFileName' => 'uploadedFileName', + ]; + + /* + * This case provides a filename longer than the allowed 250 bytes. + * @see [Comment in the PHP manual](https://www.php.net/manual/en/function.move-uploaded-file.php#103190) + */ + yield 'too long filename' => [ + 'uploadedFileName' => str_repeat('a', 260), + ]; + } + /** * Test upload method. * * @backupGlobals enabled + * @dataProvider dataTestUpload */ - public function testUpload() + public function testUpload(string $uploadedFileName): void { include_once __DIR__ . '/Stubs/PHPUploadStub.php'; - $name = 'tempFile'; - $data = 'Lorem ipsum dolor sit amet'; - $uploadedFileName = 'uploadedFileName'; - - if (!File::write($this->testPath . '/' . $name, $data)) - { - $this->markTestSkipped('The test file could not be created.'); - } + $tempFile = __DIR__ . '/fixtures/tempFile.txt'; $_FILES = [ 'test' => [ 'name' => 'test.jpg', - 'tmp_name' => $this->testPath . '/' . $name, + 'tmp_name' => $tempFile, ], ]; $this->assertTrue( - File::upload($this->testPath . '/' . $name, $this->testPath . '/' . $uploadedFileName) + File::upload($tempFile, $this->testPath . '/' . $uploadedFileName) ); } @@ -450,28 +466,22 @@ public function testUpload() * * @backupGlobals enabled */ - public function testUploadWithStreams() + public function testUploadWithStreams(): void { include_once __DIR__ . '/Stubs/PHPUploadStub.php'; - $name = 'tempFile'; - $data = 'Lorem ipsum dolor sit amet'; + $tempFile = __DIR__ . '/fixtures/tempFile.txt'; $uploadedFileName = 'uploadedFileName'; - if (!File::write($this->testPath . '/' . $name, $data)) - { - $this->markTestSkipped('The test file could not be created.'); - } - $_FILES = [ 'test' => [ 'name' => 'test.jpg', - 'tmp_name' => $this->testPath . '/' . $name, + 'tmp_name' => $tempFile, ], ]; $this->assertTrue( - File::upload($this->testPath . '/' . $name, $this->testPath . '/' . $uploadedFileName, true) + File::upload($tempFile, $this->testPath . '/' . $uploadedFileName, true) ); } @@ -480,12 +490,12 @@ public function testUploadWithStreams() * * @backupGlobals enabled */ - public function testUploadToNestedDirectory() + public function testUploadToNestedDirectory(): void { include_once __DIR__ . '/Stubs/PHPUploadStub.php'; - $name = 'tempFile'; - $data = 'Lorem ipsum dolor sit amet'; + $name = 'tempFile'; + $data = 'Lorem ipsum dolor sit amet'; $uploadedFileName = 'uploadedFileName'; if (!File::write($this->testPath . '/' . $name . '.txt', $data)) @@ -501,7 +511,10 @@ public function testUploadToNestedDirectory() ]; $this->assertTrue( - File::upload($this->testPath . '/' . $name . '.txt', $this->testPath . '/' . $name . '/' . $uploadedFileName) + File::upload( + $this->testPath . '/' . $name . '.txt', + $this->testPath . '/' . $name . '/' . $uploadedFileName + ) ); } } diff --git a/Tests/fixtures/tempFile.txt b/Tests/fixtures/tempFile.txt new file mode 100644 index 00000000..dc8344c1 --- /dev/null +++ b/Tests/fixtures/tempFile.txt @@ -0,0 +1 @@ +Lorem ipsum dolor sit amet diff --git a/src/File.php b/src/File.php index 2197f3df..00e45f8e 100644 --- a/src/File.php +++ b/src/File.php @@ -88,7 +88,9 @@ public static function copy($src, $dest, $path = null, $useStreams = false) if (!$stream->copy($src, $dest, null, false)) { - throw new FilesystemException(sprintf('%1$s(%2$s, %3$s): %4$s', __METHOD__, $src, $dest, $stream->getError())); + throw new FilesystemException( + sprintf('%1$s(%2$s, %3$s): %4$s', __METHOD__, $src, $dest, $stream->getError()) + ); } self::invalidateFileCache($dest); @@ -118,7 +120,7 @@ public static function copy($src, $dest, $path = null, $useStreams = false) */ public static function delete($file) { - $files = (array) $file; + $files = (array)$file; foreach ($files as $file) { @@ -265,7 +267,7 @@ public static function write($file, &$buffer, $useStreams = false, $appendToFile public static function upload($src, $dest, $useStreams = false) { // Ensure that the path is valid and clean - $dest = Path::clean($dest); + $dest = Path::clean(self::shortenIfTooLong($dest)); // Create the destination directory if it does not exist $baseDir = \dirname($dest); @@ -281,7 +283,9 @@ public static function upload($src, $dest, $useStreams = false) if (!$stream->upload($src, $dest, null, false)) { - throw new FilesystemException(sprintf('%1$s(%2$s, %3$s): %4$s', __METHOD__, $src, $dest, $stream->getError())); + throw new FilesystemException( + sprintf('%1$s(%2$s, %3$s): %4$s', __METHOD__, $src, $dest, $stream->getError()) + ); } self::invalidateFileCache($dest); @@ -325,4 +329,38 @@ public static function invalidateFileCache($file) } } } + + /** + * Shorten a filename to a maximum allowed length + * + * The directory name is not changed. + * + * @param string $filename The filename + * @param int $maxLen The maximum length including extension. Defaults to 240. + * + * @return string The original filename, if it is shorter than the maximum length, a shortened filename otherwise. + */ + private static function shortenIfTooLong(string $filename, int $maxLen = 240): string + { + $info = pathinfo($filename); + + if (strlen($info['filename']) > $maxLen) + { + $path = $info['dirname'] ?? ''; + if ($path > '') + { + $path .= '/'; + } + + $ext = $info['extension'] ?? ''; + if ($ext > '') + { + $ext = '.' . $ext; + } + + $filename = $path . substr($info['filename'], 0, $maxLen - strlen($ext)) . $ext; + } + + return $filename; + } } From 0ea0ba731719693464e4f8b45109105c2a82e4bf Mon Sep 17 00:00:00 2001 From: Niels Braczek Date: Sat, 12 Feb 2022 18:20:35 +0100 Subject: [PATCH 2/4] Fix - Make test more stable Accessing joomla.org does not always work; example.org is better suited for this purpose. --- Tests/HelperTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/HelperTest.php b/Tests/HelperTest.php index 45296739..0274f403 100644 --- a/Tests/HelperTest.php +++ b/Tests/HelperTest.php @@ -25,7 +25,7 @@ public function testRemotefsize() ); $this->assertTrue( - is_numeric(Helper::remotefsize('https://www.joomla.org')), + is_numeric(Helper::remotefsize('https://www.example.org')), 'Line:' . __LINE__ . ' for a valid remote file, returned size should be numeric.' ); From a00aacc86c97eba7662f55a7db7b6fe0aae957e3 Mon Sep 17 00:00:00 2001 From: Niels Braczek Date: Sun, 13 Feb 2022 16:18:34 +0100 Subject: [PATCH 3/4] Move conditional shortening to makeSafe() --- src/File.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/File.php b/src/File.php index 00e45f8e..a6b66243 100644 --- a/src/File.php +++ b/src/File.php @@ -50,6 +50,9 @@ public static function makeSafe($file, array $stripChars = ['#^\.#']) // Remove any trailing dots, as those aren't ever valid file names. $file = rtrim($file, '.'); + // Shorten to maximum length, if necessary + $file = self::shortenIfTooLong($file); + return $file; } @@ -120,7 +123,7 @@ public static function copy($src, $dest, $path = null, $useStreams = false) */ public static function delete($file) { - $files = (array)$file; + $files = (array) $file; foreach ($files as $file) { @@ -267,7 +270,7 @@ public static function write($file, &$buffer, $useStreams = false, $appendToFile public static function upload($src, $dest, $useStreams = false) { // Ensure that the path is valid and clean - $dest = Path::clean(self::shortenIfTooLong($dest)); + $dest = Path::clean($dest); // Create the destination directory if it does not exist $baseDir = \dirname($dest); @@ -347,12 +350,14 @@ private static function shortenIfTooLong(string $filename, int $maxLen = 240): s if (strlen($info['filename']) > $maxLen) { $path = $info['dirname'] ?? ''; + if ($path > '') { $path .= '/'; } $ext = $info['extension'] ?? ''; + if ($ext > '') { $ext = '.' . $ext; From f1664f5ba3284afc8ae8aaa09ca71462cb6c7d02 Mon Sep 17 00:00:00 2001 From: Niels Braczek Date: Sun, 13 Feb 2022 16:59:31 +0100 Subject: [PATCH 4/4] Fix - Adapt tests --- Tests/FileTest.php | 61 ++++++++++++++++++++++------------------------ src/File.php | 2 +- 2 files changed, 30 insertions(+), 33 deletions(-) diff --git a/Tests/FileTest.php b/Tests/FileTest.php index 3556c9f7..035cbf9f 100644 --- a/Tests/FileTest.php +++ b/Tests/FileTest.php @@ -101,6 +101,14 @@ public function dataTestMakeSafe(): \Generator '.gitignore', 'Files starting with a fullstop should be allowed when strip chars parameter is empty', ]; + + $longName = str_repeat('0123456789abcdef', 16); + yield [ + $longName . '.png', + [], + substr($longName, 0, 236) . '.png', + 'Filenames with more than 240 characters should be shortened to 240 characters preserving the extension', + ]; } /** @@ -116,7 +124,7 @@ public function dataTestMakeSafe(): \Generator */ public function testMakeSafe(string $name, array $stripChars, string $expected, string $message): void { - $this->assertEquals(File::makeSafe($name, $stripChars), $expected, $message); + $this->assertEquals($expected, File::makeSafe($name, $stripChars), $message); } /** @@ -414,50 +422,33 @@ public function testWriteWithStreams(): void ); } - /** - * Provides the data to test the upload method. - * - * @return \Generator - */ - public function dataTestUpload(): \Generator - { - /* - * This case describes the happy path - */ - yield 'valid filename' => [ - 'uploadedFileName' => 'uploadedFileName', - ]; - - /* - * This case provides a filename longer than the allowed 250 bytes. - * @see [Comment in the PHP manual](https://www.php.net/manual/en/function.move-uploaded-file.php#103190) - */ - yield 'too long filename' => [ - 'uploadedFileName' => str_repeat('a', 260), - ]; - } - /** * Test upload method. * * @backupGlobals enabled - * @dataProvider dataTestUpload */ - public function testUpload(string $uploadedFileName): void + public function testUpload(): void { include_once __DIR__ . '/Stubs/PHPUploadStub.php'; - $tempFile = __DIR__ . '/fixtures/tempFile.txt'; + $name = 'tempFile'; + $data = 'Lorem ipsum dolor sit amet'; + $uploadedFileName = 'uploadedFileName'; + + if (!File::write($this->testPath . '/' . $name, $data)) + { + $this->markTestSkipped('The test file could not be created.'); + } $_FILES = [ 'test' => [ 'name' => 'test.jpg', - 'tmp_name' => $tempFile, + 'tmp_name' => $this->testPath . '/' . $name, ], ]; $this->assertTrue( - File::upload($tempFile, $this->testPath . '/' . $uploadedFileName) + File::upload($this->testPath . '/' . $name, $this->testPath . '/' . $uploadedFileName) ); } @@ -470,18 +461,24 @@ public function testUploadWithStreams(): void { include_once __DIR__ . '/Stubs/PHPUploadStub.php'; - $tempFile = __DIR__ . '/fixtures/tempFile.txt'; + $name = 'tempFile'; + $data = 'Lorem ipsum dolor sit amet'; $uploadedFileName = 'uploadedFileName'; + if (!File::write($this->testPath . '/' . $name, $data)) + { + $this->markTestSkipped('The test file could not be created.'); + } + $_FILES = [ 'test' => [ 'name' => 'test.jpg', - 'tmp_name' => $tempFile, + 'tmp_name' => $this->testPath . '/' . $name, ], ]; $this->assertTrue( - File::upload($tempFile, $this->testPath . '/' . $uploadedFileName, true) + File::upload($this->testPath . '/' . $name, $this->testPath . '/' . $uploadedFileName, true) ); } diff --git a/src/File.php b/src/File.php index a6b66243..f51b0df2 100644 --- a/src/File.php +++ b/src/File.php @@ -349,7 +349,7 @@ private static function shortenIfTooLong(string $filename, int $maxLen = 240): s if (strlen($info['filename']) > $maxLen) { - $path = $info['dirname'] ?? ''; + $path = $info['dirname'] === '.' ? '' : $info['dirname']; if ($path > '') {