You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@allura.apache.org by br...@apache.org on 2026/07/06 16:57:40 UTC
(allura) 02/03: disallowed tags are now preserved as escaped visible text instead of disappearing.
This is an automated email from the ASF dual-hosted git repository.
asf-gitbox-commits pushed a commit to branch db/remove_html5lib_turbohtml
in repository https://gitbox.apache.org/repos/asf/allura.git
commit a96316dfa6696cab739510489958e4852dbc9b83
Author: Dave Brondsema <da...@brondsema.net>
AuthorDate: Sat Jul 4 15:31:34 2026 -0400
disallowed tags are now preserved as escaped visible text instead of disappearing.
How it works
_sanitize_element() routes every disallowed element (unknown tags, form elements, script/style, SVG/MathML, non-YouTube iframes, non-checkbox inputs) through a new _escape_element(): it reconstructs the tag as Text nodes — start tag with its attributes, the already-sanitized children hoisted in place, and an end tag unless the element is a void element like <input>. The WHATWG serializer escapes those text nodes on output, so <script>alert(1)</script> renders as visible <script> [...]
This is exactly what the old html5lib sanitizer did, so several tests I'd previously changed reverted to their original master expectations byte-for-byte: the escaped <script> test, <class 'foo'=""></class>, clean_html, solr's solarize (script text is searchable again, matching what's displayed), and the compromised-oembed test showing the escaped script after the iframe. The <foo {bar}> case that used to crash the old parser now renders as escaped text rather than the error-fallback page.
One residual difference from the old sanitizer, for completeness: disallowed attributes on allowed elements are still silently dropped (both old and new behavior), and escaped-tag output always uses name="value" form — matching how html5lib rendered them.
---
Allura/allura/lib/utils.py | 42 ++++++++++++++-----------
Allura/allura/tests/functional/test_newforge.py | 4 +--
Allura/allura/tests/test_globals.py | 11 +++----
Allura/allura/tests/test_helpers.py | 2 +-
Allura/allura/tests/test_utils.py | 12 ++++---
Allura/allura/tests/unit/test_solr.py | 3 +-
6 files changed, 41 insertions(+), 33 deletions(-)
diff --git a/Allura/allura/lib/utils.py b/Allura/allura/lib/utils.py
index 3811efd0d..5b846ba91 100644
--- a/Allura/allura/lib/utils.py
+++ b/Allura/allura/lib/utils.py
@@ -597,8 +597,14 @@ class ForgeHTMLSanitizer:
"""
Sanitize HTML by walking the turbohtml parse tree, keeping only whitelisted
elements and attributes, with some additional Allura-specific rules.
+ Disallowed elements are escaped into visible text (like our previous html5lib-based
+ sanitizer did), so someone writing about e.g. script tags doesn't have them vanish.
"""
+ # end tags aren't written for these when escaping a disallowed element
+ _void_elements = frozenset(['area', 'base', 'br', 'col', 'embed', 'hr', 'img', 'input', 'link', 'meta',
+ 'param', 'source', 'track', 'wbr'])
+
# like our previous html5lib-based whitelist, minus form elements which could be
# used for a social engineering attack to construct a form, minus obsolete/obscure
# elements, and without SVG/MathML elements
@@ -613,10 +619,6 @@ class ForgeHTMLSanitizer:
'th', 'thead', 'time', 'tr', 'tt', 'u', 'ul', 'var', 'video', 'wbr',
])
- # disallowed elements whose contents aren't meaningful on their own, so are removed entirely
- # (other disallowed elements are removed but their contents are kept)
- _removed_elements = frozenset(['script', 'style', 'textarea', 'select', 'datalist'])
-
# generic attributes allowed on any element, similar to our previous html5lib-based whitelist.
# srcset is used in our own project_list/project_summary widgets
# which are used as macros so go through markdown
@@ -691,26 +693,20 @@ def _sanitize_element(self, el: Element) -> None:
append a "(hostname)" suffix after links with misleading visible text
"""
tag = el.tag
- if tag in self._removed_elements:
- el.decompose()
- return
- if el.namespace is not Namespace.HTML or tag not in self.allowed_elements:
- # drop the tag itself but keep its (sanitized) contents
+ allowed = el.namespace is Namespace.HTML and tag in self.allowed_elements
+ if allowed and tag == 'iframe':
+ allowed = (el.attrs.get('src') or '').startswith(self.valid_iframe_srcs)
+ if allowed and tag == 'input':
+ allowed = el.attrs.get('type') == 'checkbox'
+ if not allowed:
self._sanitize_children(el)
- el.unwrap()
+ self._escape_element(el)
return
self._sanitize_attrs(el)
if tag == 'iframe':
- if (el.attrs.get('src') or '').startswith(self.valid_iframe_srcs):
- el.clear() # iframe contents are fallback-only, and would serialize unescaped
- else:
- el.decompose()
- return
- if tag == 'input':
- if el.attrs.get('type') != 'checkbox':
- el.decompose()
+ el.clear() # iframe contents are fallback-only, and would serialize unescaped
return
self._sanitize_children(el)
@@ -720,6 +716,16 @@ def _sanitize_element(self, el: Element) -> None:
if suffix:
el.insert_after(Text(suffix))
+ def _escape_element(self, el: Element) -> None:
+ # replace a disallowed element with visible text of its tags (escaped during serialization),
+ # keeping its already-sanitized contents in place
+ attrs = ''.join(f' {name}="{" ".join(value) if isinstance(value, list) else value or ""}"'
+ for name, value in el.attrs.items())
+ el.insert_before(Text(f'<{el.tag}{attrs}>'))
+ if el.tag not in self._void_elements:
+ el.insert_after(Text(f'</{el.tag}>'))
+ el.unwrap()
+
def _sanitize_attrs(self, el: Element) -> None:
for name in list(el.attrs):
if name not in self.allowed_attributes:
diff --git a/Allura/allura/tests/functional/test_newforge.py b/Allura/allura/tests/functional/test_newforge.py
index c87e80078..6d18a6185 100644
--- a/Allura/allura/tests/functional/test_newforge.py
+++ b/Allura/allura/tests/functional/test_newforge.py
@@ -34,11 +34,11 @@ def test_markdown_to_html(self):
'/nf/markdown_to_html?markdown=*aaa*bb[wiki:Home]&project=test&app=wiki&neighborhood=%s' % n._id, validate_chunk=True)
assert '<p><em>aaa</em>bb<a class="alink" href="/p/test/wiki/Home/">[wiki:Home]</a></p>' in r, r
- # this used to trigger a parsing error; now the unknown tag is simply removed
+ # this used to trigger a parsing error; now the unknown tag is escaped
bad_markdown = '<foo {bar}>'
r = self.app.get('/nf/markdown_to_html?markdown=%s&project=test&app=wiki&neighborhood=%s' %
(quote(bad_markdown), n._id))
- r.mustcontain('<div class="markdown_content"><p></p></div>')
+ r.mustcontain('<foo {bar}=""></foo>')
r = self.app.get('/nf/markdown_to_html?markdown=*aaa*bb[wiki:Home]&project=test&app=wiki&neighborhood=bogus',
status=400)
diff --git a/Allura/allura/tests/test_globals.py b/Allura/allura/tests/test_globals.py
index 65b3283a5..0af320f1b 100644
--- a/Allura/allura/tests/test_globals.py
+++ b/Allura/allura/tests/test_globals.py
@@ -355,10 +355,9 @@ def test_macro_embed(self, oembed_fetch):
}
r = g.markdown_wiki.convert('[[embed url=http://www.youtube.com/watch?v=kOLpSPEA72U]]')
assert ('<p><iframe height="270" '
- 'src="https://www.youtube-nocookie.com/embed/kOLpSPEA72U?feature=oembed" width="480"></iframe></p>' in
+ 'src="https://www.youtube-nocookie.com/embed/kOLpSPEA72U?feature=oembed" width="480"></iframe>'
+ '<script>alert(1)</script></p>' in
r.replace('\n', ''))
- assert 'script' not in r
- assert 'alert' not in r
def test_macro_embed_video_gone(self):
# this does a real fetch
@@ -473,9 +472,9 @@ def test_markdown_basics(self):
'<div class="markdown_content"><p>Multi</p>\n'
'<p>Line</p></div>')
- # should not raise an exception; unknown tag is removed:
+ # should not raise an exception:
assert g.markdown.convert("<class 'foo'>") == \
- '<div class="markdown_content"><p></p></div>'
+ '''<div class="markdown_content"><p><class 'foo'=""></class></p></div>'''
assert g.markdown.convert(dedent('''\
# Header
@@ -602,7 +601,7 @@ def test_markdown_autolink_with_escape(self):
def test_markdown_invalid_script(self):
r = g.markdown.convert('<script>alert(document.cookies)</script>')
- assert '<div class="markdown_content">\n</div>' == r
+ assert '<div class="markdown_content"><script>alert(document.cookies)</script>\n</div>' == r
def test_markdown_invalid_onerror(self):
r = g.markdown.convert('<img src=x onerror=alert(document.cookie)>')
diff --git a/Allura/allura/tests/test_helpers.py b/Allura/allura/tests/test_helpers.py
index ae318e998..5ea729e59 100644
--- a/Allura/allura/tests/test_helpers.py
+++ b/Allura/allura/tests/test_helpers.py
@@ -779,6 +779,6 @@ def test_querystring():
def test_clean_html():
- assert h.clean_html('<script>alert(1)</script>') == ''
+ assert h.clean_html('<script>alert(1)</script>') == '<script>alert(1)</script>'
assert h.clean_html('<b style="color: red; right: 0">ok</b>') == '<b style="color: red;">ok</b>'
assert isinstance(h.clean_html('foo'), Markup)
diff --git a/Allura/allura/tests/test_utils.py b/Allura/allura/tests/test_utils.py
index 797f3d04b..d66b5e434 100644
--- a/Allura/allura/tests/test_utils.py
+++ b/Allura/allura/tests/test_utils.py
@@ -248,7 +248,8 @@ def sanitize_html(self, html):
return utils.ForgeHTMLSanitizer().sanitize(html)
def test_html_sanitizer_iframe(self):
- assert self.sanitize_html('<div><iframe></iframe></div>') == '<div></div>'
+ assert (self.sanitize_html('<div><iframe></iframe></div>') ==
+ '<div><iframe></iframe></div>')
def test_html_sanitizer_youtube_iframe(self):
assert (self.sanitize_html(
@@ -260,17 +261,20 @@ def test_html_sanitizer_youtube_iframe(self):
'<div><iframe src="https://www.youtube-nocookie.com/embed/kOLpSPEA72U?feature=oembed"></iframe></div>')
assert (self.sanitize_html('<div><iframe src="https://evil.com/embed/x"></iframe></div>') ==
- '<div></div>')
+ '<div><iframe src="https://evil.com/embed/x"></iframe></div>')
def test_html_sanitizer_form_elements(self):
assert (self.sanitize_html('<p>test</p><form method="post" action="http://localhost/foo.php">'
'<input type=file><input type=text><textarea>asdf</textarea></form>') ==
- '<p>test</p>')
+ '<p>test</p><form method="post" action="http://localhost/foo.php">'
+ '<input type="file"><input type="text">'
+ '<textarea>asdf</textarea></form>')
def test_html_sanitizer_checkbox(self):
assert (self.sanitize_html('<p><input type="checkbox" disabled/><input type="text" disabled/>'
'<input type="checkbox" disabled checked/></p>') ==
- '<p><input type="checkbox" disabled=""><input type="checkbox" disabled="" checked=""></p>')
+ '<p><input type="checkbox" disabled=""><input type="text" disabled="">'
+ '<input type="checkbox" disabled="" checked=""></p>')
def test_html_sanitizer_summary(self):
assert (self.sanitize_html('<details open="open"><summary>An Summary</summary><ul><li>Bullet Item</li></ul></details>') ==
diff --git a/Allura/allura/tests/unit/test_solr.py b/Allura/allura/tests/unit/test_solr.py
index a0bdcc9a2..e25581ffd 100644
--- a/Allura/allura/tests/unit/test_solr.py
+++ b/Allura/allura/tests/unit/test_solr.py
@@ -157,9 +157,8 @@ def test_solarize_strips_markdown(self):
assert self.obj.solarize() == dict(text='Header')
def test_solarize_html_in_text(self):
- # raw script tag is removed entirely by sanitization, so not indexed (matching what is displayed)
self.obj.index = lambda: dict(text='<script>a(1)</script>')
- assert self.obj.solarize() == dict(text='')
+ assert self.obj.solarize() == dict(text='<script>a(1)</script>')
self.obj.index = lambda: dict(text='<script>a(1)</script>')
assert self.obj.solarize() == dict(text='<script>a(1)</script>')