From aae0f762dd19c5bc2d171d7c8b06d63bebebe381 Mon Sep 17 00:00:00 2001 From: euromark Date: Wed, 4 Dec 2013 01:30:57 +0100 Subject: [PATCH 1/4] Collision free approach to resolve the DOM ID issue in a clean way. Fix to generation of ids for multiple checkboxes. Resolves ticket 4064. --- .../Test/Case/View/Helper/FormHelperTest.php | 126 +++++++++++++++++- lib/Cake/Test/Case/View/HelperTest.php | 10 ++ lib/Cake/View/Helper.php | 1 + lib/Cake/View/Helper/FormHelper.php | 39 +++++- 4 files changed, 173 insertions(+), 3 deletions(-) diff --git a/lib/Cake/Test/Case/View/Helper/FormHelperTest.php b/lib/Cake/Test/Case/View/Helper/FormHelperTest.php index 5427dbdfd..bd90387e8 100644 --- a/lib/Cake/Test/Case/View/Helper/FormHelperTest.php +++ b/lib/Cake/Test/Case/View/Helper/FormHelperTest.php @@ -3222,8 +3222,8 @@ class FormHelperTest extends CakeTestCase { $expected = array( 'input' => array('type' => 'hidden', 'name' => 'data[Model][multi_field]', 'value' => '', 'id' => 'ModelMultiField'), array('div' => array('class' => 'checkbox')), - array('input' => array('type' => 'checkbox', 'name' => 'data[Model][multi_field][]', 'value' => '1/2', 'id' => 'ModelMultiField12')), - array('label' => array('for' => 'ModelMultiField12')), + array('input' => array('type' => 'checkbox', 'name' => 'data[Model][multi_field][]', 'value' => '1/2', 'id' => 'ModelMultiField1/2')), + array('label' => array('for' => 'ModelMultiField1/2')), 'half', '/label', '/div', @@ -4150,6 +4150,50 @@ class FormHelperTest extends CakeTestCase { ); } +/** + * testDomIdSuffix method + * + * @return void + */ + public function testDomIdSuffix() { + $result = $this->Form->domIdSuffix('1 string with 1$-dollar signs'); + $this->assertEquals('1StringWith1$-dollarSigns', $result); + + $result = $this->Form->domIdSuffix(''); + $this->assertEquals('AbcX=FooY=Bar', $result); + + $result = $this->Form->domIdSuffix('1 string with 1$-dollar signs', 'xhtml'); + $this->assertEquals('1StringWith1-dollarSigns', $result); + + $result = $this->Form->domIdSuffix('', 'xhtml'); + $this->assertEquals('AbcXFooYBar', $result); + } + +/** + * testDomIdSuffixCollisionResolvement() + * + * @return void + */ + public function testDomIdSuffixCollisionResolvement() { + $result = $this->Form->domIdSuffix('a>b'); + $this->assertEquals('AB', $result); + + $result = $this->Form->domIdSuffix('aassertEquals('AB1', $result); + + $result = $this->Form->domIdSuffix('a\'b'); + $this->assertEquals('AB2', $result); + + $result = $this->Form->domIdSuffix('1 string with 1$-dollar', 'xhtml'); + $this->assertEquals('1StringWith1-dollar', $result); + + $result = $this->Form->domIdSuffix('1 string with 1€-dollar', 'xhtml'); + $this->assertEquals('1StringWith1-dollar1', $result); + + $result = $this->Form->domIdSuffix('1 string with 1$-dollar', 'xhtml'); + $this->assertEquals('1StringWith1-dollar2', $result); + } + /** * testSelect method * @@ -4998,6 +5042,84 @@ class FormHelperTest extends CakeTestCase { '/div' ); $this->assertTags($result, $expected); + + $result = $this->Form->select( + 'Model.multi_field', + array('a+' => 'first', 'a++' => 'second', 'a+++' => 'third'), + array('multiple' => 'checkbox') + ); + $expected = array( + 'input' => array( + 'type' => 'hidden', 'name' => 'data[Model][multi_field]', 'value' => '', 'id' => 'ModelMultiField' + ), + array('div' => array('class' => 'checkbox')), + array('input' => array( + 'type' => 'checkbox', 'name' => 'data[Model][multi_field][]', + 'value' => 'a+', 'id' => 'ModelMultiFieldA+' + )), + array('label' => array('for' => 'ModelMultiFieldA+')), + 'first', + '/label', + '/div', + array('div' => array('class' => 'checkbox')), + array('input' => array( + 'type' => 'checkbox', 'name' => 'data[Model][multi_field][]', + 'value' => 'a++', 'id' => 'ModelMultiFieldA++' + )), + array('label' => array('for' => 'ModelMultiFieldA++')), + 'second', + '/label', + '/div', + array('div' => array('class' => 'checkbox')), + array('input' => array( + 'type' => 'checkbox', 'name' => 'data[Model][multi_field][]', + 'value' => 'a+++', 'id' => 'ModelMultiFieldA+++' + )), + array('label' => array('for' => 'ModelMultiFieldA+++')), + 'third', + '/label', + '/div' + ); + $this->assertTags($result, $expected); + + $result = $this->Form->select( + 'Model.multi_field', + array('a>b' => 'first', 'a 'second', 'a"b' => 'third'), + array('multiple' => 'checkbox') + ); + $expected = array( + 'input' => array( + 'type' => 'hidden', 'name' => 'data[Model][multi_field]', 'value' => '', 'id' => 'ModelMultiField' + ), + array('div' => array('class' => 'checkbox')), + array('input' => array( + 'type' => 'checkbox', 'name' => 'data[Model][multi_field][]', + 'value' => 'a>b', 'id' => 'ModelMultiFieldAB2' + )), + array('label' => array('for' => 'ModelMultiFieldAB2')), + 'first', + '/label', + '/div', + array('div' => array('class' => 'checkbox')), + array('input' => array( + 'type' => 'checkbox', 'name' => 'data[Model][multi_field][]', + 'value' => 'a<b', 'id' => 'ModelMultiFieldAB1' + )), + array('label' => array('for' => 'ModelMultiFieldAB1')), + 'second', + '/label', + '/div', + array('div' => array('class' => 'checkbox')), + array('input' => array( + 'type' => 'checkbox', 'name' => 'data[Model][multi_field][]', + 'value' => 'a"b', 'id' => 'ModelMultiFieldAB' + )), + array('label' => array('for' => 'ModelMultiFieldAB')), + 'third', + '/label', + '/div' + ); + $this->assertTags($result, $expected); } /** diff --git a/lib/Cake/Test/Case/View/HelperTest.php b/lib/Cake/Test/Case/View/HelperTest.php index a23ba83c3..2d5bbaa62 100644 --- a/lib/Cake/Test/Case/View/HelperTest.php +++ b/lib/Cake/Test/Case/View/HelperTest.php @@ -851,6 +851,16 @@ class HelperTest extends CakeTestCase { $this->assertEquals('&lt;script&gt;alert(document.cookie)&lt;/script&gt;', $result); } +/** + * testDomId method + * + * @return void + */ + public function testDomId() { + $result = $this->Helper->domId('Foo.bar'); + $this->assertEquals('FooBar', $result); + } + /** * testMultiDimensionalField method * diff --git a/lib/Cake/View/Helper.php b/lib/Cake/View/Helper.php index 5d38d9aa4..55fc2d12b 100644 --- a/lib/Cake/View/Helper.php +++ b/lib/Cake/View/Helper.php @@ -16,6 +16,7 @@ App::uses('Router', 'Routing'); App::uses('Hash', 'Utility'); +App::uses('Inflector', 'Utility'); /** * Abstract base class for all other Helpers in CakePHP. diff --git a/lib/Cake/View/Helper/FormHelper.php b/lib/Cake/View/Helper/FormHelper.php index db4b4cf79..4278aa1d1 100644 --- a/lib/Cake/View/Helper/FormHelper.php +++ b/lib/Cake/View/Helper/FormHelper.php @@ -17,6 +17,7 @@ App::uses('ClassRegistry', 'Utility'); App::uses('AppHelper', 'View/Helper'); App::uses('Hash', 'Utility'); +App::uses('Inflector', 'Utility'); /** * Form helper library. @@ -109,6 +110,13 @@ class FormHelper extends AppHelper { */ public $validationErrors = array(); +/** + * Holds already used DOM ID suffixes to avoid collisions with multiple form field elements. + * + * @var array + */ + protected $_domIdSuffixes = array(); + /** * Copies the validationErrors variable from the View object into this instance * @@ -2065,6 +2073,34 @@ class FormHelper extends AppHelper { return implode("\n", $select); } +/** + * Generates a valid DOM ID suffix from a string. + * Also avoids collisions when multiple values are coverted to the same suffix by + * appending a numeric value. + * + * For pre-HTML5 IDs only characters like a-z 0-9 - _ are valid. HTML5 doesn't have that + * limitation, but to avoid layout issues it still filters out some sensitive chars. + * + * @param string $value The value that should be transferred into a DOM ID suffix. + * @param string $type Doctype to use. Defaults to html5. Anything else will use limited chars. + * @return string DOM ID + */ + public function domIdSuffix($value, $type = 'html5') { + if ($type === 'html5') { + $value = str_replace(array('<', '>', ' ', '"', '\''), '_', $value); + } else { + $value = preg_replace('~[^\\pL\d-_]+~u', '_', $value); + } + $value = Inflector::camelize($value); + $count = 1; + $suffix = $value; + while (in_array($suffix, $this->_domIdSuffixes)) { + $suffix = $value . $count++; + } + $this->_domIdSuffixes[] = $suffix; + return $suffix; + } + /** * Returns a SELECT element for days. * @@ -2609,6 +2645,7 @@ class FormHelper extends AppHelper { $selectedIsEmpty = ($attributes['value'] === '' || $attributes['value'] === null); $selectedIsArray = is_array($attributes['value']); + $this->_domIdSuffixes = array(); foreach ($elements as $name => $title) { $htmlOptions = array(); if (is_array($title) && (!isset($title['name']) || !isset($title['value']))) { @@ -2677,7 +2714,7 @@ class FormHelper extends AppHelper { if ($attributes['style'] === 'checkbox') { $htmlOptions['value'] = $name; - $tagName = $attributes['id'] . Inflector::camelize(Inflector::slug($name)); + $tagName = $attributes['id'] . $this->domIdSuffix($name); $htmlOptions['id'] = $tagName; $label = array('for' => $tagName); From 587a04ab84f2ffeb5bff7ab06a7790bc201581ed Mon Sep 17 00:00:00 2001 From: euromark Date: Wed, 4 Dec 2013 01:51:39 +0100 Subject: [PATCH 2/4] prevent possible XSS attack via form helper selects and unescaped output. --- .../Test/Case/View/Helper/FormHelperTest.php | 28 +++++++++++++++++++ lib/Cake/View/Helper/FormHelper.php | 3 ++ 2 files changed, 31 insertions(+) diff --git a/lib/Cake/Test/Case/View/Helper/FormHelperTest.php b/lib/Cake/Test/Case/View/Helper/FormHelperTest.php index bd90387e8..edc27ead4 100644 --- a/lib/Cake/Test/Case/View/Helper/FormHelperTest.php +++ b/lib/Cake/Test/Case/View/Helper/FormHelperTest.php @@ -4634,6 +4634,34 @@ class FormHelperTest extends CakeTestCase { '/select' ); $this->assertTags($result, $expected); + + $result = $this->Form->select( + 'Model.multi_field', + array('a>b' => 'first', 'a 'second', 'a"b' => 'third'), + array('multiple' => true) + ); + $expected = array( + 'input' => array( + 'type' => 'hidden', 'name' => 'data[Model][multi_field]', 'value' => '', + 'id' => 'ModelMultiField_' + ), + array('select' => array('name' => 'data[Model][multi_field][]', + 'multiple' => 'multiple', 'id' => 'ModelMultiField' + )), + array('option' => array('value' => 'a>b')), + 'first', + '/option', + array('option' => array('value' => 'a<b')), + 'second', + '/option', + array('option' => array( + 'value' => 'a"b' + )), + 'third', + '/option', + '/select' + ); + $this->assertTags($result, $expected); } /** diff --git a/lib/Cake/View/Helper/FormHelper.php b/lib/Cake/View/Helper/FormHelper.php index 4278aa1d1..791f9a6e2 100644 --- a/lib/Cake/View/Helper/FormHelper.php +++ b/lib/Cake/View/Helper/FormHelper.php @@ -2733,6 +2733,9 @@ class FormHelper extends AppHelper { $item = $this->Html->useTag('checkboxmultiple', $name, $htmlOptions); $select[] = $this->Html->div($attributes['class'], $item . $label); } else { + if ($attributes['escape']) { + $name = h($name); + } $select[] = $this->Html->useTag('selectoption', $name, $htmlOptions, $title); } } From 8ebf004450778023924a70502c0537ba34ba6029 Mon Sep 17 00:00:00 2001 From: euromark Date: Wed, 4 Dec 2013 02:14:08 +0100 Subject: [PATCH 3/4] Also make DOM ids for radio element values unique. --- .../Test/Case/View/Helper/FormHelperTest.php | 36 +++++++++++++++++-- lib/Cake/View/Helper/FormHelper.php | 5 ++- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/lib/Cake/Test/Case/View/Helper/FormHelperTest.php b/lib/Cake/Test/Case/View/Helper/FormHelperTest.php index edc27ead4..c323fd5d2 100644 --- a/lib/Cake/Test/Case/View/Helper/FormHelperTest.php +++ b/lib/Cake/Test/Case/View/Helper/FormHelperTest.php @@ -3503,8 +3503,8 @@ class FormHelperTest extends CakeTestCase { $result = $this->Form->radio('Model.field', array('1/2' => 'half')); $expected = array( 'input' => array('type' => 'hidden', 'name' => 'data[Model][field]', 'value' => '', 'id' => 'ModelField_'), - array('input' => array('type' => 'radio', 'name' => 'data[Model][field]', 'value' => '1/2', 'id' => 'ModelField12')), - 'label' => array('for' => 'ModelField12'), + array('input' => array('type' => 'radio', 'name' => 'data[Model][field]', 'value' => '1/2', 'id' => 'ModelField1/2')), + 'label' => array('for' => 'ModelField1/2'), 'half', '/label' ); @@ -3631,6 +3631,38 @@ class FormHelperTest extends CakeTestCase { '/fieldset' ); $this->assertTags($result, $expected); + + $result = $this->Form->radio( + 'Model.field', + array('a>b' => 'first', 'a 'second', 'a"b' => 'third') + ); + $expected = array( + 'fieldset' => array(), + 'legend' => array(), + 'Field', + '/legend', + 'input' => array( + 'type' => 'hidden', 'name' => 'data[Model][field]', + 'id' => 'ModelField_', 'value' => '', + ), + array('input' => array('type' => 'radio', 'name' => 'data[Model][field]', + 'id' => 'ModelFieldAB', 'value' => 'a>b')), + array('label' => array('for' => 'ModelFieldAB')), + 'first', + '/label', + array('input' => array('type' => 'radio', 'name' => 'data[Model][field]', + 'id' => 'ModelFieldAB1', 'value' => 'a<b')), + array('label' => array('for' => 'ModelFieldAB1')), + 'second', + '/label', + array('input' => array('type' => 'radio', 'name' => 'data[Model][field]', + 'id' => 'ModelFieldAB2', 'value' => 'a"b')), + array('label' => array('for' => 'ModelFieldAB2')), + 'third', + '/label', + '/fieldset' + ); + $this->assertTags($result, $expected); } /** diff --git a/lib/Cake/View/Helper/FormHelper.php b/lib/Cake/View/Helper/FormHelper.php index 791f9a6e2..3592e40f7 100644 --- a/lib/Cake/View/Helper/FormHelper.php +++ b/lib/Cake/View/Helper/FormHelper.php @@ -1514,6 +1514,7 @@ class FormHelper extends AppHelper { $value = $value ? 1 : 0; } + $this->_domIdSuffixes = array(); foreach ($options as $optValue => $optTitle) { $optionsHere = array('value' => $optValue, 'disabled' => false); @@ -1524,9 +1525,7 @@ class FormHelper extends AppHelper { if ($disabled && (!is_array($disabled) || in_array((string)$optValue, $disabled, !$isNumeric))) { $optionsHere['disabled'] = true; } - $tagName = Inflector::camelize( - $attributes['id'] . '_' . Inflector::slug($optValue) - ); + $tagName = $attributes['id'] . $this->domIdSuffix($optValue); if ($label) { $labelOpts = is_array($label) ? $label : array(); From b392254c9211aa521f44f4a4b61bb5b11e4b2b81 Mon Sep 17 00:00:00 2001 From: euromark Date: Tue, 24 Dec 2013 15:20:32 +0100 Subject: [PATCH 4/4] fix cs --- lib/Cake/Test/Case/View/Helper/FormHelperTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Cake/Test/Case/View/Helper/FormHelperTest.php b/lib/Cake/Test/Case/View/Helper/FormHelperTest.php index c323fd5d2..212605a47 100644 --- a/lib/Cake/Test/Case/View/Helper/FormHelperTest.php +++ b/lib/Cake/Test/Case/View/Helper/FormHelperTest.php @@ -3643,7 +3643,7 @@ class FormHelperTest extends CakeTestCase { '/legend', 'input' => array( 'type' => 'hidden', 'name' => 'data[Model][field]', - 'id' => 'ModelField_', 'value' => '', + 'id' => 'ModelField_', 'value' => '', ), array('input' => array('type' => 'radio', 'name' => 'data[Model][field]', 'id' => 'ModelFieldAB', 'value' => 'a>b')),