Merge branch 'master' into bharat_dev

12 files changed, 81 insertions, 27 deletions

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/helpers/access.php b/modules/gallery/helpers/access.php
index 8ce7e436..e0a0e979 100644
--- a/modules/gallery/helpers/access.php
+++ b/modules/gallery/helpers/access.php
@@ -66,8 +66,8 @@
  *   the Access_Intent_Model
  */
 class access_Core {
-  const DENY      = false;
-  const ALLOW     = true;
+  const DENY      = "0";
+  const ALLOW     = "1";
   const INHERIT   = null; // access_intent
   const UNKNOWN   = null; // cache (access_cache, items)
 
diff --git a/modules/gallery/helpers/model_cache.php b/modules/gallery/helpers/model_cache.php
index 302e42d9..88756407 100644
--- a/modules/gallery/helpers/model_cache.php
+++ b/modules/gallery/helpers/model_cache.php
@@ -18,27 +18,25 @@
  * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA  02110-1301, USA.
  */
 class model_cache_Core {
-  private static $cache;
+  private static $cache = array();
 
   static function get($model_name, $id, $field_name="id") {
-    if (TEST_MODE || empty(self::$cache->$model_name->$field_name->$id)) {
+    if (TEST_MODE || empty(self::$cache[$model_name][$field_name][$id])) {
       $model = ORM::factory($model_name)->where($field_name, "=", $id)->find();
       if (!$model->loaded()) {
         throw new Exception("@todo MISSING_MODEL $model_name:$id");
       }
-      self::$cache->$model_name->$field_name->$id = $model;
+      self::$cache[$model_name][$field_name][$id] = $model;
     }
 
-    return self::$cache->$model_name->$field_name->$id;
+    return self::$cache[$model_name][$field_name][$id];
   }
 
   static function clear() {
-    self::$cache = new stdClass();
+    self::$cache = array();
   }
 
   static function set($model) {
-    self::$cache->{$model->object_name}
-      ->{$model->primary_key}
-      ->{$model->{$model->primary_key}} = $model;
+    self::$cache[$model->object_name][$model->primary_key][$model->{$model->primary_key}] = $model;
   }
 }
diff --git a/modules/gallery/tests/Access_Helper_Test.php b/modules/gallery/tests/Access_Helper_Test.php
index ac41743a..da72f12f 100644
--- a/modules/gallery/tests/Access_Helper_Test.php
+++ b/modules/gallery/tests/Access_Helper_Test.php
@@ -72,6 +72,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", "*****", "user@user.com");
     foreach ($user->groups() as $group) {
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);
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);
   }
 }
diff --git a/modules/gallery/tests/Gallery_Rest_Helper_Test.php b/modules/gallery/tests/Gallery_Rest_Helper_Test.php
index c5c8a890..dac221b3 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));
diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php
index c1df70c6..b41740d6 100644
--- a/modules/gallery/tests/Item_Model_Test.php
+++ b/modules/gallery/tests/Item_Model_Test.php
@@ -45,7 +45,7 @@ class Item_Model_Test extends Unit_Test_Case {
   public function updating_view_count_only_doesnt_change_updated_date_test() {
     $item = test::random_photo();
     $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()
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 @@
+<?php defined("SYSPATH") or die("No direct script access.");
+/**
+ * Gallery - a web based photo album viewer and editor
+ * Copyright (C) 2000-2009 Bharat Mediratta
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+class Url_Security_Test extends Unit_Test_Case {
+  public function setup() {
+    $this->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/<xss>/bar";
+    Router::$complete_uri = "foo/<xss>/bar?foo=bar";
+    $this->assert_same("foo/&lt;xss&gt;/bar", url::current());
+    $this->assert_same("foo/&lt;xss&gt;/bar?foo=bar", url::current(true));
+  }
+
+  public function xss_in_merged_url_test() {
+    Router::$current_uri = "foo/<xss>/bar";
+    Router::$complete_uri = "foo/<xss>/bar?foo=bar";
+    $_GET = array("foo" => "bar");
+    $this->assert_same("foo/&lt;xss&gt;/bar?foo=bar", url::merge(array()));
+    $this->assert_same("foo/&lt;xss&gt;/bar?foo=bar&amp;a=b", url::merge(array("a" => "b")));
+  }
+}
\ No newline at end of file
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"')
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("&", "&amp;", url::merge($_GET)));
+    $feed->uri = url::abs_site(url::merge($_GET));
     if ($page > 1) {
-      $feed->previous_page_uri =
-        url::abs_site(str_replace("&", "&amp;", 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("&", "&amp;", 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");
diff --git a/modules/tag/tests/Tag_Rest_Helper_Test.php b/modules/tag/tests/Tag_Rest_Helper_Test.php
index 185953ab..555539fd 100644
--- a/modules/tag/tests/Tag_Rest_Helper_Test.php
+++ b/modules/tag/tests/Tag_Rest_Helper_Test.php
@@ -85,8 +85,8 @@ class Tag_Rest_Helper_Test extends Unit_Test_Case {
 
     $this->assert_equal(
       json_encode(array("status" => "OK",
-                        "tags" => array(array("name" => "albums", "count" => 2),
-                                        array("name" => "photos", "count" => 2)))),
+                        "tags" => array(array("name" => "albums", "count" => "2"),
+                                        array("name" => "photos", "count" => "2")))),
       tag_rest::get($request));
   }