From b174bdbc6f4312b0bebb0f6172f00517249a8e26 Mon Sep 17 00:00:00 2001 From: n0nag0n Date: Mon, 27 Oct 2025 08:01:01 -0600 Subject: [PATCH] more changes with upload. Security enhancements. Also got rid of php-watcher --- composer.json | 3 +- flight/net/UploadedFile.php | 71 ++++++++++++++++++++++------------- tests/UploadedFileTest.php | 74 +++++++++++++++++++++++++++++++++++-- 3 files changed, 118 insertions(+), 30 deletions(-) diff --git a/composer.json b/composer.json index 96d45db..2e6ca74 100644 --- a/composer.json +++ b/composer.json @@ -51,7 +51,6 @@ "phpstan/phpstan": "^2.1", "phpunit/phpunit": "^9.6", "rregeer/phpunit-coverage-check": "^0.3.1", - "spatie/phpunit-watcher": "^1.23 || ^1.24", "squizlabs/php_codesniffer": "^3.11" }, "config": { @@ -62,7 +61,7 @@ "sort-packages": true }, "scripts": { - "test": "vendor/bin/phpunit-watcher watch", + "test": "phpunit", "test-ci": "phpunit", "test-coverage": "rm -f clover.xml && XDEBUG_MODE=coverage vendor/bin/phpunit --coverage-html=coverage --coverage-clover=clover.xml && vendor/bin/coverage-check clover.xml 100", "test-server": "echo \"Running Test Server\" && php -S localhost:8000 -t tests/server/", diff --git a/flight/net/UploadedFile.php b/flight/net/UploadedFile.php index 07e0af2..c2f3ad3 100644 --- a/flight/net/UploadedFile.php +++ b/flight/net/UploadedFile.php @@ -33,6 +33,11 @@ class UploadedFile */ private int $error; + /** + * @var bool $isPostUploadedFile Indicates if the file was uploaded via POST method. + */ + private bool $isPostUploadedFile = false; + /** * Constructs a new UploadedFile object. * @@ -41,14 +46,20 @@ class UploadedFile * @param int $size The size of the uploaded file in bytes. * @param string $tmpName The temporary name of the uploaded file. * @param int $error The error code associated with the uploaded file. + * @param bool|null $isPostUploadedFile Indicates if the file was uploaded via POST method. */ - public function __construct(string $name, string $mimeType, int $size, string $tmpName, int $error) + public function __construct(string $name, string $mimeType, int $size, string $tmpName, int $error, ?bool $isPostUploadedFile = null) { $this->name = $name; $this->mimeType = $mimeType; $this->size = $size; $this->tmpName = $tmpName; $this->error = $error; + if (is_uploaded_file($tmpName) === true) { + $this->isPostUploadedFile = true; // @codeCoverageIgnore + } else { + $this->isPostUploadedFile = $isPostUploadedFile ?? false; + } } /** @@ -114,32 +125,42 @@ class UploadedFile throw new Exception($this->getUploadErrorMessage($this->error)); } + if (is_writeable(dirname($targetPath)) === false) { + throw new Exception('Target directory is not writable'); + } + + // Prevent path traversal attacks + if (strpos($targetPath, '..') !== false) { + throw new Exception('Invalid target path: contains directory traversal'); + } + // Prevent absolute paths (basic check for Unix/Windows) + if ($targetPath[0] === '/' || (strlen($targetPath) > 1 && $targetPath[1] === ':')) { + throw new Exception('Invalid target path: absolute paths not allowed'); + } + + // Prevent overwriting existing files + if (file_exists($targetPath)) { + throw new Exception('Target file already exists'); + } + // Check if this is a legitimate uploaded file (POST method uploads) - $isUploadedFile = is_uploaded_file($this->tmpName) === true; - - if ($isUploadedFile === true) { + $isUploadedFile = $this->isPostUploadedFile; + + // Prevent symlink attacks for non-POST uploads + if (!$isUploadedFile && is_link($this->tmpName)) { + throw new Exception('Invalid temp file: symlink detected'); + } + + $uploadFunctionToCall = $isUploadedFile === true ? // Standard POST upload - use move_uploaded_file for security - if (move_uploaded_file($this->tmpName, $targetPath) === false) { - throw new Exception('Cannot move uploaded file'); // @codeCoverageIgnore - } - } elseif (getenv('PHPUNIT_TEST')) { - rename($this->tmpName, $targetPath); - } elseif (file_exists($this->tmpName) === true && is_readable($this->tmpName) === true) { - // Handle non-POST uploads (PATCH, PUT, DELETE) or other valid temp files - // Verify the file is in a valid temp directory for security - $tempDir = sys_get_temp_dir(); - $uploadTmpDir = ini_get('upload_tmp_dir') ?: $tempDir; - - if (strpos(realpath($this->tmpName), realpath($uploadTmpDir)) === 0 || - strpos(realpath($this->tmpName), realpath($tempDir)) === 0) { - if (rename($this->tmpName, $targetPath) === false) { - throw new Exception('Cannot move uploaded file'); - } - } else { - throw new Exception('Invalid temporary file location'); - } - } else { - throw new Exception('Temporary file does not exist or is not readable'); + 'move_uploaded_file' : + // Handle non-POST uploads (PATCH, PUT, DELETE) or other valid temp files + 'rename'; + + $result = $uploadFunctionToCall($this->tmpName, $targetPath); + + if ($result === false) { + throw new Exception('Cannot move uploaded file'); } } diff --git a/tests/UploadedFileTest.php b/tests/UploadedFileTest.php index 05cdc95..f64e155 100644 --- a/tests/UploadedFileTest.php +++ b/tests/UploadedFileTest.php @@ -18,14 +18,24 @@ class UploadedFileTest extends TestCase if (file_exists('tmp_name')) { unlink('tmp_name'); } + if (file_exists('existing.txt')) { + unlink('existing.txt'); + } + if (file_exists('real_file')) { + unlink('real_file'); + } + + // not found with file_exists...just delete it brute force + @unlink('tmp_symlink'); } - public function testMoveToSuccess(): void + public function testMoveToFalseSuccess(): void { + // This test would have passed in the real world but we can't actually force a post request in unit tests file_put_contents('tmp_name', 'test'); - $uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK); + $uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, true); + $this->expectExceptionMessage('Cannot move uploaded file'); $uploadedFile->moveTo('file.txt'); - $this->assertFileExists('file.txt'); } public function getFileErrorMessageTests(): array @@ -53,4 +63,62 @@ class UploadedFileTest extends TestCase $this->expectExceptionMessage($message); $uploadedFile->moveTo('file.txt'); } + + public function testMoveToBadLocation(): void + { + file_put_contents('tmp_name', 'test'); + $uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, true); + $this->expectExceptionMessage('Target directory is not writable'); + $uploadedFile->moveTo('/root/file.txt'); + } + + public function testMoveToSuccessNonPost(): void + { + file_put_contents('tmp_name', 'test'); + $uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, false); + $uploadedFile->moveTo('file.txt'); + $this->assertFileExists('file.txt'); + $this->assertEquals('test', file_get_contents('file.txt')); + } + + public function testMoveToPathTraversal(): void + { + file_put_contents('tmp_name', 'test'); + $uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, false); + $this->expectException(Exception::class); + $this->expectExceptionMessage('Invalid target path: contains directory traversal'); + $uploadedFile->moveTo('../file.txt'); + } + + public function testMoveToAbsolutePath(): void + { + file_put_contents('tmp_name', 'test'); + $uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, false); + $this->expectException(Exception::class); + $this->expectExceptionMessage('Invalid target path: absolute paths not allowed'); + $uploadedFile->moveTo('/tmp/file.txt'); + } + + public function testMoveToOverwrite(): void + { + file_put_contents('tmp_name', 'test'); + file_put_contents('existing.txt', 'existing'); + $uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_name', UPLOAD_ERR_OK, false); + $this->expectException(Exception::class); + $this->expectExceptionMessage('Target file already exists'); + $uploadedFile->moveTo('existing.txt'); + } + + public function testMoveToSymlinkNonPost(): void + { + file_put_contents('real_file', 'test'); + if (file_exists('tmp_symlink')) { + unlink('tmp_symlink'); + } + symlink('real_file', 'tmp_symlink'); + $uploadedFile = new UploadedFile('file.txt', 'text/plain', 4, 'tmp_symlink', UPLOAD_ERR_OK, false); + $this->expectException(Exception::class); + $this->expectExceptionMessage('Invalid temp file: symlink detected'); + $uploadedFile->moveTo('file.txt'); + } }