Merge pull request #1290 from nickvergessen/ticket/11362

Correctly sanitise the directory path in finder

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;
 

phpBB/config/services.yml

@@ -131,6 +131,7 @@ services:
             - @dbal.conn
             - @config
             - @migrator
+            - @filesystem
             - %tables.ext%
             - %core.root_path%
             - .%core.php_ext%
@@ -140,11 +141,15 @@ services:
         class: phpbb_extension_finder
         arguments:
             - @ext.manager
+            - @filesystem
             - %core.root_path%
             - @cache.driver
             - .%core.php_ext%
             - _ext_finder
 
+    filesystem:
+        class: phpbb_filesystem
+
     groupposition.legend:
         class: phpbb_groupposition_legend
         arguments:

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 = preg_replace('#(?:^|/)\./#', '/', $directory);
+		$directory = $this->filesystem->clean_path($directory);
 		$dir_len = strlen($directory);
 
 		if ($dir_len > 1 && $directory[$dir_len - 1] === '/')

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');
 	}
 
 	/**

phpBB/includes/filesystem.php

@@ -0,0 +1,52 @@
+<?php
+/**
+*
+* @package phpBB3
+* @copyright (c) 2013 phpBB Group
+* @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2
+*
+*/
+/**
+* @ignore
+*/
+if (!defined('IN_PHPBB'))
+{
+	exit;
+}
+
+/**
+* A class with various functions that are related to paths, files and the filesystem
+* @package phpBB3
+*/
+class phpbb_filesystem
+{
+	/**
+	* Eliminates useless . and .. components from specified path.
+	*
+	* @param string $path Path to clean
+	* @return string Cleaned path
+	*/
+	public function clean_path($path)
+	{
+		$exploded = explode('/', $path);
+		$filtered = array();
+		foreach ($exploded as $part)
+		{
+			if ($part === '.' && !empty($filtered))
+			{
+				continue;
+			}
+
+			if ($part === '..' && !empty($filtered) && $filtered[sizeof($filtered) - 1] !== '..')
+			{
+				array_pop($filtered);
+			}
+			else
+			{
+				$filtered[] = $part;
+			}
+		}
+		$path = implode('/', $filtered);
+		return $path;
+	}
+}

phpBB/includes/functions.php

@@ -1049,31 +1049,33 @@ 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)
 {
-	$exploded = explode('/', $path);
+	global $phpbb_container;
-	$filtered = array();
-	foreach ($exploded as $part)
-	{
-		if ($part === '.' && !empty($filtered))
-		{
-			continue;
-		}
 
-		if ($part === '..' && !empty($filtered) && $filtered[sizeof($filtered) - 1] !== '..')
+	if ($phpbb_container)
-		{
+	{
-			array_pop($filtered);
+		$phpbb_filesystem = $phpbb_container->get('filesystem');
-		}
-		else
-		{
-			$filtered[] = $part;
-		}
 	}
-	$path = implode('/', $filtered);
+	else
-	return $path;
+	{
+		// 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

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',

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,13 +143,28 @@ 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
 	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(
@@ -188,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(

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,

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,

tests/filesystem/clean_path_test.php

@@ -7,11 +7,17 @@
 *
 */
 
-require_once dirname(__FILE__) . '/../../phpBB/includes/functions.php';
+class phpbb_filesystem_clean_path_test extends phpbb_test_case
-
-class phpbb_clean_path_test extends phpbb_test_case
 {
-	public function clean_path_test_data()
+	protected $filesystem;
+
+	public function setUp()
+	{
+		parent::setUp();
+		$this->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, $this->filesystem->clean_path($input));
-
-		$this->assertEquals($expected, $output);
 	}
 }

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();
 	}
 }

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,