Skip to content

added content escaping for comment nodes#159

Merged
chasefleming merged 2 commits into
chasefleming:mainfrom
whisk:fix/comments-escaping
Jul 24, 2025
Merged

added content escaping for comment nodes#159
chasefleming merged 2 commits into
chasefleming:mainfrom
whisk:fix/comments-escaping

Conversation

@whisk

@whisk whisk commented Jul 14, 2025

Copy link
Copy Markdown
Contributor

Description

Comment() now escapes special character sequences for safe rendering as described in https://html.spec.whatwg.org/multipage/syntax.html#comments

@chasefleming chasefleming left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for adding this! Left a couple comments.

Comment thread utils.go
}

// EscapeCommentContents escapes the contents of a comment node to ensure safe rendering according to https://html.spec.whatwg.org/multipage/syntax.html#comments
func EscapeCommentContents(s string) string {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Covers all the major disallowed sequences, but stray “--” inside comments isn’t caught yet per the HTML spec.

Comment thread utils.go Outdated
@chasefleming chasefleming requested a review from Copilot July 15, 2025 03:55

This comment was marked as outdated.

This comment was marked as off-topic.

@chasefleming

Copy link
Copy Markdown
Owner

I think <!-- This comment contains -- double hyphens --> is still an issue, no?

@chasefleming

Copy link
Copy Markdown
Owner

@whisk Looks like a conflict now after merging your other PR.

@whisk whisk force-pushed the fix/comments-escaping branch from bfb5b83 to 1a832fc Compare July 22, 2025 21:10
@chasefleming chasefleming merged commit edbb7f2 into chasefleming:main Jul 24, 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.

3 participants