-
Notifications
You must be signed in to change notification settings - Fork 33
supported escaping for script element #168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
supported escaping for script element #168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
utils.go
Outdated
| var scriptContentsReplaces = strings.NewReplacer( | ||
| "<!--", "\\x3C!--", | ||
| "<script", "\\x3Cscript", | ||
| "</script", "\\x3C/script", | ||
| ) |
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is valid, fixed.
| func (t ScriptNode) RenderTo(builder *strings.Builder, opts RenderOptions) { | ||
| scriptContents := EscapeScriptContents(t.node.Render()) | ||
| builder.WriteString(scriptContents) |
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.