From 13476c844d7065d8036422ac49dbcf772b2a6208 Mon Sep 17 00:00:00 2001
From: Olivier Dony <odo@openerp.com>
Date: Sat, 7 Mar 2015 01:36:18 +0100
Subject: [PATCH] [FIX] fields.html, forum: opt-in stripping of @style attrs

For public-facing HTML content provided by the user,
`<style>` tags and `style` attributes should be stripped
automatically, as they can easily be abused to deface
pages for abusive users and spammers.
<style> tags were already stripped, the optional `strip_style`
for fields.html enables the automatic stripping of style
attributes.

This is opt-in because custom style attributes are still
desirable in trusted HTML fields.
---
 addons/website_forum/models/forum.py              | 2 +-
 addons/website_partner/models/res_partner.py      | 3 ++-
 openerp/addons/test_new_api/tests/test_related.py | 8 +++++++-
 openerp/fields.py                                 | 7 ++++++-
 openerp/osv/fields.py                             | 5 +++--
 openerp/tools/mail.py                             | 4 ++--
 6 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/addons/website_forum/models/forum.py b/addons/website_forum/models/forum.py
index 1d984edc409..876517f2e3b 100644
--- a/addons/website_forum/models/forum.py
+++ b/addons/website_forum/models/forum.py
@@ -252,7 +252,7 @@ class Post(osv.Model):
     _columns = {
         'name': fields.char('Title'),
         'forum_id': fields.many2one('forum.forum', 'Forum', required=True),
-        'content': fields.html('Content'),
+        'content': fields.html('Content', strip_style=True),
         'tag_ids': fields.many2many('forum.tag', 'forum_tag_rel', 'forum_id', 'forum_tag_id', 'Tags'),
         'state': fields.selection([('active', 'Active'), ('close', 'Close'), ('offensive', 'Offensive')], 'Status'),
         'views': fields.integer('Number of Views'),
diff --git a/addons/website_partner/models/res_partner.py b/addons/website_partner/models/res_partner.py
index 0d3c90f7ad1..3cf7cc8ef76 100644
--- a/addons/website_partner/models/res_partner.py
+++ b/addons/website_partner/models/res_partner.py
@@ -14,7 +14,8 @@ class WebsiteResPartner(osv.Model):
         'website_published': fields.boolean(
             'Publish', help="Publish on the website", copy=False),
         'website_description': fields.html(
-            'Website Partner Full Description'
+            'Website Partner Full Description',
+            strip_style=True
         ),
         'website_short_description': fields.text(
             'Website Partner Short Description'
diff --git a/openerp/addons/test_new_api/tests/test_related.py b/openerp/addons/test_new_api/tests/test_related.py
index b2b65e8b373..b7bb773c056 100644
--- a/openerp/addons/test_new_api/tests/test_related.py
+++ b/openerp/addons/test_new_api/tests/test_related.py
@@ -146,7 +146,7 @@ class TestHtmlField(common.TransactionCase):
 % if object.some_field and not object.oriented:
 <table>
     % if object.other_field:
-    <tr>
+    <tr style="border: 10px solid black;">
         ${object.mako_thing}
         <td>
     </tr>
@@ -173,5 +173,11 @@ class TestHtmlField(common.TransactionCase):
         # sanitize should have closed tags left open in the original html
         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')
+
+        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)
+        partner = self.partner.browse(cr, uid, pid, context=context)
+        self.assertNotIn('<tr style="', partner.comment, 'Style attr should have been stripped')
 
         self.partner._columns = old_columns
diff --git a/openerp/fields.py b/openerp/fields.py
index 9a66ab22f90..da357094285 100644
--- a/openerp/fields.py
+++ b/openerp/fields.py
@@ -1076,16 +1076,21 @@ class Text(_String):
 class Html(_String):
     type = 'html'
     sanitize = True                     # whether value must be sanitized
+    strip_style = False                 # whether to strip style attributes
 
     _column_sanitize = property(attrgetter('sanitize'))
     _related_sanitize = property(attrgetter('sanitize'))
     _description_sanitize = property(attrgetter('sanitize'))
 
+    _column_strip_style = property(attrgetter('strip_style'))
+    _related_strip_style = property(attrgetter('strip_style'))
+    _description_strip_style = property(attrgetter('strip_style'))
+
     def convert_to_cache(self, value, record, validate=True):
         if value is None or value is False:
             return False
         if validate and self.sanitize:
-            return html_sanitize(value)
+            return html_sanitize(value, strip_style=self.strip_style)
         return value
 
 
diff --git a/openerp/osv/fields.py b/openerp/osv/fields.py
index e99497e08dc..53ac8d9e942 100644
--- a/openerp/osv/fields.py
+++ b/openerp/osv/fields.py
@@ -317,11 +317,12 @@ class html(text):
             return None
         if not self._sanitize:
             return value
-        return html_sanitize(value)
+        return html_sanitize(value, strip_style=self._strip_style)
 
-    def __init__(self, string='unknown', sanitize=True, **args):
+    def __init__(self, string='unknown', sanitize=True, strip_style=False, **args):
         super(html, self).__init__(string=string, **args)
         self._sanitize = sanitize
+        self._strip_style = strip_style
         # symbol_set redefinition because of sanitize specific behavior
         self._symbol_f = self._symbol_set_html
         self._symbol_set = (self._symbol_c, self._symbol_f)
diff --git a/openerp/tools/mail.py b/openerp/tools/mail.py
index 5f620eb61d8..f62356e2068 100644
--- a/openerp/tools/mail.py
+++ b/openerp/tools/mail.py
@@ -53,7 +53,7 @@ safe_attrs = clean.defs.safe_attrs | frozenset(
      ])
 
 
-def html_sanitize(src, silent=True, strict=False):
+def html_sanitize(src, silent=True, strict=False, strip_style=False):
     if not src:
         return src
     src = ustr(src, errors='replace')
@@ -69,7 +69,7 @@ def html_sanitize(src, silent=True, strict=False):
 
     kwargs = {
         'page_structure': True,
-        'style': False,             # do not remove style attributes
+        'style': strip_style,       # True = remove style tags/attrs
         'forms': True,              # remove form tags
         'remove_unknown_tags': False,
         'allow_tags': allowed_tags,