From a11bf295078656612603c1c561e9261555d0c40c Mon Sep 17 00:00:00 2001 From: Tim Almdal Date: Sat, 9 Jan 2010 23:57:16 -0800 Subject: Fix for ticket #972 and more. In Kohana 2.4, ORM::delete_all ignores any where clauses and deletes all the entries in the table unless an array of id's are passed as the parameter. This fix used the Database_builder to specify any where conditions. Thanks psvo for find the first one. :-) --- modules/gallery/tests/Gallery_I18n_Test.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Gallery_I18n_Test.php b/modules/gallery/tests/Gallery_I18n_Test.php index 895e3051..5d2fd994 100644 --- a/modules/gallery/tests/Gallery_I18n_Test.php +++ b/modules/gallery/tests/Gallery_I18n_Test.php @@ -28,9 +28,10 @@ class Gallery_I18n_Test extends Unit_Test_Case { 'locale_dir' => VARPATH . 'locale/'); $this->i18n = Gallery_I18n::instance($config); - ORM::factory("incoming_translation") + db::build() + ->delete("incoming_translations") ->where("locale", "=", "te_ST") - ->delete_all(); + ->execute(); $messages_te_ST = array( array('Hello world', 'Hallo Welt'), -- cgit v1.2.3 From daeaca110d16128040c86727c65df225e957f7c6 Mon Sep 17 00:00:00 2001 From: Tim Almdal Date: Wed, 13 Jan 2010 11:27:09 -0800 Subject: Fix for ticket #978. Don't reset the original property as part of the save processing, because that will overwrite the original values with all the new values. The problem with the original approach is that when changed event handlers used ->original(), it had already been reset as part of the save processing. Went back and forth on either leaving this alone and forcing callers to save the original prior to calling the save function, but there were a few event handlers that used ->original(). This seemed the easier change. So to reset the original you need to call reload() or clear(). There is now an optional parameter on the reload to only reload the original. --- modules/gallery/libraries/MY_ORM.php | 20 ++++++++++++++++++-- modules/gallery/tests/Item_Model_Test.php | 1 + 2 files changed, 19 insertions(+), 2 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/libraries/MY_ORM.php b/modules/gallery/libraries/MY_ORM.php index 56c776aa..95fa9006 100644 --- a/modules/gallery/libraries/MY_ORM.php +++ b/modules/gallery/libraries/MY_ORM.php @@ -19,12 +19,11 @@ */ class ORM extends ORM_Core { // Track the original value of this ORM so that we can look it up in ORM::original() - protected $original = null; + protected $original; public function save() { model_cache::clear(); $result = parent::save(); - $this->original = clone $this; return $result; } @@ -48,7 +47,24 @@ class ORM extends ORM_Core { return parent::__unset($column); } + public function reload($original_only=false) { + if (!$original_only) { + parent::reload(); + } + $this->original = clone $this; + return $this; + } + + public function clear() { + parent::clear(); + $this->original = clone $this; + return $this; + } + public function original() { + if (!isset($this->original)) { + $this->original = clone $this; + } return $this->original; } } diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index bf5fca1a..c0ac4436 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -166,6 +166,7 @@ class Item_Model_Test extends Unit_Test_Case { $item = self::_create_random_item(); $item->title = "ORIGINAL_VALUE"; $item->save(); + $item->reload(false); $item->title = "NEW_VALUE"; $this->assert_same("ORIGINAL_VALUE", $item->original()->title); -- cgit v1.2.3 From 00c73a4b072d2ad4e45323234b847da4f69d99a3 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 15 Jan 2010 23:53:43 -0800 Subject: Revert "Fix for ticket #978. Don't reset the original property as part of the save processing, because that will overwrite the original values with all the new values. The problem with the original approach is that when changed event handlers used ->original(), it had already been reset as part of the save processing. Went back and forth on either leaving this alone and forcing callers to save the original prior to calling the save function, but there were a few event handlers that used ->original(). This seemed the easier change. So to reset the original you need to call reload() or clear(). There is now an optional parameter on the reload to only reload the original." This reverts commit daeaca110d16128040c86727c65df225e957f7c6. --- modules/gallery/libraries/MY_ORM.php | 20 ++------------------ modules/gallery/tests/Item_Model_Test.php | 1 - 2 files changed, 2 insertions(+), 19 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/libraries/MY_ORM.php b/modules/gallery/libraries/MY_ORM.php index 95fa9006..56c776aa 100644 --- a/modules/gallery/libraries/MY_ORM.php +++ b/modules/gallery/libraries/MY_ORM.php @@ -19,11 +19,12 @@ */ class ORM extends ORM_Core { // Track the original value of this ORM so that we can look it up in ORM::original() - protected $original; + protected $original = null; public function save() { model_cache::clear(); $result = parent::save(); + $this->original = clone $this; return $result; } @@ -47,24 +48,7 @@ class ORM extends ORM_Core { return parent::__unset($column); } - public function reload($original_only=false) { - if (!$original_only) { - parent::reload(); - } - $this->original = clone $this; - return $this; - } - - public function clear() { - parent::clear(); - $this->original = clone $this; - return $this; - } - public function original() { - if (!isset($this->original)) { - $this->original = clone $this; - } return $this->original; } } diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index c0ac4436..bf5fca1a 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -166,7 +166,6 @@ class Item_Model_Test extends Unit_Test_Case { $item = self::_create_random_item(); $item->title = "ORIGINAL_VALUE"; $item->save(); - $item->reload(false); $item->title = "NEW_VALUE"; $this->assert_same("ORIGINAL_VALUE", $item->original()->title); -- cgit v1.2.3 From 0dc184e99f0ca607774a68257432a9a981f4d5b7 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 11:10:37 -0800 Subject: Overload url::current() and url::merge() to make the current url XSS safe. Add tests to make sure that it doesn't relapse with future Kohana changes. Fixes ticket #983. Ref: http://gallery.menalto.com/node/93738 --- modules/gallery/helpers/MY_url.php | 14 ++++++++++ modules/gallery/tests/Url_Security_Test.php | 43 +++++++++++++++++++++++++++++ modules/rss/controllers/rss.php | 8 ++---- 3 files changed, 60 insertions(+), 5 deletions(-) create mode 100644 modules/gallery/tests/Url_Security_Test.php (limited to 'modules/gallery/tests') diff --git a/modules/gallery/helpers/MY_url.php b/modules/gallery/helpers/MY_url.php index 74284951..8a7909b6 100644 --- a/modules/gallery/helpers/MY_url.php +++ b/modules/gallery/helpers/MY_url.php @@ -89,4 +89,18 @@ class url extends url_Core { static function abs_current($qs=false) { return self::abs_site(url::current($qs)); } + + /** + * Just like url::merge except that it escapes any XSS in the path. + */ + static function merge($params) { + return htmlspecialchars(parent::merge($params)); + } + + /** + * Just like url::current except that it escapes any XSS in the path. + */ + static function current($qs=false, $suffix=false) { + return htmlspecialchars(parent::current($qs, $suffix)); + } } diff --git a/modules/gallery/tests/Url_Security_Test.php b/modules/gallery/tests/Url_Security_Test.php new file mode 100644 index 00000000..de25880f --- /dev/null +++ b/modules/gallery/tests/Url_Security_Test.php @@ -0,0 +1,43 @@ +save = array(Router::$current_uri, Router::$complete_uri, $_GET); + } + + public function teardown() { + list(Router::$current_uri, Router::$complete_uri, $_GET) = $this->save; + } + + public function xss_in_current_url_test() { + Router::$current_uri = "foo//bar"; + Router::$complete_uri = "foo//bar?foo=bar"; + $this->assert_same("foo/<xss>/bar", url::current()); + $this->assert_same("foo/<xss>/bar?foo=bar", url::current(true)); + } + + public function xss_in_merged_url_test() { + Router::$current_uri = "foo//bar"; + Router::$complete_uri = "foo//bar?foo=bar"; + $_GET = array("foo" => "bar"); + $this->assert_same("foo/<xss>/bar?foo=bar", url::merge(array())); + $this->assert_same("foo/<xss>/bar?foo=bar&a=b", url::merge(array("a" => "b"))); + } +} \ No newline at end of file diff --git a/modules/rss/controllers/rss.php b/modules/rss/controllers/rss.php index 41c781d9..3066ba16 100644 --- a/modules/rss/controllers/rss.php +++ b/modules/rss/controllers/rss.php @@ -52,14 +52,12 @@ class Rss_Controller extends Controller { $view->feed = $feed; $view->pub_date = date("D, d M Y H:i:s T"); - $feed->uri = url::abs_site(str_replace("&", "&", url::merge($_GET))); + $feed->uri = url::abs_site(url::merge($_GET)); if ($page > 1) { - $feed->previous_page_uri = - url::abs_site(str_replace("&", "&", url::merge(array("page" => $page - 1)))); + $feed->previous_page_uri = url::abs_site(url::merge(array("page" => $page - 1))); } if ($page < $feed->max_pages) { - $feed->next_page_uri = - url::abs_site(str_replace("&", "&", url::merge(array("page" => $page + 1)))); + $feed->next_page_uri = url::abs_site(url::merge(array("page" => $page + 1))); } header("Content-Type: application/rss+xml"); -- cgit v1.2.3 From 4eafe97b4858cb8d1b3367574f7e7ef473127d7c Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 11:57:23 -0800 Subject: Reload $item after removing view permissions. --- modules/gallery/tests/Access_Helper_Test.php | 1 + 1 file changed, 1 insertion(+) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Access_Helper_Test.php b/modules/gallery/tests/Access_Helper_Test.php index 084bfb47..b2244766 100644 --- a/modules/gallery/tests/Access_Helper_Test.php +++ b/modules/gallery/tests/Access_Helper_Test.php @@ -74,6 +74,7 @@ class Access_Helper_Test extends Unit_Test_Case { access::deny(identity::everybody(), "view", $item); access::deny(identity::registered_users(), "view", $item); + $item->reload(); $user = identity::create_user("access_test", "Access Test", ""); foreach ($user->groups() as $group) { -- cgit v1.2.3 From 51427d540464ffa5b4663a9c3b794cefc637925a Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:21:57 -0800 Subject: Verified --- modules/gallery/tests/xss_data.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/xss_data.txt b/modules/gallery/tests/xss_data.txt index a264286c..1530c73e 100644 --- a/modules/gallery/tests/xss_data.txt +++ b/modules/gallery/tests/xss_data.txt @@ -137,7 +137,7 @@ modules/gallery/views/l10n_client.html.php 26 DIRTY $strin modules/gallery/views/l10n_client.html.php 32 DIRTY $l10n_search_form modules/gallery/views/l10n_client.html.php 41 DIRTY access::csrf_form_field() modules/gallery/views/l10n_client.html.php 42 DIRTY form::hidden("l10n-message-key") -modules/gallery/views/l10n_client.html.php 43 DIRTY form::textarea("l10n-edit-translation","",' rows="5" class="translationField"') +modules/gallery/views/l10n_client.html.php 43 DIRTY form::textarea("l10n-edit-translation","",' id="l10n-edit-translation" rows="5" class="translationField"') modules/gallery/views/l10n_client.html.php 46 DIRTY form::textarea("l10n-edit-plural-translation-zero","",' rows="2"') modules/gallery/views/l10n_client.html.php 50 DIRTY form::textarea("l10n-edit-plural-translation-one","",' rows="2"') modules/gallery/views/l10n_client.html.php 54 DIRTY form::textarea("l10n-edit-plural-translation-two","",' rows="2"') -- cgit v1.2.3 From cac4692510d6b5da0d0a63f2ec54783df6ca86e4 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:35:26 -0800 Subject: Don't use rand() as the name. Now that ORM::load_types() is gone, it won't get coerced to a string, and then we wind up comparing: 12345 != 12345-12321 In the old approach, they'd both be strings so they'd be inequal. But in the new approach the first value is an integer (sinced it came from rand()) so the second value is typecast to an integer which drops everything after the - sign so they appear equal. --- modules/gallery/tests/Album_Helper_Test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Album_Helper_Test.php b/modules/gallery/tests/Album_Helper_Test.php index 1284b8cc..ef0905da 100644 --- a/modules/gallery/tests/Album_Helper_Test.php +++ b/modules/gallery/tests/Album_Helper_Test.php @@ -38,7 +38,7 @@ class Album_Helper_Test extends Unit_Test_Case { } public function create_conflicting_album_test() { - $rand = rand(); + $rand = "name_" . rand(); $root = ORM::factory("item", 1); $album1 = album::create($root, $rand, $rand, $rand); $album2 = album::create($root, $rand, $rand, $rand); -- cgit v1.2.3 From cf236a228a8ea3316506f6d855bcade92676674c Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:37:20 -0800 Subject: Don't assert_same() now that typecasting is gone from ORM. --- modules/gallery/tests/Item_Model_Test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index bf5fca1a..d03a03f4 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -52,7 +52,7 @@ class Item_Model_Test extends Unit_Test_Case { public function updating_view_count_only_doesnt_change_updated_date_test() { $item = self::_create_random_item(); $item->reload(); - $this->assert_same(0, $item->view_count); + $this->assert_equal(0, $item->view_count); // Force the updated date to something well known db::build() -- cgit v1.2.3 From a9f07986f64faf5062c2d86aa34710892d8ac8b3 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:39:42 -0800 Subject: The root parent id is 0, not null (this deviation exposed by the new lack of typecasting in ORM). --- modules/gallery/tests/Gallery_Installer_Test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Gallery_Installer_Test.php b/modules/gallery/tests/Gallery_Installer_Test.php index 43399fb4..74a07b1a 100644 --- a/modules/gallery/tests/Gallery_Installer_Test.php +++ b/modules/gallery/tests/Gallery_Installer_Test.php @@ -41,7 +41,7 @@ class Gallery_Installer_Test extends Unit_Test_Case { $this->assert_equal("Gallery", $root->title); $this->assert_equal(1, $root->left_ptr); $this->assert_equal($max_right_ptr, $root->right_ptr); - $this->assert_equal(null, $root->parent_id); + $this->assert_equal(0, $root->parent_id); $this->assert_equal(1, $root->level); } } -- cgit v1.2.3 From 06cabecd76781d7075cbacb5cf433042b4296dc5 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:49:58 -0800 Subject: Coerce some integers to strings now that ORM isn't typecasting anymore. --- modules/gallery/tests/Gallery_Rest_Helper_Test.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'modules/gallery/tests') diff --git a/modules/gallery/tests/Gallery_Rest_Helper_Test.php b/modules/gallery/tests/Gallery_Rest_Helper_Test.php index cd0aabae..f8cf6190 100644 --- a/modules/gallery/tests/Gallery_Rest_Helper_Test.php +++ b/modules/gallery/tests/Gallery_Rest_Helper_Test.php @@ -94,8 +94,8 @@ class Gallery_Rest_Helper_Test extends Unit_Test_Case { "path" => $photo->relative_url(), "thumb_url" => $photo->thumb_url(), "thumb_dimensions" => array( - "width" => $photo->thumb_width, - "height" => $photo->thumb_height), + "width" => (string)$photo->thumb_width, + "height" => (string)$photo->thumb_height), "has_thumb" => true, "title" => $photo->title))))), gallery_rest::get($request)); @@ -115,14 +115,14 @@ class Gallery_Rest_Helper_Test extends Unit_Test_Case { "parent_path" => $child->relative_url(), "title" => $photo->title, "thumb_url" => $photo->thumb_url(), - "thumb_size" => array("height" => $photo->thumb_height, - "width" => $photo->thumb_width), + "thumb_size" => array("height" => (string)$photo->thumb_height, + "width" => (string)$photo->thumb_width), "resize_url" => $photo->resize_url(), "resize_size" => array("height" => $photo->resize_height, "width" => $photo->resize_width), "url" => $photo->file_url(), - "size" => array("height" => $photo->height, - "width" => $photo->width), + "size" => array("height" => (string)$photo->height, + "width" => (string)$photo->width), "description" => $photo->description, "slug" => $photo->slug))), gallery_rest::get($request)); -- cgit v1.2.3