Fix inline diff for rich text fields after PloneHotfix20210518.

The first version of the hotfix escaped all html.
Now for the rich text field, use the safe html transform, otherwise the inline diff is no longer inline.
This may be added in a new version of the hotfix as well.

Products/CMFDiffTool/BinaryDiff.py

@@ -3,7 +3,7 @@
 from os import linesep
 from Products.CMFDiffTool.BaseDiff import _getValue
 from Products.CMFDiffTool.FieldDiff import FieldDiff
-from Products.CMFDiffTool.utils import html_encode
+from Products.CMFDiffTool.utils import html_escape
 
 
 class BinaryDiff(FieldDiff):
@@ -51,8 +51,8 @@ def inline_diff(self):
         if self.oldFilename != self.newFilename:
             html.append(
                 self.inlinediff_fmt % (css_class,
-                                       self.filenameTitle(html_encode(self.oldFilename)),
-                                       self.filenameTitle(html_encode(self.newFilename))),
+                                       self.filenameTitle(html_escape(self.oldFilename)),
+                                       self.filenameTitle(html_escape(self.newFilename))),
             )
 
         if html:

Products/CMFDiffTool/CMFDTHtmlDiff.py

@@ -2,7 +2,7 @@
 from AccessControl.class_init import InitializeClass
 from Products.CMFDiffTool.libs import htmldiff
 from Products.CMFDiffTool.TextDiff import TextDiff
-from Products.CMFDiffTool.utils import html_encode
+from Products.CMFDiffTool.utils import html_safe
 
 
 # Give it a dumb name so it doesn't conflict with all the other html diffs
@@ -19,7 +19,7 @@ def inline_diff(self):
                                        filename=self.oldFilename))
         b = '\n'.join(self._parseField(self.newValue,
                                        filename=self.newFilename))
-        return htmldiff.htmldiff(html_encode(a), html_encode(b))
+        return htmldiff.htmldiff(html_safe(a), html_safe(b))
 
     def _parseField(self, value, filename=None):
         """Use the field's raw value if available."""

Products/CMFDiffTool/FieldDiff.py

@@ -2,7 +2,7 @@
 from AccessControl.class_init import InitializeClass
 from Products.CMFDiffTool.BaseDiff import _getValue
 from Products.CMFDiffTool.BaseDiff import BaseDiff
-from Products.CMFDiffTool.utils import html_encode
+from Products.CMFDiffTool.utils import html_escape
 from six.moves import range
 
 import difflib
@@ -81,18 +81,18 @@ def inline_diff(self):
         for tag, alo, ahi, blo, bhi in self.getLineDiffs():
             if tag == 'replace':
                 for i in range(alo, ahi):
-                    r.append(inlinediff_fmt % (css_class, html_encode(a[i]), ''))
+                    r.append(inlinediff_fmt % (css_class, html_escape(a[i]), ''))
                 for i in range(blo, bhi):
-                    r.append(inlinediff_fmt % (css_class, '', html_encode(b[i])))
+                    r.append(inlinediff_fmt % (css_class, '', html_escape(b[i])))
             elif tag == 'delete':
                 for i in range(alo, ahi):
-                    r.append(inlinediff_fmt % (css_class, html_encode(a[i]), ''))
+                    r.append(inlinediff_fmt % (css_class, html_escape(a[i]), ''))
             elif tag == 'insert':
                 for i in range(blo, bhi):
-                    r.append(inlinediff_fmt % (css_class, '', html_encode(b[i])))
+                    r.append(inlinediff_fmt % (css_class, '', html_escape(b[i])))
             elif tag == 'equal':
                 for i in range(alo, ahi):
-                    r.append(same_fmt % (css_class, html_encode(a[i])))
+                    r.append(same_fmt % (css_class, html_escape(a[i])))
             else:
                 raise ValueError('unknown tag "%s"' % tag)
         return '\n'.join(r)

Products/CMFDiffTool/ListDiff.py

@@ -4,7 +4,7 @@
 from Products.CMFDiffTool.choicediff import get_field_object
 from Products.CMFDiffTool.choicediff import title_or_value
 from Products.CMFDiffTool.FieldDiff import FieldDiff
-from Products.CMFDiffTool.utils import html_encode
+from Products.CMFDiffTool.utils import html_escape
 from six.moves import range
 
 
@@ -89,34 +89,34 @@ def inline_diff(self):
             if tag == 'replace':
                 for i in range(alo, ahi):
                     obj = self.oldValue[i]
-                    obj_title = html_encode(obj.Title())
+                    obj_title = html_escape(obj.Title())
                     obj_url = obj.absolute_url()
                     r.append(inlinediff_fmt %
                              (css_class, 'diff_sub', obj_url, obj_title))
                 for i in range(blo, bhi):
                     obj = self.newValue[i]
-                    obj_title = html_encode(obj.Title())
+                    obj_title = html_escape(obj.Title())
                     obj_url = obj.absolute_url()
                     r.append(inlinediff_fmt %
                              (css_class, 'diff_add', obj_url, obj_title))
             elif tag == 'delete':
                 for i in range(alo, ahi):
                     obj = self.oldValue[i]
-                    obj_title = html_encode(obj.Title())
+                    obj_title = html_escape(obj.Title())
                     obj_url = obj.absolute_url()
                     r.append(inlinediff_fmt %
                              (css_class, 'diff_sub', obj_url, obj_title))
             elif tag == 'insert':
                 for i in range(blo, bhi):
                     obj = self.newValue[i]
-                    obj_title = html_encode(obj.Title())
+                    obj_title = html_escape(obj.Title())
                     obj_url = obj.absolute_url()
                     r.append(inlinediff_fmt %
                              (css_class, 'diff_add', obj_url, obj_title))
             elif tag == 'equal':
                 for i in range(alo, ahi):
                     obj = self.oldValue[i]
-                    obj_title = html_encode(obj.Title())
+                    obj_title = html_escape(obj.Title())
                     obj_url = obj.absolute_url()
                     r.append(same_fmt % (css_class, obj_url, obj_title))
             else:

Products/CMFDiffTool/TextDiff.py

@@ -3,7 +3,7 @@
 from os import linesep
 from Products.CMFDiffTool import CMFDiffToolMessageFactory as _
 from Products.CMFDiffTool.FieldDiff import FieldDiff
-from Products.CMFDiffTool.utils import html_encode
+from Products.CMFDiffTool.utils import html_escape
 from Products.CMFDiffTool.utils import safe_unicode
 from Products.CMFDiffTool.utils import safe_utf8
 from zope.component.hooks import getSite
@@ -88,11 +88,11 @@ def inline_diff(self):
         if old_fname != new_fname:
             html.append(
                 self.inlinediff_fmt % ('%s FilenameDiff' % css_class,
-                                       html_encode(old_fname), html_encode(new_fname)),
+                                       html_escape(old_fname), html_escape(new_fname)),
             )
         if a != b:
             html.append(
-                self.inlinediff_fmt % (css_class, html_encode(a), html_encode(b)),
+                self.inlinediff_fmt % (css_class, html_escape(a), html_escape(b)),
             )
         if html:
             return linesep.join(html)

Products/CMFDiffTool/namedfile.py

@@ -4,7 +4,7 @@
 from Products.CMFDiffTool.BinaryDiff import BinaryDiff
 from Products.CMFDiffTool.ListDiff import ListDiff
 from Products.CMFDiffTool.TextDiff import TextDiff
-from Products.CMFDiffTool.utils import html_encode
+from Products.CMFDiffTool.utils import html_escape
 
 
 FILE_FIELD_TYPES = []
@@ -71,7 +71,7 @@ def inline_diff(self):
         old = self._parseField(self.oldValue, self.oldFilename)[0]
         new = self._parseField(self.newValue, self.newFilename)[0]
 
-        return '' if self.same else self.inlinediff_fmt % (css_class, html_encode(old), html_encode(new))
+        return '' if self.same else self.inlinediff_fmt % (css_class, html_escape(old), html_escape(new))
 
 
 InitializeClass(NamedFileBinaryDiff)
@@ -137,9 +137,9 @@ def is_same_dict(d1, d2):
             )
 
         return '\n'.join([
-            ((self.same_fmt % (css_class, html_encode(d_old['repr'])))
+            ((self.same_fmt % (css_class, html_escape(d_old['repr'])))
              if is_same_dict(d_old, d_new) else self.inlinediff_fmt
-             % (css_class, html_encode(d_old['repr']), html_encode(d_new['repr']))
+             % (css_class, html_escape(d_old['repr']), html_escape(d_new['repr']))
              ) for (d_old, d_new) in zip(old_data, new_data)])
 
 

Products/CMFDiffTool/tests/test_richtextdiff.py

@@ -1,4 +1,5 @@
 # -*- coding: utf-8 -*-
+from plone.app.testing import PLONE_INTEGRATION_TESTING
 from plone.app.textfield.value import RichTextValue
 from Products.CMFDiffTool.CMFDTHtmlDiff import CMFDTHtmlDiff
 from Products.CMFDiffTool.interfaces import IDifference
@@ -13,6 +14,7 @@ def __init__(self, body):
 
 class RichTextDiffTestCase(unittest.TestCase):
     """Test RichTextDiff"""
+    layer = PLONE_INTEGRATION_TESTING
 
     def test_parseField_value_is_none(self):
         value = None
@@ -37,9 +39,9 @@ def test_inline_diff_same_hacker(self):
         value = RichTextValue(u'<script>alert("Hacker value")</script>')
         diff = CMFDTHtmlDiff(DummyType(value), DummyType(value), 'body')
         inline_diff = diff.inline_diff()
-        # The script tag should be escaped.
+        # The script tag should not be escaped, but totally not shown.
         self.assertNotIn("<script", inline_diff)
-        self.assertIn("&gt;", inline_diff)
+        self.assertNotIn("&gt;", inline_diff)
 
     def test_inline_diff_different(self):
         old_value = RichTextValue(u'foo')
@@ -58,14 +60,14 @@ def test_inline_diff_different_hacker(self):
         new_value = RichTextValue(u'<script>alert("Hacker value")</script>')
         diff = CMFDTHtmlDiff(DummyType(old_value), DummyType(new_value), 'body')
         inline_diff = diff.inline_diff()
-        # The script tag should be escaped.
+        # The script tag should not be escaped, but totally not shown.
         self.assertNotIn("<script", inline_diff)
-        self.assertIn("&gt;", inline_diff)
+        self.assertNotIn("&gt;", inline_diff)
 
         old_value = RichTextValue(u'<script>alert("Hacker value")</script>')
         new_value = RichTextValue(u'clean')
         diff = CMFDTHtmlDiff(DummyType(old_value), DummyType(new_value), 'body')
         inline_diff = diff.inline_diff()
-        # The script tag should be escaped.
+        # The script tag should not be escaped, but totally not shown.
         self.assertNotIn("<script", inline_diff)
-        self.assertIn("&gt;", inline_diff)
+        self.assertNotIn("&gt;", inline_diff)

Products/CMFDiffTool/utils.py

@@ -1,4 +1,6 @@
 # -*- coding: utf-8 -*-
+from Products.PortalTransforms.data import datastream
+from Products.PortalTransforms.transforms.safe_html import SafeHTML
 
 import six
 
@@ -23,18 +25,41 @@ def safe_utf8(value):
     return safe_unicode(value).encode('utf-8')
 
 
-# In both Python 2 and 3, the escape function cannot handle a non string-like value,
-# for example an integer.  Seems good to always return a string-like value though.
+def scrub_html(value):
+    # Strip illegal HTML tags from string text.
+    transform = SafeHTML()
+    # Available in Plone 5.2:
+    # return transform.scrub_html(value)
+    data = datastream("text/x-html-safe")
+    data = transform.convert(value, data)
+    return data.getData()
+
+
+# We will have two functions:
+# - html_escape: escape html, for example turn '<' into '&lt;'
+# - html_safe: return html with dangerous tags removed, using safe html transform.
+#
+# In both Python 2 and 3, the convert function that we use in safe_html
+# cannot handle a non string-like value, for example an integer.
+# Same is true for the escape function.
+# Seems good to always return a string-like value though.
 # But should that be bytes or string or unicode?
 if six.PY2:
     # We use this in places where the result gets inserted in a string/bytes,
     # so we should use a string (utf-8) here.
-    def html_encode(value):
+    def html_escape(value):
         value = safe_utf8(value)
         return escape(value, 1)
+
+    def html_safe(value):
+        value = safe_utf8(value)
+        return scrub_html(value)
 else:
-    # In Python 3 this gets inserted in a string/text,
-    # and escape cannot handle a bytes value.
-    def html_encode(value):
+    # In Python 3 this gets inserted in a string/text.
+    def html_escape(value):
         value = safe_unicode(value)
         return escape(value, 1)
+
+    def html_safe(value):
+        value = safe_unicode(value)
+        return scrub_html(value)

news/39.bugfix

@@ -1,3 +1,5 @@
-Added XSS fix from PloneHotfix20210518.
+Added XSS fix from PloneHotfix20210518 for inline diff.
 See `vulnerability <https://plone.org/security/hotfix/20210518/xss-vulnerability-in-cmfdifftool>`_.
+The first version of the hotfix escaped all html.
+Now for the rich text field, use the safe html transform, otherwise the inline diff is no longer inline.
 [maurits]