From b33f17e7ce630f5860f26f4f7c1c5824dd9472ec Mon Sep 17 00:00:00 2001 From: Ruben Calvo Date: Tue, 24 Dec 2024 19:30:05 +0100 Subject: [PATCH] [ticket/17361] Use filesystem classes for tempnam and temp dir. Code reviews. PHPBB-17361 --- .../default/container/services_console.yml | 1 + .../console/command/thumbnail/generate.php | 24 ++++++-- .../console/command/thumbnail/recreate.php | 1 + .../data/v400/storage_backup_data.php | 8 +-- phpBB/phpbb/filesystem/temp.php | 55 +++++++++++++------ phpBB/phpbb/storage/file_tracker.php | 27 ++++----- tests/console/thumbnail_test.php | 8 ++- 7 files changed, 86 insertions(+), 38 deletions(-) diff --git a/phpBB/config/default/container/services_console.yml b/phpBB/config/default/container/services_console.yml index e1c965ce17..3cfacb080b 100644 --- a/phpBB/config/default/container/services_console.yml +++ b/phpBB/config/default/container/services_console.yml @@ -299,6 +299,7 @@ services: - '@cache' - '@language' - '@storage.attachment' + - '@filesystem.temp' - '%core.root_path%' - '%core.php_ext%' tags: diff --git a/phpBB/phpbb/console/command/thumbnail/generate.php b/phpBB/phpbb/console/command/thumbnail/generate.php index c0bda918f4..38e3646648 100644 --- a/phpBB/phpbb/console/command/thumbnail/generate.php +++ b/phpBB/phpbb/console/command/thumbnail/generate.php @@ -13,8 +13,10 @@ namespace phpbb\console\command\thumbnail; +use phpbb\attachment\attachment_category; use phpbb\cache\service; use phpbb\db\driver\driver_interface; +use phpbb\filesystem\temp; use phpbb\language\language; use phpbb\storage\storage; use phpbb\user; @@ -22,6 +24,7 @@ use Symfony\Component\Console\Command\Command as symfony_command; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; +use Symfony\Component\Filesystem\Filesystem as symfony_filesystem; class generate extends \phpbb\console\command\command { @@ -41,11 +44,21 @@ class generate extends \phpbb\console\command\command */ protected $language; + /** + * @var symfony_filesystem + */ + protected $symfony_filesystem; + /** * @var storage */ protected $storage; + /** + * @var temp + */ + protected $temp; + /** * phpBB root path * @var string @@ -67,15 +80,18 @@ class generate extends \phpbb\console\command\command * @param service $cache The cache service * @param language $language Language * @param storage $storage Storage + * @param temp $temp Temp * @param string $phpbb_root_path Root path * @param string $php_ext PHP extension */ - public function __construct(user $user, driver_interface $db, service $cache, language $language, storage $storage, string $phpbb_root_path, string $php_ext) + public function __construct(user $user, driver_interface $db, service $cache, language $language, storage $storage, temp $temp, string $phpbb_root_path, string $php_ext) { $this->db = $db; $this->cache = $cache; + $this->symfony_filesystem = new symfony_filesystem(); $this->language = $language; $this->storage = $storage; + $this->temp = $temp; $this->phpbb_root_path = $phpbb_root_path; $this->php_ext = $php_ext; @@ -145,10 +161,10 @@ class generate extends \phpbb\console\command\command $thumbnail_created = array(); while ($row = $this->db->sql_fetchrow($result)) { - if (isset($extensions[$row['extension']]['display_cat']) && $extensions[$row['extension']]['display_cat'] == \phpbb\attachment\attachment_category::IMAGE) + if (isset($extensions[$row['extension']]['display_cat']) && $extensions[$row['extension']]['display_cat'] == attachment_category::IMAGE) { - $source = tempnam(sys_get_temp_dir(), 'thumbnail_source'); - $destination = tempnam(sys_get_temp_dir(), 'thumbnail_destination'); + $source = $this->symfony_filesystem->tempnam($this->temp->get_dir(), 'thumbnail_source'); + $destination = $this->symfony_filesystem->tempnam($this->temp->get_dir(), 'thumbnail_destination'); file_put_contents($source, $this->storage->read($row['physical_filename'])); diff --git a/phpBB/phpbb/console/command/thumbnail/recreate.php b/phpBB/phpbb/console/command/thumbnail/recreate.php index a0a0524b89..4b774c251d 100644 --- a/phpBB/phpbb/console/command/thumbnail/recreate.php +++ b/phpBB/phpbb/console/command/thumbnail/recreate.php @@ -29,6 +29,7 @@ class recreate extends \phpbb\console\command\command /** * Constructor * + * @param user $user User * @param language $language Language */ public function __construct(user $user, language $language) diff --git a/phpBB/phpbb/db/migration/data/v400/storage_backup_data.php b/phpBB/phpbb/db/migration/data/v400/storage_backup_data.php index 772a03b945..190445a04f 100644 --- a/phpBB/phpbb/db/migration/data/v400/storage_backup_data.php +++ b/phpBB/phpbb/db/migration/data/v400/storage_backup_data.php @@ -36,11 +36,11 @@ class storage_backup_data extends migration $methods = ['sql', 'sql.gz', 'sql.bz2']; $dir = $this->phpbb_root_path . 'store/'; - $dh = @opendir($dir); + $handle = @opendir($dir); - if ($dh) + if ($handle) { - while (($file = readdir($dh)) !== false) + while (($file = readdir($handle)) !== false) { if (preg_match('#^backup_(\d{10,})_(?:[a-z\d]{16}|[a-z\d]{32})\.(sql(?:\.(?:gz|bz2))?)$#i', $file, $matches)) { @@ -58,7 +58,7 @@ class storage_backup_data extends migration } - closedir($dh); + closedir($handle); } } } diff --git a/phpBB/phpbb/filesystem/temp.php b/phpBB/phpbb/filesystem/temp.php index 649221d802..4c8a71d600 100644 --- a/phpBB/phpbb/filesystem/temp.php +++ b/phpBB/phpbb/filesystem/temp.php @@ -13,8 +13,20 @@ namespace phpbb\filesystem; +use phpbb\filesystem\exception\filesystem_exception; + class temp { + /** + * @var filesystem + */ + protected $filesystem; + + /** + * @var string + */ + protected $cache_temp_dir; + /** * @var string Temporary directory path */ @@ -23,23 +35,10 @@ class temp /** * Constructor */ - public function __construct($filesystem, $cache_temp_dir) + public function __construct(filesystem $filesystem, string $cache_temp_dir) { - $tmp_dir = (function_exists('sys_get_temp_dir')) ? sys_get_temp_dir() : ''; - - // Prevent trying to write to system temp dir in case of open_basedir - // restrictions being in effect - if (empty($tmp_dir) || !@file_exists($tmp_dir) || !@is_writable($tmp_dir)) - { - $tmp_dir = $cache_temp_dir; - - if (!is_dir($tmp_dir)) - { - $filesystem->mkdir($tmp_dir, 0777); - } - } - - $this->temp_dir = helper::realpath($tmp_dir); + $this->filesystem = $filesystem; + $this->cache_temp_dir = $cache_temp_dir; } /** @@ -49,6 +48,30 @@ class temp */ public function get_dir() { + if ($this->temp_dir === null) + { + $tmp_dir = (function_exists('sys_get_temp_dir')) ? sys_get_temp_dir() : ''; + + // Prevent trying to write to system temp dir in case of open_basedir + // restrictions being in effect + if (empty($tmp_dir) || !@file_exists($tmp_dir) || !@is_writable($tmp_dir)) + { + $tmp_dir = $this->cache_temp_dir; + + if (!is_dir($tmp_dir)) + { + $this->filesystem->mkdir($tmp_dir, 0777); + } + } + + $this->temp_dir = helper::realpath($tmp_dir); + + if ($this->temp_dir === false) + { + throw new filesystem_exception('FILESYSTEM_CANNOT_CREATE_DIRECTORY', $tmp_dir); + } + } + return $this->temp_dir; } } diff --git a/phpBB/phpbb/storage/file_tracker.php b/phpBB/phpbb/storage/file_tracker.php index c858b733ac..ab6f1c8a11 100644 --- a/phpBB/phpbb/storage/file_tracker.php +++ b/phpBB/phpbb/storage/file_tracker.php @@ -19,7 +19,6 @@ use phpbb\storage\exception\storage_exception; class file_tracker { - /** * @var db */ @@ -59,11 +58,11 @@ class file_tracker */ public function track_file(string $storage, string $path, int $size): void { - $sql_ary = array( + $sql_ary = [ 'file_path' => $path, 'storage' => $storage, 'filesize' => $size, - ); + ]; $sql = 'INSERT INTO ' . $this->storage_table . $this->db->sql_build_array('INSERT', $sql_ary); $this->db->sql_query($sql); @@ -80,10 +79,10 @@ class file_tracker */ public function untrack_file(string $storage, $path): void { - $sql_ary = array( + $sql_ary = [ 'file_path' => $path, 'storage' => $storage, - ); + ]; $sql = 'DELETE FROM ' . $this->storage_table . ' WHERE ' . $this->db->sql_build_array('DELETE', $sql_ary); @@ -103,13 +102,14 @@ class file_tracker */ public function is_tracked(string $storage, string $path): bool { - $sql_ary = array( + $sql_ary = [ 'file_path' => $path, 'storage' => $storage, - ); + ]; - $sql = 'SELECT file_id FROM ' . $this->storage_table . ' - WHERE ' . $this->db->sql_build_array('SELECT', $sql_ary); + $sql = 'SELECT file_id + FROM ' . $this->storage_table . ' + WHERE ' . $this->db->sql_build_array('SELECT', $sql_ary); $result = $this->db->sql_query($sql); $row = $this->db->sql_fetchrow($result); $this->db->sql_freeresult($result); @@ -128,13 +128,14 @@ class file_tracker */ public function file_size(string $storage, string $path): int { - $sql_ary = array( + $sql_ary = [ 'file_path' => $path, 'storage' => $storage, - ); + ]; - $sql = 'SELECT filesize FROM ' . $this->storage_table . ' - WHERE ' . $this->db->sql_build_array('SELECT', $sql_ary); + $sql = 'SELECT filesize + FROM ' . $this->storage_table . ' + WHERE ' . $this->db->sql_build_array('SELECT', $sql_ary); $result = $this->db->sql_query($sql); $row = $this->db->sql_fetchrow($result); $this->db->sql_freeresult($result); diff --git a/tests/console/thumbnail_test.php b/tests/console/thumbnail_test.php index ebe0dff62e..3a006d27d7 100644 --- a/tests/console/thumbnail_test.php +++ b/tests/console/thumbnail_test.php @@ -25,6 +25,7 @@ class phpbb_console_command_thumbnail_test extends phpbb_database_test_case protected $cache; protected $user; protected $storage; + protected $temp; protected $phpEx; protected $phpbb_root_path; protected $application; @@ -75,8 +76,13 @@ class phpbb_console_command_thumbnail_test extends phpbb_database_test_case unlink($phpbb_root_path . 'files/' . $path); }); + $this->temp = $this->createMock('\phpbb\filesystem\temp'); + $this->temp->method('get_dir')->willReturnCallback(function () { + return sys_get_temp_dir(); + }); + $this->application = new Application(); - $this->application->add(new generate($this->user, $this->db, $this->cache, $this->language, $this->storage, $this->phpbb_root_path, $this->phpEx)); + $this->application->add(new generate($this->user, $this->db, $this->cache, $this->language, $this->storage, $this->temp, $this->phpbb_root_path, $this->phpEx)); $this->application->add(new delete($this->user, $this->db, $this->language, $this->storage)); $this->application->add(new recreate($this->user, $this->language));