From bfe7aafa7f69671219464f35ba56e34c6f1eb12f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= <tde@openerp.com>
Date: Wed, 2 Mar 2016 13:58:46 +0100
Subject: [PATCH] [IMP] tools: mail: parse style in sanitizer

Style attribute is now parsed and sanitized. Only a while list of accepted
properties is kept. The behavior is implemented directly in the cleaner
itself. As there is no easy possible inheritance, the styling cleaning is
appended directly after the legacy processing in __call__.

The styling sanitizer is called only if the style attribute is kept. If
the sanitizer is called with strip_style=True, the styling is removed
and therefore no sanitizing is performed.

Tests about html fields have been updated. Indeed the styling is now sanitized
and the test was not correct anymore, as the test html was stripped. It now
contains styling that is kept. The strip_classes is also tested.
---
 openerp/addons/base/tests/test_mail.py        | 43 ++++++++++++++++---
 .../addons/test_new_api/tests/test_related.py | 19 +++++++-
 openerp/tools/mail.py                         | 35 +++++++++++++++
 3 files changed, 90 insertions(+), 7 deletions(-)

diff --git a/openerp/addons/base/tests/test_mail.py b/openerp/addons/base/tests/test_mail.py
index 05457728c80..d4391529d3a 100644
--- a/openerp/addons/base/tests/test_mail.py
+++ b/openerp/addons/base/tests/test_mail.py
@@ -116,13 +116,45 @@ class TestSanitizer(unittest.TestCase):
                 sanitized,
                 'html_sanitize escaped valid address-like')
 
+    def test_style_parsing(self):
+        test_data = [
+            (
+                '<span style="position: fixed; top: 0px; left: 50px; width: 40%; height: 50%; background-color: red;">Coin coin </span>',
+                ['background-color: red', 'Coin coin'],
+                ['position', 'top', 'left']
+            ), (
+                """<div style='before: "Email Address; coincoin cheval: lapin";  
+   font-size: 30px; max-width: 100%; after: "Not sure
+    
+          this; means: anything ?#ùµ"
+    ; some-property: 2px; top: 3'>youplaboum</div>""",
+                ['font-size: 30px', 'youplaboum'],
+                ['some-property', 'top', 'cheval']
+            ), (
+                '<span style="width">Coincoin</span>',
+                [],
+                ['width']
+            )
+        ]
+
+        for test, in_lst, out_lst in test_data:
+            new_html = html_sanitize(test, strict=False, strip_style=False, strip_classes=False)
+            for text in in_lst:
+                self.assertIn(text, new_html)
+            for text in out_lst:
+                self.assertNotIn(text, new_html)
+
+        # style should not be sanitized if removed
+        new_html = html_sanitize(test_data[0][0], strict=False, strip_style=True, strip_classes=False)
+        self.assertEqual(new_html, u'<span>Coin coin </span>')
+
     def test_edi_source(self):
         html = html_sanitize(test_mail_examples.EDI_LIKE_HTML_SOURCE)
-        self.assertIn('div style="font-family: \'Lucida Grande\', Ubuntu, Arial, Verdana, sans-serif; font-size: 12px; color: rgb(34, 34, 34); background-color: #FFF;', html,
-            'html_sanitize removed valid style attribute')
-        self.assertIn('<span style="color: #222; margin-bottom: 5px; display: block; ">', html,
-            'html_sanitize removed valid style attribute')
-        self.assertIn('img class="oe_edi_paypal_button" src="https://www.paypal.com/en_US/i/btn/btn_paynowCC_LG.gif"', html,
+        self.assertIn(
+            'font-family: \'Lucida Grande\', Ubuntu, Arial, Verdana, sans-serif;', html,
+            'html_sanitize removed valid styling')
+        self.assertIn(
+            'src="https://www.paypal.com/en_US/i/btn/btn_paynowCC_LG.gif"', html,
             'html_sanitize removed valid img')
         self.assertNotIn('</body></html>', html, 'html_sanitize did not remove extra closing tags')
 
@@ -404,5 +436,6 @@ class TestEmailTools(unittest.TestCase):
         for text, expected in cases:
             self.assertEqual(email_split(text), expected, 'email_split is broken')
 
+
 if __name__ == '__main__':
     unittest.main()
diff --git a/openerp/addons/test_new_api/tests/test_related.py b/openerp/addons/test_new_api/tests/test_related.py
index 35508a6e44c..f7fa58847a8 100644
--- a/openerp/addons/test_new_api/tests/test_related.py
+++ b/openerp/addons/test_new_api/tests/test_related.py
@@ -138,10 +138,13 @@ class TestHtmlField(common.TransactionCase):
 % if object.some_field and not object.oriented:
 <table>
     % if object.other_field:
-    <tr style="border: 10px solid black;">
+    <tr style="margin: 0px; border: 10px solid black;">
         ${object.mako_thing}
         <td>
     </tr>
+    <tr class="custom_class">
+        This is some html.
+    </tr>
     % endif
     <tr>
 %if object.dummy_field:
@@ -156,7 +159,17 @@ class TestHtmlField(common.TransactionCase):
         self.assertEqual(partner.comment, some_ugly_html, 'Error in HTML field: content was sanitized but field has sanitize=False')
 
         self.partner._columns.update({
-            'comment': fields.html('Unsecure Html', sanitize=True),
+            'comment': fields.html('Unsecure Html', sanitize=True, strip_classes=False),
+        })
+        self.partner.write(cr, uid, [pid], {
+            'comment': some_ugly_html,
+        }, context=context)
+        partner = self.partner.browse(cr, uid, pid, context=context)
+        # classes are kept
+        self.assertIn('<tr class="', partner.comment)
+
+        self.partner._columns.update({
+            'comment': fields.html('Unsecure Html', sanitize=True, strip_classes=True),
         })
         self.partner.write(cr, uid, [pid], {
             'comment': some_ugly_html,
@@ -166,6 +179,8 @@ class TestHtmlField(common.TransactionCase):
         self.assertIn('</table>', partner.comment, 'Error in HTML field: content does not seem to have been sanitized despise sanitize=True')
         self.assertIn('</td>', partner.comment, 'Error in HTML field: content does not seem to have been sanitized despise sanitize=True')
         self.assertIn('<tr style="', partner.comment, 'Style attr should not have been stripped')
+        # sanitize does not keep classes if asked to
+        self.assertNotIn('<tr class="', partner.comment)
 
         self.partner._columns['comment'] = fields.html('Stripped Html', sanitize=True, strip_style=True)
         self.partner.write(cr, uid, [pid], {'comment': some_ugly_html}, context=context)
diff --git a/openerp/tools/mail.py b/openerp/tools/mail.py
index a020d6d72b2..cd509f4a797 100644
--- a/openerp/tools/mail.py
+++ b/openerp/tools/mail.py
@@ -38,11 +38,46 @@ safe_attrs = clean.defs.safe_attrs | frozenset(
 
 
 class _Cleaner(clean.Cleaner):
+
+    _style_re = re.compile('''([\w-]+)\s*:\s*((?:[^;"']|"[^"]*"|'[^']*')+)''')
+
+    _style_whitelist = [
+        'font-size', 'font-family', 'background-color', 'color', 'text-align',
+        'padding', 'padding-top', 'padding-left', 'padding-bottom', 'padding-right',
+        'margin', 'margin-top', 'margin-left', 'margin-bottom', 'margin-right'
+        # box model
+        'border', 'border-color', 'border-radius', 'height', 'margin', 'padding', 'width', 'max-width', 'min-width',
+        # tables
+        'border-collapse', 'border-spacing', 'caption-side', 'empty-cells', 'table-layout']
+
+    def __call__(self, doc):
+        super(_Cleaner, self).__call__(doc)
+
+        # if we keep style attribute, sanitize them
+        if not self.style:
+            for el in doc.iter():
+                self.parse_style(el)
+
+    def parse_style(self, el):
+        attributes = el.attrib
+        styling = attributes.get('style')
+        if styling:
+            valid_styles = {}
+            styles = self._style_re.findall(styling)
+            for style in styles:
+                if style[0].lower() in self._style_whitelist:
+                    valid_styles[style[0].lower()] = style[1]
+            if valid_styles:
+                el.attrib['style'] = '; '.join('%s: %s' % (key, val) for (key, val) in valid_styles.iteritems())
+            else:
+                del el.attrib['style']
+
     def allow_element(self, el):
         if el.tag == 'object' and el.get('type') == "image/svg+xml":
             return True
         return super(_Cleaner, self).allow_element(el)
 
+
 def html_sanitize(src, silent=True, strict=False, strip_style=False, strip_classes=False):
     if not src:
         return src