From 567eefb2bd2bf280391786ea171dad0bdb0b442d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 12 Mar 2013 22:35:31 +0100 Subject: [PATCH 1/9] [ticket/11362] Correctly sanitise the directory path We need to correctly remove ../ form the path if possible by removing the previous folder aswell. Otherwise the finder is unable to locate /adm/style directories in extensions as he is looking for /adm/../adm/style instead. PHPBB3-11362 --- phpBB/includes/extension/finder.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/includes/extension/finder.php b/phpBB/includes/extension/finder.php index f71e32bc8d..d9aacc38ff 100644 --- a/phpBB/includes/extension/finder.php +++ b/phpBB/includes/extension/finder.php @@ -227,7 +227,7 @@ class phpbb_extension_finder */ protected function sanitise_directory($directory) { - $directory = preg_replace('#(?:^|/)\./#', '/', $directory); + $directory = phpbb_clean_path($directory); $dir_len = strlen($directory); if ($dir_len > 1 && $directory[$dir_len - 1] === '/') From 1fd5be859e3fe85e67fe7f5632e4630b96ca3634 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 12 Mar 2013 22:45:44 +0100 Subject: [PATCH 2/9] [ticket/11362] Add unit test for ../ in directory paths PHPBB3-11362 --- tests/extension/finder_test.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/extension/finder_test.php b/tests/extension/finder_test.php index c96b11a73c..3781591b19 100644 --- a/tests/extension/finder_test.php +++ b/tests/extension/finder_test.php @@ -6,6 +6,7 @@ * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2 * */ +require_once dirname(__FILE__) . '/../../phpBB/includes/functions.php'; class phpbb_extension_finder_test extends phpbb_test_case { @@ -142,6 +143,21 @@ class phpbb_extension_finder_test extends phpbb_test_case ); } + public function test_uncleansub_directory_get_classes() + { + $classes = $this->finder + ->directory('/sub/../sub/type') + ->get_classes(); + + sort($classes); + $this->assertEquals( + array( + 'phpbb_ext_foo_sub_type_alternative', + ), + $classes + ); + } + /** * These do not work because of changes with PHPBB3-11386 * They do not seem neccessary to me, so I am commenting them out for now From 869e00a23b0400b9ad81c1130227cc4c29d6a38d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Apr 2013 17:45:49 +0200 Subject: [PATCH 3/9] [ticket/11362] Move phpbb_clean_path into a simple filesystem service PHPBB3-11362 --- phpBB/config/services.yml | 4 ++ phpBB/includes/filesystem.php | 52 +++++++++++++++++++ phpBB/includes/functions.php | 30 ----------- .../clean_path_test.php | 20 ++++--- 4 files changed, 68 insertions(+), 38 deletions(-) create mode 100644 phpBB/includes/filesystem.php rename tests/{functions => filesystem}/clean_path_test.php (71%) diff --git a/phpBB/config/services.yml b/phpBB/config/services.yml index b9c71844dc..5febe76c49 100644 --- a/phpBB/config/services.yml +++ b/phpBB/config/services.yml @@ -142,9 +142,13 @@ services: - @ext.manager - %core.root_path% - @cache.driver + - @filesystem - .%core.php_ext% - _ext_finder + filesystem: + class: phpbb_filesystem + groupposition.legend: class: phpbb_groupposition_legend arguments: diff --git a/phpBB/includes/filesystem.php b/phpBB/includes/filesystem.php new file mode 100644 index 0000000000..27cab48fb0 --- /dev/null +++ b/phpBB/includes/filesystem.php @@ -0,0 +1,52 @@ +filesystem = new phpbb_filesystem(); + } + + public function clean_path_data() { return array( array('foo', 'foo'), @@ -33,12 +39,10 @@ class phpbb_clean_path_test extends phpbb_test_case } /** - * @dataProvider clean_path_test_data + * @dataProvider clean_path_data */ public function test_clean_path($input, $expected) { - $output = phpbb_clean_path($input); - - $this->assertEquals($expected, $output); + $this->assertEquals($expected, $this->filesystem->clean_path($input)); } } From 158bce02095b1fbff19955cbad19be3d1b1a3f80 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Apr 2013 17:50:44 +0200 Subject: [PATCH 4/9] [ticket/11362] Use new filesystem class in finder PHPBB3-11362 --- phpBB/includes/extension/finder.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/extension/finder.php b/phpBB/includes/extension/finder.php index d9aacc38ff..02a9ebb8c3 100644 --- a/phpBB/includes/extension/finder.php +++ b/phpBB/includes/extension/finder.php @@ -23,6 +23,7 @@ if (!defined('IN_PHPBB')) class phpbb_extension_finder { protected $extension_manager; + protected $filesystem; protected $phpbb_root_path; protected $cache; protected $php_ext; @@ -54,15 +55,17 @@ class phpbb_extension_finder * @param phpbb_extension_manager $extension_manager An extension manager * instance that provides the finder with a list of active * extensions and their locations + * @param phpbb_filesystem $filesystem Filesystem instance * @param string $phpbb_root_path Path to the phpbb root directory * @param phpbb_cache_driver_interface $cache A cache instance or null * @param string $php_ext php file extension * @param string $cache_name The name of the cache variable, defaults to * _ext_finder */ - public function __construct(phpbb_extension_manager $extension_manager, $phpbb_root_path = '', phpbb_cache_driver_interface $cache = null, $php_ext = '.php', $cache_name = '_ext_finder') + public function __construct(phpbb_extension_manager $extension_manager, phpbb_filesystem $filesystem, $phpbb_root_path = '', phpbb_cache_driver_interface $cache = null, $php_ext = '.php', $cache_name = '_ext_finder') { $this->extension_manager = $extension_manager; + $this->filesystem = $filesystem; $this->phpbb_root_path = $phpbb_root_path; $this->cache = $cache; $this->php_ext = $php_ext; @@ -227,7 +230,7 @@ class phpbb_extension_finder */ protected function sanitise_directory($directory) { - $directory = phpbb_clean_path($directory); + $directory = $this->filesystem->clean_path($directory); $dir_len = strlen($directory); if ($dir_len > 1 && $directory[$dir_len - 1] === '/') From d7fb934a2f6fbce86563d692b9689eb5c76e31dd Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Apr 2013 17:52:00 +0200 Subject: [PATCH 5/9] [ticket/11362] Replace other calls to phpbb_clean_path Need to instantiate the object manually here, as the container is not yet set up. PHPBB3-11362 --- phpBB/common.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/phpBB/common.php b/phpBB/common.php index c33e2cbb1f..6dd65739fc 100644 --- a/phpBB/common.php +++ b/phpBB/common.php @@ -44,8 +44,11 @@ if (!defined('PHPBB_INSTALLED')) // Replace any number of consecutive backslashes and/or slashes with a single slash // (could happen on some proxy setups and/or Windows servers) $script_path = preg_replace('#[\\\\/]{2,}#', '/', $script_path); + // Eliminate . and .. from the path - $script_path = phpbb_clean_path($script_path); + require($phpbb_root_path . 'includes/filesystem.' . $phpEx); + $phpbb_filesystem = new phpbb_filesystem(); + $script_path = $phpbb_filesystem->clean_path($script_path); $url = (($secure) ? 'https://' : 'http://') . $server_name; From 423b310e2acbbc72814a4278a4cccf2900114f85 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Apr 2013 18:43:19 +0200 Subject: [PATCH 6/9] [ticket/11362] Extension manager depends on filesystem PHPBB3-11362 --- phpBB/config/services.yml | 1 + phpBB/includes/extension/manager.php | 6 ++++-- tests/dbal/migrator_test.php | 1 + tests/extension/finder_test.php | 3 ++- tests/extension/manager_test.php | 1 + tests/extension/metadata_manager_test.php | 1 + tests/mock/extension_manager.php | 1 + tests/test_framework/phpbb_functional_test_case.php | 1 + 8 files changed, 12 insertions(+), 3 deletions(-) diff --git a/phpBB/config/services.yml b/phpBB/config/services.yml index 5febe76c49..fb3e7aa964 100644 --- a/phpBB/config/services.yml +++ b/phpBB/config/services.yml @@ -131,6 +131,7 @@ services: - @dbal.conn - @config - @migrator + - @filesystem - %tables.ext% - %core.root_path% - .%core.php_ext% diff --git a/phpBB/includes/extension/manager.php b/phpBB/includes/extension/manager.php index 44a30c6280..de9a3937c3 100644 --- a/phpBB/includes/extension/manager.php +++ b/phpBB/includes/extension/manager.php @@ -44,13 +44,14 @@ class phpbb_extension_manager * @param phpbb_db_driver $db A database connection * @param phpbb_config $config phpbb_config * @param phpbb_db_migrator $migrator + * @param phpbb_filesystem $filesystem * @param string $extension_table The name of the table holding extensions * @param string $phpbb_root_path Path to the phpbb includes directory. * @param string $php_ext php file extension * @param phpbb_cache_driver_interface $cache A cache instance or null * @param string $cache_name The name of the cache variable, defaults to _ext */ - public function __construct(ContainerInterface $container, phpbb_db_driver $db, phpbb_config $config, phpbb_db_migrator $migrator, $extension_table, $phpbb_root_path, $php_ext = '.php', phpbb_cache_driver_interface $cache = null, $cache_name = '_ext') + public function __construct(ContainerInterface $container, phpbb_db_driver $db, phpbb_config $config, phpbb_db_migrator $migrator, phpbb_filesystem $filesystem, $extension_table, $phpbb_root_path, $php_ext = '.php', phpbb_cache_driver_interface $cache = null, $cache_name = '_ext') { $this->container = $container; $this->phpbb_root_path = $phpbb_root_path; @@ -58,6 +59,7 @@ class phpbb_extension_manager $this->config = $config; $this->migrator = $migrator; $this->cache = $cache; + $this->filesystem = $filesystem; $this->php_ext = $php_ext; $this->extension_table = $extension_table; $this->cache_name = $cache_name; @@ -510,7 +512,7 @@ class phpbb_extension_manager */ public function get_finder() { - return new phpbb_extension_finder($this, $this->phpbb_root_path, $this->cache, $this->php_ext, $this->cache_name . '_finder'); + return new phpbb_extension_finder($this, $this->filesystem, $this->phpbb_root_path, $this->cache, $this->php_ext, $this->cache_name . '_finder'); } /** diff --git a/tests/dbal/migrator_test.php b/tests/dbal/migrator_test.php index 89669b85ec..745d260b38 100644 --- a/tests/dbal/migrator_test.php +++ b/tests/dbal/migrator_test.php @@ -60,6 +60,7 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case $this->db, $this->config, $this->migrator, + new phpbb_filesystem(), 'phpbb_ext', dirname(__FILE__) . '/../../phpBB/', '.php', diff --git a/tests/extension/finder_test.php b/tests/extension/finder_test.php index 3781591b19..73c07ef79a 100644 --- a/tests/extension/finder_test.php +++ b/tests/extension/finder_test.php @@ -164,7 +164,7 @@ class phpbb_extension_finder_test extends phpbb_test_case public function test_get_classes_create_cache() { $cache = new phpbb_mock_cache; - $finder = new phpbb_extension_finder($this->extension_manager, dirname(__FILE__) . '/', $cache, '.php', '_custom_cache_name'); + $finder = new phpbb_extension_finder($this->extension_manager, new phpbb_filesystem(), dirname(__FILE__) . '/', $cache, '.php', '_custom_cache_name'); $files = $finder->suffix('_class.php')->get_files(); $expected_files = array( @@ -204,6 +204,7 @@ class phpbb_extension_finder_test extends phpbb_test_case $finder = new phpbb_extension_finder( $this->extension_manager, + new phpbb_filesystem(), dirname(__FILE__) . '/', new phpbb_mock_cache(array( '_ext_finder' => array( diff --git a/tests/extension/manager_test.php b/tests/extension/manager_test.php index 1f311116f4..d6bcb97586 100644 --- a/tests/extension/manager_test.php +++ b/tests/extension/manager_test.php @@ -112,6 +112,7 @@ class phpbb_extension_manager_test extends phpbb_database_test_case $db, $config, $migrator, + new phpbb_filesystem(), 'phpbb_ext', dirname(__FILE__) . '/', '.' . $php_ext, diff --git a/tests/extension/metadata_manager_test.php b/tests/extension/metadata_manager_test.php index 081a32e277..df7817b479 100644 --- a/tests/extension/metadata_manager_test.php +++ b/tests/extension/metadata_manager_test.php @@ -64,6 +64,7 @@ class metadata_manager_test extends phpbb_database_test_case $this->db, $this->config, $this->migrator, + new phpbb_filesystem(), 'phpbb_ext', $this->phpbb_root_path, $this->phpEx, diff --git a/tests/mock/extension_manager.php b/tests/mock/extension_manager.php index fdda4cbadc..954f2bf1c4 100644 --- a/tests/mock/extension_manager.php +++ b/tests/mock/extension_manager.php @@ -14,5 +14,6 @@ class phpbb_mock_extension_manager extends phpbb_extension_manager $this->phpbb_root_path = $phpbb_root_path; $this->php_ext = '.php'; $this->extensions = $extensions; + $this->filesystem = new phpbb_filesystem(); } } diff --git a/tests/test_framework/phpbb_functional_test_case.php b/tests/test_framework/phpbb_functional_test_case.php index 887dfea3b5..891fe237b3 100644 --- a/tests/test_framework/phpbb_functional_test_case.php +++ b/tests/test_framework/phpbb_functional_test_case.php @@ -153,6 +153,7 @@ class phpbb_functional_test_case extends phpbb_test_case $db, $config, $migrator, + new phpbb_filesystem(), self::$config['table_prefix'] . 'ext', dirname(__FILE__) . '/', '.' . $php_ext, From ffe9f2a93e7bb91378e462b44a786252bddbd0be Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Apr 2013 20:25:30 +0200 Subject: [PATCH 7/9] [ticket/11362] Fix service description of finder PHPBB3-11362 --- phpBB/config/services.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/config/services.yml b/phpBB/config/services.yml index fb3e7aa964..19dfb3e23b 100644 --- a/phpBB/config/services.yml +++ b/phpBB/config/services.yml @@ -141,9 +141,9 @@ services: class: phpbb_extension_finder arguments: - @ext.manager + - @filesystem - %core.root_path% - @cache.driver - - @filesystem - .%core.php_ext% - _ext_finder From ccd4a725da5269189cdb73a3d7048359a5d2cd4d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 18 Apr 2013 09:53:02 +0200 Subject: [PATCH 8/9] [ticket/11362] Add compatibility function phpbb_clean_path() again The function first depends on the container, but also works without it and without autoload. The reason for this is, it might be used before that stuff is set up, like it has been in our common.php PHPBB3-11362 --- phpBB/includes/functions.php | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index 39a8dbc880..998c52b7c6 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -1046,6 +1046,38 @@ else } } +/** +* Eliminates useless . and .. components from specified path. +* +* Deprecated, use filesystem class instead +* +* @param string $path Path to clean +* @return string Cleaned path +* +* @deprecated +*/ +function phpbb_clean_path($path) +{ + global $phpbb_container; + + if ($phpbb_container) + { + $phpbb_filesystem = new phpbb_filesystem(); + } + else + { + // The container is not yet loaded, use a new instance + if (!class_exists('phpbb_filesystem')) + { + global $phpbb_root_path, $phpEx; + require($phpbb_root_path . 'includes/filesystem.' . $phpEx); + } + $phpbb_filesystem = new phpbb_filesystem(); + } + + return $phpbb_filesystem->clean_path($path); +} + // functions used for building option fields /** From 16e70fa08610227d96e149eba2019803ad37c85f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 22 Apr 2013 00:49:41 +0200 Subject: [PATCH 9/9] [ticket/11362] Use container when available instead of creating a new instance PHPBB3-11362 --- phpBB/includes/functions.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index 998c52b7c6..231825525f 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -1062,7 +1062,7 @@ function phpbb_clean_path($path) if ($phpbb_container) { - $phpbb_filesystem = new phpbb_filesystem(); + $phpbb_filesystem = $phpbb_container->get('filesystem'); } else {