From b61c25212128ff4d50805bdecdf48da050d8793a Mon Sep 17 00:00:00 2001 From: William Desportes Date: Fri, 8 Dec 2023 00:22:42 +0100 Subject: [PATCH] Make file and folder names safe --- README.md | 14 +++++--- composer.json | 3 +- src/Resumable.php | 68 ++++++++------------------------------ test/src/ResumableTest.php | 54 ++++++++++++++++++++++++++++-- 4 files changed, 76 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index 29e560f..c250464 100644 --- a/README.md +++ b/README.md @@ -36,23 +36,27 @@ $resumable->process(); ### Setting custom filename(s) ```php -// NOTE: this fork removed the getOriginalFilename() parameter to return without an extension. $originalName = $resumable->getOriginalFilename(); // will give you the original end-user file-name -// do some slugification or whatever you need... -$slugifiedname = my_slugify($originalName); // this is up to you, it as ported out of the library. -$resumable->setFilename($slugifiedname); +$mySafeName = Security::sanitizeFileName($request->query('resumableFilename')); +$resumable->setFilename($mySafeName);// Override the safe filename // process upload as normal $resumable->process(); // you can also get file information after the upload is complete if (true === $resumable->isUploadComplete()) { // true when the final file has been uploaded and chunks reunited. - $extension = $resumable->getExtension(); $filename = $resumable->getFilename(); } ``` +## Removed features + +- `$resumable->getOriginalFilename()` does not have a parameter to return the name without the extension +- `$resumable->getExtension()` implement the logic yourself +- `preProcess()` no longer exists, it was not very useful +- the default value of `uploadFolder` was `test/files/uploads` and is now `uploads` + ## Testing ```sh diff --git a/composer.json b/composer.json index d6fa531..a50f3fe 100644 --- a/composer.json +++ b/composer.json @@ -15,7 +15,8 @@ "php": "^7.4 || ^8.0", "psr/log": "^1.1", "psr/http-message": "^1.0", - "knplabs/gaufrette": "^0.11.1" + "knplabs/gaufrette": "^0.11.1", + "ondrej-vrto/php-filename-sanitize": "^1.4" }, "require-dev": { "phpunit/phpunit": "^7 || ^8 || ^9 || ^10", diff --git a/src/Resumable.php b/src/Resumable.php index 7785f74..65acda7 100644 --- a/src/Resumable.php +++ b/src/Resumable.php @@ -11,6 +11,7 @@ use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\UploadedFileInterface; +use OndrejVrto\FilenameSanitize\FilenameSanitize; class Resumable { @@ -24,7 +25,7 @@ class Resumable public $tempFolder = 'tmp'; /** @var string */ - public $uploadFolder = 'test/files/uploads'; + public $uploadFolder = 'uploads'; /** * For testing purposes @@ -70,8 +71,6 @@ class Resumable protected $filepath; - protected $extension; - protected $originalFilename; /** @var bool */ @@ -102,8 +101,6 @@ public function __construct( } $this->logger = $logger; - - $this->preProcess(); } public static function getLocalFileSystem(string $baseDir): Filesystem @@ -120,26 +117,6 @@ public function setResumableOption(array $resumableOption): void $this->resumableOption = array_merge($this->resumableOption, $resumableOption); } - /** - * sets original filename and extension, blah blah - */ - public function preProcess(): void - { - if (!empty($this->resumableParams())) { - if (!empty($this->request->getUploadedFiles())) { - $this->originalFilename = $this->resumableParam('filename'); - $this->extension = $this->findExtension($this->originalFilename); - $this->log( - 'Defined extension', - [ - 'extension' => $this->extension, - 'originalFilename' => $this->originalFilename, - ] - ); - } - } - } - /** * @return ResponseInterface|null */ @@ -209,32 +186,14 @@ public function getFilepath(): string } /** - * Get final extension. + * Creates a safe name * - * @return string Final extension name + * @param string $name Original name + * @return string A safer name */ - public function getExtension(): string + private function createSafeName(string $name): string { - return $this->extension; - } - - /** - * Makes sure the original extension never gets overridden by user defined filename. - * - * @param string User defined filename - * @param string Original filename - * @return string Filename that always has an extension from the original file - */ - private function createSafeFilename(string $filename, string $originalFilename): string - { - return sprintf('%s.%s', $filename, $extension); - } - - private function findExtension(string $filename): string - { - $parts = explode('.', basename($filename)); - - return end($parts); + return FilenameSanitize::of($name)->get(); } /** @@ -323,17 +282,16 @@ private function createFileAndDeleteTmp(string $identifier, ?string $filename): $chunkDir )['keys']; - $finalFilename = $filename; + $this->originalFilename = $filename; // if the user has set a custom filename - if (null !== $this->filename) { - $finalFilename = $this->createSafeFilename($this->filename, $filename); - $this->log('Created safe filename', ['finalFilename' => $finalFilename]); + if (null === $this->filename) { + $this->filename = $this->createSafeName($this->originalFilename); + $this->log('Created safe filename', ['finalFilename' => $this->filename]); } // replace filename reference by the final file - $this->filepath = $this->uploadFolder . DIRECTORY_SEPARATOR . $finalFilename; - $this->extension = $this->findExtension($this->filepath); + $this->filepath = $this->uploadFolder . DIRECTORY_SEPARATOR . $this->filename; $finalFileCreated = $this->createFileFromChunks($chunkFiles, $this->filepath); @@ -409,7 +367,7 @@ public function isChunkUploaded(string $identifier, string $filename, int $chunk public function tmpChunkDir(string $identifier): string { - return $this->tempFolder . DIRECTORY_SEPARATOR . $identifier; + return $this->tempFolder . DIRECTORY_SEPARATOR . $this->createSafeName($identifier); } /** diff --git a/test/src/ResumableTest.php b/test/src/ResumableTest.php index 2ea7c6b..59bc250 100644 --- a/test/src/ResumableTest.php +++ b/test/src/ResumableTest.php @@ -141,7 +141,8 @@ public function testHandleTestChunk(): void public function testHandleChunk(): void { - $uploadedFile = tempnam(sys_get_temp_dir(), 'resumable.js_mock.png'); + $uploadedFile = strtolower(tempnam(sys_get_temp_dir(), 'resumable.js-mock')) . '.png'; + touch($uploadedFile);// Create the uploaded file $uploadedFileName = basename($uploadedFile); $resumableParams = [ 'resumableChunkNumber' => 3, @@ -167,7 +168,6 @@ public function testHandleChunk(): void ] ); - touch($uploadedFile);// Create the uploaded file $this->resumable = new Resumable($this->request, $this->response); $this->resumable->tempFolder = 'test/tmp'; $this->resumable->uploadFolder = 'test/uploads'; @@ -216,6 +216,56 @@ public function testResumableParamsGetRequest(): void $this->assertEquals($resumableParams, $this->resumable->resumableParams()); } + public static function fileNameProvider(): array + { + return [ + ['mock.png', 'mock.png'], + ['../unsafe-one-level.txt', 'unsafe-one-level.txt'], + ]; + } + + /** + * @dataProvider fileNameProvider + */ + public function testResumableSanitizeFileName(string $filename, string $filenameSanitized): void + { + $resumableParams = [ + 'resumableChunkNumber' => 1, + 'resumableTotalChunks' => 1, + 'resumableChunkSize' => 200, + 'resumableIdentifier' => 'identifier', + 'resumableFilename' => $filename, + 'resumableRelativePath' => 'upload', + ]; + + $uploadedFile = tempnam(sys_get_temp_dir(), 'resumable.js_sanitize'); + + $this->request = $this->psr17Factory->createServerRequest( + 'POST', + 'http://example.com' + ) + ->withParsedBody($resumableParams) + ->withUploadedFiles( + [ + new UploadedFile( + $uploadedFile, + 27000, // Size + 0 // Error status + ), + ] + ); + + $this->resumable = new Resumable($this->request, $this->response); + $this->resumable->uploadFolder = 'upld'; + + $this->assertNotNull($this->resumable->handleChunk()); + unlink($uploadedFile); + $this->assertTrue($this->resumable->isUploadComplete()); + $this->assertSame($filename, $this->resumable->getOriginalFilename()); + $this->assertSame($filenameSanitized, $this->resumable->getFilename()); + $this->assertSame('upld/' . $filenameSanitized, $this->resumable->getFilepath()); + } + public static function isFileUploadCompleteProvider(): array { return [