Merge pull request #209 from shadlaws/fix_2057

#2057 - Revise item name and slug validation - backslashes, refactor, error messages.

8 files changed, 104 insertions, 20 deletions

diff --git a/installer/install.sql b/installer/install.sql
index f4938f6f..3f63cf7c 100644
--- a/installer/install.sql
+++ b/installer/install.sql
@@ -245,7 +245,7 @@ CREATE TABLE {modules} (
   KEY `weight` (`weight`)
 ) AUTO_INCREMENT=10 DEFAULT CHARSET=utf8;
 /*!40101 SET character_set_client = @saved_cs_client */;
-INSERT INTO {modules} VALUES (1,1,'gallery',57,1);
+INSERT INTO {modules} VALUES (1,1,'gallery',58,1);
 INSERT INTO {modules} VALUES (2,1,'user',4,2);
 INSERT INTO {modules} VALUES (3,1,'comment',7,3);
 INSERT INTO {modules} VALUES (4,1,'organize',4,4);
diff --git a/modules/gallery/helpers/album.php b/modules/gallery/helpers/album.php
index 23aed8ac..fe6b03fc 100644
--- a/modules/gallery/helpers/album.php
+++ b/modules/gallery/helpers/album.php
@@ -34,12 +34,16 @@ class album_Core {
       ->error_messages("length", t("Your title is too long"));
     $group->textarea("description")->label(t("Description"));
     $group->input("name")->label(t("Directory name"))
-      ->error_messages("no_slashes", t("The directory name can't contain the \"/\" character"))
+      ->error_messages("no_slashes", t("The directory name can't contain a \"/\""))
+      ->error_messages("no_backslashes", t("The directory name can't contain a \"\\\""))
+      ->error_messages("no_trailing_period", t("The directory name can't end in \".\""))
       ->error_messages("required", t("You must provide a directory name"))
       ->error_messages("length", t("Your directory name is too long"))
       ->error_messages("conflict", t("There is already a movie, photo or album with this name"));
     $group->input("slug")->label(t("Internet Address"))
       ->error_messages(
+        "conflict", t("There is already a movie, photo or album with this internet address"))
+      ->error_messages(
         "reserved", t("This address is reserved and can't be used."))
       ->error_messages(
         "not_url_safe",
@@ -64,13 +68,14 @@ class album_Core {
     $group = $form->group("edit_item")->label(t("Edit Album"));
 
     $group->input("title")->label(t("Title"))->value($parent->title)
-        ->error_messages("required", t("You must provide a title"))
+      ->error_messages("required", t("You must provide a title"))
       ->error_messages("length", t("Your title is too long"));
     $group->textarea("description")->label(t("Description"))->value($parent->description);
     if ($parent->id != 1) {
       $group->input("name")->label(t("Directory Name"))->value($parent->name)
         ->error_messages("conflict", t("There is already a movie, photo or album with this name"))
         ->error_messages("no_slashes", t("The directory name can't contain a \"/\""))
+        ->error_messages("no_backslashes", t("The directory name can't contain a \"\\\""))
         ->error_messages("no_trailing_period", t("The directory name can't end in \".\""))
         ->error_messages("required", t("You must provide a directory name"))
         ->error_messages("length", t("Your directory name is too long"));
diff --git a/modules/gallery/helpers/gallery_installer.php b/modules/gallery/helpers/gallery_installer.php
index d49be83f..f1604150 100644
--- a/modules/gallery/helpers/gallery_installer.php
+++ b/modules/gallery/helpers/gallery_installer.php
@@ -809,6 +809,26 @@ class gallery_installer {
         ->execute();
       module::set_version("gallery", $version = 57);
     }
+
+    if ($version == 57) {
+      // In v58 we changed the Item_Model validation code to disallow files or directories with
+      // backslashes in them, and we need to fix any existing items that have them.  This is
+      // pretty unlikely, as having backslashes would have probably already caused other issues for
+      // users, but we should check anyway.  This might be slow, but if it times out it can just
+      // pick up where it left off.
+      foreach (db::build()
+               ->from("items")
+               ->select("id")
+               ->where(db::expr("`name` REGEXP '\\\\\\\\'"), "=", 1)  // one \, 3x escaped
+               ->order_by("id", "asc")
+               ->execute() as $row) {
+        set_time_limit(30);
+        $item = ORM::factory("item", $row->id);
+        $item->name = str_replace("\\", "_", $item->name);
+        $item->save();
+      }
+      module::set_version("gallery", $version = 58);
+    }
   }
 
   static function uninstall() {
diff --git a/modules/gallery/helpers/movie.php b/modules/gallery/helpers/movie.php
index 2f190881..4613df61 100644
--- a/modules/gallery/helpers/movie.php
+++ b/modules/gallery/helpers/movie.php
@@ -38,6 +38,7 @@ class movie_Core {
       ->error_messages(
         "conflict", t("There is already a movie, photo or album with this name"))
       ->error_messages("no_slashes", t("The movie name can't contain a \"/\""))
+      ->error_messages("no_backslashes", t("The movie name can't contain a \"\\\""))
       ->error_messages("no_trailing_period", t("The movie name can't end in \".\""))
       ->error_messages("illegal_data_file_extension", t("You cannot change the movie file extension"))
       ->error_messages("required", t("You must provide a movie file name"))
diff --git a/modules/gallery/helpers/photo.php b/modules/gallery/helpers/photo.php
index 004cc7c4..ecf81e66 100644
--- a/modules/gallery/helpers/photo.php
+++ b/modules/gallery/helpers/photo.php
@@ -35,6 +35,7 @@ class photo_Core {
     $group->input("name")->label(t("Filename"))->value($photo->name)
       ->error_messages("conflict", t("There is already a movie, photo or album with this name"))
       ->error_messages("no_slashes", t("The photo name can't contain a \"/\""))
+      ->error_messages("no_backslashes", t("The photo name can't contain a \"\\\""))
       ->error_messages("no_trailing_period", t("The photo name can't end in \".\""))
       ->error_messages("illegal_data_file_extension", t("You cannot change the photo file extension"))
       ->error_messages("required", t("You must provide a photo file name"))
diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 1e16d307..b708c503 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -889,12 +889,17 @@ class Item_Model_Core extends ORM_MPTT {
   }
 
   /**
-   * Validate that the desired slug does not conflict.
+   * Validate the item slug.  It can return the following error messages:
+   * - not_url_safe: has illegal characters
+   * - conflict: has conflicting slug
+   * - reserved (items in root only): has same slug as a controller
    */
   public function valid_slug(Validation $v, $field) {
     if (preg_match("/[^A-Za-z0-9-_]/", $this->slug)) {
       $v->add_error("slug", "not_url_safe");
-    } else if (db::build()
+    }
+
+    if (db::build()
         ->from("items")
         ->where("parent_id", "=", $this->parent_id)
         ->where("id", "<>", $this->id)
@@ -902,11 +907,20 @@ class Item_Model_Core extends ORM_MPTT {
         ->count_records()) {
       $v->add_error("slug", "conflict");
     }
+
+    if ($this->parent_id == 1 && Kohana::auto_load("{$this->slug}_Controller")) {
+      $v->add_error("slug", "reserved");
+      return;
+    }
   }
 
   /**
-   * Validate the item name.  It can't conflict with other names, can't contain slashes or
-   * trailing periods.
+   * Validate the item name.  It can return the following error messages:
+   * - no_slashes: contains slashes
+   * - no_backslashes: contains backslashes
+   * - no_trailing_period: has a trailing period
+   * - illegal_data_file_extension (non-albums only): has double, no, or illegal extension
+   * - conflict: has conflicting name
    */
   public function valid_name(Validation $v, $field) {
     if (strpos($this->name, "/") !== false) {
@@ -914,18 +928,23 @@ class Item_Model_Core extends ORM_MPTT {
       return;
     }
 
-    if (rtrim($this->name, ".") !== $this->name) {
-      $v->add_error("name", "no_trailing_period");
+    if (strpos($this->name, "\\") !== false) {
+      $v->add_error("name", "no_backslashes");
       return;
     }
 
-    // Do not accept files with double extensions, they can cause problems on some
-    // versions of Apache.
-    if (!$this->is_album() && substr_count($this->name, ".") > 1) {
-      $v->add_error("name", "illegal_data_file_extension");
+    if (rtrim($this->name, ".") !== $this->name) {
+      $v->add_error("name", "no_trailing_period");
+      return;
     }
 
     if ($this->is_movie() || $this->is_photo()) {
+      if (substr_count($this->name, ".") > 1) {
+        // Do not accept files with double extensions, as they can
+        // cause problems on some versions of Apache.
+        $v->add_error("name", "illegal_data_file_extension");
+      }
+
       $ext = pathinfo($this->name, PATHINFO_EXTENSION);
 
       if (!$this->loaded() && !$ext) {
@@ -967,11 +986,6 @@ class Item_Model_Core extends ORM_MPTT {
         return;
       }
     }
-
-    if ($this->parent_id == 1 && Kohana::auto_load("{$this->slug}_Controller")) {
-      $v->add_error("slug", "reserved");
-      return;
-    }
   }
 
   /**
diff --git a/modules/gallery/module.info b/modules/gallery/module.info
index 7f49b72e..49023e45 100644
--- a/modules/gallery/module.info
+++ b/modules/gallery/module.info
@@ -1,6 +1,6 @@
 name = "Gallery 3"
 description = "Gallery core application"
-version = 57
+version = 58
 author_name = "Gallery Team"
 author_url = "http://codex.galleryproject.org/Gallery:Team"
 info_url = "http://codex.galleryproject.org/Gallery3:Modules:gallery"
diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php
index 83c9f79d..e3a4a6b7 100644
--- a/modules/gallery/tests/Item_Model_Test.php
+++ b/modules/gallery/tests/Item_Model_Test.php
@@ -124,13 +124,56 @@ class Item_Model_Test extends Gallery_Unit_Test_Case {
     $this->assert_equal($fullsize_file, file_get_contents($photo->file_path()));
   }
 
-  public function item_rename_wont_accept_slash_test() {
+  public function photo_rename_wont_accept_slash_test() {
     $item = test::random_photo();
     $item->name = "/no_slashes/allowed/";
     $item->save();
     $this->assert_equal("no_slashes_allowed.jpg", $item->name);
   }
 
+  public function photo_rename_wont_accept_backslash_test() {
+    $item = test::random_photo();
+    $item->name = "\\no_backslashes\\allowed\\";
+    $item->save();
+    $this->assert_equal("no_backslashes_allowed.jpg", $item->name);
+  }
+
+  public function album_rename_wont_accept_slash_test() {
+    try {
+      $item = test::random_album();
+      $item->name = "/no_album_slashes/allowed/";
+      $item->save();
+    } catch (ORM_Validation_Exception $e) {
+      $this->assert_same(array("name" => "no_slashes"), $e->validation->errors());
+      return;  // pass
+    }
+    $this->assert_true(false, "Shouldn't get here");
+  }
+
+  public function album_rename_wont_accept_backslash_test() {
+    try {
+      $item = test::random_album();
+      $item->name = "\\no_album_backslashes\\allowed\\";
+      $item->save();
+    } catch (ORM_Validation_Exception $e) {
+      $this->assert_same(array("name" => "no_backslashes"), $e->validation->errors());
+      return;  // pass
+    }
+    $this->assert_true(false, "Shouldn't get here");
+  }
+
+  public function album_rename_wont_accept_trailing_period_test() {
+    try {
+      $item = test::random_album();
+      $item->name = ".no_trailing_period.allowed.";
+      $item->save();
+    } catch (ORM_Validation_Exception $e) {
+      $this->assert_same(array("name" => "no_trailing_period"), $e->validation->errors());
+      return;  // pass
+    }
+    $this->assert_true(false, "Shouldn't get here");
+  }
+
   public function move_album_test() {
     $album2 = test::random_album();
     $album1 = test::random_album($album2);