#1951 - Make metadata generation more flexible (photo and movie helpers).

- added photo_get_file_metadata and movie_get_file_metadata events
- modified photo::get_file_metadata and movie::get_file_metadata to use them
- ensure that non-readable files throw exceptions
- redirected other photo metadata calls in core to photo::get_file_metadata (the helper function already exists, but in many places getimagesize is still called directly)
- added some unit tests (neither of the functions above had one)

6 files changed, 218 insertions, 61 deletions

diff --git a/modules/gallery/helpers/graphics.php b/modules/gallery/helpers/graphics.php
index 51437d4b..0c5f8366 100644
--- a/modules/gallery/helpers/graphics.php
+++ b/modules/gallery/helpers/graphics.php
@@ -195,9 +195,8 @@ class graphics_Core {
         } else {
           copy(MODPATH . "gallery/images/missing_photo.png", $item->thumb_path());
         }
-        $dims = getimagesize($item->thumb_path());
-        $item->thumb_width = $dims[0];
-        $item->thumb_height = $dims[1];
+        list ($item->thumb_width, $item->thumb_height) =
+          photo::get_file_metadata($item->thumb_path());
       }
 
       if (!empty($ops["resize"]))  {
@@ -206,9 +205,8 @@ class graphics_Core {
         } else {
           copy(MODPATH . "gallery/images/missing_photo.png", $item->resize_path());
         }
-        $dims = getimagesize($item->resize_path());
-        $item->resize_width = $dims[0];
-        $item->resize_height = $dims[1];
+        list ($item->resize_width, $item->resize_height) =
+          photo::get_file_metadata($item->resize_path());
       }
       $item->save();
     } catch (Exception $e) {
diff --git a/modules/gallery/helpers/movie.php b/modules/gallery/helpers/movie.php
index 7e6a2e55..6844771b 100644
--- a/modules/gallery/helpers/movie.php
+++ b/modules/gallery/helpers/movie.php
@@ -123,41 +123,79 @@ class movie_Core {
 
   /**
    * Return the width, height, mime_type, extension and duration of the given movie file.
+   * Metadata is first generated using ffmpeg (or set to defaults if it fails),
+   * then can be modified by other modules using movie_get_file_metadata events.
+   *
+   * This function and its use cases are symmetric to those of photo::get_file_metadata.
+   *
+   * @param  string $file_path
+   * @return array  array($width, $height, $mime_type, $extension, $duration)
+   *
+   * Use cases in detail:
+   *   Input is standard movie type (flv/mp4/m4v)
+   *     -> return metadata from ffmpeg
+   *   Input is *not* standard movie type that is supported by ffmpeg (e.g. avi, mts...)
+   *     -> return metadata from ffmpeg
+   *   Input is *not* standard movie type that is *not* supported by ffmpeg but is legal
+   *     -> return zero width, height, and duration; mime type and extension according to legal_file
+   *   Input is *not* standard movie type that is *not* supported by ffmpeg and is *not* legal
+   *     -> return zero width, height, and duration; null mime type and extension
+   *   Input is not readable or does not exist
+   *     -> throw exception
+   * Note: movie_get_file_metadata events can change any of the above cases (except the last one).
    */
   static function get_file_metadata($file_path) {
-    $ffmpeg = movie::find_ffmpeg();
-    if (empty($ffmpeg)) {
-      throw new Exception("@todo MISSING_FFMPEG");
+    if (!is_readable($file_path)) {
+      throw new Exception("@todo UNREADABLE_FILE");
     }
 
-    $cmd = escapeshellcmd($ffmpeg) . " -i " . escapeshellarg($file_path) . " 2>&1";
-    $result = `$cmd`;
-    if (preg_match("/Stream.*?Video:.*?, (\d+)x(\d+)/", $result, $matches_res)) {
-      if (preg_match("/Stream.*?Video:.*? \[.*?DAR (\d+):(\d+).*?\]/", $result, $matches_dar) &&
-          $matches_dar[1] >= 1 && $matches_dar[2] >= 1) {
-        // DAR is defined - determine width based on height and DAR
-        // (should always be int, but adding round to be sure)
-        $matches_res[1] = round($matches_res[2] * $matches_dar[1] / $matches_dar[2]);
+    $metadata = new stdClass();
+    $ffmpeg = movie::find_ffmpeg();
+    if (!empty($ffmpeg)) {
+      // ffmpeg found - use it to get width, height, and duration.
+      $cmd = escapeshellcmd($ffmpeg) . " -i " . escapeshellarg($file_path) . " 2>&1";
+      $result = `$cmd`;
+      if (preg_match("/Stream.*?Video:.*?, (\d+)x(\d+)/", $result, $matches_res)) {
+        if (preg_match("/Stream.*?Video:.*? \[.*?DAR (\d+):(\d+).*?\]/", $result, $matches_dar) &&
+            $matches_dar[1] >= 1 && $matches_dar[2] >= 1) {
+          // DAR is defined - determine width based on height and DAR
+          // (should always be int, but adding round to be sure)
+          $matches_res[1] = round($matches_res[2] * $matches_dar[1] / $matches_dar[2]);
+        }
+        list ($metadata->width, $metadata->height) = array($matches_res[1], $matches_res[2]);
+      } else {
+        list ($metadata->width, $metadata->height) = array(0, 0);
+      }
+
+      if (preg_match("/Duration: (\d+:\d+:\d+\.\d+)/", $result, $matches)) {
+        $metadata->duration = movie::hhmmssdd_to_seconds($matches[1]);
+      } else if (preg_match("/duration.*?:.*?(\d+)/", $result, $matches)) {
+        $metadata->duration = $matches[1];
+      } else {
+        $metadata->duration = 0;
       }
-      list ($width, $height) = array($matches_res[1], $matches_res[2]);
     } else {
-      list ($width, $height) = array(0, 0);
+      // ffmpeg not found - set width, height, and duration to zero.
+      $metadata->width = 0;
+      $metadata->height = 0;
+      $metadata->duration = 0;
     }
 
-    $extension = strtolower(pathinfo($file_path, PATHINFO_EXTENSION));
-    $extension = $extension ? $extension : "flv"; // No extension?  Assume FLV.
-    $mime_type = legal_file::get_movie_types_by_extension($extension);
-    $mime_type = $mime_type ? $mime_type : "video/x-flv"; // No MIME found?  Default to video/x-flv.
-
-    if (preg_match("/Duration: (\d+:\d+:\d+\.\d+)/", $result, $matches)) {
-      $duration = movie::hhmmssdd_to_seconds($matches[1]);
-    } else if (preg_match("/duration.*?:.*?(\d+)/", $result, $matches)) {
-      $duration = $matches[1];
+    $extension = pathinfo($file_path, PATHINFO_EXTENSION);
+    if (!$extension ||
+        (!$metadata->mime_type = legal_file::get_movie_types_by_extension($extension))) {
+        // Extension is empty or illegal.
+        $metadata->extension = null;
+        $metadata->mime_type = null;
     } else {
-      $duration = 0;
+      // Extension is legal (and mime is already set above).
+      $metadata->extension = strtolower($extension);
     }
 
-    return array($width, $height, $mime_type, $extension, $duration);
+    // Run movie_get_file_metadata events which can modify the class, then return results.
+    module::event("movie_get_file_metadata", $file_path, $metadata);
+    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 855cd0ae..51e51507 100644
--- a/modules/gallery/helpers/photo.php
+++ b/modules/gallery/helpers/photo.php
@@ -80,20 +80,61 @@ class photo_Core {
 
   /**
    * Return the width, height, mime_type and extension of the given image file.
+   * Metadata is first generated using getimagesize (or the legal_file mapping if it fails),
+   * then can be modified by other modules using photo_get_file_metadata events.
+   *
+   * This function and its use cases are symmetric to those of photo::get_file_metadata.
+   *
+   * @param  string $file_path
+   * @return array  array($width, $height, $mime_type, $extension)
+   *
+   * Use cases in detail:
+   *   Input is standard photo type (jpg/png/gif)
+   *     -> return metadata from getimagesize()
+   *   Input is *not* standard photo type that is supported by getimagesize (e.g. tif, bmp...)
+   *     -> return metadata from getimagesize()
+   *   Input is *not* standard photo type that is *not* supported by getimagesize but is legal
+   *     -> return zero width and height, mime type and extension according to legal_file
+   *   Input is *not* standard photo type that is *not* supported by getimagesize and is *not* legal
+   *     -> return zero width and height, null mime type and extension
+   *   Input is not readable or does not exist
+   *     -> throw exception
+   * Note: photo_get_file_metadata events can change any of the above cases (except the last one).
    */
   static function get_file_metadata($file_path) {
-    $image_info = getimagesize($file_path);
-    if ($image_info) {
-      $width = $image_info[0];
-      $height = $image_info[1];
-      $mime_type = $image_info["mime"];
-      $extension = image_type_to_extension($image_info[2], false);
-      return array($width, $height, $mime_type, $extension);
+    if (!is_readable($file_path)) {
+      throw new Exception("@todo UNREADABLE_FILE");
+    }
+
+    $metadata = new stdClass();
+    if ($image_info = getimagesize($file_path)) {
+      // getimagesize worked - use its results.
+      $metadata->width = $image_info[0];
+      $metadata->height = $image_info[1];
+      $metadata->mime_type = $image_info["mime"];
+      $metadata->extension = image_type_to_extension($image_info[2], false);
+      // We prefer jpg instead of jpeg (which is returned by image_type_to_extension).
+      if ($metadata->extension == "jpeg") {
+        $metadata->extension = "jpg";
+      }
     } else {
-      // getimagesize failed - use legal_file mapping instead.
-      $extension = strtolower(pathinfo($file_path, PATHINFO_EXTENSION));
-      $mime_type = legal_file::get_photo_types_by_extension($extension);
-      return array(0, 0, $mime_type, $extension);
+      // getimagesize failed - try to use legal_file mapping instead.
+      $extension = pathinfo($file_path, PATHINFO_EXTENSION);
+      if (!$extension ||
+          (!$metadata->mime_type = legal_file::get_photo_types_by_extension($extension))) {
+        // Extension is empty or illegal.
+        $metadata->extension = null;
+        $metadata->mime_type = null;
+      } else {
+        // Extension is legal (and mime is already set above).
+        $metadata->extension = strtolower($extension);
+      }
+      $metadata->width = 0;
+      $metadata->height = 0;
     }
+
+    // Run photo_get_file_metadata events which can modify the class, then return results.
+    module::event("photo_get_file_metadata", $file_path, $metadata);
+    return array($metadata->width, $metadata->height, $metadata->mime_type, $metadata->extension);
   }
 }
diff --git a/modules/gallery/tests/Movie_Helper_Test.php b/modules/gallery/tests/Movie_Helper_Test.php
index ff7f798c..0c262620 100644
--- a/modules/gallery/tests/Movie_Helper_Test.php
+++ b/modules/gallery/tests/Movie_Helper_Test.php
@@ -46,4 +46,36 @@ class Movie_Helper_Test extends Gallery_Unit_Test_Case {
       $this->assert_equal($seconds, movie::hhmmssdd_to_seconds($hhmmssdd));
     }
   }
+
+  public function get_file_metadata_test() {
+    $movie = test::random_movie();
+    $this->assert_equal(array(360, 288, "video/x-flv", "flv", 6.00),
+                        movie::get_file_metadata($movie->file_path()));
+  }
+
+  public function get_file_metadata_with_non_existent_file_test() {
+    try {
+      $metadata = movie::get_file_metadata(MODPATH . "gallery/tests/this_does_not_exist");
+      $this->assert_true(false, "Shouldn't get here");
+    } catch (Exception $e) {
+      // pass
+    }
+  }
+
+  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"));
+  }
+
+  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"));
+  }
+
+  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"));
+  }
 }
diff --git a/modules/gallery/tests/Photo_Helper_Test.php b/modules/gallery/tests/Photo_Helper_Test.php
new file mode 100644
index 00000000..5207a6db
--- /dev/null
+++ b/modules/gallery/tests/Photo_Helper_Test.php
@@ -0,0 +1,56 @@
+<?php defined("SYSPATH") or die("No direct script access.");
+/**
+ * Gallery - a web based photo album viewer and editor
+ * Copyright (C) 2000-2013 Bharat Mediratta
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+class Photo_Helper_Test extends Gallery_Unit_Test_Case {
+  public function get_file_metadata_test() {
+    $photo = test::random_photo();
+    $this->assert_equal(array(1024, 768, "image/jpeg", "jpg"),
+                        photo::get_file_metadata($photo->file_path()));
+  }
+
+  public function get_file_metadata_with_non_existent_file_test() {
+    try {
+      $metadata = photo::get_file_metadata(MODPATH . "gallery/tests/this_does_not_exist");
+      $this->assert_true(false, "Shouldn't get here");
+    } catch (Exception $e) {
+      // pass
+    }
+  }
+
+  public function get_file_metadata_with_no_extension_test() {
+    copy(MODPATH . "gallery/tests/test.jpg", TMPPATH . "test_jpg_with_no_extension");
+    $this->assert_equal(array(1024, 768, "image/jpeg", "jpg"),
+                        photo::get_file_metadata(TMPPATH . "test_jpg_with_no_extension"));
+  }
+
+  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"));
+  }
+
+  public function get_file_metadata_with_illegal_extension_but_valid_file_contents_test() {
+    // This ensures that we correctly "re-type" files with invalid extensions if the contents
+    // themselves are valid.  This is needed to ensure that issues similar to those corrected by
+    // ticket #1855, where an image that looked valid (header said jpg) with a php extension was
+    // previously accepted without changing its extension, do not arise and cause security issues.
+    copy(MODPATH . "gallery/tests/test.jpg", TMPPATH . "test_jpg_with_php_extension.php");
+    $this->assert_equal(array(1024, 768, "image/jpeg", "jpg"),
+                        photo::get_file_metadata(TMPPATH . "test_jpg_with_php_extension.php"));
+  }
+}
diff --git a/modules/watermark/controllers/admin_watermarks.php b/modules/watermark/controllers/admin_watermarks.php
index 0e6e214b..14c2b394 100644
--- a/modules/watermark/controllers/admin_watermarks.php
+++ b/modules/watermark/controllers/admin_watermarks.php
@@ -100,32 +100,24 @@ class Admin_Watermarks_Controller extends Admin_Controller {
       $name = preg_replace("/uploadfile-[^-]+-(.*)/", '$1', $pathinfo["basename"]);
       $name = legal_file::smash_extensions($name);
 
-      if (!($image_info = getimagesize($file)) ||
-          !in_array($image_info[2], array(IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_PNG))) {
-        message::error(t("Unable to identify this image file"));
+      list ($width, $height, $mime_type, $extension) = photo::get_file_metadata($file);
+      if (!legal_file::get_photo_extensions($extension)) {
+        message::error(t("Invalid or unidentifiable image file"));
         @unlink($file);
         return;
-      }
-
-      if (!in_array($pathinfo["extension"], legal_file::get_photo_extensions())) {
-        switch ($image_info[2]) {
-        case IMAGETYPE_GIF:
-          $name = legal_file::change_extension($name, "gif");
-          break;
-        case IMAGETYPE_JPEG:
-          $name = legal_file::change_extension($name, "jpg");
-          break;
-        case IMAGETYPE_PNG:
-          $name = legal_file::change_extension($name, "png");
-          break;
-        }
+      } else {
+        // 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);
       }
 
       rename($file, VARPATH . "modules/watermark/$name");
       module::set_var("watermark", "name", $name);
-      module::set_var("watermark", "width", $image_info[0]);
-      module::set_var("watermark", "height", $image_info[1]);
-      module::set_var("watermark", "mime_type", $image_info["mime"]);
+      module::set_var("watermark", "width", $width);
+      module::set_var("watermark", "height", $height);
+      module::set_var("watermark", "mime_type", $mime_type);
       module::set_var("watermark", "position", $form->add_watermark->position->value);
       module::set_var("watermark", "transparency", $form->add_watermark->transparency->value);
       $this->_update_graphics_rules();