From 5b3b675b6d8a1cd9a5f2b9455c551791e18d88ff Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Thu, 16 Jul 2009 11:19:34 -0700 Subject: Non-trivial changes to the event handling code: 1) The item_updated event no longer takes the old and new items. Instead we overload ORM to track the original data and make that available via the item. This will allow us to move event publishing down into the API methods which in turn will give us more stability since we won't require each controller to remember to do it. 2) ORM class now tracks the original values. It doesn't track the original relationships (no need for that, yet) 3) Added new events: item_deleted group_deleted user_deleted --- modules/gallery/libraries/MY_ORM.php | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) (limited to 'modules/gallery/libraries/MY_ORM.php') diff --git a/modules/gallery/libraries/MY_ORM.php b/modules/gallery/libraries/MY_ORM.php index 2bd9b4eb..319cbe09 100644 --- a/modules/gallery/libraries/MY_ORM.php +++ b/modules/gallery/libraries/MY_ORM.php @@ -18,6 +18,9 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class ORM extends ORM_Core { + // Track the original value of this ORM instance so that we can look it up in ORM::original() + protected $original = null; + public function open_paren() { $this->db->open_paren(); return $this; @@ -30,7 +33,29 @@ class ORM extends ORM_Core { public function save() { model_cache::clear($this->object_name, $this->{$this->primary_key}, $this->primary_key); - return parent::save(); + $result = parent::save(); + $this->original = $this->object; + return $result; + } + + public function __set($column, $value) { + if (!isset($this->original)) { + $this->original = $this->object; + } + + return parent::__set($column, $value); + } + + public function __unset($column) { + if (!isset($this->original)) { + $this->original = $this->object; + } + + return parent::__unset($column); + } + + public function original($column) { + return $this->original[$column]; } } -- cgit v1.2.3 From cd907c2b42f8b50ebe6d490aab42365e16deb258 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Fri, 17 Jul 2009 12:51:27 -0700 Subject: Change model_cache::clear() API to clear everything. This prevents old ORM relationships from hanging around, which was causing problems when doing MPTT manipulations (resulting in incorrect permission propagation-- very bad!) --- modules/gallery/helpers/access.php | 4 ++++ modules/gallery/helpers/item.php | 4 ++-- modules/gallery/helpers/model_cache.php | 6 ++---- modules/gallery/libraries/MY_ORM.php | 2 +- modules/gallery/libraries/ORM_MPTT.php | 1 + modules/gallery/models/item.php | 1 + 6 files changed, 11 insertions(+), 7 deletions(-) (limited to 'modules/gallery/libraries/MY_ORM.php') diff --git a/modules/gallery/helpers/access.php b/modules/gallery/helpers/access.php index 5dd1e465..63324e5d 100644 --- a/modules/gallery/helpers/access.php +++ b/modules/gallery/helpers/access.php @@ -205,6 +205,7 @@ class access_Core { } self::_update_htaccess_files($album, $group, $perm_name, $value); + model_cache::clear(); } /** @@ -256,6 +257,7 @@ class access_Core { } } } + model_cache::clear(); } /** @@ -426,6 +428,7 @@ class access_Core { $cache_table = $perm_name == "view" ? "items" : "access_caches"; $db->query("ALTER TABLE {{$cache_table}} DROP `$field`"); $db->query("ALTER TABLE {access_intents} DROP `$field`"); + model_cache::clear(); ORM::factory("access_intent")->clear_cache(); } @@ -443,6 +446,7 @@ class access_Core { $db->query("ALTER TABLE {{$cache_table}} ADD `$field` SMALLINT NOT NULL DEFAULT 0"); $db->query("ALTER TABLE {access_intents} ADD `$field` BOOLEAN DEFAULT NULL"); $db->update("access_intents", array($field => 0), array("item_id" => 1)); + model_cache::clear(); ORM::factory("access_intent")->clear_cache(); } diff --git a/modules/gallery/helpers/item.php b/modules/gallery/helpers/item.php index f40b5c97..80c25862 100644 --- a/modules/gallery/helpers/item.php +++ b/modules/gallery/helpers/item.php @@ -53,7 +53,7 @@ class item_Core { access::required("view", $parent); access::required("edit", $parent); - model_cache::clear("item", $parent->album_cover_item_id); + model_cache::clear(); $parent->album_cover_item_id = $item->is_album() ? $item->album_cover_item_id : $item->id; $parent->thumb_dirty = 1; $parent->save(); @@ -69,7 +69,7 @@ class item_Core { access::required("edit", $album); @unlink($album->thumb_path()); - model_cache::clear("item", $album->album_cover_item_id) ; + model_cache::clear(); $album->album_cover_item_id = null; $album->thumb_width = 0; $album->thumb_height = 0; diff --git a/modules/gallery/helpers/model_cache.php b/modules/gallery/helpers/model_cache.php index 2649fdbd..a3e09862 100644 --- a/modules/gallery/helpers/model_cache.php +++ b/modules/gallery/helpers/model_cache.php @@ -32,10 +32,8 @@ class model_cache_Core { return self::$cache->$model_name->$field_name->$id; } - static function clear($model_name, $id, $field_name="id") { - if (!empty(self::$cache->$model_name->$field_name->$id)) { - unset(self::$cache->$model_name->$field_name->$id); - } + static function clear() { + self::$cache = new stdClass(); } static function set($model) { diff --git a/modules/gallery/libraries/MY_ORM.php b/modules/gallery/libraries/MY_ORM.php index 319cbe09..1d3c1ef3 100644 --- a/modules/gallery/libraries/MY_ORM.php +++ b/modules/gallery/libraries/MY_ORM.php @@ -32,7 +32,7 @@ class ORM extends ORM_Core { } public function save() { - model_cache::clear($this->object_name, $this->{$this->primary_key}, $this->primary_key); + model_cache::clear(); $result = parent::save(); $this->original = $this->object; return $result; diff --git a/modules/gallery/libraries/ORM_MPTT.php b/modules/gallery/libraries/ORM_MPTT.php index 46280d95..e371f159 100644 --- a/modules/gallery/libraries/ORM_MPTT.php +++ b/modules/gallery/libraries/ORM_MPTT.php @@ -285,6 +285,7 @@ class ORM_MPTT_Core extends ORM { // Lets reload to get the changes. $this->reload(); + $target->reload(); return $this; } diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index 6512e9e5..05c4e656 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -140,6 +140,7 @@ class Item_Model extends ORM_MPTT { $original_parent = $this->parent(); parent::move_to($target, true); + model_cache::clear(); $this->relative_path_cache = null; rename($original_path, $this->file_path()); -- cgit v1.2.3 From 7ad0808a117fd1db4e94da8d7763ccca1d69350a Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sun, 2 Aug 2009 12:09:00 -0700 Subject: Change the API for getting to the original state of an ORM. Old API: $obj->original("field_name") New API: $obj->original()->field_name This allows us to revert the varous xxx_updated events back to passing an original ORM as well as the the updated one. This makes for a cleaner event API. Old API: comment_updated($comment) { $comment->original("field_name") } Old API: comment_updated($old, $new) { $old->field_name } --- modules/akismet/helpers/akismet_event.php | 6 +++--- modules/comment/models/comment.php | 8 ++++---- modules/gallery/libraries/MY_ORM.php | 12 ++++++------ modules/gallery/models/item.php | 2 +- modules/gallery/tests/Item_Model_Test.php | 2 +- modules/notification/helpers/notification_event.php | 8 ++++---- modules/search/helpers/search_event.php | 4 ++-- modules/user/models/group.php | 2 +- modules/user/models/user.php | 2 +- 9 files changed, 23 insertions(+), 23 deletions(-) (limited to 'modules/gallery/libraries/MY_ORM.php') diff --git a/modules/akismet/helpers/akismet_event.php b/modules/akismet/helpers/akismet_event.php index d6cde222..cec6d95d 100644 --- a/modules/akismet/helpers/akismet_event.php +++ b/modules/akismet/helpers/akismet_event.php @@ -40,14 +40,14 @@ class akismet_event_Core { $comment->save(); } - static function comment_updated($comment) { + static function comment_updated($original, $new) { if (!module::get_var("akismet", "api_key")) { return; } - if ($comment->original("state") != "spam" && $comment->state == "spam") { + if ($original->state != "spam" && $new->state == "spam") { akismet::submit_spam($new); - } else if ($comment->original("state") == "spam" && $comment->state != "spam") { + } else if ($original->state == "spam" && $new->state != "spam") { akismet::submit_ham($new); } } diff --git a/modules/comment/models/comment.php b/modules/comment/models/comment.php index d052a39c..83d0888a 100644 --- a/modules/comment/models/comment.php +++ b/modules/comment/models/comment.php @@ -64,17 +64,17 @@ class Comment_Model extends ORM { $created = true; } } + $visible_change = $this->original()->state == "published" || $this->state == "published"; parent::save(); if (isset($created)) { module::event("comment_created", $this); } else { - module::event("comment_updated", $this); + module::event("comment_updated", $this->original(), $this); } - // We only notify on the related items if we're making a visible change, which means moving in - // or out of a published state - if ($this->original("state") == "published" || $this->state == "published") { + // We only notify on the related items if we're making a visible change. + if ($visible_change) { module::event("item_related_update", $this->item()); } diff --git a/modules/gallery/libraries/MY_ORM.php b/modules/gallery/libraries/MY_ORM.php index 1d3c1ef3..de8adc1d 100644 --- a/modules/gallery/libraries/MY_ORM.php +++ b/modules/gallery/libraries/MY_ORM.php @@ -18,7 +18,7 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class ORM extends ORM_Core { - // Track the original value of this ORM instance so that we can look it up in ORM::original() + // Track the original value of this ORM so that we can look it up in ORM::original() protected $original = null; public function open_paren() { @@ -34,13 +34,13 @@ class ORM extends ORM_Core { public function save() { model_cache::clear(); $result = parent::save(); - $this->original = $this->object; + $this->original = clone $this; return $result; } public function __set($column, $value) { if (!isset($this->original)) { - $this->original = $this->object; + $this->original = clone $this; } return parent::__set($column, $value); @@ -48,14 +48,14 @@ class ORM extends ORM_Core { public function __unset($column) { if (!isset($this->original)) { - $this->original = $this->object; + $this->original = clone $this; } return parent::__unset($column); } - public function original($column) { - return $this->original[$column]; + public function original() { + return $this->original; } } diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php index b3c7998b..f3e6b8f3 100644 --- a/modules/gallery/models/item.php +++ b/modules/gallery/models/item.php @@ -365,7 +365,7 @@ class Item_Model extends ORM_MPTT { } parent::save(); if (isset($send_event)) { - module::event("item_updated", $this); + module::event("item_updated", $this->original(), $this); } return $this; } diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index c2773097..0940d076 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -147,7 +147,7 @@ class Item_Model_Test extends Unit_Test_Case { $item->save(); $item->title = "NEW_VALUE"; - $this->assert_same("ORIGINAL_VALUE", $item->original("title")); + $this->assert_same("ORIGINAL_VALUE", $item->original()->title); $this->assert_same("NEW_VALUE", $item->title); } } diff --git a/modules/notification/helpers/notification_event.php b/modules/notification/helpers/notification_event.php index c6e770a7..d1b76e93 100644 --- a/modules/notification/helpers/notification_event.php +++ b/modules/notification/helpers/notification_event.php @@ -18,8 +18,8 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class notification_event_Core { - static function item_updated($item) { - notification::send_item_updated($item); + static function item_updated($original, $new) { + notification::send_item_updated($new); } static function item_created($item) { @@ -40,8 +40,8 @@ class notification_event_Core { } } - static function comment_updated($item) { - if ($item->state == "published" && $item->original("state") != "published") { + static function comment_updated($original, $new) { + if ($new->state == "published" && $original->state != "published") { notification::send_comment_published($new); } } diff --git a/modules/search/helpers/search_event.php b/modules/search/helpers/search_event.php index 764fdd18..b65763af 100644 --- a/modules/search/helpers/search_event.php +++ b/modules/search/helpers/search_event.php @@ -22,8 +22,8 @@ class search_event_Core { search::update($item); } - static function item_updated($item) { - search::update($item); + static function item_updated($original, $new) { + search::update($new); } static function item_deleted($item) { diff --git a/modules/user/models/group.php b/modules/user/models/group.php index bb3fb58b..8af78012 100644 --- a/modules/user/models/group.php +++ b/modules/user/models/group.php @@ -41,7 +41,7 @@ class Group_Model extends ORM { if (isset($created)) { module::event("group_created", $this); } else { - module::event("group_updated", $this); + module::event("group_updated", $this->original(), $this); } return $this; } diff --git a/modules/user/models/user.php b/modules/user/models/user.php index def65a6f..4b43adff 100644 --- a/modules/user/models/user.php +++ b/modules/user/models/user.php @@ -68,7 +68,7 @@ class User_Model extends ORM { if (isset($created)) { module::event("user_created", $this); } else { - module::event("user_updated", $this); + module::event("user_updated", $this->original(), $this); } return $this; } -- cgit v1.2.3 From 020281d932c566476222e6c825ada3affff239a6 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Sat, 29 Aug 2009 10:45:47 -0700 Subject: Adding SafeString which is going to replace p::clean() and p::purify(). Refactoring of Xss_Security_Test. t() and t2() return a SafeString instance. TODO: - Update all code to use SafeString where appropriate. - Update golden fole of Xss_Security_Test - Stop reporting CLEAN vars in Xss_Security_Test --- modules/gallery/helpers/p.php | 16 +- modules/gallery/libraries/I18n.php | 20 +- modules/gallery/libraries/MY_ORM.php | 4 + modules/gallery/libraries/SafeString.php | 142 ++++++++++++ modules/gallery/tests/SafeString_Test.php | 111 ++++++++++ modules/gallery/tests/Xss_Security_Test.php | 325 ++++++++++++++++++++++------ modules/user/views/login.html.php | 6 +- 7 files changed, 535 insertions(+), 89 deletions(-) create mode 100644 modules/gallery/libraries/SafeString.php create mode 100644 modules/gallery/tests/SafeString_Test.php (limited to 'modules/gallery/libraries/MY_ORM.php') diff --git a/modules/gallery/helpers/p.php b/modules/gallery/helpers/p.php index 862c769b..e852c086 100644 --- a/modules/gallery/helpers/p.php +++ b/modules/gallery/helpers/p.php @@ -18,22 +18,12 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class p_Core { - private static $_purifier = null; static function clean($dirty_html) { - return html::specialchars($dirty_html); + return new SafeString($dirty_html); } + // Deprecated: Please use p::clean($var).purified_html() static function purify($dirty_html) { - if (empty(self::$_purifier)) { - require_once(dirname(__file__) . "/../lib/HTMLPurifier/HTMLPurifier.auto.php"); - $config = HTMLPurifier_Config::createDefault(); - foreach (Kohana::config('purifier') as $category => $key_value) { - foreach ($key_value as $key => $value) { - $config->set("$category.$key", $value); - } - } - self::$_purifier = new HTMLPurifier($config); - } - return self::$_purifier->purify($dirty_html); + return SafeString::of($dirty_html)->purified_html(); } } diff --git a/modules/gallery/libraries/I18n.php b/modules/gallery/libraries/I18n.php index 03a6d8f6..8dc42e04 100644 --- a/modules/gallery/libraries/I18n.php +++ b/modules/gallery/libraries/I18n.php @@ -84,6 +84,12 @@ class I18n_Core { /** * Translates a localizable message. + * + * Security: + * The returned string is safe for use in HTML (it contains a safe subset of HTML and + * interpolation parameters are converted to HTML entities). + * For use in JavaScript, please call ->for_js() on it. + * * @param $message String|array The message to be translated. E.g. "Hello world" * or array("one" => "One album", "other" => "%count albums") * @param $options array (optional) Options array for key value pairs which are used @@ -110,7 +116,7 @@ class I18n_Core { $entry = $this->interpolate($locale, $entry, $values); - return $entry; + return SafeString::of($entry)->mark_html_safe(); } private function lookup($locale, $message) { @@ -179,17 +185,19 @@ class I18n_Core { return is_array($message); } - private function interpolate($locale, $string, $values) { + private function interpolate($locale, $string, $key_values) { // TODO: Handle locale specific number formatting. // Replace x_y before replacing x. - krsort($values, SORT_STRING); + krsort($key_values, SORT_STRING); $keys = array(); - foreach (array_keys($values) as $key) { + $values = array(); + foreach ($key_values as $key => $value) { $keys[] = "%$key"; + $values[] = new SafeString($value); } - return str_replace($keys, array_values($values), $string); + return str_replace($keys, $values, $string); } private function pluralize($locale, $entry, $count) { @@ -414,4 +422,4 @@ class I18n_Core { return $count == 1 ? 'one' : 'other'; } } -} \ No newline at end of file +} diff --git a/modules/gallery/libraries/MY_ORM.php b/modules/gallery/libraries/MY_ORM.php index de8adc1d..2c9ad1d7 100644 --- a/modules/gallery/libraries/MY_ORM.php +++ b/modules/gallery/libraries/MY_ORM.php @@ -43,6 +43,10 @@ class ORM extends ORM_Core { $this->original = clone $this; } + if ($value instanceof SafeString) { + $value = $value->unescaped(); + } + return parent::__set($column, $value); } diff --git a/modules/gallery/libraries/SafeString.php b/modules/gallery/libraries/SafeString.php new file mode 100644 index 00000000..53bcb27a --- /dev/null +++ b/modules/gallery/libraries/SafeString.php @@ -0,0 +1,142 @@ +_is_safe_html = $string->_is_safe_html; + $string = $string->unescaped(); + } + $this->_raw_string = (string) $string; + } + + /** + * Factory method returning a new SafeString instance for the given string. + */ + static function of($string) { + return new SafeString($string); + } + + /** + * Marks this string as safe to be used in HTML without any escaping. + */ + function mark_html_safe() { + $this->_is_safe_html = true; + return $this; + } + + /** + * Safe for use in HTML. + * @see #for_html() + */ + function __toString() { + if ($this->_is_safe_html) { + return $this->_raw_string; + } else { + return self::_escape_for_html($this->_raw_string); + } + } + + /** + * Safe for use in HTML. + * + * Example:
+   *   
 
+   *
+ * @return the string escaped for use in HTML. + */ + function for_html() { + return $this; + } + + /** + * Safe for use in JavaScript. + * + * Example:
+   *    block?
+	if (is_array($token) && $token[0] == T_INLINE_HTML) {
+	  $inline_html = $token[1];
+	  // T_INLINE_HTML blocks can be split. Need to handle the case
+	  // where one token has "expr_append($inline_html);
+	  }
+
+	  // Note: This approach won't catch }i', $inline_html, $matches, PREG_OFFSET_CAPTURE)) {
+	    $last_match = array_pop($matches[0]);
+	    if (is_array($last_match)) {
+	      $closing_script_pos = $last_match[1];
+	    } else {
+	      $closing_script_pos = $last_match;
+	    }
+	  }
+	  if (preg_match('{]*>}i', $inline_html, $matches, PREG_OFFSET_CAPTURE)) {
+	    $last_match = array_pop($matches[0]);
+	    if (is_array($last_match)) {
+	      $opening_script_pos = $last_match[1];
+	    } else {
+	      $opening_script_pos = $last_match;
+	    }
+	  }
+	  if ($opening_script_pos != $closing_script_pos) {
+	    $in_script_block = $opening_script_pos > $closing_script_pos;
+	  }
+	}
+
+	// Look and report each instance of < ? = ... ? >
+	if (!is_array($token)) {
+	  // A single char token, e.g: ; ( )
+	  if ($frame) {
+	    $frame->expr_append($token);
+	  }
+	} else if ($token[0] == T_OPEN_TAG_WITH_ECHO) {
+	  // No need for a stack here - assume < ? = cannot be nested.
+	  $frame = self::_create_frame($token, $in_script_block);
+        } else if ($frame && $token[0] == T_CLOSE_TAG) {
+	  // Store the < ? = ... ? > block that just ended here.
+	  $found[$view][] = $frame;
+	  $frame = null;
+        } else if ($frame && $token[0] == T_VARIABLE) {
+	  $frame->expr_append($token[1]);
+	} else if ($frame && $token[0] == T_STRING) {
+	  $frame->expr_append($token[1]);
+	  // t() and t2() are special in that they're guaranteed to return a SafeString().
+	  if (in_array($token[1], array("t", "t2"))) {
+	    if (self::_token_matches("(", $tokens, $token_number + 1)) {
+	      $frame->is_safestring(true);
+	      $frame->expr_append("(");
+
+	      $token_number++;
+	      $token = $tokens[$token_number];
+	    }
+	  } else if ($token[1] == "SafeString") {
+	    // Looking for SafeString::of(...
+	    if (self::_token_matches(array(T_DOUBLE_COLON, "::"), $tokens, $token_number + 1) &&
+		self::_token_matches(array(T_STRING, "of"), $tokens, $token_number + 2)	&&
+		self::_token_matches("(", $tokens, $token_number + 3)) {
+	      $frame->is_safestring(true);
+	      $frame->expr_append("::of(");
+
+	      $token_number += 3;
+	      $token = $tokens[$token_number];
+	    }
+	  } else if ($token[1] == "json_encode") {
+	    if (self::_token_matches("(", $tokens, $token_number + 1)) {
+	      $frame->json_encode_called(true);
+	      $frame->expr_append("(");
+
+	      $token_number++;
+	      $token = $tokens[$token_number];
+	    }
+	  }
+	} else if ($frame && $token[0] == T_OBJECT_OPERATOR) {
+	  $frame->expr_append($token[1]);
+
+	  if (self::_token_matches(array(T_STRING), $tokens, $token_number + 1) &&
+	      in_array($tokens[$token_number + 1][1],
+		       array("for_js", "for_html", "purified_html")) &&
+	      self::_token_matches("(", $tokens, $token_number + 2)) {
+
+	    $method = $tokens[$token_number + 1][1];
+	    $frame->expr_append("$method(");
+
+	    $token_number += 2;
+	    $token = $tokens[$token_number];
+
+	    if ("for_js" == $method) {
+	      $frame->for_js_called(true);
+	    } else if ("for_html" == $method) {
+	      $frame->for_html_called(true);
+	    } else if ("purified_html" == $method) {
+	      $frame->purified_html_called(true);
+	    }
+	  }
+        } else if ($frame) {
+	  $frame->expr_append($token[1]);
+	}
       }
     }
 
-    $canonical = MODPATH . "gallery/tests/xss_data.txt";
+    // Generate the report.
+    /*
+     * States for uses of < ? = X ? >:
+     * JS_XSS:
+     *   In