From 2da4d9dd1f99707778b61a320601921243202d2c Mon Sep 17 00:00:00 2001
From: shadlaws <shad@shadlaws.com>
Date: Thu, 7 Feb 2013 09:49:28 +0100
Subject: #1992 - Improve examples for thumb/resize/file path/url functions in
 item model. Documentation-only changes to examples used in: - Item_Model::url
 - Item_Model::abs_url - Item_Model::file_path - Item_Model::file_url -
 Item_Model::resize_path - Item_Model::resize_url - Item_Model::thumb_path -
 Item_Model::thumb_url

---
 modules/gallery/models/item.php | 64 +++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 18 deletions(-)

(limited to 'modules/gallery/models')

diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index f80fc53a..197d3057 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -129,10 +129,11 @@ class Item_Model_Core extends ORM_MPTT {
 
   /**
    * Return the server-relative url to this item, eg:
-   *   /gallery3/index.php/BobsWedding?page=2
-   *   /gallery3/index.php/BobsWedding/Eating-Cake.jpg
+   *   album: /gallery3/index.php/Bobs%20Wedding?page=2
+   *   photo: /gallery3/index.php/Bobs%20Wedding/Eating-Cake
+   *   movie: /gallery3/index.php/Bobs%20Wedding/First-Dance
    *
-   * @param string $query the query string (eg "show=3")
+   * @param string $query the query string (eg "page=2")
    */
   public function url($query=null) {
     $url = url::site($this->relative_url());
@@ -144,10 +145,11 @@ class Item_Model_Core extends ORM_MPTT {
 
   /**
    * Return the full url to this item, eg:
-   *   http://example.com/gallery3/index.php/BobsWedding?page=2
-   *   http://example.com/gallery3/index.php/BobsWedding/Eating-Cake.jpg
+   *   album: http://example.com/gallery3/index.php/Bobs%20Wedding?page=2
+   *   photo: http://example.com/gallery3/index.php/Bobs%20Wedding/Eating-Cake
+   *   movie: http://example.com/gallery3/index.php/Bobs%20Wedding/First-Dance
    *
-   * @param string $query the query string (eg "show=3")
+   * @param string $query the query string (eg "page=2")
    */
   public function abs_url($query=null) {
     $url = url::abs_site($this->relative_url());
@@ -158,16 +160,24 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * album: /var/albums/album1/album2
-   * photo: /var/albums/album1/album2/photo.jpg
+   * Return the full path to this item's file, eg:
+   *   album: /usr/home/www/gallery3/var/albums/Bobs Wedding
+   *   photo: /usr/home/www/gallery3/var/albums/Bobs Wedding/Eating-Cake.jpg
+   *   movie: /usr/home/www/gallery3/var/albums/Bobs Wedding/First-Dance.mp4
    */
   public function file_path() {
     return VARPATH . "albums/" . urldecode($this->relative_path());
   }
 
   /**
-   * album: http://example.com/gallery3/var/resizes/album1/
-   * photo: http://example.com/gallery3/var/albums/album1/photo.jpg
+   * Return the relative url to this item's file, with cache buster, eg:
+   *   album: var/albums/Bobs%20Wedding?m=1234567890
+   *   photo: var/albums/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: var/albums/Bobs%20Wedding/First-Dance.mp4?m=1234567890
+   * If $full_uri==true, return the full url to this item's file, with cache buster, eg:
+   *   album: http://example.com/gallery3/var/albums/Bobs%20Wedding?m=1234567890
+   *   photo: http://example.com/gallery3/var/albums/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: http://example.com/gallery3/var/albums/Bobs%20Wedding/First-Dance.mp4?m=1234567890
    */
   public function file_url($full_uri=false) {
     $relative_path = "var/albums/" . $this->relative_path();
@@ -177,8 +187,10 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * album: /var/resizes/album1/.thumb.jpg
-   * photo: /var/albums/album1/photo.thumb.jpg
+   * Return the full path to this item's thumb, eg:
+   *   album: /usr/home/www/gallery3/var/thumbs/Bobs Wedding/.album.jpg
+   *   photo: /usr/home/www/gallery3/var/thumbs/Bobs Wedding/Eating-Cake.jpg
+   *   movie: /usr/home/www/gallery3/var/thumbs/Bobs Wedding/First-Dance.jpg
    */
   public function thumb_path() {
     $base = VARPATH . "thumbs/" . urldecode($this->relative_path());
@@ -200,8 +212,14 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * album: http://example.com/gallery3/var/resizes/album1/.thumb.jpg
-   * photo: http://example.com/gallery3/var/albums/album1/photo.thumb.jpg
+   * Return the relative url to this item's thumb, with cache buster, eg:
+   *   album: var/thumbs/Bobs%20Wedding/.album.jpg?m=1234567890
+   *   photo: var/thumbs/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: var/thumbs/Bobs%20Wedding/First-Dance.mp4?m=1234567890
+   * If $full_uri==true, return the full url to this item's file, with cache buster, eg:
+   *   album: http://example.com/gallery3/var/thumbs/Bobs%20Wedding/.album.jpg?m=1234567890
+   *   photo: http://example.com/gallery3/var/thumbs/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: http://example.com/gallery3/var/thumbs/Bobs%20Wedding/First-Dance.mp4?m=1234567890
    */
   public function thumb_url($full_uri=false) {
     $cache_buster = $this->_cache_buster($this->thumb_path());
@@ -219,8 +237,11 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * album: /var/resizes/album1/.resize.jpg
-   * photo: /var/albums/album1/photo.resize.jpg
+   * Return the full path to this item's resize, eg:
+   *   album: /usr/home/www/gallery3/var/resizes/Bobs Wedding/.album.jpg      (*)
+   *   photo: /usr/home/www/gallery3/var/resizes/Bobs Wedding/Eating-Cake.jpg
+   *   movie: /usr/home/www/gallery3/var/resizes/Bobs Wedding/First-Dance.mp4 (*)
+   * (*) Since only photos have resizes, album and movie paths are fictitious.
    */
   public function resize_path() {
     return VARPATH . "resizes/" . urldecode($this->relative_path()) .
@@ -228,8 +249,15 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * album: http://example.com/gallery3/var/resizes/album1/.resize.jpg
-   * photo: http://example.com/gallery3/var/albums/album1/photo.resize.jpg
+   * Return the relative url to this item's resize, with cache buster, eg:
+   *   album: var/resizes/Bobs%20Wedding/.album.jpg?m=1234567890      (*)
+   *   photo: var/resizes/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: var/resizes/Bobs%20Wedding/First-Dance.mp4?m=1234567890 (*)
+   * If $full_uri==true, return the full url to this item's file, with cache buster, eg:
+   *   album: http://example.com/gallery3/var/resizes/Bobs%20Wedding/.album.jpg?m=1234567890      (*)
+   *   photo: http://example.com/gallery3/var/resizes/Bobs%20Wedding/Eating-Cake.jpg?m=1234567890
+   *   movie: http://example.com/gallery3/var/resizes/Bobs%20Wedding/First-Dance.mp4?m=1234567890 (*)
+   * (*) Since only photos have resizes, album and movie urls are fictitious.
    */
   public function resize_url($full_uri=false) {
     $relative_path = "var/resizes/" . $this->relative_path();
-- 
cgit v1.2.3


From 0312d1b071bd4434ddb3f82888b0323da6bf3732 Mon Sep 17 00:00:00 2001
From: shadlaws <shad@shadlaws.com>
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/models')

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


From d04a6fc87d96b70ab0f70414f2ff40d1f1e7f482 Mon Sep 17 00:00:00 2001
From: shadlaws <shad@shadlaws.com>
Date: Tue, 12 Feb 2013 00:37:33 +0100
Subject: #2001 - Make filename sanitizing more consistent. - legal_file -
 added sanitize_filname() to sanitize photo/movie filenames. -
 admin_watermarks - revised add() to use new function. - item model - added
 _process_data_file_info() to validate the data file, get its metadata, and
 sanitize the item name. - item model - revised save() for new items to use
 _process_data_file_info *before* the slug is checked. - item model - revised
 save() for updated items to use _process_data_file_info. - item model -
 revised save() for updated items to sanitize name if changed. - uploader -
 removed call to smash_extensions (item model does this when it calls
 sanitize_filename). - Legal_File_Helper_Test - added unit tests for
 sanitize_filename. - Item_Model_Test - revised existing unit tests based on
 changes. - Item_Model_Test - added new unit tests for names with legal but
 incorrect extensions. - Averted take over by HAL with fix #2001...

---
 modules/gallery/controllers/uploader.php           |   4 -
 modules/gallery/helpers/legal_file.php             |  57 ++++++++++++
 modules/gallery/models/item.php                    |  95 ++++++++++---------
 modules/gallery/tests/Item_Model_Test.php          | 101 ++++++++++-----------
 modules/gallery/tests/Legal_File_Helper_Test.php   |  44 +++++++++
 modules/watermark/controllers/admin_watermarks.php |   9 +-
 6 files changed, 202 insertions(+), 108 deletions(-)

(limited to 'modules/gallery/models')

diff --git a/modules/gallery/controllers/uploader.php b/modules/gallery/controllers/uploader.php
index 55c65c95..78437071 100644
--- a/modules/gallery/controllers/uploader.php
+++ b/modules/gallery/controllers/uploader.php
@@ -63,10 +63,6 @@ class Uploader_Controller extends Controller {
         $item->parent_id = $album->id;
         $item->set_data_file($temp_filename);
 
-        // Remove double extensions from the filename - they'll be disallowed in the model but if
-        // we don't do it here then it'll result in a failed upload.
-        $item->name = legal_file::smash_extensions($item->name);
-
         $path_info = @pathinfo($temp_filename);
         if (array_key_exists("extension", $path_info) &&
             legal_file::get_movie_extensions($path_info["extension"])) {
diff --git a/modules/gallery/helpers/legal_file.php b/modules/gallery/helpers/legal_file.php
index ef588ceb..5a852f2b 100644
--- a/modules/gallery/helpers/legal_file.php
+++ b/modules/gallery/helpers/legal_file.php
@@ -250,4 +250,61 @@ class legal_file_Core {
     $result .= isset($parts["extension"]) ? "{$parts['filename']}.{$parts['extension']}" : $parts["filename"];
     return $result;
   }
+
+  /**
+   * Sanitize a filename for a given type (given as "photo" or "movie") and a target file format
+   * (given as an extension).  This returns a completely legal and valid filename,
+   * or throws an exception if the type or extension given is invalid or illegal.  It tries to
+   * maintain the filename's original extension even if it's not identical to the given extension
+   * (e.g. don't change "JPG" or "jpeg" to "jpg").
+   *
+   * Note: it is not okay if the extension given is legal but does not match the type (e.g. if
+   * extension is "mp4" and type is "photo", it will throw an exception)
+   *
+   * @param  string $filename  (with no directory)
+   * @param  string $extension (can be uppercase or lowercase)
+   * @param  string $type      (as "photo" or "movie")
+   * @return string sanitized filename (or null if bad extension argument)
+   */
+  static function sanitize_filename($filename, $extension, $type) {
+    // Check if the type is valid - if so, get the mime types of the
+    // original and target extensions; if not, throw an exception.
+    $original_extension = pathinfo($filename, PATHINFO_EXTENSION);
+    switch ($type) {
+      case "photo":
+        $mime_type = legal_file::get_photo_types_by_extension($extension);
+        $original_mime_type = legal_file::get_photo_types_by_extension($original_extension);
+        break;
+      case "movie":
+        $mime_type = legal_file::get_movie_types_by_extension($extension);
+        $original_mime_type = legal_file::get_movie_types_by_extension($original_extension);
+        break;
+      default:
+        throw new Exception("@todo INVALID_TYPE");
+    }
+
+    // Check if the target extension is blank or invalid - if so, throw an exception.
+    if (!$extension || !$mime_type) {
+      throw new Exception("@todo ILLEGAL_EXTENSION");
+    }
+
+    // Check if the mime types of the original and target extensions match - if not, fix it.
+    if (!$original_extension || ($mime_type != $original_mime_type)) {
+      $filename = legal_file::change_extension($filename, $extension);
+    }
+
+    // It should be a filename without a directory - remove all slashes (and backslashes).
+    $filename = str_replace("/", "_", $filename);
+    $filename = str_replace("\\", "_", $filename);
+
+    // Remove extra dots from the filename.  This will also remove extraneous underscores.
+    $filename = legal_file::smash_extensions($filename);
+
+    // It's possible that the filename has no base (e.g. ".jpg") - if so, give it a generic one.
+    if (empty($filename) || (substr($filename, 0, 1) == ".")) {
+      $filename = $type . $filename;  // e.g. "photo.jpg" or "movie.mp4"
+    }
+
+    return $filename;
+  }
 }
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 33b8a89d..43b9a292 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -365,6 +365,14 @@ class Item_Model_Core extends ORM_MPTT {
           $this->weight = item::get_max_weight();
         }
 
+        // Process the data file info.
+        if (isset($this->data_file)) {
+          $this->_process_data_file_info();
+        } else if (!$this->is_album()) {
+          // Unless it's an album, new items must have a data file.
+          $this->data_file_error = true;
+        }
+
         // Make an url friendly slug from the name, if necessary
         if (empty($this->slug)) {
           $this->slug = item::convert_filename_to_slug(pathinfo($this->name, PATHINFO_FILENAME));
@@ -377,31 +385,6 @@ 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()) {
-          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;
-          }
-        }
-
         $this->_check_and_fix_conflicts();
 
         parent::save();
@@ -439,31 +422,19 @@ class Item_Model_Core extends ORM_MPTT {
         // keep it around.
         $original = ORM::factory("item", $this->id);
 
-        // Preserve the extension of the data file. Many helpers, (e.g. ImageMagick), assume
+        // If we have a new data file, process its info.  This will get its metadata and
+        // preserve the extension of the data file. Many helpers, (e.g. ImageMagick), assume
         // the MIME type from the extension. So when we adopt the new data file, it's important
         // to adopt the new extension. That ensures that the item's extension is always
         // 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)) {
-          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;
-          }
+          $this->_process_data_file_info();
+        } else if (!$this->is_album() && array_key_exists("name", $this->changed)) {
+          // There's no new data file, but the name changed.  If it's a photo or movie,
+          // make sure the new name still agrees with the file type.
+          $this->name = legal_file::sanitize_filename($this->name,
+            pathinfo($original->name, PATHINFO_EXTENSION), $this->type);
         }
 
         // If an album's cover has changed (or been removed), delete any existing album cover,
@@ -624,6 +595,40 @@ class Item_Model_Core extends ORM_MPTT {
     }
   }
 
+  /**
+   * Process the data file info.  Get its metadata and extension.
+   * If valid, use it to sanitize the item name and update the
+   * width, height, and mime type.
+   */
+  private function _process_data_file_info() {
+    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);
+      } else {
+        // Albums don't have data files.
+        $this->data_file = null;
+        return;
+      }
+
+      // Sanitize the name based on the idenified extension, but only set $this->name if different
+      // to ensure it isn't unnecessarily marked as "changed"
+      $name = legal_file::sanitize_filename($this->name, $extension, $this->type);
+      if ($this->name != $name) {
+        $this->name = $name;
+      }
+
+      // 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;
+    }
+  }
+
   /**
    * Return the Item_Model representing the cover for this album.
    * @return Item_Model or null if there's no cover
diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php
index a93498dd..fcb5c2ad 100644
--- a/modules/gallery/tests/Item_Model_Test.php
+++ b/modules/gallery/tests/Item_Model_Test.php
@@ -126,14 +126,9 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
 
   public function item_rename_wont_accept_slash_test() {
     $item = test::random_photo();
-    try {
-      $item->name = test::random_name() . "/";
-      $item->save();
-    } catch (ORM_Validation_Exception $e) {
-      $this->assert_equal(array("name" => "no_slashes"), $e->validation->errors());
-      return;
-    }
-    $this->assert_true(false, "Shouldn't get here");
+    $item->name = "/no_slashes/allowed/";
+    $item->save();
+    $this->assert_equal("no_slashes_allowed.jpg", $item->name);
   }
 
   public function move_album_test() {
@@ -328,30 +323,17 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
   }
 
   public function photo_files_must_have_an_extension_test() {
-    try {
-      $photo = test::random_photo_unsaved();
-      $photo->mime_type = "image/jpeg";
-      $photo->name = "no_extension";
-      $photo->save();
-    } catch (ORM_Validation_Exception $e) {
-      $this->assert_same(array("name" => "illegal_data_file_extension"), $e->validation->errors());
-      return;  // pass
-    }
-    $this->assert_true(false, "Shouldn't get here");
+    $photo = test::random_photo_unsaved();
+    $photo->name = "no_extension_photo";
+    $photo->save();
+    $this->assert_equal("no_extension_photo.jpg", $photo->name);
   }
 
   public function movie_files_must_have_an_extension_test() {
-    try {
-      $movie = test::random_movie_unsaved();
-      $movie->type = "movie";
-      $movie->mime_type = "video/x-flv";
-      $movie->name = "no_extension";
-      $movie->save();
-    } catch (ORM_Validation_Exception $e) {
-      $this->assert_same(array("name" => "illegal_data_file_extension"), $e->validation->errors());
-      return;  // pass
-    }
-    $this->assert_true(false, "Shouldn't get here");
+    $movie = test::random_movie_unsaved();
+    $movie->name = "no_extension_movie";
+    $movie->save();
+    $this->assert_equal("no_extension_movie.flv", $movie->name);
   }
 
   public function cant_delete_root_album_test() {
@@ -445,7 +427,7 @@ 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("name" => "illegal_data_file_extension"), $e->validation->errors());
+      $this->assert_same(array("name" => "invalid_data_file"), $e->validation->errors());
       return;  // pass
     }
     $this->assert_true(false, "Shouldn't get here");
@@ -462,6 +444,7 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
       $this->assert_same(array("name" => "invalid_data_file"), $e->validation->errors());
       return;  // pass
     }
+    $this->assert_true(false, "Shouldn't get here");
   }
 
   public function urls_test() {
@@ -493,43 +476,55 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
       $album->thumb_url() . " is malformed");
   }
 
-  public function legal_extension_test() {
-    foreach (array("test.gif", "test.GIF", "test.Gif", "test.jpeg", "test.JPG") as $name) {
+  public function legal_extension_that_does_match_gets_used_test() {
+    foreach (array("jpg", "JPG", "Jpg", "jpeg") as $extension) {
       $photo = test::random_photo_unsaved(item::root());
-      $photo->name = $name;
+      $photo->name = test::random_name() . ".{$extension}";
       $photo->save();
+      // Should get renamed with the correct jpg extension of the data file.
+      $this->assert_equal($extension, pathinfo($photo->name, PATHINFO_EXTENSION));
     }
   }
 
   public function illegal_extension_test() {
     foreach (array("test.php", "test.PHP", "test.php5", "test.php4",
                    "test.pl", "test.php.png") as $name) {
-      try {
-        $photo = test::random_photo_unsaved(item::root());
-        $photo->name = $name;
-        $photo->save();
-      } catch (ORM_Validation_Exception $e) {
-        $this->assert_equal(array("name" => "illegal_data_file_extension"),
-                            $e->validation->errors());
-        continue;
-      }
-      $this->assert_true(false, "Shouldn't get here");
+      $photo = test::random_photo_unsaved(item::root());
+      $photo->name = $name;
+      $photo->save();
+      // Should get renamed with the correct jpg extension of the data file.
+      $this->assert_equal("jpg", pathinfo($photo->name, PATHINFO_EXTENSION));
     }
   }
 
   public function cant_rename_to_illegal_extension_test() {
     foreach (array("test.php.test", "test.php", "test.PHP",
                    "test.php5", "test.php4", "test.pl") as $name) {
-      try {
-        $photo = test::random_photo(item::root());
-        $photo->name = $name;
-        $photo->save();
-      } catch (ORM_Validation_Exception $e) {
-        $this->assert_equal(array("name" => "illegal_data_file_extension"),
-                            $e->validation->errors());
-        continue;
-      }
-      $this->assert_true(false, "Shouldn't get here");
+      $photo = test::random_photo(item::root());
+      $photo->name = $name;
+      $photo->save();
+      // Should get renamed with the correct jpg extension of the data file.
+      $this->assert_equal("jpg", pathinfo($photo->name, PATHINFO_EXTENSION));
+    }
+  }
+
+  public function legal_extension_that_doesnt_match_gets_fixed_test() {
+    foreach (array("test.png", "test.mp4", "test.GIF") as $name) {
+      $photo = test::random_photo_unsaved(item::root());
+      $photo->name = $name;
+      $photo->save();
+      // Should get renamed with the correct jpg extension of the data file.
+      $this->assert_equal("jpg", pathinfo($photo->name, PATHINFO_EXTENSION));
+    }
+  }
+
+  public function rename_to_legal_extension_that_doesnt_match_gets_fixed_test() {
+    foreach (array("test.png", "test.mp4", "test.GIF") as $name) {
+      $photo = test::random_photo(item::root());
+      $photo->name = $name;
+      $photo->save();
+      // Should get renamed with the correct jpg extension of the data file.
+      $this->assert_equal("jpg", pathinfo($photo->name, PATHINFO_EXTENSION));
     }
   }
 
diff --git a/modules/gallery/tests/Legal_File_Helper_Test.php b/modules/gallery/tests/Legal_File_Helper_Test.php
index 203d5616..7ed5214b 100644
--- a/modules/gallery/tests/Legal_File_Helper_Test.php
+++ b/modules/gallery/tests/Legal_File_Helper_Test.php
@@ -150,4 +150,48 @@ class Legal_File_Helper_Test extends Gallery_Unit_Test_Case {
     $this->assert_equal("", legal_file::smash_extensions(""));
     $this->assert_equal(null, legal_file::smash_extensions(null));
   }
+
+  public function sanitize_filename_with_no_rename_test() {
+    $this->assert_equal("foo.jpeg", legal_file::sanitize_filename("foo.jpeg", "jpg", "photo"));
+    $this->assert_equal("foo.jpg", legal_file::sanitize_filename("foo.jpg", "jpeg", "photo"));
+    $this->assert_equal("foo.MP4", legal_file::sanitize_filename("foo.MP4", "mp4", "movie"));
+    $this->assert_equal("foo.mp4", legal_file::sanitize_filename("foo.mp4", "MP4", "movie"));
+  }
+
+  public function sanitize_filename_with_corrected_extension_test() {
+    $this->assert_equal("foo.jpg", legal_file::sanitize_filename("foo.png", "jpg", "photo"));
+    $this->assert_equal("foo.MP4", legal_file::sanitize_filename("foo.jpg", "MP4", "movie"));
+    $this->assert_equal("foo.jpg", legal_file::sanitize_filename("foo.php", "jpg", "photo"));
+  }
+
+  public function sanitize_filename_with_non_standard_chars_and_dots_test() {
+    $this->assert_equal("foo.jpg", legal_file::sanitize_filename("foo", "jpg", "photo"));
+    $this->assert_equal("foo.mp4", legal_file::sanitize_filename("foo.", "mp4", "movie"));
+    $this->assert_equal("foo.jpeg", legal_file::sanitize_filename(".foo.jpeg", "jpg", "photo"));
+    $this->assert_equal("foo_2013_02_10.jpeg",
+      legal_file::sanitize_filename("foo.2013/02/10.jpeg", "jpg", "photo"));
+    $this->assert_equal("foo_bar_baz.jpg",
+      legal_file::sanitize_filename("...foo...bar..baz...png", "jpg", "photo"));
+    $this->assert_equal("j'écris@un#nom_bizarre(mais quand_même_ça_passe.jpg",
+      legal_file::sanitize_filename("/j'écris@un#nom/bizarre(mais quand.même/ça_passe.\$ÇÀ@€#_", "jpg", "photo"));
+  }
+
+  public function sanitize_filename_with_no_base_name_test() {
+    $this->assert_equal("photo.jpg", legal_file::sanitize_filename(".png", "jpg", "photo"));
+    $this->assert_equal("movie.mp4", legal_file::sanitize_filename("__..__", "mp4", "movie"));
+    $this->assert_equal("photo.jpg", legal_file::sanitize_filename(".", "jpg", "photo"));
+    $this->assert_equal("movie.mp4", legal_file::sanitize_filename(null, "mp4", "movie"));
+  }
+
+  public function sanitize_filename_with_invalid_arguments_test() {
+    foreach (array("flv" => "photo", "jpg" => "movie", "php" => "photo",
+                   null => "movie", "jpg" => "album", "jpg" => null) as $extension => $type) {
+      try {
+        legal_file::sanitize_filename("foo.jpg", $extension, $type);
+        $this->assert_true(false, "Shouldn't get here");
+      } catch (Exception $e) {
+        // pass
+      }
+    }
+  }
 }
\ No newline at end of file
diff --git a/modules/watermark/controllers/admin_watermarks.php b/modules/watermark/controllers/admin_watermarks.php
index 59bb7fa9..b058d6a5 100644
--- a/modules/watermark/controllers/admin_watermarks.php
+++ b/modules/watermark/controllers/admin_watermarks.php
@@ -97,18 +97,15 @@ class Admin_Watermarks_Controller extends Admin_Controller {
     // validation logic will correctly reject it.  So, we skip validation when we're running tests.
     if (TEST_MODE || $form->validate()) {
       $file = $_POST["file"];
-      $pathinfo = pathinfo($file);
       // Forge prefixes files with "uploadfile-xxxxxxx" for uniqueness
-      $name = preg_replace("/uploadfile-[^-]+-(.*)/", '$1', $pathinfo["basename"]);
-      $name = legal_file::smash_extensions($name);
+      $name = preg_replace("/uploadfile-[^-]+-(.*)/", '$1', basename($file));
 
       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
+        // Sanitize filename, which ensures a valid extension.  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);
+        $name = legal_file::sanitize_filename($name, $extension, "photo");
       } catch (Exception $e) {
         message::error(t("Invalid or unidentifiable image file"));
         @unlink($file);
-- 
cgit v1.2.3