Adding tests/fixes for possible XSS injections in view helpers

Signed-off-by: Marco Pivetta <ocramius@gmail.com>
zendframework · Apr 15, 2014 · fd43a95 · fd43a95
1 parent 0a1edd7
commit fd43a95
Show file tree

Hide file tree

Showing 16 changed files with 80 additions and 32 deletions.
diff --git a/src/View/Helper/AbstractHelper.php b/src/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/test/View/Helper/AbstractHelperTest.php b/test/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/test/View/Helper/Captcha/DumbTest.php b/test/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/test/View/Helper/Captcha/FigletTest.php b/test/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/test/View/Helper/Captcha/ImageTest.php b/test/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/test/View/Helper/CommonTestCase.php b/test/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/test/View/Helper/FormCaptchaTest.php b/test/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/test/View/Helper/FormCollectionTest.php b/test/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/test/View/Helper/FormFileTest.php b/test/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/test/View/Helper/FormInputTest.php b/test/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/test/View/Helper/FormMultiCheckboxTest.php b/test/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/test/View/Helper/FormResetTest.php b/test/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/test/View/Helper/FormRowTest.php b/test/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/test/View/Helper/FormSelectTest.php b/test/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/test/View/Helper/FormSubmitTest.php b/test/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/test/View/Helper/FormTest.php b/test/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);
         }