ZF-8743: StripTags no longer allows whitelisting comments

git-svn-id: http://framework.zend.com/svn/framework/standard/trunk@20136 44c647ce-9c0f-0410-b52a-842ac1e357ba

documentation/manual/en/module_specs/Zend_Filter-Set.xml

@@ -106,13 +106,11 @@
             This filter returns the input string, with all HTML and <acronym>PHP</acronym> tags
             stripped from it, except those that have been explicitly allowed. In addition to the
             ability to specify which tags are allowed, developers can specify which attributes are
-            allowed across all allowed tags and for specific tags only. Finally, this filter offers
-            control over whether comments (e.g., <command>&lt;!-- ... --&gt;</command>) are removed
-            or allowed.
+            allowed across all allowed tags and for specific tags only. 
         </para>
     </sect2>
 
 </sect1>
 <!--
 vim:se ts=4 sw=4 et:
--->
+-->

documentation/manual/en/ref/migration-110.xml

@@ -120,7 +120,7 @@ $authors = $feed->getAuthors()->getValues();
         </sect3>
     </sect2>
 
-    <sect2 id="migration.19.zend.filter.html-entities">
+    <sect2 id="migration.110.zend.filter.html-entities">
         <title>Zend_Filter_HtmlEntities</title>
 
         <para>
@@ -141,6 +141,22 @@ $authors = $feed->getAuthors()->getValues();
         </para>
     </sect2>
 
+    <sect2 id="migration.110.zend.filter.strip-tags">
+        <title>Zend_Filter_StripTags</title>
+
+        <para>
+            <classname>Zend_Filter_StripTags</classname> contains a flag,
+            <varname>commentsAllowed</varname>, that, in previous versions, allowed you to
+            optionally whitelist HTML comments in HTML text filtered by the class. However, this
+            opens code enabling the flag to <acronym>XSS</acronym> attacks, particularly in Internet
+            Explorer (which allows specifying conditional functionality via HTML comments). Starting
+            in version 1.9.7 (and backported to versions 1.8.5 and 1.7.9), the
+            <varname>commentsAllowed</varname> flag no longer has any meaning, and all HTML
+            comments, including those containing other HTML tags or nested commments, will be
+            stripped from the final output of the filter.
+        </para>
+    </sect2>
+
     <sect2 id="migration.110.zend.translate">
         <title>Zend_Translate</title>
 

documentation/manual/en/ref/migration-19.xml

@@ -397,6 +397,22 @@ $container = new Zend_Navigation(array(
             please check your code and unit tests to ensure everything still continues to work.
         </para>
     </sect2>
+
+    <sect2 id="migration.19.zend.filter.strip-tags">
+        <title>Zend_Filter_StripTags</title>
+
+        <para>
+            <classname>Zend_Filter_StripTags</classname> contains a flag,
+            <varname>commentsAllowed</varname>, that, in previous versions, allowed you to
+            optionally whitelist HTML comments in HTML text filtered by the class. However, this
+            opens code enabling the flag to <acronym>XSS</acronym> attacks, particularly in Internet
+            Explorer (which allows specifying conditional functionality via HTML comments). Starting
+            in version 1.9.7 (and backported to versions 1.8.5 and 1.7.9), the
+            <varname>commentsAllowed</varname> flag no longer has any meaning, and all HTML
+            comments, including those containing other HTML tags or nested commments, will be
+            stripped from the final output of the filter.
+        </para>
+    </sect2>
 </sect1>
 <!--
 vim:se ts=4 sw=4 et:

library/Zend/Filter/StripTags.php

@@ -45,8 +45,10 @@ class Zend_Filter_StripTags implements Zend_Filter_Interface
      *
      * If false (the default), then comments are removed from the input string.
      *
-     * @var boolean
+     * This setting is now deprecated, and ignored internally.
+     *
      * @deprecated
+     * @var boolean
      */
     public $commentsAllowed = false;
 
@@ -112,6 +114,11 @@ class Zend_Filter_StripTags implements Zend_Filter_Interface
 
     /**
      * Returns the commentsAllowed option
+     *
+     * This setting is now deprecated and ignored internally.
+     *
+     * @deprecated
+     * @return bool
      */
     public function getCommentsAllowed()
     {
@@ -121,7 +128,10 @@ class Zend_Filter_StripTags implements Zend_Filter_Interface
     /**
      * Sets the commentsAllowed option
      *
-     * @param boolean $commentsAllowed
+     * This setting is now deprecated and ignored internally.
+     *
+     * @deprecated
+     * @param  boolean $commentsAllowed
      * @return Zend_Filter_StripTags Provides a fluent interface
      */
     public function setCommentsAllowed($commentsAllowed)
@@ -227,16 +237,8 @@ class Zend_Filter_StripTags implements Zend_Filter_Interface
      */
     public function filter($value)
     {
-        $valueCopy = (string) $value;
-
-        // If comments are allowed, then replace them with unique identifiers
-        if ($this->getCommentsAllowed()) {
-            preg_match_all('/<\!--.*?--\s*>/s' , (string) $valueCopy, $matches);
-            $comments = array_unique($matches[0]);
-            foreach ($comments as $k => $v) {
-                $valueCopy = str_replace($v, self::UNIQUE_ID_PREFIX . $k, $valueCopy);
-            }
-        }
+        // Strip HTML comments first
+        $valueCopy = preg_replace('#<!--(?:[^<]+|<(?!\!--))*?(--\s*>)#us', '', (string) $value);
 
         // Initialize accumulator for filtered data
         $dataFiltered = '';
@@ -260,13 +262,6 @@ class Zend_Filter_StripTags implements Zend_Filter_Interface
             $dataFiltered .= $preTag . $tagFiltered;
         }
 
-        // If comments are allowed, then replace the unique identifiers with the corresponding comments
-        if ($this->getCommentsAllowed()) {
-            foreach ($comments as $k => $v) {
-                $dataFiltered = str_replace(self::UNIQUE_ID_PREFIX . $k, $v, $dataFiltered);
-            }
-        }
-
         // Return the filtered data
         return $dataFiltered;
     }

tests/Zend/Filter/StripTagsTest.php

@@ -405,14 +405,15 @@ class Zend_Filter_StripTagsTest extends PHPUnit_Framework_TestCase
     }
 
     /**
-     * Ensures that a comment is not removed when comments are allowed
+     * Ensures that a comment IS removed when comments are flagged as allowed
      *
+     * @group ZF-8473
      * @return void
      */
-    public function testFilterCommentAllowed()
+    public function testSpecifyingCommentsAllowedStillStripsComments()
     {
         $input    = '<!-- a comment -->';
-        $expected = '<!-- a comment -->';
+        $expected = '';
         $this->_filter->setCommentsAllowed(true);
         $this->assertEquals($expected, $this->_filter->filter($input));
     }
@@ -420,25 +421,29 @@ class Zend_Filter_StripTagsTest extends PHPUnit_Framework_TestCase
     /**
      * Ensures that a comment containing tags is untouched when comments are allowed
      *
+     * @group ZF-8473
      * @return void
      */
-    public function testFilterCommentAllowedContainsTags()
+    public function testSpecifyingCommentsAllowedStripsCommentsContainingTags()
     {
         $input    = '<!-- a comment <br /> <h1>SuperLarge</h1> -->';
-        $expected = '<!-- a comment <br /> <h1>SuperLarge</h1> -->';
+        $expected = '';
         $this->_filter->setCommentsAllowed(true);
         $this->assertEquals($expected, $this->_filter->filter($input));
     }
 
     /**
-     * Ensures expected behavior when comments are allowed and a comment contains tags and linebreaks
+     * Ensures expected behavior when comments are marked as allowed (in our 
+     * case, this should have no effect) and a comment contains tags and 
+     * linebreaks
      *
+     * @group ZF-8473
      * @return void
      */
-    public function testFilterCommentsAllowedContainsTagsLinebreaks()
+    public function testSpecifyingCommentsAllowedFiltersCommentsContainingTagsAndLinebreaks()
     {
         $input    = "<br> test <p> text </p> with <!-- comments --> and <!-- hidd\n\nen <br> -->";
-        $expected = " test  text  with <!-- comments --> and <!-- hidd\n\nen <br> -->";
+        $expected = " test  text  with  and ";
         $this->_filter->setCommentsAllowed(true);
         $this->assertEquals($expected, $this->_filter->filter($input));
     }
@@ -446,26 +451,28 @@ class Zend_Filter_StripTagsTest extends PHPUnit_Framework_TestCase
     /**
      * Ensures expected behavior when comments are allowed but nested
      *
+     * @group ZF-8473
      * @return void
      */
-    public function testFilterCommentsAllowedNested()
+    public function testSpecifyingCommentsAllowedShouldStillStripNestedComments()
     {
         $input    = '<a> <!-- <b> <!-- <c> --> <d> --> <e>';
-        $expected = ' <!-- <b> <!-- <c> -->  -- ';
+        $expected = '   ';
         $this->_filter->setCommentsAllowed(true);
         $this->assertEquals($expected, $this->_filter->filter($input));
     }
 
     /**
-     * Ensures that space is allowed between the double-hyphen '--' and the ending delimiter '>'
+     * Ensures that space between double-hyphen and closing bracket still matches as a comment delimiter
      *
+     * @group ZF-8473
      * @see    http://www.w3.org/TR/1999/REC-html401-19991224/intro/sgmltut.html#h-3.2.4
      * @return void
      */
     public function testFilterCommentsAllowedDelimiterEndingWhiteSpace()
     {
         $input    = '<a> <!-- <b> --  > <c>';
-        $expected = ' <!-- <b> --  > ';
+        $expected = '  ';
         $this->_filter->setCommentsAllowed(true);
         $this->assertEquals($expected, $this->_filter->filter($input));
     }