From 0dc184e99f0ca607774a68257432a9a981f4d5b7 Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
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

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 @@
+<?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/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");
-- 
cgit v1.2.3