[ticket/13904] Set visibility in files and improve test coverage

PHPBB3-13904

phpBB/phpbb/files/filespec.php

@@ -22,49 +22,52 @@ use \phpbb\language\language;
 class filespec
 {
 	/** @var string File name */
-	var $filename = '';
+	protected $filename = '';
 
 	/** @var string Real name of file */
-	var $realname = '';
+	protected $realname = '';
 
 	/** @var string Upload name of file */
-	var $uploadname = '';
+	protected $uploadname = '';
 
 	/** @var string Mimetype of file */
-	var $mimetype = '';
+	protected $mimetype = '';
 
 	/** @var string File extension */
-	var $extension = '';
+	public $extension = '';
 
 	/** @var int File size */
-	var $filesize = 0;
+	public $filesize = 0;
 
 	/** @var int Width of file */
-	var $width = 0;
+	protected $width = 0;
 
 	/** @var int Height of file */
-	var $height = 0;
+	protected $height = 0;
 
 	/** @var array Image info including type and size */
-	var $image_info = array();
+	protected $image_info = array();
 
 	/** @var string Destination file name */
-	var $destination_file = '';
+	protected $destination_file = '';
 
 	/** @var string Destination file path */
-	var $destination_path = '';
+	protected $destination_path = '';
 
 	/** @var bool Whether file was moved */
-	var $file_moved = false;
+	public $file_moved = false;
 
-	/** @var bool  Whether file is local */
+	/** @var bool Whether file is local */
-	var $local = false;
+	public $local = false;
+
+	/** @var bool Class initialization flag */
+	protected $class_initialized = false;
 
 	/** @var array Error array */
-	var $error = array();
+	public $error = array();
 
 	/** @var upload Instance of upload class  */
-	var $upload;
+	public $upload;
 
 	/**
 	 * @var \phpbb\filesystem\filesystem_interface
@@ -98,7 +101,7 @@ class filespec
 	 * @param \phpbb\mimetype\guesser					$mimetype_guesser Mime type guesser
 	 * @param \phpbb\plupload\plupload					$plupload Plupload
 	 */
-	function __construct(\phpbb\filesystem\filesystem_interface $phpbb_filesystem, language $language, $phpbb_root_path, \phpbb\mimetype\guesser $mimetype_guesser = null, \phpbb\plupload\plupload $plupload = null)
+	public function __construct(\phpbb\filesystem\filesystem_interface $phpbb_filesystem, language $language, $phpbb_root_path, \phpbb\mimetype\guesser $mimetype_guesser = null, \phpbb\plupload\plupload $plupload = null)
 	{
 		$this->plupload = $plupload;
 		$this->mimetype_guesser = $mimetype_guesser;
@@ -116,6 +119,12 @@ class filespec
 	 */
 	public function set_upload_ary($upload_ary)
 	{
+		if (!isset($upload_ary) || !sizeof($upload_ary))
+		{
+			return $this;
+		}
+
+		$this->class_initialized = true;
 		$this->filename = $upload_ary['tmp_name'];
 		$this->filesize = $upload_ary['size'];
 		$name = (STRIP) ? stripslashes($upload_ary['name']) : $upload_ary['name'];
@@ -165,7 +174,7 @@ class filespec
 	 */
 	public function init_error()
 	{
-		return !isset($this->filename);
+		return !$this->class_initialized;
 	}
 
 	/**
@@ -193,7 +202,7 @@ class filespec
 	 *
 	 *@access public
 	 */
-	function clean_filename($mode = 'unique', $prefix = '', $user_id = '')
+	public function clean_filename($mode = 'unique', $prefix = '', $user_id = '')
 	{
 		if ($this->init_error())
 		{
@@ -216,22 +225,21 @@ class filespec
 				$this->realname = preg_replace("/%(\w{2})/", '_', $this->realname);
 
 				$this->realname = $prefix . $this->realname . '.' . $this->extension;
-				break;
+			break;
 
 			case 'unique':
 				$this->realname = $prefix . md5(unique_id());
-				break;
+			break;
 
 			case 'avatar':
 				$this->extension = strtolower($this->extension);
 				$this->realname = $prefix . $user_id . '.' . $this->extension;
 
-				break;
+			break;
 
 			case 'unique_ext':
 			default:
 				$this->realname = $prefix . md5(unique_id()) . '.' . $this->extension;
-				break;
 		}
 	}
 
@@ -242,7 +250,7 @@ class filespec
 	 *
 	 * @return mixed Content of property
 	 */
-	function get($property)
+	public function get($property)
 	{
 		if ($this->init_error() || !isset($this->$property))
 		{
@@ -257,7 +265,7 @@ class filespec
 	 *
 	 * @return bool true if it is an image, false if not
 	 */
-	function is_image()
+	public function is_image()
 	{
 		return (strpos($this->mimetype, 'image/') === 0);
 	}
@@ -267,7 +275,7 @@ class filespec
 	 *
 	 * @return bool true if it is a valid upload, false if not
 	 */
-	function is_uploaded()
+	public function is_uploaded()
 	{
 		$is_plupload = $this->plupload && $this->plupload->is_active();
 
@@ -287,7 +295,7 @@ class filespec
 	/**
 	 * Remove file
 	 */
-	function remove()
+	public function remove()
 	{
 		if ($this->file_moved)
 		{
@@ -321,7 +329,7 @@ class filespec
 	 * @param string $filename Filename that needs to be checked
 	 * @return string Mime type of supplied filename
 	 */
-	function get_mimetype($filename)
+	public function get_mimetype($filename)
 	{
 		if ($this->mimetype_guesser !== null)
 		{
@@ -343,7 +351,7 @@ class filespec
 	 *
 	 * @return int File size
 	 */
-	function get_filesize($filename)
+	public function get_filesize($filename)
 	{
 		return @filesize($filename);
 	}
@@ -356,7 +364,7 @@ class filespec
 	 *
 	 * @return bool False if disallowed content found, true if not
 	 */
-	function check_content($disallowed_content)
+	public function check_content($disallowed_content)
 	{
 		if (empty($disallowed_content))
 		{
@@ -393,7 +401,7 @@ class filespec
 	 * @return bool True if file was moved, false if not
 	 * @access public
 	 */
-	function move_file($destination, $overwrite = false, $skip_image_check = false, $chmod = false)
+	public function move_file($destination, $overwrite = false, $skip_image_check = false, $chmod = false)
 	{
 		if (sizeof($this->error))
 		{
@@ -443,7 +451,7 @@ class filespec
 						}
 					}
 
-					break;
+				break;
 
 				case 'move':
 
@@ -455,7 +463,7 @@ class filespec
 						}
 					}
 
-					break;
+				break;
 
 				case 'local':
 
@@ -464,7 +472,7 @@ class filespec
 						$this->error[] = $this->language->lang($this->upload->error_prefix . 'GENERAL_UPLOAD_ERROR', $this->destination_file);
 					}
 
-					break;
+				break;
 			}
 
 			// Remove temporary filename
@@ -544,7 +552,7 @@ class filespec
 	 *
 	 * @return bool False if issue was found, true if not
 	 */
-	function additional_checks()
+	public function additional_checks()
 	{
 		if (!$this->file_moved)
 		{

phpBB/phpbb/files/upload.php

@@ -24,31 +24,31 @@ use \phpbb\request\request_interface;
 class upload
 {
 	/** @var array Allowed file extensions */
-	var $allowed_extensions = array();
+	public $allowed_extensions = array();
 
 	/** @var array Disallowed content */
-	var $disallowed_content = array('body', 'head', 'html', 'img', 'plaintext', 'a href', 'pre', 'script', 'table', 'title');
+	protected $disallowed_content = array('body', 'head', 'html', 'img', 'plaintext', 'a href', 'pre', 'script', 'table', 'title');
 
 	/** @var int Maximum filesize */
-	var $max_filesize = 0;
+	public $max_filesize = 0;
 
 	/** @var int Minimum width of images */
-	var $min_width = 0;
+	public $min_width = 0;
 
 	/** @var int Minimum height of images */
-	var $min_height = 0;
+	public $min_height = 0;
 
 	/** @var int Maximum width of images */
-	var $max_width = 0;
+	public $max_width = 0;
 
 	/** @var int Maximum height of images */
-	var $max_height = 0;
+	public $max_height = 0;
 
 	/** @var string Prefix for language variables of errors */
-	var $error_prefix = '';
+	public $error_prefix = '';
 
 	/** @var int Timeout for remote upload */
-	var $upload_timeout = 6;
+	public $upload_timeout = 6;
 
 	/** @var filesystem_interface */
 	protected $filesystem;
@@ -86,7 +86,7 @@ class upload
 	/**
 	 * Reset vars
 	 */
-	function reset_vars()
+	public function reset_vars()
 	{
 		$this->max_filesize = 0;
 		$this->min_width = $this->min_height = $this->max_width = $this->max_height = 0;
@@ -102,7 +102,7 @@ class upload
 	 *
 	 * @return \phpbb\files\upload This instance of upload
 	 */
-	function set_allowed_extensions($allowed_extensions)
+	public function set_allowed_extensions($allowed_extensions)
 	{
 		if ($allowed_extensions !== false && is_array($allowed_extensions))
 		{
@@ -122,7 +122,7 @@ class upload
 	 *
 	 * @return \phpbb\files\upload This instance of upload
 	 */
-	function set_allowed_dimensions($min_width, $min_height, $max_width, $max_height)
+	public function set_allowed_dimensions($min_width, $min_height, $max_width, $max_height)
 	{
 		$this->min_width = (int) $min_width;
 		$this->min_height = (int) $min_height;
@@ -139,7 +139,7 @@ class upload
 	 *
 	 * @return \phpbb\files\upload This instance of upload
 	 */
-	function set_max_filesize($max_filesize)
+	public function set_max_filesize($max_filesize)
 	{
 		if ($max_filesize !== false && (int) $max_filesize)
 		{
@@ -156,7 +156,7 @@ class upload
 	 *
 	 * @return \phpbb\files\upload This instance of upload
 	 */
-	function set_disallowed_content($disallowed_content)
+	public function set_disallowed_content($disallowed_content)
 	{
 		if ($disallowed_content !== false && is_array($disallowed_content))
 		{
@@ -173,7 +173,7 @@ class upload
 	 *
 	 * @return \phpbb\files\upload This instance of upload
 	 */
-	function set_error_prefix($error_prefix)
+	public function set_error_prefix($error_prefix)
 	{
 		$this->error_prefix = $error_prefix;
 
@@ -264,7 +264,7 @@ class upload
 	 *
 	 * @param filespec $file Instance of filespec class
 	 */
-	function common_checks(&$file)
+	public function common_checks(&$file)
 	{
 		// Filesize is too big or it's 0 if it was larger than the maxsize in the upload form
 		if ($this->max_filesize && ($file->get('filesize') > $this->max_filesize || $file->get('filesize') == 0))
@@ -300,7 +300,7 @@ class upload
 	 *
 	 * @return bool True if extension is allowed, false if not
 	 */
-	function valid_extension(&$file)
+	public function valid_extension(&$file)
 	{
 		return (in_array($file->get('extension'), $this->allowed_extensions)) ? true : false;
 	}
@@ -313,7 +313,7 @@ class upload
 	 * @return bool True if dimensions are valid or no constraints set, false
 	 *			if not
 	 */
-	function valid_dimensions(&$file)
+	public function valid_dimensions(&$file)
 	{
 		if (!$this->max_width && !$this->max_height && !$this->min_width && !$this->min_height)
 		{
@@ -338,7 +338,7 @@ class upload
 	 *
 	 * @return bool True if form upload is valid, false if not
 	 */
-	function is_valid($form_name)
+	public function is_valid($form_name)
 	{
 		$upload = $this->request->file($form_name);
 
@@ -353,7 +353,7 @@ class upload
 	 *
 	 * @return bool True if content is valid, false if not
 	 */
-	function valid_content(&$file)
+	public function valid_content(&$file)
 	{
 		return ($file->check_content($this->disallowed_content));
 	}

tests/files/upload_test.php

@@ -76,16 +76,19 @@ class phpbb_files_upload_test extends phpbb_test_case
 	public function test_set_disallowed_content()
 	{
 		$upload = new \phpbb\files\upload($this->filesystem, $this->factory, $this->language, $this->request, $this->phpbb_root_path);
+		$disallowed_content = new ReflectionProperty($upload, 'disallowed_content');
+		$disallowed_content->setAccessible(true);
+
 		$upload->set_disallowed_content(array('foo'));
-		$this->assertEquals(array('foo'), $upload->disallowed_content);
+		$this->assertEquals(array('foo'), $disallowed_content->getValue($upload));
 		$upload->set_disallowed_content(array('foo', 'bar', 'meh'));
-		$this->assertEquals(array('foo', 'bar', 'meh'), $upload->disallowed_content);
+		$this->assertEquals(array('foo', 'bar', 'meh'), $disallowed_content->getValue($upload));
 		$upload->set_disallowed_content('');
-		$this->assertEquals(array('foo', 'bar', 'meh'), $upload->disallowed_content);
+		$this->assertEquals(array('foo', 'bar', 'meh'), $disallowed_content->getValue($upload));
 		$this->assertINstanceOf('\phpbb\files\upload', $upload->set_disallowed_content(array()));
-		$this->assertEquals(array(), $upload->disallowed_content);
+		$this->assertEquals(array(), $disallowed_content->getValue($upload));
 		$upload->reset_vars();
-		$this->assertEquals(array(), $upload->disallowed_content);
+		$this->assertEquals(array(), $disallowed_content->getValue($upload));
 	}
 
 	public function test_is_valid()

tests/functional/fileupload_remote_test.php

@@ -98,7 +98,7 @@ class phpbb_functional_fileupload_remote_test extends phpbb_functional_test_case
 			->set_max_filesize(1000);
 		$file = $upload->handle_upload('remote', self::$root_url . 'styles/prosilver/theme/images/forum_read.gif');
 		$this->assertEquals(0, sizeof($file->error));
-		$this->assertTrue(file_exists($file->filename));
+		$this->assertTrue(file_exists($file->get('filename')));
 	}
 
 	public function test_too_large()

tests/upload/filespec_test.php

@@ -133,7 +133,7 @@ class phpbb_filespec_test extends phpbb_test_case
 	{
 		$upload = new phpbb_mock_fileupload();
 		$filespec = $this->get_filespec();
-		$filespec->upload = $upload;
+		$filespec->set_upload_namespace($upload);
 		$filespec->file_moved = true;
 		$filespec->filesize = $filespec->get_filesize($this->path . $filename);
 
@@ -174,6 +174,7 @@ class phpbb_filespec_test extends phpbb_test_case
 			array($chunks[2] . $chunks[9]),
 			array($chunks[3] . $chunks[4]),
 			array($chunks[5] . $chunks[6]),
+			array('foobar.png'),
 		);
 	}
 
@@ -185,7 +186,7 @@ class phpbb_filespec_test extends phpbb_test_case
 		$bad_chars = array("'", "\\", ' ', '/', ':', '*', '?', '"', '<', '>', '|');
 		$filespec = $this->get_filespec(array('name' => $filename));
 		$filespec->clean_filename('real', self::PREFIX);
-		$name = $filespec->realname;
+		$name = $filespec->get('realname');
 
 		$this->assertEquals(0, preg_match('/%(\w{2})/', $name));
 		foreach ($bad_chars as $char)
@@ -201,7 +202,7 @@ class phpbb_filespec_test extends phpbb_test_case
 		{
 			$filespec = $this->get_filespec();
 			$filespec->clean_filename('unique', self::PREFIX);
-			$name = $filespec->realname;
+			$name = $filespec->get('realname');
 
 			$this->assertEquals(strlen($name), 32 + strlen(self::PREFIX));
 			$this->assertRegExp('#^[A-Za-z0-9]+$#', substr($name, strlen(self::PREFIX)));
@@ -210,6 +211,55 @@ class phpbb_filespec_test extends phpbb_test_case
 		}
 	}
 
+	public function test_clean_filename_unique_ext()
+	{
+		$filenames = array();
+		for ($tests = 0; $tests < self::TEST_COUNT; $tests++)
+		{
+			$filespec = $this->get_filespec(array('name' => 'foobar.jpg'));
+			$filespec->clean_filename('unique_ext', self::PREFIX);
+			$name = $filespec->get('realname');
+
+			$this->assertEquals(strlen($name), 32 + strlen(self::PREFIX) + strlen('.jpg'));
+			$this->assertRegExp('#^[A-Za-z0-9]+\.jpg$#', substr($name, strlen(self::PREFIX)));
+			$this->assertFalse(isset($filenames[$name]));
+			$filenames[$name] = true;
+		}
+	}
+
+	public function data_clean_filename_avatar()
+	{
+		return array(
+			array(false, false, ''),
+			array('foobar.png', 'u5.png', 'avatar', 'u', 5),
+			array('foobar.png', 'g9.png', 'avatar', 'g', 9),
+
+		);
+	}
+
+	/**
+	 * @dataProvider data_clean_filename_avatar
+	 */
+	public function test_clean_filename_avatar($filename, $expected, $mode, $prefix = '', $user_id = '')
+	{
+		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, $this->phpbb_root_path, $this->mimetype_guesser);
+
+		if ($filename)
+		{
+			$upload_ary = array(
+				'name' => $filename,
+				'type' => '',
+				'size' => '',
+				'tmp_name' => '',
+				'error' => '',
+			);
+			$filespec->set_upload_ary($upload_ary);
+		}
+		$filespec->clean_filename($mode, $prefix, $user_id);
+
+		$this->assertSame($expected, $filespec->get('realname'));
+	}
+
 	public function get_extension_variables()
 	{
 		return array(
@@ -312,7 +362,7 @@ class phpbb_filespec_test extends phpbb_test_case
 			'type' => $mime_type,
 		));
 		$filespec->extension = $extension;
-		$filespec->upload = $upload;
+		$filespec->set_upload_namespace($upload);
 		$filespec->local = true;
 
 		$this->assertEquals($expected, $filespec->move_file($this->path . 'copies'));
@@ -336,6 +386,6 @@ class phpbb_filespec_test extends phpbb_test_case
 		$type_cast_helper->set_var($upload_name, $filename, 'string', true, true);
 		$filespec = $this->get_filespec(array('name'=> $upload_name));
 
-		$this->assertSame(trim(utf8_basename(htmlspecialchars($filename))), $filespec->uploadname);
+		$this->assertSame(trim(utf8_basename(htmlspecialchars($filename))), $filespec->get('uploadname'));
 	}
 }