HTML style comment breaks script

Description

$.append fails when I try to append script element with HTML style comment.

There is no errors when I try to do the same thing using vanila JS:

var script = document.createElement('script');
script.text = '<!-- html style comment in javascript -->';
document.querySelector('html').append(script); // no errors

When I do it using jQuery:

var script = document.createElement('script');
script.text = '<!--html style comment in javascript -->';
$('html').append(script); // Uncaught SyntaxError: Unexpected identifier

The root cause:
The domManip function does RegExp replacement before calling DOMEval and removes :

jquery/src/manipulation.js

Line 30 in a684e6b

rcleanScript = /^\s*<!(?:\[CDATA\[|--)|(?:\]\]|--)>\s*$/g;

jquery/src/manipulation.js

Line 164 in a684e6b

DOMEval( node.textContent.replace( rcleanScript, "" ), node, doc );

Link to test case

https://codepen.io/ovarn/pen/vYmgjzP (open browser console to see the error)

Thanks for the report. The purpose of this regex is to both clean HTML comments and CDATA sections in XML documents, e.g.:

<script>
<![CDATA[
console.log("<");
]]>
</script>

HTML comments themselves should not be an issue as ECMAScript now requires HTML-style comment delimiters to also serve as JS comment ones; a relevant test case from Compat Tables:

--> A comment
<!-- Another comment
var a = 3; <!-- Another comment
return a === 3;

Unfortunately, IE 11 doesn't support this feature. 😞

Using regexes to parse JS is always going to have edge cases, we've learned that the hard way when we had to release jQuery 3.5. However, we need to cut out CDATA & HTML comments to be able to execute inline scripts that may contain them and using a full parser would be too slow & huge.

If there's any other way to remove CDATA & HTML comments without hitting issues like this one, we'd gladly replace the current implementation.

Hmm, since the spec requires  to serve as single-line comment delimiters, maybe we should just tweak the regex to match not just those tokens but also what's after them until the end of the line?

Looking at it again, it seems we could skip using rcleanScript completely outside of IE and in IE fix the regex to match full lines.

I also don't see a point of having the CDATA token in the regex. In HTML mode CDATA sections are not recognized and have to be commented out, either via JS comments or HTML-style comments. In XML mode, CDATA sections are not put in the script element textContent so jQuery would also not pick it up (just in case, I tested it in both IE 9 & 11). Therefore, we should be able to just workaround the HTML comments issue for IE and leave the modern browsers without any replacement. This would also play nice with things like trusted types which may not allow manual modifications of evaluated strings taken from the page.

I'll submit a PR.

I'm thinking about making the fix for IE just for the 3.x-stable branch and to remove that logic completely on main. The rationale is that CDATA behind regular single-line JS comments (//) would already work and the problem is just with script contents wrapped in HTML-style comments () and only in IE. Such wrapping was necessary for very old browsers without script support so that they don't render contents of script tags. AFAIK this stopped being necessary ages before jQuery came out.

There is some risk some tools will still generate script tags like that and the removal of that logic would break those scenarios but the replacement itself is not and cannot be ideal and there will always be cases it cannot match like HTML comments in the middle of a script. Trying to not mess with the content before processing it seems to be a golden standard to prevent XSS issues (apart from more substantial solutions like CSP).

Here's a good explanation of why HTML comments, JS comments & CDATA markers are sometimes used together in weird combinations: https://www.sitepoint.com/community/t/cdata-comment/2072/7.

Another update: while IE doesn't conform to the spec in regular scripts or in eval, jQuery.globalEval - used in dom manipulation for executing scritps - is now always using a script tag where these semantics are preserved even in IE 9. Therefore, we can cut the HTML comments escaping completely.

We still need to leave the CDATA part in 3.x as getting rid of that would be a breaking change but we can remove it in 4.x without breaking handling of HTML comments in HTML documents or CDATA in XHTML ones even in IE.

Manipulation: Don't remove HTML comments from scripts

PRs to:

main (for 4.0): Manipulation: Don't remove HTML comments from scripts #4906
3.x-stable (for 3.x): Manipulation: Don't remove HTML comments from scripts #4905

Manipulation: Don't remove HTML comments from scripts

Landed in 2f8f39e on main and in 924b515 on 3.x-stable.

Michał, thank you for the quick resolution and problem clarification!

Navigation Menu

Repository navigation

HTML style comment breaks script #4904

Description

Link to test case

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

HTML style comment breaks script #4904

Description

Description

Link to test case

Activity

mgol commented on Jul 14, 2021

mgol commented on Jul 14, 2021

mgol commented on Jul 15, 2021

mgol commented on Jul 15, 2021

mgol commented on Jul 15, 2021

mgol commented on Jul 15, 2021

mgol commented on Jul 15, 2021

mgol commented on Jul 19, 2021

ovarn commented on Jul 20, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions