#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.

3 files changed, 60 insertions, 10 deletions

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
+    }
+  }
 }