Xavier Morel
01875541b1
Add a big fat warning when the qweb compiler finds a `t-raw`.
`t-esc` should now be used everywhere, the use-case for `t-raw` should
be handled by converting the corresponding values to `Markup`
objects. Even though it's convenient, this constructor *should never
be made available in the qweb rendering context* (maybe that should be
checked for explicitely?).
Replace `werkzeug.escape` by `markupsafe.escape` in
`odoo.tools.html_escape`, this means the output of `html_escape` is
markup-safe.
Updated qweb to work correctly with escaping and `Markup`, amongst
other things QWeb bodies should be markup-safe internally (so that a
`t-set` value can be fed into a `t-esc`). See at the bottom for the
attributes handling as it's a bit complicated.
`to_text` needed updating: `markupsafe.Markup` is a subclass of `str`,
but `str` is not a passthrough for strings. So `Markup` instances
going through would be converted to normal `str`, losing their safety
flag. Since qweb internally uses `to_text` on pretty much
everything (in order to handle None / False), this would then cause
almost every `Markup` to get mistakenly double-escaped.
Also mark a bunch of APIs as markup-safe by default
* html_sanitize output.
* HTML fields content, sanitization is applied on intake (so stripped
by the trip through the database) and if the field is unsanitised
the injection is very much intentional, probably. Note: this
includes automatically decoding bytes as a number of default values
& computes yield bytes, which Markup will happily accept... by
repr-ing them which is useless. This is hard to notice without `-b`.
* Script-safe json, it's rather the point (though it uses a
non-standard escaping scheme).
* Note that `nl2br`, kinda: it should work correctly whether or not
the input is markup-safe, this means we should not need to escape
values fed to `nl2br`, but it doesn't hurt either.
Update some qweb field serialisations to mark their output as
markup-safe when necessary (e.g. monetary, barcode,
contact). Otherwise either using proper escaping internally or doing
nothing should do the trick.
Also update qweb to return markup-safe bytes: we want qweb to return
markup-safe contents as a common use-case is to render something with
one template, and inject its content in an other one (with Python code
inbetween, as `t-call` works a bit differently and does not go through
the external rendering interface).
However qweb returns `bytes` while `Markup` extends `str`. After a
quick experiment with changing qweb rendering to return `str` (rather
unmitigated failure I fear), it looks like the safest tack is to add a
somewhat similar bytes-based type, which decodes to a `Markup` but
keeps to bytes semantics.
For debugging and convenience reasons, MarkupSafeBytes does *not*
stringify and raises an error instead (`__repr__` works fine). This is
to avoid implicit stringifications which do the wrong thing (namely
create a string `"b'foo'"`).
Also add some configuration around BytesWarning (which still has to be
enabled at the interpreter level via `-b`, there's no way to enable it
programmatically smh), and monkeypatch `showwarning` to show warning
tracebacks, as it's common for warnings to be triggered in the bowels
of the application, and hard to relate to business logic without the
complete traceback.
`t-out`
=======
`t-esc` is a bit confusing for the new behaviour of "maybe escape
maybe not", so add a `t-out` alias with the same behaviour.
Unlike `t-raw`, `t-esc` is only soft-deprecated for now: there are
thousands of instances, so editing all the templates is not
great. Eventually we'll add a `ci/style` to prevent addition of new
ones, and eventually we might do a bulk-replace and hard-deprecate.
Attributes handling
===================
There are a few issues with respect to attributes. The first issue is
that markup-safe content is not necessarily attributes-safe
e.g. markup-safe content can contain unescaped `<` or double-quotes
while attributes can not. So we must forcefully escape the input, even
if it's supposedly markup-safe already.
This causes a problem for script-safe JSON: it's markup-safe but
really does its own thing. So instead of escaping it up-front and
wrapping it in Markup, make script-safe JSON its own type which
applies JSON-escaping *during the `__html__` call.
This way if a script-safe JSON object goes through `markupsafe.escape`
we'll apply script-safe escaping, otherwise it'll be treated as a
regular strings and eventually escaped the normal way.
A second issue was the processing of format-valued
attributes (`t-attf`): literal segments should always be markup-safe,
while non-literal may or may not be. This turns out to be an issue if
the non-literal segment *is* markup-safe: in that case when the
literal and non-literal segments get concatenated the literal segments
will get escaped, then attributes serialization will escape
them *again* leading to doubly-escaped content in attributes.
The most visible instance of this was the `snippet_options` template,
specifically:
<t t-set="so_content_addition_selector" t-translation="off">blockquote, ...</t>
<div id="so_content_addition"
t-att-data-selector="so_content_addition_selector"
t-attf-data-drop-near="p, h1, h2, h3, .row > div > img, #{so_content_addition_selector}"
data-drop-in=".content, nav"/>
Here `so_content_addition_selector` is a qweb body therefore
markup-safe, When concatenated with the literal part of
`t-atff-data-drop-near` it would cause the HTML-escaping of that
yielding a new Markup object. Normal attributes processing would then
strip the markup flag (using `str()`) and escape it again, leading to
doubly-escaped literals.
The original hack around was to unescape() `Markup` content before
stringifying it and escaping it again, in the attribute serialization
method (`_append_attributes`).
That's pretty disgusting, after some more consideration & testing it
looks like a much better and safer fix is to ensure the
expression (non-literal) segments of format strings always result in
`str`, never `Markup`, which is easy enough: just all `str()` on the
output of strexpr. We could also have concatenated all the bits using
`''.join` instead of repeated concatenation (`+`).
Also add a check on the type of the format string for safety, I think
it should always be a proper str and the bytes thing is only when
running in py2 (where lxml uses bytestrings as a space optimization
for ascii-only values) but it should not hurt too much to perform a
single typecheck assertion on the value... instead of performing one
per literal segment.
Note: we may need to implement unescape anyway, because it's still
possible to get double-escaping with the current scheme: given an
explicitly escape-ed `foo` and `t-att-foo="foo"`, `foo` will be
re-escaped.
fixup! [CHG] core, web: deprecate t-raw
56 lines
2.0 KiB
Python
56 lines
2.0 KiB
Python
# -*- coding: utf-8 -*-
|
|
import json as json_
|
|
import re
|
|
|
|
import markupsafe
|
|
|
|
JSON_SCRIPTSAFE_MAPPER = {
|
|
'&': r'\u0026',
|
|
'<': r'\u003c',
|
|
'>': r'\u003e',
|
|
'\u2028': r'\u2028',
|
|
'\u2029': r'\u2029'
|
|
}
|
|
class _ScriptSafe(str):
|
|
def __html__(self):
|
|
# replacement can be done straight in the serialised JSON as the
|
|
# problematic characters are not JSON metacharacters (and can thus
|
|
# only occur in strings)
|
|
return markupsafe.Markup(re.sub(
|
|
r'[<>&\u2028\u2029]',
|
|
lambda m: JSON_SCRIPTSAFE_MAPPER[m[0]],
|
|
self,
|
|
))
|
|
class JSON:
|
|
def loads(self, *args, **kwargs):
|
|
return json_.loads(*args, **kwargs)
|
|
def dumps(self, *args, **kwargs):
|
|
""" JSON used as JS in HTML (script tags) is problematic: <script>
|
|
tags are a special context which only waits for </script> but doesn't
|
|
interpret anything else, this means standard htmlescaping does not
|
|
work (it breaks double quotes, and e.g. `<` will become `<` *in
|
|
the resulting JSON/JS* not just inside the page).
|
|
|
|
However, failing to escape embedded json means the json strings could
|
|
contains `</script>` and thus become XSS vector.
|
|
|
|
The solution turns out to be very simple: use JSON-level unicode
|
|
escapes for HTML-unsafe characters (e.g. "<" -> "\u003C". This removes
|
|
the XSS issue without breaking the json, and there is no difference to
|
|
the end result once it's been parsed back from JSON. So it will work
|
|
properly even for HTML attributes or raw text.
|
|
|
|
Also handle U+2028 and U+2029 the same way just in case as these are
|
|
interpreted as newlines in javascript but not in JSON, which could
|
|
lead to oddities and issues.
|
|
|
|
.. warning::
|
|
|
|
except inside <script> elements, this should be escaped following
|
|
the normal rules of the containing format
|
|
|
|
Cf https://code.djangoproject.com/ticket/17419#comment:27
|
|
"""
|
|
return _ScriptSafe(json_.dumps(*args, **kwargs))
|
|
scriptsafe = JSON()
|