Merge pull request #134 from shadlaws/fix_2001

#2001 - Make filename sanitizing more consistent.

6 files changed, 202 insertions, 108 deletions

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 debd1e6d..eb9c25de 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,
@@ -625,6 +596,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);