From fff31b316f5abd314f5a3eba291fc0e95aedbb71 Mon Sep 17 00:00:00 2001 From: Romain Derie Date: Mon, 16 Jan 2023 12:06:50 +0000 Subject: [PATCH] [FIX] core: extract normalize from HTML sanitizer Since [1], it's possible to conditionnally bypass the HTML sanitizer in field definition with the `sanitize_overridable` attribute. In a nutshell, when someone is part of the required group(s), it won't go through the sanitizer, while the people not part of the group(s) will. A behavior was thus introcuded to prevent a "restricted" user to wipe the changes done previously by an "elevated" user (which bypassed the sanitizer). But that behavior was not correct as there was unforeseen cases which led to raise this error which are not due to the sanitizer but to normalization. Indeed, while named `html_sanitize()`, it also does some normalize stuff on top of the real sanitize part. For instance, there is also (not exhaustive): - some MAKO compatibility, replacing some chars - special case for quotes, related to mail clients, which will add data attributes, add nodes in dom etc. This happen when the following are found: - `
` tag - text-based quotes (>, >>) and signatures (-- Signature) - html signature (--
blah) - some editor compatibility which removed the wrapping `
` element - `nbsp` handling.. See commit list below for detail about how/when/why those normalize cases where introduced. At the end, the issue was that the normalize part should not prevent a "restricted" user to modify the content of an "elevated" user. Only the sanitize part should. For instance, the `Quotes` snippet dropped by an "elevated" user was preventing further edition by a "restricted" user because there was a "false positive" raised when checking if the save would wipe the existing changes. Indeed, when the "elevated" user droped the snippet, it was saved as: ```html
``` But when the "restricted" user then wanted to do some changes, it would become: ```html
``` Same for `Share` snippet: ```html ``` [1]: https://github.com/odoo/odoo/commit/cf844e34dd0ce4830eb99fd0fa5b6b9cb58c867c Normalize commit list: https://github.com/odoo/odoo/commit/5f1ec49ecdac6d72cd42755c41fbe75d6a1f3587 https://github.com/odoo/odoo/commit/69af79ff3d705d19a71ba3ba7851b981cb301077 https://github.com/odoo/odoo/commit/2bcf4cca79a57dfba84d1f3e3fa7b8908bfe66e8 https://github.com/odoo/odoo/commit/f5688cd8fd515d1b668e8eb1d74de68faa681a01 https://github.com/odoo/odoo/commit/cb8c2d2b7e15c7c16e02d078767e27a07e5012c6 https://github.com/odoo/odoo/commit/275ee5825d38841a3eb21bb195722f3ceed09005 https://github.com/odoo/odoo/commit/b51d21c5b83b88e8d56dbbbb7600bcbe554d1b07 closes odoo/odoo#110903 X-original-commit: 3a2e82cf40f3265650b4f79f9ad5fe309906311d Signed-off-by: Romain Derie (rde) --- .../test_new_api/tests/test_new_fields.py | 27 +++- odoo/fields.py | 10 +- odoo/tools/mail.py | 134 +++++++++++------- 3 files changed, 116 insertions(+), 55 deletions(-) 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 94c6269d160..94a88355312 100644 --- a/odoo/addons/test_new_api/tests/test_new_fields.py +++ b/odoo/addons/test_new_api/tests/test_new_fields.py @@ -3045,7 +3045,23 @@ class TestHtmlField(common.TransactionCase): }) record = self.env['test_new_api.mixed'].create({}) - # 1. Test main use case: prevent restricted user to wipe non restricted + # 1. Test normalize case: diff due to normalize should not prevent the + # changes + val = '
Something
' + normalized_val = '
Something
' + write_vals = {'comment5': val} + + record.with_user(internal_user).write(write_vals) + self.assertEqual(record.comment5, normalized_val, + "should be normalized (not in groups)") + record.with_user(bypass_user).write(write_vals) + self.assertEqual(record.comment5, val, + "should not be normalized (has group)") + record.with_user(internal_user).write(write_vals) + self.assertEqual(record.comment5, normalized_val, + "should be normalized (not in groups) despite admin previous diff") + + # 2. Test main use case: prevent restricted user to wipe non restricted # user previous change val = '' write_vals = {'comment5': val} @@ -3061,7 +3077,7 @@ class TestHtmlField(common.TransactionCase): # other user that bypassed the sanitize) record.with_user(internal_user).write(write_vals) - # 2. Make sure field compare in `_convert` is working as expected with + # 3. Make sure field compare in `_convert` is working as expected with # special content / format val = 'é@ 

' write_vals = {'comment5': val} @@ -3078,6 +3094,13 @@ class TestHtmlField(common.TransactionCase): # Next write shouldn't raise a sanitize right error record.with_user(internal_user).write(write_vals) + # 4. Ensure our exception handling is fine + val = '' + write_vals = {'comment5': val} + record.with_user(internal_user).write(write_vals) + self.assertEqual(record.comment5, '', + "should be sanitized (not in groups)") + class TestMagicFields(common.TransactionCase): diff --git a/odoo/fields.py b/odoo/fields.py index bc3e235a431..17bb3750cc0 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -27,9 +27,10 @@ from difflib import get_close_matches from hashlib import sha256 from .tools import ( - float_repr, float_round, float_compare, float_is_zero, html_sanitize, human_size, + float_repr, float_round, float_compare, float_is_zero, human_size, pg_varchar, ustr, OrderedSet, pycompat, sql, date_utils, unique, image_process, merge_sequences, SQL_ORDER_BY_TYPE, is_list_of, has_list_types, + html_normalize, html_sanitize, ) from .tools import DEFAULT_SERVER_DATE_FORMAT as DATE_FORMAT from .tools import DEFAULT_SERVER_DATETIME_FORMAT as DATETIME_FORMAT @@ -1988,14 +1989,13 @@ class Html(_String): original_value = record[self.name] if original_value: + # Note that sanitize also normalize original_value_sanitized = html_sanitize(original_value, **sanitize_vals) - - def get_parsed(val): - return etree.tostring(html.fromstring(val)) + original_value_normalized = html_normalize(original_value) if ( not original_value_sanitized # sanitizer could empty it - or get_parsed(original_value) != get_parsed(original_value_sanitized) + or original_value_normalized != original_value_sanitized ): # The field contains element(s) that would be removed if # sanitized. It means that someone who was part of a group diff --git a/odoo/tools/mail.py b/odoo/tools/mail.py index 26b43998e01..b2f80cd7e60 100644 --- a/odoo/tools/mail.py +++ b/odoo/tools/mail.py @@ -14,7 +14,7 @@ from urllib.parse import urlparse import idna import markupsafe -from lxml import etree +from lxml import etree, html from lxml.html import clean from werkzeug import urls @@ -74,10 +74,6 @@ class _Cleaner(clean.Cleaner): sanitize_style = False def __call__(self, doc): - # perform quote detection before cleaning and class removal - for el in doc.iter(tag=etree.Element): - tag_quote(el) - super(_Cleaner, self).__call__(doc) # if we keep attributes but still remove classes @@ -178,68 +174,110 @@ def tag_quote(el): el.set('data-o-mail-quote', '1') -def html_sanitize(src, silent=True, sanitize_tags=True, sanitize_attributes=False, sanitize_style=False, sanitize_form=True, strip_style=False, strip_classes=False): +def html_normalize(src, filter_callback=None): + """ Normalize `src` for storage as an html field value. + + The string is parsed as an html tag soup, made valid, then decorated for + "email quote" detection, and prepared for an optional filtering. + The filtering step (e.g. sanitization) should be performed by the + `filter_callback` function (to avoid multiple parsing operations, and + normalize the result). + + :param src: the html string to normalize + :param filter_callback: optional callable taking a single `etree._Element` + document parameter, to be called during normalization in order to + filter the output document + """ + if not src: return src + src = ustr(src, errors='replace') # html: remove encoding attribute inside tags doctype = re.compile(r'(<[^>]*\s)(encoding=(["\'][^"\']*?["\']|[^\s\n\r>]+)(\s[^>]*|/)?>)', re.IGNORECASE | re.DOTALL) src = doctype.sub(u"", src) - logger = logging.getLogger(__name__ + '.html_sanitize') - - kwargs = { - 'page_structure': True, - 'style': strip_style, # True = remove style tags/attrs - 'sanitize_style': sanitize_style, # True = sanitize styling - 'forms': sanitize_form, # True = remove form tags - 'remove_unknown_tags': False, - 'comments': False, - 'processing_instructions': False - } - if sanitize_tags: - kwargs.update(SANITIZE_TAGS) - - if sanitize_attributes: # We keep all attributes in order to keep "style" - if strip_classes: - current_safe_attrs = safe_attrs - frozenset(['class']) - else: - current_safe_attrs = safe_attrs - kwargs.update({ - 'safe_attrs_only': True, - 'safe_attrs': current_safe_attrs, - }) - else: - kwargs.update({ - 'safe_attrs_only': False, # keep oe-data attributes + style - 'strip_classes': strip_classes, # remove classes, even when keeping other attributes - }) - try: - # some corner cases make the parser crash (such as in test_mail) - cleaner = _Cleaner(**kwargs) - cleaned = cleaner.clean_html(src) - assert isinstance(cleaned, str) - # html considerations so real html content match database value - cleaned = cleaned.replace(u'\xa0', u' ') + doc = html.fromstring(src) except etree.ParserError as e: + # HTML comment only string, whitespace only.. if 'empty' in str(e): return u"" + raise + + # perform quote detection before cleaning and class removal + if doc is not None: + for el in doc.iter(tag=etree.Element): + tag_quote(el) + + if filter_callback: + doc = filter_callback(doc) + + src = html.tostring(doc, encoding='unicode') + + # this is ugly, but lxml/etree tostring want to put everything in a + # 'div' that breaks the editor -> remove that + if src.startswith('
') and src.endswith('
'): + src = src[5:-6] + + # html considerations so real html content match database value + src = src.replace(u'\xa0', u' ') + + return src + + +def html_sanitize(src, silent=True, sanitize_tags=True, sanitize_attributes=False, sanitize_style=False, sanitize_form=True, strip_style=False, strip_classes=False): + if not src: + return src + + logger = logging.getLogger(__name__ + '.html_sanitize') + + def sanitize_handler(doc): + kwargs = { + 'page_structure': True, + 'style': strip_style, # True = remove style tags/attrs + 'sanitize_style': sanitize_style, # True = sanitize styling + 'forms': sanitize_form, # True = remove form tags + 'remove_unknown_tags': False, + 'comments': False, + 'processing_instructions': False + } + if sanitize_tags: + kwargs.update(SANITIZE_TAGS) + + if sanitize_attributes: # We keep all attributes in order to keep "style" + if strip_classes: + current_safe_attrs = safe_attrs - frozenset(['class']) + else: + current_safe_attrs = safe_attrs + kwargs.update({ + 'safe_attrs_only': True, + 'safe_attrs': current_safe_attrs, + }) + else: + kwargs.update({ + 'safe_attrs_only': False, # keep oe-data attributes + style + 'strip_classes': strip_classes, # remove classes, even when keeping other attributes + }) + + cleaner = _Cleaner(**kwargs) + cleaner(doc) + return doc + + try: + sanitized = html_normalize(src, filter_callback=sanitize_handler) + except etree.ParserError: if not silent: raise logger.warning(u'ParserError obtained when sanitizing %r', src, exc_info=True) - cleaned = u'

ParserError when sanitizing

' + sanitized = '

ParserError when sanitizing

' except Exception: if not silent: raise logger.warning(u'unknown error obtained when sanitizing %r', src, exc_info=True) - cleaned = u'

Unknown error when sanitizing

' + sanitized = '

Unknown error when sanitizing

' - # this is ugly, but lxml/etree tostring want to put everything in a 'div' that breaks the editor -> remove that - if cleaned.startswith(u'
') and cleaned.endswith(u'
'): - cleaned = cleaned[5:-6] - - return markupsafe.Markup(cleaned) + return markupsafe.Markup(sanitized) # ---------------------------------------------------------- # HTML/Text management