diff --git a/src/Filesystem/File.php b/src/Filesystem/File.php index bdde829..70a9363 100644 --- a/src/Filesystem/File.php +++ b/src/Filesystem/File.php @@ -13,6 +13,7 @@ defined('ABSPATH') || exit; use Exception; +use Framework\Container; use InvalidArgumentException; use SplFileInfo; @@ -61,29 +62,17 @@ public function move(string $directory, ?string $name = null) { $target = $this->get_target_file($directory, $name); - // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_set_error_handler -- Captures a PHP warning from rename()/move_uploaded_file() into a catchable value; restored in the finally block immediately after. - set_error_handler(static function ($type, $msg) use (&$error) { - $error = $msg; - }); + $filesystem = Container::get_instance()->make(Filesystem::class); - try { - $renamed = rename($this->getPathname(), $target); - } finally { - restore_error_handler(); - } + $renamed = $filesystem->move($this->getPathname(), $target); throw_unless( $renamed, - sprintf( - 'Could not move the file "%s" to "%s" (%s).', - $this->getPathname(), - $target, - wp_strip_all_tags($error ?? '') - ), + sprintf('Could not move the file "%s" to "%s".', $this->getPathname(), $target), Exception::class ); - @chmod($target, 0666 & ~umask()); + $filesystem->chmod($target, 0666 & ~umask()); return $target; } @@ -120,7 +109,7 @@ public function get_content() */ protected function get_target_file(string $directory, ?string $name = null) { - if (!is_dir($directory) && !@mkdir($directory, 0777, true) && !is_dir($directory)) { + if (!is_dir($directory) && !wp_mkdir_p($directory) && !is_dir($directory)) { throw_if( is_file($directory), sprintf('Unable to create the "%s" directory. A similar named file exists.', $directory), @@ -128,7 +117,7 @@ protected function get_target_file(string $directory, ?string $name = null) ); throw_anyway(sprintf('Unable to create the "%s" directory.', $directory)); - } elseif (!is_writable($directory)) { + } elseif (!wp_is_writable($directory)) { throw_anyway(sprintf('Unable to write in the "%s" directory.', $directory)); } diff --git a/src/Filesystem/UploadedFile.php b/src/Filesystem/UploadedFile.php index 769a9db..aef6d95 100644 --- a/src/Filesystem/UploadedFile.php +++ b/src/Filesystem/UploadedFile.php @@ -262,29 +262,17 @@ public function move(string $directory, ?string $name = null) if ($this->is_valid()) { $target = $this->get_target_file($directory, $name); - // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_set_error_handler -- Captures a PHP warning from rename()/move_uploaded_file() into a catchable value; restored in the finally block immediately after. - set_error_handler(static function ($type, $msg) use (&$error) { - $error = $msg; - }); + $filesystem = Container::get_instance()->make(Filesystem::class); - try { - $moved = move_uploaded_file($this->getPathname(), $target); - } finally { - restore_error_handler(); - } + $moved = $filesystem->move($this->getPathname(), $target); throw_unless( $moved, - message( - 'upload.move_failed', - $this->getPathname(), - $target, - wp_strip_all_tags($error ?? '') - ), + message('upload.move_failed', $this->getPathname(), $target), Exception::class ); - @chmod($target, 0666 & ~umask()); + $filesystem->chmod($target, 0666 & ~umask()); return $target; } diff --git a/src/Http/Superglobals.php b/src/Http/Superglobals.php index 94eb5d8..3d0bd0d 100644 --- a/src/Http/Superglobals.php +++ b/src/Http/Superglobals.php @@ -127,7 +127,9 @@ protected static function read(array $superglobal, ?string $key, $default, $type // A single-key read expects a scalar; an unexpected array (e.g. a crafted // "name[]=x" request) is treated as absent rather than stringified by Sanitizer. - if (!is_scalar($value)) { + // The array sanitization type is the deliberate exception: an array value is + // exactly what it expects, and Sanitizer::apply_rule() already handles it. + if (!is_scalar($value) && $type !== Sanitizer::ARRAY) { return $default; } diff --git a/src/Managers/EventManager.php b/src/Managers/EventManager.php index 0f9eaf8..a75c34b 100644 --- a/src/Managers/EventManager.php +++ b/src/Managers/EventManager.php @@ -165,6 +165,7 @@ protected function resolve($listener, $event) { throw_unless(is_subclass_of($listener, Listener::class), sprintf( 'The listener [%s] must be a subclass of [%s]', + $listener, Listener::class ), InvalidArgumentException::class); diff --git a/src/Supports/MessagesBag.php b/src/Supports/MessagesBag.php index 5a241a1..11f2c08 100644 --- a/src/Supports/MessagesBag.php +++ b/src/Supports/MessagesBag.php @@ -173,7 +173,7 @@ protected function defaults() ], 'upload' => [ 'directory_unavailable' => 'Upload directory is not available.', - 'move_failed' => 'Could not move the file "%s" to "%s" (%s).', + 'move_failed' => 'Could not move the file "%s" to "%s".', 'ini_size_exceeded' => 'The file "%s" exceeds your upload_max_filesize ini directive (limit is %d KiB).', 'form_size_exceeded' => 'The file "%s" exceeds the upload limit defined in your form.', 'partial' => 'The file "%s" was only partially uploaded.', diff --git a/tests/Support/Cache/TestFilesystem.php b/tests/Support/Cache/TestFilesystem.php index cdef166..0922b9b 100644 --- a/tests/Support/Cache/TestFilesystem.php +++ b/tests/Support/Cache/TestFilesystem.php @@ -104,6 +104,15 @@ public function move($path, $target) return rename($path, $target); } + public function chmod($path, $mode = null) + { + if ($mode) { + return chmod($path, $mode); + } + + return substr(sprintf('%o', fileperms($path)), -4); + } + public function delete($paths, bool $recursive = true) { $paths = is_array($paths) ? $paths : [$paths]; diff --git a/tests/Support/Filesystem/UploadedFileStub.php b/tests/Support/Filesystem/UploadedFileStub.php new file mode 100644 index 0000000..f37c27e --- /dev/null +++ b/tests/Support/Filesystem/UploadedFileStub.php @@ -0,0 +1,19 @@ +', ':', ';', ',', "'", '"', '&', '$', '#', '*', '(', ')', '|', '~', '`', '!', '{', '}', '%', '+']; + $filename = str_replace($special_chars, '', $filename); + $filename = preg_replace('/\s+/', '-', trim($filename)); + + return trim($filename, '.-_'); + } +} diff --git a/tests/Unit/Filesystem/FileTest.php b/tests/Unit/Filesystem/FileTest.php new file mode 100644 index 0000000..a19fc41 --- /dev/null +++ b/tests/Unit/Filesystem/FileTest.php @@ -0,0 +1,148 @@ +base_dir = sys_get_temp_dir() . '/framework-file-' . uniqid(); + mkdir($this->base_dir, 0777, true); + + $this->files = new TestFilesystem(); + $this->bind_filesystem($this->files); + } + + protected function tearDown(): void + { + $this->files->delete($this->base_dir, true); + unset($GLOBALS['framework_test_unwritable_paths']); + + parent::tearDown(); + } + + protected function bind_filesystem(Filesystem $filesystem): void + { + $container = new Container(); + $container->instance('app', $container); + $container->instance(Filesystem::class, $filesystem); + + $this->set_container_instance($container); + } + + protected function make_source_file(string $name = 'source.txt', string $contents = 'hello'): string + { + $path = $this->base_dir . '/' . $name; + file_put_contents($path, $contents); + + return $path; + } + + public function test_move_creates_missing_target_directory(): void + { + $source = $this->make_source_file(); + $target_dir = $this->base_dir . '/uploads'; + + $result = (new File($source))->move($target_dir); + + $this->assertDirectoryExists($target_dir); + $this->assertFileExists($target_dir . '/source.txt'); + $this->assertSame($target_dir . '/source.txt', $result->getPathname()); + } + + public function test_move_into_existing_directory_succeeds(): void + { + $source = $this->make_source_file(); + $target_dir = $this->base_dir . '/uploads'; + mkdir($target_dir, 0777, true); + + (new File($source))->move($target_dir); + + $this->assertFileExists($target_dir . '/source.txt'); + } + + public function test_move_with_custom_name_uses_given_name(): void + { + $source = $this->make_source_file(); + $target_dir = $this->base_dir . '/uploads'; + + $result = (new File($source))->move($target_dir, 'renamed.txt'); + + $this->assertSame('renamed.txt', basename($result->getPathname())); + $this->assertFileExists($target_dir . '/renamed.txt'); + } + + public function test_move_sets_normalized_permissions(): void + { + $source = $this->make_source_file(); + $target_dir = $this->base_dir . '/uploads'; + + $result = (new File($source))->move($target_dir); + + $expected = 0666 & ~umask(); + $actual = fileperms($result->getPathname()) & 0777; + + $this->assertSame($expected, $actual); + } + + public function test_move_throws_when_target_path_is_blocked_by_a_file(): void + { + $source = $this->make_source_file(); + $blocking_path = $this->base_dir . '/blocked'; + file_put_contents($blocking_path, 'i am a file, not a directory'); + + $this->expectException(Exception::class); + $this->expectExceptionMessageMatches('/similar named file exists/'); + + (new File($source))->move($blocking_path); + } + + public function test_move_throws_when_target_directory_is_not_writable(): void + { + $source = $this->make_source_file(); + $target_dir = $this->base_dir . '/readonly'; + mkdir($target_dir, 0777, true); + + $GLOBALS['framework_test_unwritable_paths'] = [$target_dir]; + + $this->expectException(Exception::class); + $this->expectExceptionMessageMatches('/Unable to write in the/'); + + (new File($source))->move($target_dir); + } + + public function test_move_throws_when_underlying_move_fails(): void + { + $source = $this->make_source_file(); + $target_dir = $this->base_dir . '/uploads'; + + $this->bind_filesystem(new class extends TestFilesystem { + public function move($path, $target) + { + return false; + } + }); + + $this->expectException(Exception::class); + $this->expectExceptionMessage(sprintf( + 'Could not move the file "%s" to "%s".', + $source, + $target_dir . '/source.txt' + )); + + (new File($source))->move($target_dir); + } +} diff --git a/tests/Unit/Filesystem/UploadedFileTest.php b/tests/Unit/Filesystem/UploadedFileTest.php new file mode 100644 index 0000000..2834a1a --- /dev/null +++ b/tests/Unit/Filesystem/UploadedFileTest.php @@ -0,0 +1,118 @@ +base_dir = sys_get_temp_dir() . '/framework-uploaded-file-' . uniqid(); + mkdir($this->base_dir, 0777, true); + + $this->files = new TestFilesystem(); + + $container = new Container(); + $container->instance('app', $container); + $container->instance(Filesystem::class, $this->files); + + $this->set_container_instance($container); + } + + protected function tearDown(): void + { + $this->files->delete($this->base_dir, true); + + unset( + $GLOBALS['framework_test_uploaded_files'], + $GLOBALS['framework_test_upload_dir'], + $GLOBALS['framework_test_user_can'] + ); + + parent::tearDown(); + } + + protected function make_tmp_upload(string $name = 'source.txt', string $contents = 'hello'): string + { + $path = $this->base_dir . '/' . $name; + file_put_contents($path, $contents); + + return $path; + } + + public function test_valid_uploaded_file_moves_successfully_with_normalized_permissions(): void + { + $tmp = $this->make_tmp_upload(); + $GLOBALS['framework_test_uploaded_files'] = [$tmp]; + + $uploaded = new UploadedFile($tmp, 'source.txt', 'text/plain', \UPLOAD_ERR_OK, strlen('hello')); + + $target_dir = $this->base_dir . '/uploads'; + $result = $uploaded->move($target_dir); + + $this->assertFileExists($target_dir . '/source.txt'); + + $expected = 0666 & ~umask(); + $actual = fileperms($result->getPathname()) & 0777; + + $this->assertSame($expected, $actual); + } + + public function test_file_not_genuinely_uploaded_is_rejected_before_any_relocation(): void + { + $tmp = $this->make_tmp_upload(); + + // Deliberately not added to framework_test_uploaded_files: is_uploaded_file() stays false. + $uploaded = new UploadedFile($tmp, 'source.txt', 'text/plain', \UPLOAD_ERR_OK, strlen('hello')); + + $target_dir = $this->base_dir . '/uploads'; + + $this->expectException(Exception::class); + + try { + $uploaded->move($target_dir); + } finally { + $this->assertFileExists($tmp); + $this->assertDirectoryDoesNotExist($target_dir); + } + } + + public function test_upload_error_throws_via_error_message_without_attempting_a_move(): void + { + $uploaded = new UploadedFile('/nonexistent/path.txt', 'source.txt', null, \UPLOAD_ERR_NO_FILE, 0); + + $this->expectException(Exception::class); + $this->expectExceptionMessage('No file was uploaded.'); + + $uploaded->move($this->base_dir . '/uploads'); + } + + public function test_store_and_store_as_succeed_end_to_end_through_filesystem_upload(): void + { + $GLOBALS['framework_test_upload_dir'] = $this->base_dir; + + $tmp = $this->make_tmp_upload('avatar.png', 'binary-ish contents'); + $GLOBALS['framework_test_uploaded_files'] = [$tmp]; + + $uploaded = new UploadedFile($tmp, 'avatar.png', 'image/png', \UPLOAD_ERR_OK, strlen('binary-ish contents')); + + $stored_path = $uploaded->store('profile-photos'); + + $this->assertFileExists($stored_path); + $this->assertStringContainsString('profile-photos', $stored_path); + } +} diff --git a/tests/Unit/Http/SuperglobalsTest.php b/tests/Unit/Http/SuperglobalsTest.php index 0e035ce..69841cc 100644 --- a/tests/Unit/Http/SuperglobalsTest.php +++ b/tests/Unit/Http/SuperglobalsTest.php @@ -70,6 +70,24 @@ public function test_non_scalar_value_is_treated_as_absent(): void $_POST = []; } + public function test_array_typed_single_key_read_returns_the_array(): void + { + $_GET = ['cat_ids' => ['1', '2']]; + + $this->assertSame(['1', '2'], Superglobals::query('cat_ids', [], Sanitizer::ARRAY)); + + $_GET = []; + } + + public function test_array_value_is_still_rejected_when_a_non_array_type_is_requested(): void + { + $_GET = ['cat_ids' => ['1', '2']]; + + $this->assertSame('fallback', Superglobals::query('cat_ids', 'fallback', Sanitizer::INT)); + + $_GET = []; + } + public function test_files_returns_unslashed_whole_array(): void { $_FILES = ['upload' => ['name' => "photo\\'s.png", 'error' => 0]];