From dc7bf9486f9eaa3f8633c51c786434cec1605834 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Wed, 6 Mar 2013 14:33:24 -0500 Subject: Follow-on for #2043. Fix some tests that made bad assumptions about the state of the world. These were uncovered when we ran the tests on Travis which runs the tests in a different order than what I (and apparently others) use on their dev boxes. --- modules/gallery/tests/Item_Model_Test.php | 2 ++ 1 file changed, 2 insertions(+) (limited to 'modules/gallery/tests/Item_Model_Test.php') diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index fcb5c2ad..83c9f79d 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -362,6 +362,7 @@ class Item_Model_Test extends Gallery_Unit_Test_Case { $response = item::root()->as_restful_array(); $this->assert_true($response["can_edit"]); + access::deny(identity::everybody(), "edit", item::root()); identity::set_active_user(identity::guest()); $response = item::root()->as_restful_array(); $this->assert_false($response["can_edit"]); @@ -371,6 +372,7 @@ class Item_Model_Test extends Gallery_Unit_Test_Case { $response = item::root()->as_restful_array(); $this->assert_true($response["can_add"]); + access::deny(identity::everybody(), "add", item::root()); identity::set_active_user(identity::guest()); $response = item::root()->as_restful_array(); $this->assert_false($response["can_add"]); -- cgit v1.2.3 From ed20798b99c0c6ab90e4d141ff74d7c2ca606ae7 Mon Sep 17 00:00:00 2001 From: shadlaws Date: Tue, 12 Mar 2013 12:14:34 +0100 Subject: #2057 - Revise item name and slug validation - backslashes, refactor, error messages. - disallowed backslashes in item validation. - refactored the validation logic in the item model a bit. - added no_backslash error messages in edit album/photo/movie forms. - fixed error messages in add album forum (some missing, some text different from edit) - added unit tests - updated to v58 to correct any existing backslashes in item names --- installer/install.sql | 2 +- modules/gallery/helpers/album.php | 9 ++++-- modules/gallery/helpers/gallery_installer.php | 20 ++++++++++++ modules/gallery/helpers/movie.php | 1 + modules/gallery/helpers/photo.php | 1 + modules/gallery/models/item.php | 44 +++++++++++++++++--------- modules/gallery/module.info | 2 +- modules/gallery/tests/Item_Model_Test.php | 45 ++++++++++++++++++++++++++- 8 files changed, 104 insertions(+), 20 deletions(-) (limited to 'modules/gallery/tests/Item_Model_Test.php') 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,11 +34,15 @@ 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( @@ -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); -- cgit v1.2.3 From 8d0e1b4c4d456d9d2d94c29412629374d0b26d35 Mon Sep 17 00:00:00 2001 From: shadlaws Date: Wed, 13 Mar 2013 10:07:58 +0100 Subject: #2059 - Add album name sanitizing similar to photo/movie filename sanitizing. - added legal_file::sanitize_dirname(), analogous to sanitize_filename. - revised item model to use new function when adding or updating an album. - added some legal_file unit tests. - revised some item model unit tests. --- modules/gallery/helpers/legal_file.php | 29 +++++- modules/gallery/models/item.php | 23 +++-- modules/gallery/tests/Item_Model_Test.php | 110 ++++++++++++++++++----- modules/gallery/tests/Legal_File_Helper_Test.php | 18 ++++ 4 files changed, 153 insertions(+), 27 deletions(-) (limited to 'modules/gallery/tests/Item_Model_Test.php') diff --git a/modules/gallery/helpers/legal_file.php b/modules/gallery/helpers/legal_file.php index f8547011..9f02fe70 100644 --- a/modules/gallery/helpers/legal_file.php +++ b/modules/gallery/helpers/legal_file.php @@ -298,7 +298,7 @@ class legal_file_Core { $filename = str_replace("/", "_", $filename); $filename = str_replace("\\", "_", $filename); - // Remove extra dots from the filename. This will also remove extraneous underscores. + // Remove extra dots from the filename. Also removes extraneous and leading/trailing 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. @@ -308,4 +308,31 @@ class legal_file_Core { return $filename; } + + /** + * Sanitize a directory name for an album. This returns a completely legal and valid + * directory name. + * + * @param string $dirname (with no parent directory) + * @return string sanitized dirname + */ + static function sanitize_dirname($dirname) { + // It should be a dirname without a parent directory - remove all slashes (and backslashes). + $dirname = str_replace("/", "_", $dirname); + $dirname = str_replace("\\", "_", $dirname); + + // Remove extraneous and leading/trailing underscores. + $dirname = preg_replace("/[_]+/", "_", $dirname); + $dirname = trim($dirname, "_"); + + // Remove any trailing dots. + $dirname = rtrim($dirname, "."); + + // It's possible that the dirname is now empty - if so, give it a generic one. + if (empty($dirname)) { + $dirname = "album"; + } + + return $dirname; + } } diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index b708c503..1d4f35da 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -365,14 +365,20 @@ 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; + if ($this->is_album()) { + // Sanitize the album name. + $this->name = legal_file::sanitize_dirname($this->name); + } else { + // Process the data file info. This also sanitizes the item name. + if (isset($this->data_file)) { + $this->_process_data_file_info(); + } else { + // New photos and movies 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)); @@ -437,6 +443,11 @@ class Item_Model_Core extends ORM_MPTT { pathinfo($original->name, PATHINFO_EXTENSION), $this->type); } + // If an album's name changed, sanitize it. + if ($this->is_album() && array_key_exists("name", $this->changed)) { + $this->name = legal_file::sanitize_dirname($this->name); + } + // If an album's cover has changed (or been removed), delete any existing album cover, // reset the thumb metadata, and mark the thumb as dirty. if (array_key_exists("album_cover_item_id", $this->changed) && $this->is_album()) { diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index e3a4a6b7..b6849413 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -125,53 +125,123 @@ class Item_Model_Test extends Gallery_Unit_Test_Case { } public function photo_rename_wont_accept_slash_test() { - $item = test::random_photo(); + $item = test::random_photo_unsaved(); + $item->name = "/no_slashes/allowed/"; + // Should fail on validate. + try { + $item->validate(); + $this->assert_true(false, "Shouldn't get here"); + } catch (ORM_Validation_Exception $e) { + $errors = $e->validation->errors(); + $this->assert_same("no_slashes", $errors["name"]); + } + // Should be corrected on save. + $item->save(); + $this->assert_equal("no_slashes_allowed.jpg", $item->name); + // Should be corrected on update. $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 = test::random_photo_unsaved(); + $item->name = "\\no_backslashes\\allowed\\"; + // Should fail on validate. + try { + $item->validate(); + $this->assert_true(false, "Shouldn't get here"); + } catch (ORM_Validation_Exception $e) { + $errors = $e->validation->errors(); + $this->assert_same("no_backslashes", $errors["name"]); + } + // Should be corrected on save. + $item->save(); + $this->assert_equal("no_backslashes_allowed.jpg", $item->name); + // Should be corrected on update. $item->name = "\\no_backslashes\\allowed\\"; $item->save(); $this->assert_equal("no_backslashes_allowed.jpg", $item->name); } + public function photo_rename_wont_accept_trailing_period_test() { + $item = test::random_photo_unsaved(); + $item->name = "no_trailing_period_allowed."; + // Should fail on validate. + try { + $item->validate(); + $this->assert_true(false, "Shouldn't get here"); + } catch (ORM_Validation_Exception $e) { + $errors = $e->validation->errors(); + $this->assert_same("no_trailing_period", $errors["name"]); + } + // Should be corrected on save. + $item->save(); + $this->assert_equal("no_trailing_period_allowed.jpg", $item->name); + // Should be corrected on update. + $item->name = "no_trailing_period_allowed."; + $item->save(); + $this->assert_equal("no_trailing_period_allowed.jpg", $item->name); + } + public function album_rename_wont_accept_slash_test() { + $item = test::random_album_unsaved(); + $item->name = "/no_album_slashes/allowed/"; + // Should fail on validate. try { - $item = test::random_album(); - $item->name = "/no_album_slashes/allowed/"; - $item->save(); + $item->validate(); + $this->assert_true(false, "Shouldn't get here"); } catch (ORM_Validation_Exception $e) { - $this->assert_same(array("name" => "no_slashes"), $e->validation->errors()); - return; // pass + $errors = $e->validation->errors(); + $this->assert_same("no_slashes", $errors["name"]); } - $this->assert_true(false, "Shouldn't get here"); + // Should be corrected on save. + $item->save(); + $this->assert_equal("no_album_slashes_allowed", $item->name); + // Should be corrected on update. + $item->name = "/no_album_slashes/allowed/"; + $item->save(); + $this->assert_equal("no_album_slashes_allowed", $item->name); } public function album_rename_wont_accept_backslash_test() { + $item = test::random_album_unsaved(); + $item->name = "\\no_album_backslashes\\allowed\\"; + // Should fail on validate. try { - $item = test::random_album(); - $item->name = "\\no_album_backslashes\\allowed\\"; - $item->save(); + $item->validate(); + $this->assert_true(false, "Shouldn't get here"); } catch (ORM_Validation_Exception $e) { - $this->assert_same(array("name" => "no_backslashes"), $e->validation->errors()); - return; // pass + $errors = $e->validation->errors(); + $this->assert_same("no_backslashes", $errors["name"]); } - $this->assert_true(false, "Shouldn't get here"); + // Should be corrected on save. + $item->save(); + $this->assert_equal("no_album_backslashes_allowed", $item->name); + // Should be corrected on update. + $item->name = "\\no_album_backslashes\\allowed\\"; + $item->save(); + $this->assert_equal("no_album_backslashes_allowed", $item->name); } public function album_rename_wont_accept_trailing_period_test() { + $item = test::random_album_unsaved(); + $item->name = ".no_trailing_period.allowed."; + // Should fail on validate. try { - $item = test::random_album(); - $item->name = ".no_trailing_period.allowed."; - $item->save(); + $item->validate(); + $this->assert_true(false, "Shouldn't get here"); } catch (ORM_Validation_Exception $e) { - $this->assert_same(array("name" => "no_trailing_period"), $e->validation->errors()); - return; // pass + $errors = $e->validation->errors(); + $this->assert_same("no_trailing_period", $errors["name"]); } - $this->assert_true(false, "Shouldn't get here"); + // Should be corrected on save. + $item->save(); + $this->assert_equal(".no_trailing_period.allowed", $item->name); + // Should be corrected on update. + $item->name = ".no_trailing_period.allowed."; + $item->save(); + $this->assert_equal(".no_trailing_period.allowed", $item->name); } public function move_album_test() { diff --git a/modules/gallery/tests/Legal_File_Helper_Test.php b/modules/gallery/tests/Legal_File_Helper_Test.php index 3f520131..aab41c41 100644 --- a/modules/gallery/tests/Legal_File_Helper_Test.php +++ b/modules/gallery/tests/Legal_File_Helper_Test.php @@ -194,4 +194,22 @@ class Legal_File_Helper_Test extends Gallery_Unit_Test_Case { } } } + + public function sanitize_dirname_with_no_rename_test() { + $this->assert_equal("foo", legal_file::sanitize_dirname("foo")); + $this->assert_equal("foo.bar", legal_file::sanitize_dirname("foo.bar")); + $this->assert_equal(".foo.bar...baz", legal_file::sanitize_dirname(".foo.bar...baz")); + $this->assert_equal("foo bar spaces", legal_file::sanitize_dirname("foo bar spaces")); + $this->assert_equal("j'écris@un#nom_bizarre(mais quand_même_ça_passe \$ÇÀ@€", + legal_file::sanitize_dirname("j'écris@un#nom_bizarre(mais quand_même_ça_passe \$ÇÀ@€")); + } + + public function sanitize_filename_with_corrections_test() { + $this->assert_equal("foo_bar", legal_file::sanitize_dirname("/foo/bar/")); + $this->assert_equal("foo_bar", legal_file::sanitize_dirname("\\foo\\bar\\")); + $this->assert_equal(".foo..bar", legal_file::sanitize_dirname(".foo..bar.")); + $this->assert_equal("foo_bar", legal_file::sanitize_dirname("_foo__bar_")); + $this->assert_equal("album", legal_file::sanitize_dirname("_")); + $this->assert_equal("album", legal_file::sanitize_dirname(null)); + } } \ No newline at end of file -- cgit v1.2.3