Adding tests/fixes for possible XSS injections in view helpers

Signed-off-by: Marco Pivetta <[email protected]>
HolgerW1 · Apr 15, 2014 · 1dd4f8c · 1dd4f8c
1 parent 692a9d1
commit 1dd4f8c
Show file tree

Hide file tree

Showing 50 changed files with 369 additions and 268 deletions.
diff --git a/library/Zend/Form/View/Helper/AbstractHelper.php b/library/Zend/Form/View/Helper/AbstractHelper.php
@@ -208,9 +208,12 @@ public function createAttributesString(array $attributes)
     {
         $attributes = $this->prepareAttributes($attributes);
         $escape     = $this->getEscapeHtmlHelper();
+        $escapeAttr = $this->getEscapeHtmlAttrHelper();
         $strings    = array();
+
         foreach ($attributes as $key => $value) {
             $key = strtolower($key);
+
             if (!$value && isset($this->booleanAttributes[$key])) {
                 // Skip boolean attributes that expect empty string as false value
                 if ('' === $this->booleanAttributes[$key]['off']) {
@@ -221,15 +224,14 @@ public function createAttributesString(array $attributes)
             //check if attribute is translatable
             if (isset($this->translatableAttributes[$key]) && !empty($value)) {
                 if (($translator = $this->getTranslator()) !== null) {
-                    $value = $translator->translate(
-                            $value, $this->getTranslatorTextDomain()
-                    );
+                    $value = $translator->translate($value, $this->getTranslatorTextDomain());
                 }
             }
 
             //@TODO Escape event attributes like AbstractHtmlElement view helper does in htmlAttribs ??
-            $strings[] = sprintf('%s="%s"', $escape($key), $escape($value));
+            $strings[] = sprintf('%s="%s"', $escape($key), $escapeAttr($value));
         }
+
         return implode(' ', $strings);
     }
 

diff --git a/library/Zend/View/Helper/AbstractHtmlElement.php b/library/Zend/View/Helper/AbstractHtmlElement.php
@@ -65,8 +65,9 @@ protected function isXhtml()
      */
     protected function htmlAttribs($attribs)
     {
-        $xhtml   = '';
-        $escaper = $this->getView()->plugin('escapehtml');
+        $xhtml          = '';
+        $escaper        = $this->getView()->plugin('escapehtml');
+        $escapeHtmlAttr = $this->getView()->plugin('escapehtmlattr');
 
         foreach ((array) $attribs as $key => $val) {
             $key = $escaper($key);
@@ -77,18 +78,14 @@ protected function htmlAttribs($attribs)
                     // non-scalar data should be cast to JSON first
                     $val = \Zend\Json\Json::encode($val);
                 }
-                // Escape single quotes inside event attribute values.
-                // This will create html, where the attribute value has
-                // single quotes around it, and escaped single quotes or
-                // non-escaped double quotes inside of it
-                $val = str_replace('\'', '&#39;', $val);
             } else {
                 if (is_array($val)) {
                     $val = implode(' ', $val);
                 }
-                $val = $escaper($val);
             }
 
+            $val = $escapeHtmlAttr($val);
+
             if ('id' == $key) {
                 $val = $this->normalizeId($val);
             }

diff --git a/library/Zend/View/Helper/Navigation/Menu.php b/library/Zend/View/Helper/Navigation/Menu.php
@@ -152,7 +152,9 @@ protected function renderDeepestMenu(
             $active['page'] = $active['page']->getParent();
         }
 
-        $ulClass = $ulClass ? ' class="' . $ulClass . '"' : '';
+        /* @var $escaper \Zend\View\Helper\EscapeHtmlAttr */
+        $escaper = $this->view->plugin('escapeHtmlAttr');
+        $ulClass = $ulClass ? ' class="' . $escaper($ulClass) . '"' : '';
         $html = $indent . '<ul' . $ulClass . '>' . PHP_EOL;
 
         foreach ($active['page'] as $subPage) {
@@ -170,7 +172,7 @@ protected function renderDeepestMenu(
             if ($addClassToListItem && $subPage->getClass()) {
                 $liClasses[] = $subPage->getClass();
             }
-            $liClass = empty($liClasses) ? '' : ' class="' . implode(' ', $liClasses) . '"';
+            $liClass = empty($liClasses) ? '' : ' class="' . $escaper(implode(' ', $liClasses)) . '"';
 
             $html .= $indent . '    <li' . $liClass . '>' . PHP_EOL;
             $html .= $indent . '        ' . $this->htmlify($subPage, $escapeLabels, $addClassToListItem) . PHP_EOL;
@@ -262,6 +264,9 @@ protected function renderNormalMenu(
 
         // find deepest active
         $found = $this->findActive($container, $minDepth, $maxDepth);
+        /* @var $escaper \Zend\View\Helper\EscapeHtmlAttr */
+        $escaper = $this->view->plugin('escapeHtmlAttr');
+
         if ($found) {
             $foundPage  = $found['page'];
             $foundDepth = $found['depth'];
@@ -314,7 +319,7 @@ protected function renderNormalMenu(
             if ($depth > $prevDepth) {
                 // start new ul tag
                 if ($ulClass && $depth ==  0) {
-                    $ulClass = ' class="' . $ulClass . '"';
+                    $ulClass = ' class="' . $escaper($ulClass) . '"';
                 } else {
                     $ulClass = '';
                 }
@@ -343,7 +348,7 @@ protected function renderNormalMenu(
             if ($addClassToListItem && $page->getClass()) {
                 $liClasses[] = $page->getClass();
             }
-            $liClass = empty($liClasses) ? '' : ' class="' . implode(' ', $liClasses) . '"';
+            $liClass = empty($liClasses) ? '' : ' class="' . $escaper(implode(' ', $liClasses)) . '"';
 
             $html .= $myIndent . '    <li' . $liClass . '>' . PHP_EOL
                 . $myIndent . '        ' . $this->htmlify($page, $escapeLabels, $addClassToListItem) . PHP_EOL;

diff --git a/tests/ZendTest/Form/View/Helper/AbstractHelperTest.php b/tests/ZendTest/Form/View/Helper/AbstractHelperTest.php
@@ -0,0 +1,36 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2014 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license   http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace ZendTest\Form\View\Helper;
+
+/**
+ * Tests for {@see \Zend\Form\View\Helper\AbstractHelper}
+ *
+ * @covers \Zend\Form\View\Helper\AbstractHelper
+ */
+class AbstractHelperTest extends CommonTestCase
+{
+    public function setUp()
+    {
+        $this->helper = $this->getMockForAbstractClass('Zend\Form\View\Helper\AbstractHelper');
+
+        parent::setUp();
+    }
+
+    /**
+     * @group 5991
+     */
+    public function testWillEscapeValueAttributeValuesCorrectly()
+    {
+        $this->assertSame(
+            'data-value="breaking&#x20;your&#x20;HTML&#x20;like&#x20;a&#x20;boss&#x21;&#x20;&#x5C;"',
+            $this->helper->createAttributesString(array('data-value' => 'breaking your HTML like a boss! \\'))
+        );
+    }
+}
diff --git a/tests/ZendTest/Form/View/Helper/Captcha/DumbTest.php b/tests/ZendTest/Form/View/Helper/Captcha/DumbTest.php
@@ -44,15 +44,17 @@ public function testRendersHiddenInputForId()
     {
         $element = $this->getElement();
         $markup  = $this->helper->render($element);
-        $this->assertRegExp('#(name="' . $element->getName() . '\[id\]").*?(type="hidden")#', $markup);
-        $this->assertRegExp('#(name="' . $element->getName() . '\[id\]").*?(value="' . $this->captcha->getId() . '")#', $markup);
+
+        $this->assertRegExp('#(name="' . $element->getName() . '\&\#x5B\;id\&\#x5D\;").*?(type="hidden")#', $markup);
+        $this->assertRegExp('#(name="' . $element->getName() . '\&\#x5B\;id\&\#x5D\;").*?(value="' . $this->captcha->getId() . '")#', $markup);
     }
 
     public function testRendersTextInputForInput()
     {
         $element = $this->getElement();
         $markup  = $this->helper->render($element);
-        $this->assertRegExp('#(name="' . $element->getName() . '\[input\]").*?(type="text")#', $markup);
+
+        $this->assertRegExp('#(name="' . $element->getName() . '\&\#x5B\;input\&\#x5D\;").*?(type="text")#', $markup);
     }
 
     public function testRendersLabelPriorToInputByDefault()
@@ -92,7 +94,8 @@ public function testRenderSeparatorOneTimeAfterText()
         $element = $this->getElement();
         $this->helper->setSeparator('<br />');
         $markup  = $this->helper->render($element);
-        $this->assertContains($this->captcha->getLabel() . ' <b>' . strrev($this->captcha->getWord()) . '</b>' . $this->helper->getSeparator() . '<input name="foo[id]" type="hidden"', $markup);
+
+        $this->assertContains($this->captcha->getLabel() . ' <b>' . strrev($this->captcha->getWord()) . '</b>' . $this->helper->getSeparator() . '<input name="foo&#x5B;id&#x5D;" type="hidden"', $markup);
         $this->assertNotContains($this->helper->getSeparator() . '<input name="foo[input]" type="text"', $markup);
     }
 }
diff --git a/tests/ZendTest/Form/View/Helper/Captcha/FigletTest.php b/tests/ZendTest/Form/View/Helper/Captcha/FigletTest.php
@@ -44,15 +44,15 @@ public function testRendersHiddenInputForId()
     {
         $element = $this->getElement();
         $markup  = $this->helper->render($element);
-        $this->assertRegExp('#(name="' . $element->getName() . '\[id\]").*?(type="hidden")#', $markup);
-        $this->assertRegExp('#(name="' . $element->getName() . '\[id\]").*?(value="' . $this->captcha->getId() . '")#', $markup);
+        $this->assertRegExp('#(name="' . $element->getName() . '\&\#x5B\;id\&\#x5D\;").*?(type="hidden")#', $markup);
+        $this->assertRegExp('#(name="' . $element->getName() . '\&\#x5B\;id\&\#x5D\;").*?(value="' . $this->captcha->getId() . '")#', $markup);
     }
 
     public function testRendersTextInputForInput()
     {
         $element = $this->getElement();
         $markup  = $this->helper->render($element);
-        $this->assertRegExp('#(name="' . $element->getName() . '\[input\]").*?(type="text")#', $markup);
+        $this->assertRegExp('#(name="' . $element->getName() . '\&\#x5B\;input\&\#x5D\;").*?(type="text")#', $markup);
     }
 
     public function testRendersFigletPriorToInputByDefault()

diff --git a/tests/ZendTest/Form/View/Helper/Captcha/ImageTest.php b/tests/ZendTest/Form/View/Helper/Captcha/ImageTest.php
@@ -103,15 +103,15 @@ public function testRendersHiddenInputForId()
     {
         $element = $this->getElement();
         $markup  = $this->helper->render($element);
-        $this->assertRegExp('#(name="' . $element->getName() . '\[id\]").*?(type="hidden")#', $markup);
-        $this->assertRegExp('#(name="' . $element->getName() . '\[id\]").*?(value="' . $this->captcha->getId() . '")#', $markup);
+        $this->assertRegExp('#(name="' . $element->getName() . '\&\#x5B\;id\&\#x5D\;").*?(type="hidden")#', $markup);
+        $this->assertRegExp('#(name="' . $element->getName() . '\&\#x5B\;id\&\#x5D\;").*?(value="' . $this->captcha->getId() . '")#', $markup);
     }
 
     public function testRendersTextInputForInput()
     {
         $element = $this->getElement();
         $markup  = $this->helper->render($element);
-        $this->assertRegExp('#(name="' . $element->getName() . '\[input\]").*?(type="text")#', $markup);
+        $this->assertRegExp('#(name="' . $element->getName() . '\&\#x5B\;input\&\#x5D\;").*?(type="text")#', $markup);
     }
 
     public function testRendersImageTagPriorToInputByDefault()

diff --git a/tests/ZendTest/Form/View/Helper/CommonTestCase.php b/tests/ZendTest/Form/View/Helper/CommonTestCase.php
@@ -19,7 +19,14 @@
  */
 abstract class CommonTestCase extends TestCase
 {
+    /**
+     * @var \Zend\Form\View\Helper\AbstractHelper
+     */
     public $helper;
+
+    /**
+     * @var \Zend\View\Renderer\RendererInterface
+     */
     public $renderer;
 
     public function setUp()

diff --git a/tests/ZendTest/Form/View/Helper/FormCaptchaTest.php b/tests/ZendTest/Form/View/Helper/FormCaptchaTest.php
@@ -133,7 +133,7 @@ public function testPassingElementWithImageCaptchaRendersCorrectly()
         $markup = $this->helper->render($element);
 
         $this->assertContains('<img ', $markup);
-        $this->assertContains($captcha->getImgUrl(), $markup);
+        $this->assertContains(str_replace('/', '&#x2F;', $captcha->getImgUrl()), $markup);
         $this->assertContains($captcha->getId(), $markup);
         $this->assertRegExp('#<img[^>]*(id="' . $element->getAttribute('id') . '-image")[^>]*>#', $markup);
         $this->assertRegExp('#<input[^>]*(id="' . $element->getAttribute('id') . '")[^>]*(type="text")[^>]*>#', $markup);

diff --git a/tests/ZendTest/Form/View/Helper/FormCollectionTest.php b/tests/ZendTest/Form/View/Helper/FormCollectionTest.php
@@ -78,8 +78,8 @@ public function testCorrectlyIndexElementsInCollection()
         $collection = $form->get('colors');
 
         $markup = $this->helper->render($collection);
-        $this->assertContains('name="colors[0]"', $markup);
-        $this->assertContains('name="colors[1]"', $markup);
+        $this->assertContains('name="colors&#x5B;0&#x5D;"', $markup);
+        $this->assertContains('name="colors&#x5B;1&#x5D;"', $markup);
     }
 
     public function testCorrectlyIndexNestedElementsInCollection()
@@ -88,9 +88,9 @@ public function testCorrectlyIndexNestedElementsInCollection()
         $collection = $form->get('fieldsets');
 
         $markup = $this->helper->render($collection);
-        $this->assertContains('fieldsets[0][field]', $markup);
-        $this->assertContains('fieldsets[1][field]', $markup);
-        $this->assertContains('fieldsets[1][nested_fieldset][anotherField]', $markup);
+        $this->assertContains('fieldsets&#x5B;0&#x5D;&#x5B;field&#x5D;', $markup);
+        $this->assertContains('fieldsets&#x5B;1&#x5D;&#x5B;field&#x5D;', $markup);
+        $this->assertContains('fieldsets&#x5B;1&#x5D;&#x5B;nested_fieldset&#x5D;&#x5B;anotherField&#x5D;', $markup);
     }
 
     public function testRenderWithCustomHelper()

diff --git a/tests/ZendTest/Form/View/Helper/FormFileTest.php b/tests/ZendTest/Form/View/Helper/FormFileTest.php
@@ -179,7 +179,7 @@ public function testNameShouldHaveArrayNotationWhenMultipleIsSpecified()
         $element = new Element\File('foo');
         $element->setAttribute('multiple', true);
         $markup = $this->helper->render($element);
-        $this->assertRegexp('#<input[^>]*?(name="foo\[\]")#', $markup);
+        $this->assertRegexp('#<input[^>]*?(name="foo\&\#x5B\;\&\#x5D\;")#', $markup);
     }
 
     /**

diff --git a/tests/ZendTest/Form/View/Helper/FormInputTest.php b/tests/ZendTest/Form/View/Helper/FormInputTest.php
@@ -476,7 +476,7 @@ public function testCanTranslatePlaceholder()
 
         $markup = $this->helper->__invoke($element);
 
-        $this->assertContains('placeholder="translated string"', $markup);
+        $this->assertContains('placeholder="translated&#x20;string"', $markup);
     }
 
     public function testCanTranslateTitle()
@@ -497,6 +497,6 @@ public function testCanTranslateTitle()
 
         $markup = $this->helper->__invoke($element);
 
-        $this->assertContains('title="translated string"', $markup);
+        $this->assertContains('title="translated&#x20;string"', $markup);
     }
 }
diff --git a/tests/ZendTest/Form/View/Helper/FormMultiCheckboxTest.php b/tests/ZendTest/Form/View/Helper/FormMultiCheckboxTest.php
@@ -195,7 +195,7 @@ public function testNameShouldHaveBracketsAppended()
     {
         $element = $this->getElement();
         $markup  = $this->helper->render($element);
-        $this->assertContains('foo[]', $markup);
+        $this->assertContains('foo&#x5B;&#x5D;', $markup);
     }
 
     public function testInvokeWithNoElementChainsHelper()

diff --git a/tests/ZendTest/Form/View/Helper/FormResetTest.php b/tests/ZendTest/Form/View/Helper/FormResetTest.php
@@ -167,6 +167,6 @@ public function testCanTranslateValue()
         $this->assertTrue($this->helper->hasTranslator());
 
         $markup = $this->helper->__invoke($element);
-        $this->assertContains('value="translated content"', $markup);
+        $this->assertContains('value="translated&#x20;content"', $markup);
     }
 }
diff --git a/tests/ZendTest/Form/View/Helper/FormRowTest.php b/tests/ZendTest/Form/View/Helper/FormRowTest.php
@@ -166,7 +166,7 @@ public function testDoesNotOverrideClassesIfAlreadyPresentWhenThereAreErrors()
         $element->setAttribute('class', 'foo bar');
 
         $markup = $this->helper->setInputErrorClass('custom-error-class')->render($element);
-        $this->assertRegexp('/<input name="foo" class="foo bar custom-error-class" type="text" [^\/>]*\/?>/', $markup);
+        $this->assertRegexp('/<input name="foo" class="foo\&\#x20\;bar\&\#x20\;custom-error-class" type="text" [^\/>]*\/?>/', $markup);
     }
 
     public function testInvokeWithNoElementChainsHelper()

diff --git a/tests/ZendTest/Form/View/Helper/FormSelectTest.php b/tests/ZendTest/Form/View/Helper/FormSelectTest.php
@@ -133,7 +133,7 @@ public function testOptgroupsAreCreatedWhenAnOptionHasAnOptionsKey()
         $element->setValueOptions($options);
 
         $markup = $this->helper->render($element);
-        $this->assertRegexp('#optgroup[^>]*?label="This is the second label"[^>]*>\s*<option[^>]*?value="bar"[^>]*?>foo.*?</optgroup>#s', $markup);
+        $this->assertRegexp('#optgroup[^>]*?label="This\&\#x20\;is\&\#x20\;the\&\#x20\;second\&\#x20\;label"[^>]*>\s*<option[^>]*?value="bar"[^>]*?>foo.*?</optgroup>#s', $markup);
     }
 
     public function testCanDisableAnOptgroup()
@@ -150,7 +150,7 @@ public function testCanDisableAnOptgroup()
         $element->setValueOptions($options);
 
         $markup = $this->helper->render($element);
-        $this->assertRegexp('#optgroup .*?label="This is the second label"[^>]*?disabled="disabled"[^>]*?>\s*<option[^>]*?value="bar"[^>]*?>foo.*?</optgroup>#', $markup);
+        $this->assertRegexp('#optgroup .*?label="This\&\#x20\;is\&\#x20\;the\&\#x20\;second\&\#x20\;label"[^>]*?disabled="disabled"[^>]*?>\s*<option[^>]*?value="bar"[^>]*?>foo.*?</optgroup>#', $markup);
     }
 
     /**
@@ -183,7 +183,7 @@ public function testNameShouldHaveArrayNotationWhenMultipleIsSpecified()
         $element->setAttribute('multiple', true);
         $element->setAttribute('value', array('value1', 'value2'));
         $markup = $this->helper->render($element);
-        $this->assertRegexp('#<select[^>]*?(name="foo\[\]")#', $markup);
+        $this->assertRegexp('#<select[^>]*?(name="foo\&\#x5B\;\&\#x5D\;")#', $markup);
     }
 
     public function getScalarOptionsDataProvider()
@@ -276,7 +276,7 @@ public function testCanTranslateOptGroupLabel()
 
         $markup = $this->helper->__invoke($element);
 
-        $this->assertContains('label="translated label"', $markup);
+        $this->assertContains('label="translated&#x20;label"', $markup);
         $this->assertContains('>translated foo<', $markup);
         $this->assertContains('>translated bar<', $markup);
     }

diff --git a/tests/ZendTest/Form/View/Helper/FormSubmitTest.php b/tests/ZendTest/Form/View/Helper/FormSubmitTest.php
@@ -166,6 +166,6 @@ public function testCanTranslateValue()
         $this->assertTrue($this->helper->hasTranslator());
 
         $markup = $this->helper->__invoke($element);
-        $this->assertContains('value="translated content"', $markup);
+        $this->assertContains('value="translated&#x20;content"', $markup);
     }
 }
diff --git a/tests/ZendTest/Form/View/Helper/FormTest.php b/tests/ZendTest/Form/View/Helper/FormTest.php
@@ -58,7 +58,7 @@ public function testCallingOpenTagWithFormUsesFormAttributes()
 
         $markup = $this->helper->openTag($form);
 
-        $escape = $this->renderer->plugin('escapehtml');
+        $escape = $this->renderer->plugin('escapehtmlattr');
         foreach ($attributes as $attribute => $value) {
             $this->assertContains(sprintf('%s="%s"', $attribute, $escape($value)), $markup);
         }