From af021cb4b230708938be04781bd191872f13bfa3 Mon Sep 17 00:00:00 2001 From: mark_story <mark@mark-story.com> Date: Wed, 25 Feb 2009 19:51:41 +0000 Subject: [PATCH] Adding Html entity conversion to all urls generated by helpers, fixing potential for merged passedArgs to create xss vectors. Adding integer cast in paginate() to page param. Tests added/updated. Fixes #6134 git-svn-id: https://svn.cakephp.org/repo/branches/1.2.x.x@8061 3807eeeb-6ff5-0310-8944-8be069107fe0 --- cake/libs/controller/controller.php | 1 + cake/libs/view/helper.php | 2 +- .../cases/libs/controller/controller.test.php | 8 +++++--- cake/tests/cases/libs/view/helper.test.php | 15 +++++++++++++++ 4 files changed, 22 insertions(+), 4 deletions(-) diff --git a/cake/libs/controller/controller.php b/cake/libs/controller/controller.php index 4a85cac6c..3834d970c 100644 --- a/cake/libs/controller/controller.php +++ b/cake/libs/controller/controller.php @@ -1042,6 +1042,7 @@ class Controller extends Object { } elseif (intval($page) < 1) { $options['page'] = $page = 1; } + $page = $options['page'] = (integer)$page; if (method_exists($object, 'paginate')) { $results = $object->paginate($conditions, $fields, $order, $limit, $page, $recursive, $extra); diff --git a/cake/libs/view/helper.php b/cake/libs/view/helper.php index b365d7d74..fc12c7ac2 100644 --- a/cake/libs/view/helper.php +++ b/cake/libs/view/helper.php @@ -175,7 +175,7 @@ class Helper extends Overloadable { * @return string Full translated URL with base path. */ function url($url = null, $full = false) { - return Router::url($url, array('full' => $full, 'escape' => true)); + return h(Router::url($url, $full)); } /** * Checks if a file exists when theme is used, if no file is found default location is returned diff --git a/cake/tests/cases/libs/controller/controller.test.php b/cake/tests/cases/libs/controller/controller.test.php index 3730ad253..e0bbfeda9 100644 --- a/cake/tests/cases/libs/controller/controller.test.php +++ b/cake/tests/cases/libs/controller/controller.test.php @@ -503,10 +503,12 @@ class ControllerTest extends CakeTestCase { $results = Set::extract($Controller->paginate('ControllerPost'), '{n}.ControllerPost.id'); $this->assertEqual($Controller->ControllerPost->lastQuery['order'][0], array('ControllerPost.author_id' => 'asc')); $this->assertEqual($results, array(1, 3, 2)); - - $Controller->passedArgs = array('page' => '" onclick="alert(\'xss\');">'); + + $Controller->passedArgs = array('page' => '1 " onclick="alert(\'xss\');">'); + $Controller->paginate = array('limit' => 1); $Controller->paginate('ControllerPost'); - $this->assertEqual($Controller->params['paging']['ControllerPost']['page'], 1, 'XSS exploit opened %s'); + $this->assertIdentical($Controller->params['paging']['ControllerPost']['page'], 1, 'XSS exploit opened %s'); + $this->assertIdentical($Controller->params['paging']['ControllerPost']['options']['page'], 1, 'XSS exploit opened %s'); } /** * testPaginateExtraParams method diff --git a/cake/tests/cases/libs/view/helper.test.php b/cake/tests/cases/libs/view/helper.test.php index 922719c99..ad4a9b6db 100644 --- a/cake/tests/cases/libs/view/helper.test.php +++ b/cake/tests/cases/libs/view/helper.test.php @@ -347,6 +347,21 @@ class HelperTest extends CakeTestCase { $result = $this->Helper->value('Post.2.created.year'); $this->assertEqual($result, '2008'); } +/** + * Ensure HTML escaping of url params. So link addresses are valid and not exploited + * + * @return void + **/ + function testUrlConversion() { + $result = $this->Helper->url('/controller/action/1'); + $this->assertEqual($result, '/controller/action/1'); + + $result = $this->Helper->url('/controller/action/1?one=1&two=2'); + $this->assertEqual($result, '/controller/action/1?one=1&two=2'); + + $result = $this->Helper->url(array('controller' => 'posts', 'action' => 'index', 'page' => '1" onclick="alert(\'XSS\');"')); + $this->assertEqual($result, "/posts/index/page:1" onclick="alert('XSS');""); + } /** * testFieldsWithSameName method *