supported escaping for script element#168
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds HTML5-compliant escaping for <script> element contents while preserving backward compatibility with existing Script(...) usage that accepted arbitrary child nodes.
- Introduces EscapeScriptContents with a strings.Replacer and wraps script children in a new ScriptNode to apply escaping.
- Updates Script(...) to transform children into ScriptNode instances.
- Adds tests for basic escaping scenarios.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| utils.go | Adds script content escaping logic and public EscapeScriptContents function. |
| elem.go | Introduces ScriptNode to apply escaping when rendering script children. |
| elements.go | Wraps Script(...) children with ScriptNode to enforce escaping. |
| elements_test.go | Adds tests validating escaping behavior for specific substrings. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| var scriptContentsReplaces = strings.NewReplacer( | ||
| "<!--", "\\x3C!--", | ||
| "<script", "\\x3Cscript", | ||
| "</script", "\\x3C/script", | ||
| ) |
There was a problem hiding this comment.
Escaping is case-sensitive and will fail for sequences like </SCRIPT>, <SCRIPT, or <!-- with uppercase letters (end/script tags are ASCII case-insensitive in HTML); implement case-insensitive matching (e.g. a manual scan comparing lowercased substrings) to ensure all variants are escaped.
There was a problem hiding this comment.
This is valid, fixed.
| func (t ScriptNode) RenderTo(builder *strings.Builder, opts RenderOptions) { | ||
| scriptContents := EscapeScriptContents(t.node.Render()) | ||
| builder.WriteString(scriptContents) |
There was a problem hiding this comment.
Calling t.node.Render() allocates the full child content before escaping; for large script bodies a streaming approach (scan and write with replacements on the fly) would avoid an extra full-string allocation.
There was a problem hiding this comment.
This is overengineering with little gain.
f0d9447 to
cb5d07c
Compare
cb5d07c to
51727b4
Compare
Added support for
scriptcontents escaping according to the HTML5 standard: https://html.spec.whatwg.org/multipage/scripting.html#restrictions-for-contents-of-script-elementsScript with contents can be used like this:
It required a workaround with the
ScriptNodestruct to maintain backward compatibility. Whilescripttag may contain only a text node (formally any number of text nodes which are concatenated) the originalScript()implementation allowed any number of arbitrary nodes or elements.