From f52a5732e6a77d4ba819cf7950b7ce41675eb2df Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 22 May 2024 11:55:35 -0700 Subject: [PATCH 1/9] Add comments and annotations --- php/WP_CLI/FileCache.php | 6 +++--- php/WP_CLI/WpHttpCacheManager.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/php/WP_CLI/FileCache.php b/php/WP_CLI/FileCache.php index ee5681e3b1..a8126dc4eb 100644 --- a/php/WP_CLI/FileCache.php +++ b/php/WP_CLI/FileCache.php @@ -156,7 +156,7 @@ public function read( $key, $ttl = null ) { * Copy a file into the cache * * @param string $key cache key - * @param string $source source filename + * @param string $source source filename; tmp file filepath from HTTP response * @return bool */ public function import( $key, $source ) { @@ -339,7 +339,7 @@ protected function ensure_dir_exists( $dir ) { * Prepare cache write * * @param string $key cache key - * @return bool|string filename or false + * @return bool|string The destination filename or false when cache disabled, key invalid, or directory creation failed. */ protected function prepare_write( $key ) { if ( ! $this->enabled ) { @@ -381,7 +381,7 @@ protected function validate_key( $key ) { } /** - * Filename from key + * Destination filename from key * * @param string $key * @return string filename diff --git a/php/WP_CLI/WpHttpCacheManager.php b/php/WP_CLI/WpHttpCacheManager.php index 23dc58f739..0c1cbffca1 100644 --- a/php/WP_CLI/WpHttpCacheManager.php +++ b/php/WP_CLI/WpHttpCacheManager.php @@ -12,7 +12,7 @@ class WpHttpCacheManager { /** - * @var array map whitelisted urls to keys and ttls + * @var array map whitelisted urls to keys and ttls */ protected $whitelist = []; From 6f0b0e98f8059c4693cc45f36844766f5e7a88d6 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 22 May 2024 12:03:06 -0700 Subject: [PATCH 2/9] Add `FileCacheTest::test_import()` --- tests/test-file-cache.php | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/test-file-cache.php b/tests/test-file-cache.php index d4841f01fc..c69bf27621 100644 --- a/tests/test-file-cache.php +++ b/tests/test-file-cache.php @@ -102,4 +102,30 @@ public function test_export() { unlink( $target ); rmdir( $target_dir ); } + + public function test_import() { + $max_size = 32; + $ttl = 60; + $cache_dir = Utils\get_temp_dir() . uniqid( 'wp-cli-test-file-cache', true ); + $cache = new FileCache( $cache_dir, $ttl, $max_size ); + + // "$group/$slug-$version.$ext"; + $key = 'plugin/my-fixture-plugin-1.0.0.zip'; + $fixture_filepath = sys_get_temp_dir() . '/my-downloaded-fixture-plugin-1.0.0.zip'; + + $zip = new ZipArchive(); + $zip->open( $fixture_filepath, ZIPARCHIVE::CREATE ); + $zip->addFile( __FILE__ ); + $zip->close(); + + $result = $cache->import( $key, $fixture_filepath ); + + // Assert file is imported. + self::assertTrue( $result ); + self::assertFileExists( "{$cache_dir}/{$key}" ); + + // Clean up. + $cache->clear(); + unlink( $fixture_filepath ); + } } From 32ba58dbf4d68ed257c3f5b213702ad52bde1bac Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 22 May 2024 13:17:59 -0700 Subject: [PATCH 3/9] Handle `Failed to open stream: Permission denied` in `FileCache::import()` --- php/WP_CLI/FileCache.php | 4 ++++ tests/test-file-cache.php | 43 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/php/WP_CLI/FileCache.php b/php/WP_CLI/FileCache.php index a8126dc4eb..a5eaef5fd4 100644 --- a/php/WP_CLI/FileCache.php +++ b/php/WP_CLI/FileCache.php @@ -162,6 +162,10 @@ public function read( $key, $ttl = null ) { public function import( $key, $source ) { $filename = $this->prepare_write( $key ); + if ( ! is_readable( $source ) ) { + return false; + } + if ( $filename ) { return copy( $source, $filename ) && touch( $filename ); } diff --git a/tests/test-file-cache.php b/tests/test-file-cache.php index c69bf27621..8b510cedd0 100644 --- a/tests/test-file-cache.php +++ b/tests/test-file-cache.php @@ -128,4 +128,47 @@ public function test_import() { $cache->clear(); unlink( $fixture_filepath ); } + + /** + * @see https://github.com/wp-cli/wp-cli/pull/5947 + */ + public function test_import_do_not_use_cache_file_cannot_be_read() { + $max_size = 32; + $ttl = 60; + $cache_dir = Utils\get_temp_dir() . uniqid( 'wp-cli-test-file-cache', true ); + $cache = new FileCache( $cache_dir, $ttl, $max_size ); + + $tmp_dir = Utils\get_temp_dir() . uniqid( 'wp-cli-test-file-cache-import', true ); + mkdir( $tmp_dir ); + + $key = 'plugin/my-fixture-plugin-1.0.0.zip'; + $fixture_filepath = $tmp_dir . '/my-bad-permissions-fixture-plugin-1.0.0.zip'; + + $zip = new ZipArchive(); + $zip->open( $fixture_filepath, ZIPARCHIVE::CREATE ); + $zip->addFile( __FILE__ ); + $zip->close(); + + chmod( $fixture_filepath, 0000 ); + + // "Warning: copy(-.): Failed to open stream: Permission denied". + $error = null; + set_error_handler( + function ( $errno, $errstr ) use ( &$error ) { + $error = $errstr; + } + ); + + $result = $cache->import( $key, $fixture_filepath ); + + restore_error_handler(); + + self::assertNull( $error ); + self::assertFalse( $result ); + + // Clean up. + $cache->clear(); + unlink( $fixture_filepath ); + rmdir( $tmp_dir ); + } } From ccbda25b7d7fc0b191e820d6ef046c2377534ce7 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 22 May 2024 13:18:16 -0700 Subject: [PATCH 4/9] Update test to use its own temp dir --- tests/test-file-cache.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test-file-cache.php b/tests/test-file-cache.php index 8b510cedd0..504a4e0ed0 100644 --- a/tests/test-file-cache.php +++ b/tests/test-file-cache.php @@ -109,9 +109,12 @@ public function test_import() { $cache_dir = Utils\get_temp_dir() . uniqid( 'wp-cli-test-file-cache', true ); $cache = new FileCache( $cache_dir, $ttl, $max_size ); + $tmp_dir = Utils\get_temp_dir() . uniqid( 'wp-cli-test-file-cache-import', true ); + mkdir( $tmp_dir ); + // "$group/$slug-$version.$ext"; $key = 'plugin/my-fixture-plugin-1.0.0.zip'; - $fixture_filepath = sys_get_temp_dir() . '/my-downloaded-fixture-plugin-1.0.0.zip'; + $fixture_filepath = $tmp_dir . '/my-downloaded-fixture-plugin-1.0.0.zip'; $zip = new ZipArchive(); $zip->open( $fixture_filepath, ZIPARCHIVE::CREATE ); From 9e826e7ad9cad54da73b9e8b50a957fb90b6874f Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 22 May 2024 13:27:06 -0700 Subject: [PATCH 5/9] Remove temp dir after test --- tests/test-file-cache.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test-file-cache.php b/tests/test-file-cache.php index 504a4e0ed0..b72f441828 100644 --- a/tests/test-file-cache.php +++ b/tests/test-file-cache.php @@ -130,6 +130,7 @@ public function test_import() { // Clean up. $cache->clear(); unlink( $fixture_filepath ); + rmdir( $tmp_dir ); } /** From c38a3713eb14701682d473ec10457541309b8103 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 22 May 2024 14:13:04 -0700 Subject: [PATCH 6/9] Remove trailing `.` from validated `$key` which are used as filenames --- php/WP_CLI/FileCache.php | 2 +- tests/test-file-cache.php | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/php/WP_CLI/FileCache.php b/php/WP_CLI/FileCache.php index a5eaef5fd4..787bf2610f 100644 --- a/php/WP_CLI/FileCache.php +++ b/php/WP_CLI/FileCache.php @@ -381,7 +381,7 @@ protected function validate_key( $key ) { $parts = preg_replace( "#[^{$this->whitelist}]#i", '-', $parts ); - return implode( '/', $parts ); + return rtrim( implode( '/', $parts ), '.' ); } /** diff --git a/tests/test-file-cache.php b/tests/test-file-cache.php index b72f441828..d889537928 100644 --- a/tests/test-file-cache.php +++ b/tests/test-file-cache.php @@ -175,4 +175,31 @@ function ( $errno, $errstr ) use ( &$error ) { unlink( $fixture_filepath ); rmdir( $tmp_dir ); } + + /** + * Windows filenames cannot end in periods. + * + * @covers \WP_CLI\FileCache::validate_key + * + * @see https://github.com/wp-cli/wp-cli/pull/5947 + * @see https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions + */ + public function test_validate_key_ending_in_period() { + $max_size = 32; + $ttl = 60; + $cache_dir = Utils\get_temp_dir() . uniqid( 'wp-cli-test-file-cache', true ); + $cache = new FileCache( $cache_dir, $ttl, $max_size ); + + $key = 'plugin/advanced-sidebar-menu-pro-9.5.7.'; + + $reflection = new ReflectionClass( $cache ); + + $method = $reflection->getMethod( 'validate_key' ); + $method->setAccessible( true ); + + $result = $method->invoke( $cache, $key ); + + self::assertFalse( str_ends_with( $result, '.' ) ); + self::assertEquals('plugin/advanced-sidebar-menu-pro-9.5.7', $result); + } } From 5f347dc781e449dec5afc1c5c691c86723b50ae6 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 22 May 2024 14:37:12 -0700 Subject: [PATCH 7/9] Fix incorrect comment --- php/WP_CLI/FileCache.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/php/WP_CLI/FileCache.php b/php/WP_CLI/FileCache.php index 787bf2610f..cfc6480e14 100644 --- a/php/WP_CLI/FileCache.php +++ b/php/WP_CLI/FileCache.php @@ -343,7 +343,7 @@ protected function ensure_dir_exists( $dir ) { * Prepare cache write * * @param string $key cache key - * @return bool|string The destination filename or false when cache disabled, key invalid, or directory creation failed. + * @return bool|string The destination filename or false when cache disabled or directory creation fails. */ protected function prepare_write( $key ) { if ( ! $this->enabled ) { From c04d67456dbe1f9a08175f4fbcf9c8b52395d324 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Wed, 22 May 2024 14:49:29 -0700 Subject: [PATCH 8/9] lint --- tests/test-file-cache.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-file-cache.php b/tests/test-file-cache.php index d889537928..53a16e6dd1 100644 --- a/tests/test-file-cache.php +++ b/tests/test-file-cache.php @@ -200,6 +200,6 @@ public function test_validate_key_ending_in_period() { $result = $method->invoke( $cache, $key ); self::assertFalse( str_ends_with( $result, '.' ) ); - self::assertEquals('plugin/advanced-sidebar-menu-pro-9.5.7', $result); + self::assertEquals( 'plugin/advanced-sidebar-menu-pro-9.5.7', $result ); } } From a970cd47af049e463cf98fcc984b423edd921ac6 Mon Sep 17 00:00:00 2001 From: Brian Henry Date: Thu, 23 May 2024 10:49:27 -0700 Subject: [PATCH 9/9] Apply suggestions from code review Co-authored-by: Pascal Birchler --- tests/test-file-cache.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/test-file-cache.php b/tests/test-file-cache.php index 53a16e6dd1..301397fe0c 100644 --- a/tests/test-file-cache.php +++ b/tests/test-file-cache.php @@ -124,8 +124,8 @@ public function test_import() { $result = $cache->import( $key, $fixture_filepath ); // Assert file is imported. - self::assertTrue( $result ); - self::assertFileExists( "{$cache_dir}/{$key}" ); + $this->assertTrue( $result ); + $this->assertFileExists( "{$cache_dir}/{$key}" ); // Clean up. $cache->clear(); @@ -167,8 +167,8 @@ function ( $errno, $errstr ) use ( &$error ) { restore_error_handler(); - self::assertNull( $error ); - self::assertFalse( $result ); + $this->assertNull( $error ); + $this->assertFalse( $result ); // Clean up. $cache->clear(); @@ -199,7 +199,7 @@ public function test_validate_key_ending_in_period() { $result = $method->invoke( $cache, $key ); - self::assertFalse( str_ends_with( $result, '.' ) ); - self::assertEquals( 'plugin/advanced-sidebar-menu-pro-9.5.7', $result ); + $this->assertStringEndsNotWith( '.', $result ); + $this->assertSame( 'plugin/advanced-sidebar-menu-pro-9.5.7', $result ); } }