Skip to content
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.

Sign up for GitHub

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

Jump to bottom

html/template: comment handling introduced in 1.21.1 breaks valid scripts #63183

Open
jupenur opened this issue Sep 23, 2023 · 4 comments
Open

html/template: comment handling introduced in 1.21.1 breaks valid scripts #63183

jupenur opened this issue Sep 23, 2023 · 4 comments
Assignees
@rolandshoemaker
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Security
Milestone
Backlog

Comments

@jupenur
Copy link

jupenur commented Sep 23, 2023

What version of Go are you using (go version)?

1.21.1

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Playground

What did you do?

Run this code on the playground:

package main

import (
	"html/template"
	"os"
)

func main() {
	t, _ := template.New("T").Parse(`
		<script>
			if (a-->b) {
				// do stuff
			}
		</script>
	`)
	t.ExecuteTemplate(os.Stdout, "T", "")
}

What did you expect to see?

		<script>
			if (a-->b) {
				
			}
		</script>

What did you see instead?

		<script>
			if (a
				
			}
		</script>
@jupenur jupenur changed the title html/template: comment handing introduced in 1.21.1 breaks valid scripts html/template: comment handling introduced in 1.21.1 breaks valid scripts Sep 23, 2023
@seankhliao
Copy link
Member

I believe this is #62196, CL 526098
Though as far as I can tell, working as intended?

cc @rolandshoemaker

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 23, 2023
@rolandshoemaker
Copy link
Member

Bah, this is due to a misreading of the specification by myself, I misread SingleLineHTMLCloseComment and HTMLCloseComment, which allows SingleLineDelimitedCommentSequence before the --> token.

Slightly confusingly this means you can have /* comment */ --> more comment but not real text --> comment.

This is an unexpected breaking change, so we should backport the fix.

@gopherbot please open backport issues.

@rolandshoemaker rolandshoemaker self-assigned this Sep 25, 2023
@gopherbot gopherbot mentioned this issue Sep 25, 2023
Closed
@gopherbot
Copy link

Backport issue(s) opened: #63207 (for 1.20), #63208 (for 1.21).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot gopherbot mentioned this issue Sep 25, 2023
Open
@rolandshoemaker
Copy link
Member

This turns out to be painfully complicated to get right, SingleLineHTMLCloseComment is a lot more subtle than it appears at first glance.

Since the parser doesn't consume scripts line-by-line but rather in (possibly) larger contextual chunks, and because it does not track previous contexts that have appeared on a line, it is not immediately obvious how to determine if any content preceding the --> token fits SingleLineDelimitedCommentSequence, especially if other content on the line is in another state which was processed before we even get to the --> token (i.e. var a = "asd" --> comment? is complex, as tJS will be passed var a = "asd" --> comment? and then
--> comment?, meaning by the time we get to the close comment token, we have no information about the preceding contexts to make our decision on if this is a valid comment or not).

@dmitshur dmitshur added this to the Go1.22 milestone Oct 6, 2023
@dmitshur dmitshur modified the milestones: Go1.22, Backlog Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rolandshoemaker rolandshoemaker

Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Security
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@dmitshur @jupenur @rolandshoemaker @bcmills @gopherbot @seankhliao