From 7de15bc54c14ce4fa74480f3894f9aec23dbcfba Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 3 Sep 2014 23:06:02 +0200 Subject: [PATCH 1/3] [ticket/13031] Only use mimetype guesser guess if it helps us If we already have a mimetype and the guesser's guess is the default fallback, we should keep the already existing mimetype the browser supplied. Otherwise, platforms that might not support mimetype guessers will cause us to always have the mimetype set to application/octet-stream on images. This will prevent users from uploading images. PHPBB3-13031 --- phpBB/includes/functions_upload.php | 7 ++++++- tests/functional/fileupload_form_test.php | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/phpBB/includes/functions_upload.php b/phpBB/includes/functions_upload.php index a0a67ccf3d..ae04fe737c 100644 --- a/phpBB/includes/functions_upload.php +++ b/phpBB/includes/functions_upload.php @@ -232,7 +232,12 @@ class filespec { if ($this->mimetype_guesser !== null) { - $this->mimetype = $this->mimetype_guesser->guess($filename); + $mimetype = $this->mimetype_guesser->guess($filename); + + if (empty($this->mimetype) || $mimetype !== 'application/octet-stream') + { + $this->mimetype = $mimetype; + } } return $this->mimetype; diff --git a/tests/functional/fileupload_form_test.php b/tests/functional/fileupload_form_test.php index e87953367f..b8c48389e0 100644 --- a/tests/functional/fileupload_form_test.php +++ b/tests/functional/fileupload_form_test.php @@ -107,9 +107,9 @@ class phpbb_functional_fileupload_form_test extends phpbb_functional_test_case $crawler = $this->upload_file('disallowed.jpg', 'image/jpeg'); - // Hitting the ATTACHED_IMAGE_NOT_IMAGE error means we passed the + // Hitting the UNABLE_GET_IMAGE_SIZE error means we passed the // DISALLOWED_CONTENT check - $this->assertContains($this->lang('ATTACHED_IMAGE_NOT_IMAGE'), $crawler->text()); + $this->assertContainsLang('UNABLE_GET_IMAGE_SIZE', $crawler->text()); } public function test_too_large() From d31ff51785f42e3de255722a32f098ab49d1489c Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 4 Sep 2014 22:32:08 +0200 Subject: [PATCH 2/3] [ticket/13031] Guess with all mimetype guessers and pick best guess PHPBB3-13031 --- phpBB/includes/functions_upload.php | 4 +-- phpBB/phpbb/mimetype/guesser.php | 39 +++++++++++++++++++++++------ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/phpBB/includes/functions_upload.php b/phpBB/includes/functions_upload.php index ae04fe737c..f179b2fd70 100644 --- a/phpBB/includes/functions_upload.php +++ b/phpBB/includes/functions_upload.php @@ -232,9 +232,9 @@ class filespec { if ($this->mimetype_guesser !== null) { - $mimetype = $this->mimetype_guesser->guess($filename); + $mimetype = $this->mimetype_guesser->guess($filename, $this->uploadname); - if (empty($this->mimetype) || $mimetype !== 'application/octet-stream') + if ($mimetype !== 'application/octet-stream') { $this->mimetype = $mimetype; } diff --git a/phpBB/phpbb/mimetype/guesser.php b/phpBB/phpbb/mimetype/guesser.php index 773a1f822a..c019cb5b07 100644 --- a/phpBB/phpbb/mimetype/guesser.php +++ b/phpBB/phpbb/mimetype/guesser.php @@ -115,17 +115,42 @@ class guesser return false; } + $mimetype = 'application/octet-stream'; + foreach ($this->guessers as $guesser) { - $mimetype = $guesser->guess($file, $file_name); + $mimetype_guess = $guesser->guess($file, $file_name); - // Try to guess something that is not the fallback application/octet-stream - if ($mimetype !== null && $mimetype !== 'application/octet-stream') - { - return $mimetype; - } + $mimetype = $this->choose_mime_type($mimetype, $mimetype_guess); } // Return any mimetype if we got a result or the fallback value - return (!empty($mimetype)) ? $mimetype : 'application/octet-stream'; + return $mimetype; + } + + /** + * Choose the best mime type based on the current mime type and the guess + * If a guesser returns nulls or application/octet-stream, we will keep + * the current guess. Guesses with a slash inside them will be favored over + * already existing without slashes. However, any guess that will pass the + * first check will always overwrite the default application/octet-stream. + * + * @param string $mime_type The current mime type + * @param string $guess The current mime type guess + * + * @return string The best mime type based on current mime type and guess + */ + protected function choose_mime_type($mime_type, $guess) + { + if ($guess === null || $guess == 'application/octet-stream') + { + return $mime_type; + } + + if ((strpos($mime_type, '/') === false || $mime_type == 'application/octet-stream') && strpos($guess, '/') !== false) + { + $mime_type = $guess; + } + + return $mime_type; } } From 21029e9fd295bedfcc34555d32afd928a03392a1 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 5 Sep 2014 21:55:49 +0200 Subject: [PATCH 3/3] [ticket/13031] Slightly change behavior of choose_mime_type and add unit tests The mime type 'application/octet-stream' will still always be overwritten by proper guesses. However, guesses with guessers that have a higher priority will now overwrite previous guesses even if the mime types of these guesses had a slash in them. PHPBB3-13031 --- phpBB/phpbb/mimetype/guesser.php | 8 ++++---- tests/mimetype/guesser_test.php | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/phpBB/phpbb/mimetype/guesser.php b/phpBB/phpbb/mimetype/guesser.php index c019cb5b07..8baa77089b 100644 --- a/phpBB/phpbb/mimetype/guesser.php +++ b/phpBB/phpbb/mimetype/guesser.php @@ -131,22 +131,22 @@ class guesser * Choose the best mime type based on the current mime type and the guess * If a guesser returns nulls or application/octet-stream, we will keep * the current guess. Guesses with a slash inside them will be favored over - * already existing without slashes. However, any guess that will pass the - * first check will always overwrite the default application/octet-stream. + * already existing ones. However, any guess that will pass the first check + * will always overwrite the default application/octet-stream. * * @param string $mime_type The current mime type * @param string $guess The current mime type guess * * @return string The best mime type based on current mime type and guess */ - protected function choose_mime_type($mime_type, $guess) + public function choose_mime_type($mime_type, $guess) { if ($guess === null || $guess == 'application/octet-stream') { return $mime_type; } - if ((strpos($mime_type, '/') === false || $mime_type == 'application/octet-stream') && strpos($guess, '/') !== false) + if ($mime_type == 'application/octet-stream' || strpos($guess, '/') !== false) { $mime_type = $guess; } diff --git a/tests/mimetype/guesser_test.php b/tests/mimetype/guesser_test.php index b74a9f236e..fa53e6c8c4 100644 --- a/tests/mimetype/guesser_test.php +++ b/tests/mimetype/guesser_test.php @@ -206,4 +206,25 @@ class guesser_test extends \phpbb_test_case $this->assertInstanceOf('\phpbb\mimetype\content_guesser', $guessers[0]); $this->assertInstanceOf('\phpbb\mimetype\extension_guesser', $guessers[3]); } + + public function data_choose_mime_type() + { + return array( + array('application/octet-stream', 'application/octet-stream', null), + array('application/octet-stream', 'application/octet-stream', 'application/octet-stream'), + array('binary', 'application/octet-stream', 'binary'), + array('image/jpeg', 'application/octet-stream', 'image/jpeg'), + array('image/jpeg', 'binary', 'image/jpeg'), + array('image/jpeg', 'image/jpg', 'image/jpeg'), + array('image/jpeg', 'image/jpeg', 'binary'), + ); + } + + /** + * @dataProvider data_choose_mime_type + */ + public function test_choose_mime_type($expected, $mime_type, $guess) + { + $this->assertSame($expected, $this->guesser->choose_mime_type($mime_type, $guess)); + } }