From 9bc6e641bfbe53c3920662ee8e24a723ac34e6a1 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 9 Jan 2014 23:50:40 +0100 Subject: [PATCH 1/9] [ticket/11148] Add mimetype guesser to filespec and fileupload class The mimetype guesser will be used to get the mimetype of uploaded files. Until now, this was only used for files uploaded with plupload. If a file doesn't have a mimetype supplied, we will now try to get the correct mimetype. PHPBB3-11148 --- phpBB/includes/functions_upload.php | 60 +++++++++++++---------------- tests/upload/filespec_test.php | 39 ++++++++++++++++++- 2 files changed, 65 insertions(+), 34 deletions(-) diff --git a/phpBB/includes/functions_upload.php b/phpBB/includes/functions_upload.php index c640865212..fb153a9e0c 100644 --- a/phpBB/includes/functions_upload.php +++ b/phpBB/includes/functions_upload.php @@ -52,11 +52,17 @@ class filespec */ protected $plupload; + /** + * phpBB Mimetype guesser + * @var \phpbb\mimetype\guesser + */ + protected $mimetype_guesser; + /** * File Class * @access private */ - function filespec($upload_ary, $upload_namespace, \phpbb\plupload\plupload $plupload = null) + function filespec($upload_ary, $upload_namespace, \phpbb\mimetype\guesser $mimetype_guesser = null, \phpbb\plupload\plupload $plupload = null) { if (!isset($upload_ary)) { @@ -90,6 +96,7 @@ class filespec $this->local = (isset($upload_ary['local_mode'])) ? true : false; $this->upload = $upload_namespace; $this->plupload = $plupload; + $this->mimetype_guesser = $mimetype_guesser; } /** @@ -215,25 +222,22 @@ class filespec } /** - * Get mimetype. Utilize mime_content_type if the function exist. - * Not used at the moment... + * Get mimetype + * */ function get_mimetype($filename) { - $mimetype = ''; - - if (function_exists('mime_content_type')) + if ($this->mimetype_guesser !== null) { - $mimetype = mime_content_type($filename); + $mimetype = $this->mimetype_guesser->guess($filename); + + if ($mimetype !== 'application/octet-stream') + { + $this->mimetype = $mimetype; + } } - // Some browsers choke on a mimetype of application/octet-stream - if (!$mimetype || $mimetype == 'application/octet-stream') - { - $mimetype = 'application/octetstream'; - } - - return $mimetype; + return $this->mimetype; } /** @@ -372,6 +376,9 @@ class filespec // Try to get real filesize from destination folder $this->filesize = (@filesize($this->destination_file)) ? @filesize($this->destination_file) : $this->filesize; + // Get mimetype of supplied file + $this->mimetype = $this->get_mimetype($this->destination_file); + if ($this->is_image() && !$skip_image_check) { $this->width = $this->height = 0; @@ -580,7 +587,7 @@ class fileupload * @return object $file Object "filespec" is returned, all further operations can be done with this object * @access public */ - function form_upload($form_name, \phpbb\plupload\plupload $plupload = null) + function form_upload($form_name, \phpbb\mimetype\guesser $mimetype_guesser = null, \phpbb\plupload\plupload $plupload = null) { global $user, $request; @@ -596,7 +603,7 @@ class fileupload } } - $file = new filespec($upload, $this, $plupload); + $file = new filespec($upload, $this, $mimetype_guesser, $plupload); if ($file->init_error) { @@ -656,7 +663,7 @@ class fileupload /** * Move file from another location to phpBB */ - function local_upload($source_file, $filedata = false) + function local_upload($source_file, $filedata = false, \phpbb\mimetype\guesser $mimetype_guesser = null) { global $user, $request; @@ -670,19 +677,6 @@ class fileupload $upload['name'] = utf8_basename($source_file); $upload['size'] = 0; $mimetype = ''; - - if (function_exists('mime_content_type')) - { - $mimetype = mime_content_type($source_file); - } - - // Some browsers choke on a mimetype of application/octet-stream - if (!$mimetype || $mimetype == 'application/octet-stream') - { - $mimetype = 'application/octetstream'; - } - - $upload['type'] = $mimetype; } else { @@ -691,7 +685,7 @@ class fileupload $upload['type'] = $filedata['type']; } - $file = new filespec($upload, $this); + $file = new filespec($upload, $this, $mimetype_guesser); if ($file->init_error) { @@ -749,7 +743,7 @@ class fileupload * @return object $file Object "filespec" is returned, all further operations can be done with this object * @access public */ - function remote_upload($upload_url) + function remote_upload($upload_url, \phpbb\mimetype\guesser $mimetype_guesser = null) { global $user, $phpbb_root_path; @@ -904,7 +898,7 @@ class fileupload $upload_ary['tmp_name'] = $filename; - $file = new filespec($upload_ary, $this); + $file = new filespec($upload_ary, $this, $mimetype_guesser); $this->common_checks($file); return $file; diff --git a/tests/upload/filespec_test.php b/tests/upload/filespec_test.php index 5e333213f4..d8fa82e2b5 100644 --- a/tests/upload/filespec_test.php +++ b/tests/upload/filespec_test.php @@ -65,6 +65,16 @@ class phpbb_filespec_test extends phpbb_test_case copy($fileinfo->getPathname(), $this->path . 'copies/' . $fileinfo->getFilename() . '_copy_2'); } } + + $guessers = array( + new \Symfony\Component\HttpFoundation\File\MimeType\FileinfoMimeTypeGuesser(), + new \Symfony\Component\HttpFoundation\File\MimeType\FileBinaryMimeTypeGuesser(), + new \phpbb\mimetype\content_guesser(), + new \phpbb\mimetype\extension_guesser(), + ); + $guessers[2]->set_priority(-2); + $guessers[3]->set_priority(-2); + $this->mimetype_guesser = new \phpbb\mimetype\guesser($guessers); } private function get_filespec($override = array()) @@ -78,7 +88,7 @@ class phpbb_filespec_test extends phpbb_test_case 'error' => '', ); - return new filespec(array_merge($upload_ary, $override), null); + return new filespec(array_merge($upload_ary, $override), null, $this->mimetype_guesser); } protected function tearDown() @@ -222,6 +232,9 @@ class phpbb_filespec_test extends phpbb_test_case array('png', 'image/png', true), array('tif', 'image/tif', true), array('txt', 'text/plain', false), + array('jpg', 'application/octet-stream', false), + array('gif', 'application/octetstream', false), + array('png', 'application/mime', false), ); } @@ -234,6 +247,30 @@ class phpbb_filespec_test extends phpbb_test_case $this->assertEquals($expected, $filespec->is_image()); } + public function is_image_get_mimetype() + { + return array( + array('gif', 'image/gif', true), + array('jpg', 'image/jpg', true), + array('png', 'image/png', true), + array('tif', 'image/tif', true), + array('txt', 'text/plain', false), + array('jpg', 'application/octet-stream', true), + array('gif', 'application/octetstream', true), + array('png', 'application/mime', true), + ); + } + + /** + * @dataProvider is_image_get_mimetype + */ + public function test_is_image_get_mimetype($filename, $mimetype, $expected) + { + $filespec = $this->get_filespec(array('tmp_name' => $this->path . $filename, 'type' => $mimetype)); + $filespec->get_mimetype($this->path . $filename); + $this->assertEquals($expected, $filespec->is_image()); + } + public function move_file_variables() { return array( From 94a81fa01d0106f6deba6cbb9000f4c8bbbf607a Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 9 Jan 2014 23:53:19 +0100 Subject: [PATCH 2/9] [ticket/11148] Pass mimetype guesser to upload_attachment() function PHPBB3-11148 --- phpBB/includes/functions_posting.php | 5 +++-- phpBB/includes/message_parser.php | 21 ++++++++++++++++++++- phpBB/posting.php | 2 ++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/phpBB/includes/functions_posting.php b/phpBB/includes/functions_posting.php index 23fdd809e2..f9be0f10f6 100644 --- a/phpBB/includes/functions_posting.php +++ b/phpBB/includes/functions_posting.php @@ -398,11 +398,12 @@ function posting_gen_topic_types($forum_id, $cur_topic_type = POST_NORMAL) * @param string $local_storage The path to the local file * @param bool $is_message Whether it is a PM or not * @param \filespec $local_filedata A filespec object created for the local file +* @param \phpbb\mimetype\guesser $mimetype_guesser The mimetype guesser object if used * @param \phpbb\plupload\plupload $plupload The plupload object if one is being used * * @return object filespec */ -function upload_attachment($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = false, \phpbb\plupload\plupload $plupload = null) +function upload_attachment($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = false, \phpbb\mimetype\guesser $mimetype_guesser = null, \phpbb\plupload\plupload $plupload = null) { global $auth, $user, $config, $db, $cache; global $phpbb_root_path, $phpEx; @@ -434,7 +435,7 @@ function upload_attachment($form_name, $forum_id, $local = false, $local_storage $extensions = $cache->obtain_attach_extensions((($is_message) ? false : (int) $forum_id)); $upload->set_allowed_extensions(array_keys($extensions['_allowed_'])); - $file = ($local) ? $upload->local_upload($local_storage, $local_filedata) : $upload->form_upload($form_name, $plupload); + $file = ($local) ? $upload->local_upload($local_storage, $local_filedata, $mimetype_guesser) : $upload->form_upload($form_name, $mimetype_guesser, $plupload); if ($file->init_error) { diff --git a/phpBB/includes/message_parser.php b/phpBB/includes/message_parser.php index 8d926ec70a..19571d6bd3 100644 --- a/phpBB/includes/message_parser.php +++ b/phpBB/includes/message_parser.php @@ -1082,6 +1082,12 @@ class parse_message extends bbcode_firstpass */ protected $plupload; + /** + * The mimetype guesser object used for attachment mimetypes + * @var \phpbb\mimetype\guesser + */ + protected $mimetype_guesser; + /** * Init - give message here or manually */ @@ -1560,7 +1566,7 @@ class parse_message extends bbcode_firstpass { if ($num_attachments < $cfg['max_attachments'] || $auth->acl_gets('m_', 'a_', $forum_id)) { - $filedata = upload_attachment($form_name, $forum_id, false, '', $is_message, false, $this->plupload); + $filedata = upload_attachment($form_name, $forum_id, false, '', $is_message, false, $this->mimetype_guesser, $this->plupload); $error = array_merge($error, $filedata['error']); if (!sizeof($error)) @@ -1792,4 +1798,17 @@ class parse_message extends bbcode_firstpass { $this->plupload = $plupload; } + + /** + * Setter function for passing the mimetype_guesser object + * + * @param \phpbb\mimetype\guesser $mimetype_guesser The mimetype_guesser + * object + * + * @return null + */ + public function set_mimetype_guesser(\phpbb\mimetype\guesser $mimetype_guesser) + { + $this->mimetype_guesser = $mimetype_guesser; + } } diff --git a/phpBB/posting.php b/phpBB/posting.php index 1b8fa6debf..0e912330e0 100644 --- a/phpBB/posting.php +++ b/phpBB/posting.php @@ -523,7 +523,9 @@ $orig_poll_options_size = sizeof($post_data['poll_options']); $message_parser = new parse_message(); $plupload = $phpbb_container->get('plupload'); +$mimetype_guesser = $phpbb_container->get('mimetype.guesser'); $message_parser->set_plupload($plupload); +$message_parser->set_mimetype_guesser($mimetype_guesser); if (isset($post_data['post_text'])) { From c22983cbdb75b687fb5d75444b0e919460dca65b Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 9 Jan 2014 23:55:22 +0100 Subject: [PATCH 3/9] [ticket/11148] Use mimetype guesser for uploaded avatars PHPBB3-11148 --- phpBB/config/avatars.yml | 1 + phpBB/phpbb/avatar/driver/upload.php | 30 ++++++++++++++++++++++++++-- tests/avatar/manager_test.php | 17 +++++++++++++++- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/phpBB/config/avatars.yml b/phpBB/config/avatars.yml index d22a5db2ae..8e5b1fdbfe 100644 --- a/phpBB/config/avatars.yml +++ b/phpBB/config/avatars.yml @@ -45,6 +45,7 @@ services: - %core.root_path% - %core.php_ext% - @path_helper + - @mimetype.guesser - @cache.driver calls: - [set_name, [avatar.driver.upload]] diff --git a/phpBB/phpbb/avatar/driver/upload.php b/phpBB/phpbb/avatar/driver/upload.php index c43004f340..edc5941602 100644 --- a/phpBB/phpbb/avatar/driver/upload.php +++ b/phpBB/phpbb/avatar/driver/upload.php @@ -18,6 +18,32 @@ namespace phpbb\avatar\driver; */ class upload extends \phpbb\avatar\driver\driver { + /** + * @var \phpbb\mimetype\guesser + */ + protected $mimetype_guesser; + + /** + * Construct a driver object + * + * @param \phpbb\config\config $config phpBB configuration + * @param \phpbb\request\request $request Request object + * @param string $phpbb_root_path Path to the phpBB root + * @param string $php_ext PHP file extension + * @param \phpbb_path_helper $path_helper phpBB path helper + * @param \phpbb\mimetype\guesser $mimetype_guesser Mimetype guesser + * @param \phpbb\cache\driver\driver_interface $cache Cache driver + */ + public function __construct(\phpbb\config\config $config, $phpbb_root_path, $php_ext, \phpbb\path_helper $path_helper, \phpbb\mimetype\guesser $mimetype_guesser, \phpbb\cache\driver\driver_interface $cache = null) + { + $this->config = $config; + $this->phpbb_root_path = $phpbb_root_path; + $this->php_ext = $php_ext; + $this->path_helper = $path_helper; + $this->mimetype_guesser = $mimetype_guesser; + $this->cache = $cache; + } + /** * {@inheritdoc} */ @@ -70,7 +96,7 @@ class upload extends \phpbb\avatar\driver\driver if (!empty($upload_file['name'])) { - $file = $upload->form_upload('avatar_upload_file'); + $file = $upload->form_upload('avatar_upload_file', $this->mimetype_guesser); } else if (!empty($this->config['allow_avatar_remote_upload']) && !empty($url)) { @@ -100,7 +126,7 @@ class upload extends \phpbb\avatar\driver\driver return false; } - $file = $upload->remote_upload($url); + $file = $upload->remote_upload($url, $this->mimetype_guesser); } else { diff --git a/tests/avatar/manager_test.php b/tests/avatar/manager_test.php index 246397ad6c..97cfbfb104 100644 --- a/tests/avatar/manager_test.php +++ b/tests/avatar/manager_test.php @@ -42,6 +42,14 @@ class phpbb_avatar_manager_test extends \phpbb_test_case $phpEx ); + $guessers = array( + new \Symfony\Component\HttpFoundation\File\MimeType\FileinfoMimeTypeGuesser(), + new \Symfony\Component\HttpFoundation\File\MimeType\FileBinaryMimeTypeGuesser(), + new \phpbb\mimetype\extension_guesser, + new \phpbb\mimetype\content_guesser, + ); + $guesser = new \phpbb\mimetype\guesser($guessers); + // $this->avatar_foobar will be needed later on $this->avatar_foobar = $this->getMock('\phpbb\avatar\driver\foobar', array('get_name'), array($config, $phpbb_root_path, $phpEx, $path_helper, $cache)); $this->avatar_foobar->expects($this->any()) @@ -56,7 +64,14 @@ class phpbb_avatar_manager_test extends \phpbb_test_case foreach ($this->avatar_drivers() as $driver) { - $cur_avatar = $this->getMock('\phpbb\avatar\driver\\' . $driver, array('get_name'), array($config, $phpbb_root_path, $phpEx, $path_helper, $cache)); + if ($driver !== 'upload') + { + $cur_avatar = $this->getMock('\phpbb\avatar\driver\\' . $driver, array('get_name'), array($config, $phpbb_root_path, $phpEx, $path_helper, $cache)); + } + else + { + $cur_avatar = $this->getMock('\phpbb\avatar\driver\\' . $driver, array('get_name'), array($config, $phpbb_root_path, $phpEx, $path_helper, $guesser, $cache)); + } $cur_avatar->expects($this->any()) ->method('get_name') ->will($this->returnValue('avatar.driver.' . $driver)); From de404002c75f770cc923262b9c16d899662ae114 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 30 Jan 2014 23:04:29 +0100 Subject: [PATCH 4/9] [ticket/11148] Default to application/octet-stream if no mimetype given This should prevent us from having an empty mimetype while uploading a file using local_upload(). PHPBB3-11148 --- phpBB/includes/functions_upload.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/includes/functions_upload.php b/phpBB/includes/functions_upload.php index fb153a9e0c..c3381aa68b 100644 --- a/phpBB/includes/functions_upload.php +++ b/phpBB/includes/functions_upload.php @@ -676,7 +676,7 @@ class fileupload { $upload['name'] = utf8_basename($source_file); $upload['size'] = 0; - $mimetype = ''; + $mimetype = 'application/octet-stream'; } else { From ea5bc9c8339327afb1519f8c3986c257880c54dc Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 30 Jan 2014 23:14:48 +0100 Subject: [PATCH 5/9] [ticket/11148] Add missing parts to docblock of get_mimetype() method PHPBB3-11148 --- phpBB/includes/functions_upload.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/phpBB/includes/functions_upload.php b/phpBB/includes/functions_upload.php index c3381aa68b..6ca430d9ac 100644 --- a/phpBB/includes/functions_upload.php +++ b/phpBB/includes/functions_upload.php @@ -224,6 +224,8 @@ class filespec /** * Get mimetype * + * @param string $filename Filename that needs to be checked + * @return string Mimetype of supplied filename */ function get_mimetype($filename) { From a402d619b458df69ae9c336f3324b357fcd1a52a Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 15 Mar 2014 12:25:33 +0100 Subject: [PATCH 6/9] [ticket/11148] Get rid of extra line in mimetype guesser setter doc block PHPBB3-11148 --- phpBB/includes/message_parser.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/phpBB/includes/message_parser.php b/phpBB/includes/message_parser.php index 19571d6bd3..7cee4252a3 100644 --- a/phpBB/includes/message_parser.php +++ b/phpBB/includes/message_parser.php @@ -1802,8 +1802,7 @@ class parse_message extends bbcode_firstpass /** * Setter function for passing the mimetype_guesser object * - * @param \phpbb\mimetype\guesser $mimetype_guesser The mimetype_guesser - * object + * @param \phpbb\mimetype\guesser $mimetype_guesser The mimetype_guesser object * * @return null */ From 4eb7485b397733c8765f8bd37a04b7a034b29792 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 1 Jun 2014 00:19:22 +0200 Subject: [PATCH 7/9] [ticket/11148] Always use the output of the mimetype guesser in get_mimetype PHPBB3-11148 --- phpBB/includes/functions_upload.php | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/phpBB/includes/functions_upload.php b/phpBB/includes/functions_upload.php index 6ca430d9ac..f50ce9432f 100644 --- a/phpBB/includes/functions_upload.php +++ b/phpBB/includes/functions_upload.php @@ -231,12 +231,7 @@ class filespec { if ($this->mimetype_guesser !== null) { - $mimetype = $this->mimetype_guesser->guess($filename); - - if ($mimetype !== 'application/octet-stream') - { - $this->mimetype = $mimetype; - } + $this->mimetype = $this->mimetype_guesser->guess($filename); } return $this->mimetype; From 3823fe355fbd5793247bc291e814ec46dc7334ec Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 1 Jun 2014 15:50:28 +0200 Subject: [PATCH 8/9] [ticket/11148] Change expected output with disallowed content in test PHPBB3-11148 --- tests/functional/fileupload_form_test.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/functional/fileupload_form_test.php b/tests/functional/fileupload_form_test.php index b9d55fbd3c..29036c821e 100644 --- a/tests/functional/fileupload_form_test.php +++ b/tests/functional/fileupload_form_test.php @@ -109,9 +109,9 @@ class phpbb_functional_fileupload_form_test extends phpbb_functional_test_case $crawler = $this->upload_file('disallowed.jpg', 'image/jpeg'); - // Hitting the UNABLE_GET_IMAGE_SIZE error means we passed the + // Hitting the ATTACHED_IMAGE_NOT_IMAGE error means we passed the // DISALLOWED_CONTENT check - $this->assertEquals($this->lang('UNABLE_GET_IMAGE_SIZE'), $crawler->filter('p.error')->text()); + $this->assertContains($this->lang('ATTACHED_IMAGE_NOT_IMAGE'), $crawler->text()); } public function test_too_large() From ff56f0dcfe6355a9bd24c1e33dee15c60d01526b Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 2 Jun 2014 11:57:37 +0200 Subject: [PATCH 9/9] [ticket/11148] Remove unneeded variable mimetype and use type octet-stream The variable $mimetype is not used in the method local_upload() afterwards so it shouldn't be assigned. The correct default mimetype should be application/octet-stream and not application/octetstream according to RFC 2046. PHPBB3-11148 --- phpBB/includes/functions_upload.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/phpBB/includes/functions_upload.php b/phpBB/includes/functions_upload.php index f50ce9432f..0d33d32a1f 100644 --- a/phpBB/includes/functions_upload.php +++ b/phpBB/includes/functions_upload.php @@ -82,7 +82,7 @@ class filespec if (!$this->mimetype) { - $this->mimetype = 'application/octetstream'; + $this->mimetype = 'application/octet-stream'; } $this->extension = strtolower(self::get_extension($this->realname)); @@ -673,7 +673,6 @@ class fileupload { $upload['name'] = utf8_basename($source_file); $upload['size'] = 0; - $mimetype = 'application/octet-stream'; } else {