Skip to content

Conversation

@whisk
Copy link
Contributor

@whisk whisk commented Oct 11, 2025

Added support for script contents escaping according to the HTML5 standard: https://html.spec.whatwg.org/multipage/scripting.html#restrictions-for-contents-of-script-elements

Script with contents can be used like this:

Script(attrs.Props{}, Text("alert(1);"))

It required a workaround with the ScriptNode struct to maintain backward compatibility. While script tag may contain only a text node (formally any number of text nodes which are concatenated) the original Script() implementation allowed any number of arbitrary nodes or elements.

func Script(attrs attrs.Props, children ...Node) *Element {

@chasefleming chasefleming requested a review from Copilot October 16, 2025 23:49
Copy link

Copilot AI left a 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
Comment on lines 21 to 25
var scriptContentsReplaces = strings.NewReplacer(
"<!--", "\\x3C!--",
"<script", "\\x3Cscript",
"</script", "\\x3C/script",
)
Copy link

Copilot AI Oct 16, 2025

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid, fixed.

Comment on lines +128 to +130
func (t ScriptNode) RenderTo(builder *strings.Builder, opts RenderOptions) {
scriptContents := EscapeScriptContents(t.node.Render())
builder.WriteString(scriptContents)
Copy link

Copilot AI Oct 16, 2025

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

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.

@whisk whisk force-pushed the feature/script-escaping branch from f0d9447 to cb5d07c Compare October 17, 2025 20:26
@whisk whisk force-pushed the feature/script-escaping branch from cb5d07c to 51727b4 Compare October 17, 2025 20:38
@chasefleming chasefleming merged commit fd27928 into chasefleming:main Oct 18, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants