Feature #44634
closed
Escape the tags covered by the tagfilter extension removed in commonmarker 2.10.1
Description
After updating the commonmarker gem from 2.10.0 to 2.10.1, the following test fails:
Failure:
Redmine::WikiFormatting::CommonMark::FormatterTest#test_should_escape_unwanted_tags [test/unit/lib/redmine/wiki_formatting/common_mark/formatter_test.rb:278]:
--- expected
+++ actual
@@ -1 +1 @@
-"<p>sit<br>amet <style>.foo { color: #fff; }</style> <script>alert(\"hello world\");</script></p>"
+"<p>sit<br>amet .foo { color: #fff; } </p>"
bin/rails test test/unit/lib/redmine/wiki_formatting/common_mark/formatter_test.rb:271
With commonmarker 2.10.0, the <style> and <script> tags in the input are escaped and shown as text. With 2.10.1, they are removed from the output instead.
Files
Updated by Go MAEDA 26 days ago
- File 0001-Escape-tags-that-commonmarker-escaped-with-its-tagfi.patch 0001-Escape-tags-that-commonmarker-escaped-with-its-tagfi.patch added
- Subject changed from CommonMark formatter test fails with commonmarker 2.10.1 to CommonMark no longer escapes the tags covered by the tagfilter extension removed in commonmarker 2.10.1
The cause of this issue is that commonmarker 2.10.1 removed the tagfilter extension. This follows the removal of the extension in comrak 0.56.0, the Rust library that commonmarker is built on. The author of comrak removed it as neither sufficient nor necessary for safety (see comrak#829 and comrak#845). Although the release notes of commonmarker 2.10.1 do not mention this change, the description of the tagfilter option has been removed from its README.md.
commonmarker silently ignores unknown options, so the tagfilter: true setting in Redmine::WikiFormatting::CommonMark::PIPELINE_CONFIG no longer has any effect. As a result, tags such as <script> and <style> in raw HTML are no longer escaped. Instead, they are removed by the sanitizer, which causes the test failure.
The attached patch escapes the same nine tags that the tagfilter extension escaped (title, textarea, style, xmp, iframe, noembed, noframes, script, and plaintext; see Disallowed Raw HTML in the GFM specification). It does so in the HTML output of commonmarker, before the output is parsed and sanitized. The patch also removes the tagfilter option, which is now ignored.
As far as security is concerned, the sanitizer is sufficient, since it removes these tags even without the tagfilter extension. However, relying on the sanitizer alone breaks the display of existing content. The sanitizer works on the parsed document, and when parsing an unclosed tag of these kinds, the HTML parser treats the rest of the text as its content. For example, with commonmarker 2.10.1 and without the patch, the following text:
Unclosed <script> tag Next paragraph
is rendered as:
<p>Unclosed </p>
Everything after the <script> tag, including the following paragraph, is lost. With an unclosed <title> or <iframe> tag, the rest of the text is shown as HTML source instead. Existing content that was displayed correctly with commonmarker 2.10.0 would therefore be broken after upgrading.
The patch adds a test for the unclosed tag case. All tests in test/unit/lib/redmine/wiki_formatting/common_mark/ and test/helpers/application_helper_test.rb pass with both commonmarker 2.10.0 and 2.10.1.
Updated by Go MAEDA 20 days ago
- Status changed from New to Closed
- Assignee set to Go MAEDA
- Resolution set to Fixed
Committed the fix in r25309.
Since commonmarker 2.10.1 removed the tagfilter extension, Redmine now escapes the tags that the extension covered by itself (r25309). As a result, these tags are escaped in the same way as with commonmarker 2.10.0, and r25310 reverts r25267, which pinned commonmarker to 2.10.0.
Updated by Go MAEDA 19 days ago
- Tracker changed from Defect to Feature
- Subject changed from CommonMark no longer escapes the tags covered by the tagfilter extension removed in commonmarker 2.10.1 to Escape the tags covered by the tagfilter extension removed in commonmarker 2.10.1
Changed the tracker from Defect to Feature and updated the subject, because this issue did not affect any released Redmine version. These versions do not use commonmarker 2.10.x, and the test failure occurred only in trunk, where it has been resolved. The new subject describes the change made in response to the removal of the tagfilter extension in commonmarker 2.10.1, instead of a problem that users have not experienced.