From d45a73777935c86fc5131955831833d7465b5e9d Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 21 Jan 2013 01:22:01 -0500 Subject: Update copyright to 2013. Fixes #1953. --- modules/gallery/helpers/gallery_graphics.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/gallery/helpers/gallery_graphics.php') diff --git a/modules/gallery/helpers/gallery_graphics.php b/modules/gallery/helpers/gallery_graphics.php index d2b92c87..1f0728c0 100644 --- a/modules/gallery/helpers/gallery_graphics.php +++ b/modules/gallery/helpers/gallery_graphics.php @@ -1,7 +1,7 @@ Date: Fri, 25 Jan 2013 23:47:08 +0100 Subject: #1965 - Improve sanity checks and copy/convert/process logic for rotate and resize. - resize: ensured that resize is skipped *only* if the metadata is valid or the options are well-defined and would upscale. Then, if resize is skipped, check to see if it still needs to be converted. Previous conditions would allow a small PNG to get copied to a JPG, and would allow a corrupted JPG to be copied to the output. - rotate: add checks for empty file or empty options. - use get_file_metadata instead of direct getimagesize call. - add unit tests for rotate and resize, including some for corrupted input files and missing options. --- modules/gallery/helpers/gallery_graphics.php | 36 +++++- .../gallery/tests/Gallery_Graphics_Helper_Test.php | 137 +++++++++++++++++++++ 2 files changed, 167 insertions(+), 6 deletions(-) create mode 100644 modules/gallery/tests/Gallery_Graphics_Helper_Test.php (limited to 'modules/gallery/helpers/gallery_graphics.php') diff --git a/modules/gallery/helpers/gallery_graphics.php b/modules/gallery/helpers/gallery_graphics.php index 1f0728c0..e81d5468 100644 --- a/modules/gallery/helpers/gallery_graphics.php +++ b/modules/gallery/helpers/gallery_graphics.php @@ -31,6 +31,15 @@ class gallery_graphics_Core { module::event("graphics_rotate", $input_file, $output_file, $options, $item); + if (@filesize($input_file) == 0) { + throw new Exception("@todo EMPTY_INPUT_FILE"); + } + + if (!isset($options["degrees"])) { + $options["degrees"] = 0; + } + + // Rotate the image. This also implicitly converts its format if needed. Image::factory($input_file) ->quality(module::get_var("gallery", "image_quality")) ->rotate($options["degrees"]) @@ -57,11 +66,26 @@ class gallery_graphics_Core { throw new Exception("@todo EMPTY_INPUT_FILE"); } - $dims = getimagesize($input_file); - if (max($dims[0], $dims[1]) <= min($options["width"], $options["height"])) { - // Image would get upscaled; do nothing - copy($input_file, $output_file); + list ($input_width, $input_height, $input_mime, $input_extension) = + photo::get_file_metadata($input_file); + if ($input_width && $input_height && + (empty($options["width"]) || empty($options["height"]) || empty($options["master"]) || + (max($input_width, $input_height) <= min($options["width"], $options["height"])))) { + // Photo dimensions well-defined, but options not well-defined or would upscale the image. + // Do not resize. Check mimes to see if we can copy the file or if we need to convert it. + // (checking mimes avoids needlessly converting jpg to jpeg, etc.) + $output_mime = legal_file::get_photo_types_by_extension(pathinfo($output_file, PATHINFO_EXTENSION)); + if ($input_mime && $output_mime && ($input_mime == $output_mime)) { + // Mimes well-defined and identical - copy input to output + copy($input_file, $output_file); + } else { + // Mimes not well-defined or not the same - convert input to output + $image = Image::factory($input_file) + ->quality(module::get_var("gallery", "image_quality")) + ->save($output_file); + } } else { + // Resize the image. This also implicitly converts its format if needed. $image = Image::factory($input_file) ->resize($options["width"], $options["height"], $options["master"]) ->quality(module::get_var("gallery", "image_quality")); @@ -96,8 +120,8 @@ class gallery_graphics_Core { module::event("graphics_composite", $input_file, $output_file, $options, $item); - list ($width, $height) = getimagesize($input_file); - list ($w_width, $w_height) = getimagesize($options["file"]); + list ($width, $height) = photo::get_file_metadata($input_file); + list ($w_width, $w_height) = photo::get_file_metadata($options["file"]); $pad = isset($options["padding"]) ? $options["padding"] : 10; $top = $pad; diff --git a/modules/gallery/tests/Gallery_Graphics_Helper_Test.php b/modules/gallery/tests/Gallery_Graphics_Helper_Test.php new file mode 100644 index 00000000..20096b23 --- /dev/null +++ b/modules/gallery/tests/Gallery_Graphics_Helper_Test.php @@ -0,0 +1,137 @@ + 90); + gallery_graphics::rotate($input_file, $output_file, $options, null); + + // Output is rotated to 768x1024 jpg + $this->assert_equal(array(768, 1024, "image/jpeg", "jpg"), photo::get_file_metadata($output_file)); + } + + public function rotate_jpg_without_options_test() { + // Input is a 1024x768 jpg, output options undefined + $input_file = MODPATH . "gallery/tests/test.jpg"; + $output_file = TMPPATH . test::random_name() . ".jpg"; + gallery_graphics::rotate($input_file, $output_file, null, null); + + // Output is not rotated, still a 1024x768 jpg + $this->assert_equal(array(1024, 768, "image/jpeg", "jpg"), photo::get_file_metadata($output_file)); + } + + public function rotate_bad_jpg_test() { + // Input is a garbled jpg, output is jpg autofit to 300x300 + $input_file = TMPPATH . test::random_name() . ".jpg"; + $output_file = TMPPATH . test::random_name() . ".jpg"; + $options = array("degrees" => 90); + file_put_contents($input_file, test::lorem_ipsum(200)); + + // Should get passed to Image library and throw an exception + try { + gallery_graphics::rotate($input_file, $output_file, $options, null); + $this->assert_true(false, "Shouldn't get here"); + } catch (Exception $e) { + // pass + } + } + + public function resize_jpg_test() { + // Input is a 1024x768 jpg, output is jpg autofit to 300x300 + $input_file = MODPATH . "gallery/tests/test.jpg"; + $output_file = TMPPATH . test::random_name() . ".jpg"; + $options = array("width" => 300, "height" => 300, "master" => Image::AUTO); + gallery_graphics::resize($input_file, $output_file, $options, null); + + // Output is resized to 300x225 jpg + $this->assert_equal(array(300, 225, "image/jpeg", "jpg"), photo::get_file_metadata($output_file)); + } + + public function resize_jpg_to_png_test() { + // Input is a 1024x768 jpg, output is png autofit to 300x300 + $input_file = MODPATH . "gallery/tests/test.jpg"; + $output_file = TMPPATH . test::random_name() . ".png"; + $options = array("width" => 300, "height" => 300, "master" => Image::AUTO); + gallery_graphics::resize($input_file, $output_file, $options, null); + + // Output is resized to 300x225 png + $this->assert_equal(array(300, 225, "image/png", "png"), photo::get_file_metadata($output_file)); + } + + public function resize_jpg_with_no_upscale_test() { + // Input is a 1024x768 jpg, output is jpg autofit to 1200x1200 - should not upscale + $input_file = MODPATH . "gallery/tests/test.jpg"; + $output_file = TMPPATH . test::random_name() . ".jpg"; + $options = array("width" => 1200, "height" => 1200, "master" => Image::AUTO); + gallery_graphics::resize($input_file, $output_file, $options, null); + + // Output is copied directly from input + $this->assert_equal(file_get_contents($input_file), file_get_contents($output_file)); + } + + public function resize_jpg_to_png_with_no_upscale_test() { + // Input is a 1024x768 jpg, output is png autofit to 1200x1200 - should not upscale + $input_file = MODPATH . "gallery/tests/test.jpg"; + $output_file = TMPPATH . test::random_name() . ".png"; + $options = array("width" => 1200, "height" => 1200, "master" => Image::AUTO); + gallery_graphics::resize($input_file, $output_file, $options, null); + + // Output is converted from input without resize + $this->assert_equal(array(1024, 768, "image/png", "png"), photo::get_file_metadata($output_file)); + } + + public function resize_jpg_without_options_test() { + // Input is a 1024x768 jpg, output is jpg without options - should not attempt resize + $input_file = MODPATH . "gallery/tests/test.jpg"; + $output_file = TMPPATH . test::random_name() . ".jpg"; + gallery_graphics::resize($input_file, $output_file, null, null); + + // Output is copied directly from input + $this->assert_equal(file_get_contents($input_file), file_get_contents($output_file)); + } + + public function resize_jpg_to_png_without_options_test() { + // Input is a 1024x768 jpg, output is png without options - should not attempt resize + $input_file = MODPATH . "gallery/tests/test.jpg"; + $output_file = TMPPATH . test::random_name() . ".png"; + gallery_graphics::resize($input_file, $output_file, null, null); + + // Output is converted from input without resize + $this->assert_equal(array(1024, 768, "image/png", "png"), photo::get_file_metadata($output_file)); + } + + public function resize_bad_jpg_test() { + // Input is a garbled jpg, output is jpg autofit to 300x300 + $input_file = TMPPATH . test::random_name() . ".jpg"; + $output_file = TMPPATH . test::random_name() . ".jpg"; + $options = array("width" => 300, "height" => 300, "master" => Image::AUTO); + file_put_contents($input_file, test::lorem_ipsum(200)); + + // Should get passed to Image library and throw an exception + try { + gallery_graphics::resize($input_file, $output_file, $options, null); + $this->assert_true(false, "Shouldn't get here"); + } catch (Exception $e) { + // pass + } + } +} \ No newline at end of file -- cgit v1.2.3 From 9e20a30d22c48f5aacc09035405dd0added499de Mon Sep 17 00:00:00 2001 From: shadlaws Date: Sun, 27 Jan 2013 21:55:24 +0100 Subject: #1969 - Give graphics events the ability to override the standard process. While graphics_rotate, graphics_resize, and graphics_composite events already exist, they don't have the ability to *override* the standard process. This makes it a bit tricky when you want to replace the standard procedure with another (e.g. use jpegtran to perform lossless jpg rotation). Solution: - make a temp filename. - tell the events to use it as the output file. - if an event makes something, use it and skip the standard process. --- modules/gallery/helpers/gallery_graphics.php | 158 +++++++++++++++------------ 1 file changed, 90 insertions(+), 68 deletions(-) (limited to 'modules/gallery/helpers/gallery_graphics.php') diff --git a/modules/gallery/helpers/gallery_graphics.php b/modules/gallery/helpers/gallery_graphics.php index e81d5468..b78bd9a7 100644 --- a/modules/gallery/helpers/gallery_graphics.php +++ b/modules/gallery/helpers/gallery_graphics.php @@ -29,21 +29,28 @@ class gallery_graphics_Core { static function rotate($input_file, $output_file, $options, $item=null) { graphics::init_toolkit(); - module::event("graphics_rotate", $input_file, $output_file, $options, $item); + $temp_file = system::temp_filename("rotate_", pathinfo($output_file, PATHINFO_EXTENSION)); + module::event("graphics_rotate", $input_file, $temp_file, $options, $item); - if (@filesize($input_file) == 0) { - throw new Exception("@todo EMPTY_INPUT_FILE"); - } + if (@filesize($temp_file) > 0) { + // A graphics_rotate event made an image - move it to output_file and use it. + @rename($temp_file, $output_file); + } else { + // No events made an image - proceed with standard process. + if (@filesize($input_file) == 0) { + throw new Exception("@todo EMPTY_INPUT_FILE"); + } - if (!isset($options["degrees"])) { - $options["degrees"] = 0; - } + if (!isset($options["degrees"])) { + $options["degrees"] = 0; + } - // Rotate the image. This also implicitly converts its format if needed. - Image::factory($input_file) - ->quality(module::get_var("gallery", "image_quality")) - ->rotate($options["degrees"]) - ->save($output_file); + // Rotate the image. This also implicitly converts its format if needed. + Image::factory($input_file) + ->quality(module::get_var("gallery", "image_quality")) + ->rotate($options["degrees"]) + ->save($output_file); + } module::event("graphics_rotate_completed", $input_file, $output_file, $options, $item); } @@ -60,39 +67,46 @@ class gallery_graphics_Core { static function resize($input_file, $output_file, $options, $item=null) { graphics::init_toolkit(); - module::event("graphics_resize", $input_file, $output_file, $options, $item); + $temp_file = system::temp_filename("resize_", pathinfo($output_file, PATHINFO_EXTENSION)); + module::event("graphics_resize", $input_file, $temp_file, $options, $item); - if (@filesize($input_file) == 0) { - throw new Exception("@todo EMPTY_INPUT_FILE"); - } + if (@filesize($temp_file) > 0) { + // A graphics_resize event made an image - move it to output_file and use it. + @rename($temp_file, $output_file); + } else { + // No events made an image - proceed with standard process. + if (@filesize($input_file) == 0) { + throw new Exception("@todo EMPTY_INPUT_FILE"); + } - list ($input_width, $input_height, $input_mime, $input_extension) = - photo::get_file_metadata($input_file); - if ($input_width && $input_height && - (empty($options["width"]) || empty($options["height"]) || empty($options["master"]) || - (max($input_width, $input_height) <= min($options["width"], $options["height"])))) { - // Photo dimensions well-defined, but options not well-defined or would upscale the image. - // Do not resize. Check mimes to see if we can copy the file or if we need to convert it. - // (checking mimes avoids needlessly converting jpg to jpeg, etc.) - $output_mime = legal_file::get_photo_types_by_extension(pathinfo($output_file, PATHINFO_EXTENSION)); - if ($input_mime && $output_mime && ($input_mime == $output_mime)) { - // Mimes well-defined and identical - copy input to output - copy($input_file, $output_file); + list ($input_width, $input_height, $input_mime, $input_extension) = + photo::get_file_metadata($input_file); + if ($input_width && $input_height && + (empty($options["width"]) || empty($options["height"]) || empty($options["master"]) || + (max($input_width, $input_height) <= min($options["width"], $options["height"])))) { + // Photo dimensions well-defined, but options not well-defined or would upscale the image. + // Do not resize. Check mimes to see if we can copy the file or if we need to convert it. + // (checking mimes avoids needlessly converting jpg to jpeg, etc.) + $output_mime = legal_file::get_photo_types_by_extension(pathinfo($output_file, PATHINFO_EXTENSION)); + if ($input_mime && $output_mime && ($input_mime == $output_mime)) { + // Mimes well-defined and identical - copy input to output + copy($input_file, $output_file); + } else { + // Mimes not well-defined or not the same - convert input to output + $image = Image::factory($input_file) + ->quality(module::get_var("gallery", "image_quality")) + ->save($output_file); + } } else { - // Mimes not well-defined or not the same - convert input to output + // Resize the image. This also implicitly converts its format if needed. $image = Image::factory($input_file) - ->quality(module::get_var("gallery", "image_quality")) - ->save($output_file); - } - } else { - // Resize the image. This also implicitly converts its format if needed. - $image = Image::factory($input_file) - ->resize($options["width"], $options["height"], $options["master"]) - ->quality(module::get_var("gallery", "image_quality")); - if (graphics::can("sharpen")) { - $image->sharpen(module::get_var("gallery", "image_sharpen")); + ->resize($options["width"], $options["height"], $options["master"]) + ->quality(module::get_var("gallery", "image_quality")); + if (graphics::can("sharpen")) { + $image->sharpen(module::get_var("gallery", "image_sharpen")); + } + $image->save($output_file); } - $image->save($output_file); } module::event("graphics_resize_completed", $input_file, $output_file, $options, $item); @@ -118,35 +132,43 @@ class gallery_graphics_Core { try { graphics::init_toolkit(); - module::event("graphics_composite", $input_file, $output_file, $options, $item); - - list ($width, $height) = photo::get_file_metadata($input_file); - list ($w_width, $w_height) = photo::get_file_metadata($options["file"]); - - $pad = isset($options["padding"]) ? $options["padding"] : 10; - $top = $pad; - $left = $pad; - $y_center = max($height / 2 - $w_height / 2, $pad); - $x_center = max($width / 2 - $w_width / 2, $pad); - $bottom = max($height - $w_height - $pad, $pad); - $right = max($width - $w_width - $pad, $pad); - - switch ($options["position"]) { - case "northwest": $x = $left; $y = $top; break; - case "north": $x = $x_center; $y = $top; break; - case "northeast": $x = $right; $y = $top; break; - case "west": $x = $left; $y = $y_center; break; - case "center": $x = $x_center; $y = $y_center; break; - case "east": $x = $right; $y = $y_center; break; - case "southwest": $x = $left; $y = $bottom; break; - case "south": $x = $x_center; $y = $bottom; break; - case "southeast": $x = $right; $y = $bottom; break; - } + $temp_file = system::temp_filename("composite_", pathinfo($output_file, PATHINFO_EXTENSION)); + module::event("graphics_composite", $input_file, $temp_file, $options, $item); - Image::factory($input_file) - ->composite($options["file"], $x, $y, $options["transparency"]) - ->quality(module::get_var("gallery", "image_quality")) - ->save($output_file); + if (@filesize($temp_file) > 0) { + // A graphics_composite event made an image - move it to output_file and use it. + @rename($temp_file, $output_file); + } else { + // No events made an image - proceed with standard process. + + list ($width, $height) = photo::get_file_metadata($input_file); + list ($w_width, $w_height) = photo::get_file_metadata($options["file"]); + + $pad = isset($options["padding"]) ? $options["padding"] : 10; + $top = $pad; + $left = $pad; + $y_center = max($height / 2 - $w_height / 2, $pad); + $x_center = max($width / 2 - $w_width / 2, $pad); + $bottom = max($height - $w_height - $pad, $pad); + $right = max($width - $w_width - $pad, $pad); + + switch ($options["position"]) { + case "northwest": $x = $left; $y = $top; break; + case "north": $x = $x_center; $y = $top; break; + case "northeast": $x = $right; $y = $top; break; + case "west": $x = $left; $y = $y_center; break; + case "center": $x = $x_center; $y = $y_center; break; + case "east": $x = $right; $y = $y_center; break; + case "southwest": $x = $left; $y = $bottom; break; + case "south": $x = $x_center; $y = $bottom; break; + case "southeast": $x = $right; $y = $bottom; break; + } + + Image::factory($input_file) + ->composite($options["file"], $x, $y, $options["transparency"]) + ->quality(module::get_var("gallery", "image_quality")) + ->save($output_file); + } module::event("graphics_composite_completed", $input_file, $output_file, $options, $item); } catch (ErrorException $e) { -- cgit v1.2.3 From 0312d1b071bd4434ddb3f82888b0323da6bf3732 Mon Sep 17 00:00:00 2001 From: shadlaws Date: Fri, 8 Feb 2013 13:51:41 +0100 Subject: #1994 - Make get_file_metadata throw an exception if photo or movie is unidentifiable/illegal. - photo & movie helpers: modified to throw exceptions when file is known to be unidentifiable/illegal. - item model: revised to work with exceptions and be more explicit when the data file is invalid. - item model: removed duplicate get_file_metadata call for updated items. - admin_watermarks controller: revised to work with exceptions (really cleans up logic here). - graphics helper: revised to handle invalid placeholders (a nearly-impossible corner case, but still...). - photo & movie helper tests: revised to work with exceptions, added new tests for illegal files with valid extensions. - item model tests: revised to work with exceptions, added new tests for illegal files with valid extensions. --- modules/gallery/helpers/gallery_graphics.php | 5 ++ modules/gallery/helpers/graphics.php | 11 +++- modules/gallery/helpers/movie.php | 10 ++- modules/gallery/helpers/photo.php | 9 ++- modules/gallery/models/item.php | 71 +++++++++++++--------- modules/gallery/tests/Item_Model_Test.php | 16 ++++- modules/gallery/tests/Movie_Helper_Test.php | 36 +++++++++-- modules/gallery/tests/Photo_Helper_Test.php | 18 +++++- modules/watermark/controllers/admin_watermarks.php | 13 ++-- 9 files changed, 139 insertions(+), 50 deletions(-) (limited to 'modules/gallery/helpers/gallery_graphics.php') diff --git a/modules/gallery/helpers/gallery_graphics.php b/modules/gallery/helpers/gallery_graphics.php index b78bd9a7..eb76353f 100644 --- a/modules/gallery/helpers/gallery_graphics.php +++ b/modules/gallery/helpers/gallery_graphics.php @@ -172,6 +172,11 @@ class gallery_graphics_Core { module::event("graphics_composite_completed", $input_file, $output_file, $options, $item); } catch (ErrorException $e) { + // Unlike rotate and resize, composite catches its exceptions here. This is because + // composite is typically called for watermarks. If during thumb/resize generation + // the watermark fails, we'd still like the image resized, just without its watermark. + // If the exception isn't caught here, graphics::generate will replace it with a + // placeholder. Kohana_Log::add("error", $e->getMessage()); } } diff --git a/modules/gallery/helpers/graphics.php b/modules/gallery/helpers/graphics.php index 4df57fba..e34af018 100644 --- a/modules/gallery/helpers/graphics.php +++ b/modules/gallery/helpers/graphics.php @@ -224,7 +224,16 @@ class graphics_Core { graphics::_replace_image_with_placeholder($item, "resize"); } graphics::_replace_image_with_placeholder($item, "thumb"); - graphics::_update_item_dimensions($item); + try { + graphics::_update_item_dimensions($item); + } catch (Exception $e) { + // Looks like get_file_metadata couldn't identify our placeholders. We should never get + // here, but in the odd case we do, we need to do something. Let's put in hardcoded values. + if ($item->is_photo()) { + list ($item->resize_width, $item->resize_height) = array(200, 200); + } + list ($item->thumb_width, $item->thumb_height) = array(200, 200); + } $item->save(); throw $e; } diff --git a/modules/gallery/helpers/movie.php b/modules/gallery/helpers/movie.php index 6844771b..d4b907a2 100644 --- a/modules/gallery/helpers/movie.php +++ b/modules/gallery/helpers/movie.php @@ -192,8 +192,16 @@ class movie_Core { $metadata->extension = strtolower($extension); } - // Run movie_get_file_metadata events which can modify the class, then return results. + // Run movie_get_file_metadata events which can modify the class. module::event("movie_get_file_metadata", $file_path, $metadata); + + // If the post-events results are invalid, throw an exception. Note that, unlike photos, having + // zero width and height isn't considered invalid (as is the case when FFmpeg isn't installed). + if (!$metadata->mime_type || !$metadata->extension || + ($metadata->mime_type != legal_file::get_movie_types_by_extension($metadata->extension))) { + throw new Exception("@todo ILLEGAL_OR_UNINDENTIFIABLE_FILE"); + } + return array($metadata->width, $metadata->height, $metadata->mime_type, $metadata->extension, $metadata->duration); } diff --git a/modules/gallery/helpers/photo.php b/modules/gallery/helpers/photo.php index 51e51507..2d32f0d3 100644 --- a/modules/gallery/helpers/photo.php +++ b/modules/gallery/helpers/photo.php @@ -133,8 +133,15 @@ class photo_Core { $metadata->height = 0; } - // Run photo_get_file_metadata events which can modify the class, then return results. + // Run photo_get_file_metadata events which can modify the class. module::event("photo_get_file_metadata", $file_path, $metadata); + + // If the post-events results are invalid, throw an exception. + if (!$metadata->width || !$metadata->height || !$metadata->mime_type || !$metadata->extension || + ($metadata->mime_type != legal_file::get_photo_types_by_extension($metadata->extension))) { + throw new Exception("@todo ILLEGAL_OR_UNINDENTIFIABLE_FILE"); + } + return array($metadata->width, $metadata->height, $metadata->mime_type, $metadata->extension); } } diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 197d3057..33b8a89d 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -21,6 +21,7 @@ class Item_Model_Core extends ORM_MPTT { protected $children = "items"; protected $sorting = array(); public $data_file = null; + private $data_file_error = null; public function __construct($id=null) { parent::__construct($id); @@ -378,18 +379,26 @@ class Item_Model_Core extends ORM_MPTT { // Get the width, height and mime type from our data file for photos and movies. if ($this->is_photo() || $this->is_movie()) { - if ($this->is_photo()) { - list ($this->width, $this->height, $this->mime_type, $extension) = - photo::get_file_metadata($this->data_file); - } else if ($this->is_movie()) { - list ($this->width, $this->height, $this->mime_type, $extension) = - movie::get_file_metadata($this->data_file); - } - - // Force an extension onto the name if necessary - $pi = pathinfo($this->data_file); - if (empty($pi["extension"])) { - $this->name = "{$this->name}.$extension"; + try { + if ($this->is_photo()) { + list ($this->width, $this->height, $this->mime_type, $extension) = + photo::get_file_metadata($this->data_file); + } else if ($this->is_movie()) { + list ($this->width, $this->height, $this->mime_type, $extension) = + movie::get_file_metadata($this->data_file); + } + + // Force an extension onto the name if necessary + $pi = pathinfo($this->data_file); + if (empty($pi["extension"])) { + $this->name = "{$this->name}.$extension"; + } + + // Data file valid - make sure the flag is reset to false. + $this->data_file_error = false; + } catch (Exception $e) { + // Data file invalid - set the flag so it's reported during item validation. + $this->data_file_error = true; } } @@ -436,17 +445,24 @@ class Item_Model_Core extends ORM_MPTT { // appropriate for its data. We don't try to preserve the name of the data file, though, // because the name is typically a temporary randomly-generated name. if (isset($this->data_file)) { - $extension = pathinfo($this->data_file, PATHINFO_EXTENSION); - $new_name = pathinfo($this->name, PATHINFO_FILENAME) . ".$extension"; - if (!empty($extension) && strcmp($this->name, $new_name)) { - $this->name = $new_name; - } - if ($this->is_photo()) { - list ($this->width, $this->height, $this->mime_type, $extension) = - photo::get_file_metadata($this->data_file); - } else if ($this->is_movie()) { - list ($this->width, $this->height, $this->mime_type, $extension) = - movie::get_file_metadata($this->data_file); + try { + $extension = pathinfo($this->data_file, PATHINFO_EXTENSION); + $new_name = pathinfo($this->name, PATHINFO_FILENAME) . ".$extension"; + if (!empty($extension) && strcmp($this->name, $new_name)) { + $this->name = $new_name; + } + if ($this->is_photo()) { + list ($this->width, $this->height, $this->mime_type, $extension) = + photo::get_file_metadata($this->data_file); + } else if ($this->is_movie()) { + list ($this->width, $this->height, $this->mime_type, $extension) = + movie::get_file_metadata($this->data_file); + } + // Data file valid - make sure the flag is reset to false. + $this->data_file_error = false; + } catch (Exception $e) { + // Data file invalid - set the flag so it's reported during item validation. + $this->data_file_error = true; } } @@ -524,13 +540,6 @@ class Item_Model_Core extends ORM_MPTT { // Replace the data file, if requested. if ($this->data_file && ($this->is_photo() || $this->is_movie())) { copy($this->data_file, $this->file_path()); - - // Get the width, height and mime type from our data file for photos and movies. - if ($this->is_photo()) { - list ($this->width, $this->height) = photo::get_file_metadata($this->file_path()); - } else if ($this->is_movie()) { - list ($this->width, $this->height) = movie::get_file_metadata($this->file_path()); - } $this->thumb_dirty = 1; $this->resize_dirty = 1; } @@ -966,6 +975,8 @@ class Item_Model_Core extends ORM_MPTT { $v->add_error("name", "bad_data_file_path"); } else if (filesize($this->data_file) == 0) { $v->add_error("name", "empty_data_file"); + } else if ($this->data_file_error) { + $v->add_error("name", "invalid_data_file"); } } diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index a1c5bce6..a93498dd 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -445,13 +445,25 @@ class Item_Model_Test extends Gallery_Unit_Test_Case { $photo->set_data_file(MODPATH . "gallery/tests/Item_Model_Test.php"); $photo->save(); } catch (ORM_Validation_Exception $e) { - $this->assert_same(array("mime_type" => "invalid", "name" => "illegal_data_file_extension"), - $e->validation->errors()); + $this->assert_same(array("name" => "illegal_data_file_extension"), $e->validation->errors()); return; // pass } $this->assert_true(false, "Shouldn't get here"); } + public function unsafe_data_file_replacement_with_valid_extension_test() { + $temp_file = TMPPATH . "masquerading_php.jpg"; + copy(MODPATH . "gallery/tests/Item_Model_Test.php", $temp_file); + try { + $photo = test::random_photo(); + $photo->set_data_file($temp_file); + $photo->save(); + } catch (ORM_Validation_Exception $e) { + $this->assert_same(array("name" => "invalid_data_file"), $e->validation->errors()); + return; // pass + } + } + public function urls_test() { $photo = test::random_photo(); $this->assert_true( diff --git a/modules/gallery/tests/Movie_Helper_Test.php b/modules/gallery/tests/Movie_Helper_Test.php index 0c262620..03fa2da9 100644 --- a/modules/gallery/tests/Movie_Helper_Test.php +++ b/modules/gallery/tests/Movie_Helper_Test.php @@ -64,18 +64,42 @@ class Movie_Helper_Test extends Gallery_Unit_Test_Case { public function get_file_metadata_with_no_extension_test() { copy(MODPATH . "gallery/tests/test.flv", TMPPATH . "test_flv_with_no_extension"); - $this->assert_equal(array(360, 288, null, null, 6.00), - movie::get_file_metadata(TMPPATH . "test_flv_with_no_extension")); + // Since mime type and extension are based solely on the filename, this is considered invalid. + try { + $metadata = movie::get_file_metadata(TMPPATH . "test_flv_with_no_extension"); + $this->assert_true(false, "Shouldn't get here"); + } catch (Exception $e) { + // pass + } } public function get_file_metadata_with_illegal_extension_test() { - $this->assert_equal(array(0, 0, null, null, 0), - movie::get_file_metadata(MODPATH . "gallery/tests/Movie_Helper_Test.php")); + try { + $metadata = movie::get_file_metadata(MODPATH . "gallery/tests/Movie_Helper_Test.php"); + $this->assert_true(false, "Shouldn't get here"); + } catch (Exception $e) { + // pass + } } public function get_file_metadata_with_illegal_extension_but_valid_file_contents_test() { copy(MODPATH . "gallery/tests/test.flv", TMPPATH . "test_flv_with_php_extension.php"); - $this->assert_equal(array(360, 288, null, null, 6.00), - movie::get_file_metadata(TMPPATH . "test_flv_with_php_extension.php")); + // Since mime type and extension are based solely on the filename, this is considered invalid. + try { + $metadata = movie::get_file_metadata(TMPPATH . "test_flv_with_php_extension.php"); + $this->assert_true(false, "Shouldn't get here"); + } catch (Exception $e) { + // pass + } + } + + public function get_file_metadata_with_valid_extension_but_illegal_file_contents_test() { + copy(MODPATH . "gallery/tests/Photo_Helper_Test.php", TMPPATH . "test_php_with_flv_extension.flv"); + // Since mime type and extension are based solely on the filename, this is considered valid. + // Of course, FFmpeg cannot extract width, height, or duration from the file. Note that this + // isn't a really a security problem, since the filename doesn't have a php extension and + // therefore will never be executed. + $this->assert_equal(array(0, 0, "video/x-flv", "flv", 0), + movie::get_file_metadata(TMPPATH . "test_php_with_flv_extension.flv")); } } diff --git a/modules/gallery/tests/Photo_Helper_Test.php b/modules/gallery/tests/Photo_Helper_Test.php index 5207a6db..79b5ccfd 100644 --- a/modules/gallery/tests/Photo_Helper_Test.php +++ b/modules/gallery/tests/Photo_Helper_Test.php @@ -40,8 +40,12 @@ class Photo_Helper_Test extends Gallery_Unit_Test_Case { } public function get_file_metadata_with_illegal_extension_test() { - $this->assert_equal(array(0, 0, null, null), - photo::get_file_metadata(MODPATH . "gallery/tests/Photo_Helper_Test.php")); + try { + $metadata = photo::get_file_metadata(MODPATH . "gallery/tests/Photo_Helper_Test.php"); + $this->assert_true(false, "Shouldn't get here"); + } catch (Exception $e) { + // pass + } } public function get_file_metadata_with_illegal_extension_but_valid_file_contents_test() { @@ -53,4 +57,14 @@ class Photo_Helper_Test extends Gallery_Unit_Test_Case { $this->assert_equal(array(1024, 768, "image/jpeg", "jpg"), photo::get_file_metadata(TMPPATH . "test_jpg_with_php_extension.php")); } + + public function get_file_metadata_with_valid_extension_but_illegal_file_contents_test() { + copy(MODPATH . "gallery/tests/Photo_Helper_Test.php", TMPPATH . "test_php_with_jpg_extension.jpg"); + try { + $metadata = photo::get_file_metadata(TMPPATH . "test_php_with_jpg_extension.jpg"); + $this->assert_true(false, "Shouldn't get here"); + } catch (Exception $e) { + // pass + } + } } diff --git a/modules/watermark/controllers/admin_watermarks.php b/modules/watermark/controllers/admin_watermarks.php index 27c2efc9..59bb7fa9 100644 --- a/modules/watermark/controllers/admin_watermarks.php +++ b/modules/watermark/controllers/admin_watermarks.php @@ -102,18 +102,17 @@ class Admin_Watermarks_Controller extends Admin_Controller { $name = preg_replace("/uploadfile-[^-]+-(.*)/", '$1', $pathinfo["basename"]); $name = legal_file::smash_extensions($name); - list ($width, $height, $mime_type, $extension) = photo::get_file_metadata($file); - if (!$width || !$height || !$mime_type || !$extension || - !legal_file::get_photo_extensions($extension)) { - message::error(t("Invalid or unidentifiable image file")); - @unlink($file); - return; - } else { + try { + list ($width, $height, $mime_type, $extension) = photo::get_file_metadata($file); // Force correct, legal extension type on file, which will be of our canonical type // (i.e. all lowercase, jpg instead of jpeg, etc.). This renaming prevents the issues // addressed in ticket #1855, where an image that looked valid (header said jpg) with a // php extension was previously accepted without changing its extension. $name = legal_file::change_extension($name, $extension); + } catch (Exception $e) { + message::error(t("Invalid or unidentifiable image file")); + @unlink($file); + return; } rename($file, VARPATH . "modules/watermark/$name"); -- cgit v1.2.3