diff --git a/phpBB/phpbb/files/filespec.php b/phpBB/phpbb/files/filespec.php index ed64b7ff5c..e07aef9892 100644 --- a/phpBB/phpbb/files/filespec.php +++ b/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 $local = false; + /** @var bool Whether file is local */ + 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) { diff --git a/phpBB/phpbb/files/upload.php b/phpBB/phpbb/files/upload.php index 234eb69735..397eb5af36 100644 --- a/phpBB/phpbb/files/upload.php +++ b/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)); } diff --git a/tests/files/upload_test.php b/tests/files/upload_test.php index 057bb8a4f2..dc0080d25c 100644 --- a/tests/files/upload_test.php +++ b/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() diff --git a/tests/functional/fileupload_remote_test.php b/tests/functional/fileupload_remote_test.php index a754d8c91d..a6aa233aaf 100644 --- a/tests/functional/fileupload_remote_test.php +++ b/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() diff --git a/tests/upload/filespec_test.php b/tests/upload/filespec_test.php index b0df72a6ff..f885b1acfc 100644 --- a/tests/upload/filespec_test.php +++ b/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')); } }