From cf844e34dd0ce4830eb99fd0fa5b6b9cb58c867c Mon Sep 17 00:00:00 2001 From: Romain Derie Date: Wed, 27 Jul 2022 17:41:13 +0000 Subject: [PATCH] [IMP] core: allow some users to bypass the sanitize of HTML field Add the possibility to flag a HTML field as `sanitize_overridable`. The sanitizer will then be bypassed if the user doing the operation is part of the new group `base.group_sanitize_override`. The "Settings" users are part of that new group. If such a user wrote some HTML that would have normally been removed by the sanitizer, then users without the right to bypass the sanitizer won't be able to write on that field anymore. Otherwise, it would sanitize the previously written data. Such cases are detected, and the modification prevented by the system, which will warn the user about it. For instance, with a `field.html(sanitize_overridable=True)`: one being part of `group_sanitize_override` could write ``. Then, someone not part of the group trying to add an element inside that field like appending a `

` -> `

New Content

` would not be able to because it would go through the sanitizer and ultimately, removing part of the original value: `

New Content

` (`` would be removed). == Real use case == In the website builder, there is 2 editor rights: - group_website_publisher: restricted editor - group_website_designer: editor & designer The designer editor can edit pages and views, while the restricted editor can't do anything unless he is part of other groups. In edit mode, the restricted editor will only be able to edit fields of record he has access to. For instance, being a sales manager allows you to edit a product description on the website. Being an event manager -> edit event. Slide manager -> Slides etc. As those restricted editor are able to edit those fields, they are (almost) always sanitized to prevent them to introduce malicious code. Since those fields are sanitized, even admins / designer editor are not able to fully use the website builder in such fields. Some clients don't really care about that sanitation, they'd prefer to avoid it as they trust their manager and would prefer to have the full builder capability instead. This is typically the case in small project (butcher, hairdresser, reseller etc) and in SMEs. With the new `sanitize_overridable` feature, they will be able to do that, as the "Designer & Editor" group now also receive the group `base.group_sanitize_override` (done in next commit). Part-of: odoo/odoo#97398 --- addons/web_editor/models/ir_qweb_fields.py | 2 +- odoo/addons/base/security/base_groups.xml | 6 +- .../test_new_api/models/test_new_api.py | 1 + .../test_new_api/tests/test_new_fields.py | 49 ++++++++++++++ odoo/fields.py | 67 +++++++++++++------ 5 files changed, 101 insertions(+), 24 deletions(-) diff --git a/addons/web_editor/models/ir_qweb_fields.py b/addons/web_editor/models/ir_qweb_fields.py index b121e83e086..3e8c2158e33 100644 --- a/addons/web_editor/models/ir_qweb_fields.py +++ b/addons/web_editor/models/ir_qweb_fields.py @@ -380,7 +380,7 @@ class HTML(models.AbstractModel): attrs = super().attributes(record, field_name, options, values) if options.get('inherit_branding'): field = record._fields[field_name] - if field.sanitize: + if field.sanitize and not (field.sanitize_overridable and record.user_has_groups('base.group_sanitize_override')): attrs['data-oe-sanitize'] = 1 if field.sanitize_form else 'allow_form' return attrs diff --git a/odoo/addons/base/security/base_groups.xml b/odoo/addons/base/security/base_groups.xml index cf53bcda031..44a102fce50 100644 --- a/odoo/addons/base/security/base_groups.xml +++ b/odoo/addons/base/security/base_groups.xml @@ -12,9 +12,13 @@ + + Bypass HTML Field Sanitize + + Settings - + diff --git a/odoo/addons/test_new_api/models/test_new_api.py b/odoo/addons/test_new_api/models/test_new_api.py index cbe2be2d055..d22e99e13a9 100644 --- a/odoo/addons/test_new_api/models/test_new_api.py +++ b/odoo/addons/test_new_api/models/test_new_api.py @@ -315,6 +315,7 @@ class MixedModel(models.Model): comment2 = fields.Html(sanitize_attributes=True, strip_classes=False) comment3 = fields.Html(sanitize_attributes=True, strip_classes=True) comment4 = fields.Html(sanitize_attributes=True, strip_style=True) + comment5 = fields.Html(sanitize_overridable=True, sanitize_attributes=False) currency_id = fields.Many2one('res.currency', default=lambda self: self.env.ref('base.EUR')) amount = fields.Monetary() diff --git a/odoo/addons/test_new_api/tests/test_new_fields.py b/odoo/addons/test_new_api/tests/test_new_fields.py index 34f219cdc25..05f78fadb49 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -2969,6 +2969,55 @@ class TestHtmlField(common.TransactionCase): self.assertNotIn('é@ 

' + write_vals = {'comment5': val} + # Once sent through `html_sanitize()` this is becoming: + # `é@\xa0

` + # Notice those change: + # - `attr1 =` -> `attr1=` (space before `=`) + # - ` attr2` -> ` attr2` (multi space -> single space) + # - `=\'attr2\'` -> `="attr2"` (escaped single quote -> double quote) + # - ` ` -> `\xa0` + # Still, those 2 archs should be considered equals and not raise + + record.with_user(bypass_user).write(write_vals) + # Next write shouldn't raise a sanitize right error + record.with_user(internal_user).write(write_vals) + class TestMagicFields(common.TransactionCase): diff --git a/odoo/fields.py b/odoo/fields.py index 8a1fe92e5cf..6cf1ce83149 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -5,6 +5,7 @@ from collections import defaultdict from datetime import date, datetime, time +from lxml import etree, html from operator import attrgetter from xmlrpc.client import MAXINT import base64 @@ -1817,6 +1818,8 @@ class Html(_String): """ Encapsulates an html code content. :param bool sanitize: whether value must be sanitized (default: ``True``) + :param bool sanitize_overridable: whether the sanitation can be bypassed by + the users part of the `base.group_sanitize_override` group (default: ``False``) :param bool sanitize_tags: whether to sanitize tags (only a white list of attributes is accepted, default: ``True``) :param bool sanitize_attributes: whether to sanitize attributes @@ -1831,6 +1834,7 @@ class Html(_String): column_cast_from = ('varchar',) sanitize = True # whether value must be sanitized + sanitize_overridable = False # whether the sanitation can be bypassed by the users part of the `base.group_sanitize_override` group sanitize_tags = True # whether to sanitize tags (only a white list of attributes is accepted) sanitize_attributes = True # whether to sanitize attributes (only a white list of attributes is accepted) sanitize_style = False # whether to sanitize style attributes @@ -1861,32 +1865,51 @@ class Html(_String): _description_strip_classes = property(attrgetter('strip_classes')) def convert_to_column(self, value, record, values=None, validate=True): - if value is None or value is False: - return None - if self.sanitize: - return html_sanitize( - value, silent=True, - sanitize_tags=self.sanitize_tags, - sanitize_attributes=self.sanitize_attributes, - sanitize_style=self.sanitize_style, - sanitize_form=self.sanitize_form, - strip_style=self.strip_style, - strip_classes=self.strip_classes) - return value + return self._convert(value, record, True) def convert_to_cache(self, value, record, validate=True): + return self._convert(value, record, validate) + + def _convert(self, value, record, validate): if value is None or value is False: return None - if validate and self.sanitize: - return html_sanitize( - value, silent=True, - sanitize_tags=self.sanitize_tags, - sanitize_attributes=self.sanitize_attributes, - sanitize_style=self.sanitize_style, - sanitize_form=self.sanitize_form, - strip_style=self.strip_style, - strip_classes=self.strip_classes) - return value + + if not validate or not self.sanitize: + return value + + sanitize_vals = { + 'silent': True, + 'sanitize_tags': self.sanitize_tags, + 'sanitize_attributes': self.sanitize_attributes, + 'sanitize_style': self.sanitize_style, + 'sanitize_form': self.sanitize_form, + 'strip_style': self.strip_style, + 'strip_classes': self.strip_classes + } + + if self.sanitize_overridable: + if record.user_has_groups('base.group_sanitize_override'): + return value + + original_value = record[self.name] + if original_value: + initial_value_sanitized = html_sanitize(original_value, **sanitize_vals) + + def get_parsed(val): + return etree.tostring(html.fromstring(val)) + + # could have been emptied by the sanitizer + if ( + not initial_value_sanitized + or get_parsed(original_value) != get_parsed(initial_value_sanitized) + ): + # The field contains element(s) that would be removed if + # sanitized. It means that someone who was part of a group + # allowing to bypass the sanitation saved that field + # previously. + raise UserError(_("Someone with escalated rights previously modified this field (%s %s), you are therefore not able to modify it yourself.", record._description, self.string)) + + return html_sanitize(value, **sanitize_vals) def convert_to_record(self, value, record): r = super().convert_to_record(value, record)