pengyuthon/odoo_source
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[FIX] core: extract normalize from HTML sanitizer

Browse Source 
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:
  - `<blockquote/>` tag
  - text-based quotes (>, >>) and signatures (-- Signature)
  - html signature (-- <br />blah)
- some editor compatibility which removed the wrapping `<div/>` 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
<blockquote class=".." data-name="Blockquote">
```
But when the "restricted" user then wanted to do some changes, it would become:
```html
<blockquote class=".." data-name="Blockquote" data-o-mail-quote-node="1" data-o-mail-quote="1">
```

Same for `Share` snippet:
```html
<a href="https://www.facebook.com/sharer/sharer.php?u={url}">
<a href="https://www.facebook.com/sharer/sharer.php?u=%7Burl%7D">
```

[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) <rde@odoo.com>
This commit is contained in:
Romain Derie
2023-01-25 05:06:02 +01:00
parent c5ebd4dc4e
commit fff31b316f
3 changed files with 116 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files
odoo/addons/test_new_api/tests/test_new_fields.py
+25 -2
View File
@@ -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 = '<blockquote>Something</blockquote>'
normalized_val = '<blockquote data-o-mail-quote-node="1" data-o-mail-quote="1">Something</blockquote>'
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 = '<script></script>'
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 = '<span attr1 ="att1" attr2=\'attr2\'>é@&nbsp;</span><p><span/></p>'
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 = '<!-- I am a comment -->'
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):
odoo/fields.py
+5 -5
View File
@@ -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
odoo/tools/mail.py
+86 -48
View File
@@ -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 <SCRIPT/XSS SRC=\"http://ha.ckers.org/xss.js\"></SCRIPT> 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'&nbsp;')
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('<div>') and src.endswith('</div>'):
src = src[5:-6]
# html considerations so real html content match database value
src = src.replace(u'\xa0', u'&nbsp;')
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'<p>ParserError when sanitizing</p>'
sanitized = '<p>ParserError when sanitizing</p>'
except Exception:
if not silent:
raise
logger.warning(u'unknown error obtained when sanitizing %r', src, exc_info=True)
cleaned = u'<p>Unknown error when sanitizing</p>'
sanitized = '<p>Unknown error when sanitizing</p>'
# this is ugly, but lxml/etree tostring want to put everything in a 'div' that breaks the editor -> remove that
if cleaned.startswith(u'<div>') and cleaned.endswith(u'</div>'):
cleaned = cleaned[5:-6]
return markupsafe.Markup(cleaned)
return markupsafe.Markup(sanitized)
# ----------------------------------------------------------
# HTML/Text management