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.

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

html/template: ``literal cut off #29406

Closed
eternal-flame-AD opened this issue Dec 24, 2018 · 5 comments
Closed

html/template: ``literal cut off #29406

eternal-flame-AD opened this issue Dec 24, 2018 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@eternal-flame-AD
Copy link

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

$ go version
go version go1.11.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ef/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ef/code/go/"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build993540070=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I tried to insert a raw js string literal in an html template with a substring // (two forward slashes)

https://play.golang.org/p/XKpExxgMybM

What did you expect to see?

Template is:

	<script>
	console.log(`a/${url}`);
        console.log(`a//${url}`);
	</script>

Result is:

<script>
console.log(`a/${url}`);
    console.log(`a//${url}`);
</script>
The original template was output as their are no template directives present

What did you see instead?

Template is:

	<script>
	console.log(`a/${url}`);
        console.log(`a//${url}`);
	</script>

Result is:

<script>
console.log(`a/${url}`);
    console.log(`a
</script>
The second raw string literal is cut off in the middle
@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 24, 2018
@agnivade agnivade added this to the Go1.13 milestone Dec 24, 2018
@agnivade
Copy link
Contributor

/cc @mvdan

@mvdan
Copy link
Member

mvdan commented Dec 24, 2018

Smaller repro: https://play.golang.org/p/Me6SXmTA4Kt

I see that this happens with backquotes, but not single or double quotes. It appears like backquotes were added much later in JS, and have been modified in ES2016 and ES2018: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

Adding support for them, as long as they're well understood and the code is simple, seems fair enough to me. I don't know what the policy is in regards to what JS features html/template shoud incorporate.

@mvdan
Copy link
Member

mvdan commented Dec 24, 2018

After a bit of search, I realised this was brought up years ago, so I'm closing this issue as a duplicate of #9200.

@mvdan mvdan closed this as completed Dec 24, 2018
@eternal-flame-AD
Copy link
Author

eternal-flame-AD commented Dec 25, 2018

After a bit of search, I realised this was brought up years ago, so I'm closing this issue as a duplicate of #9200.

@mvdan Wow that is a brilliiant discovery, but I think there is a bit of a difference between these two issues.

#9200 mentioned that template directives that took place inside quasi literals are not properly escaped to get rid of injected placeholders with executable code. Thus it requires at least one formatting directive to be reproduced. However, in this issue the reproduction does not need any formatting directive, the reproduction only consisted of a plain template string.

Though I am pretty sure that both issues are caused by lack of support on ES6 formatting string. I still have another concern regarding the package:

Shouldn't the package just copy what is written on the template as is, if it does not understand what it means? I think instead of silently discarding values without returning any error, which might cause confusion while debugging, maybe this behavior is better:

If (part of) the template could not be understood:

  • If this part does not include any formatting directives,
    • If the parser did not hit any formatting directive before the affected region is considered definitely ended(a </script> tag, etc.), continue executing the template.
    • If the parser could not be sure if the unknown part could play a role in the behavior of the following parts of the template and a formatting directive is hit, the template is considered not being able to be executed safely and thus an error is returned.
  • If this part include formatting directives, give up executing the template and return an error.

@mvdan
Copy link
Member

mvdan commented Dec 25, 2018

I've retitled the older issue to be about adding support for template strings. Now this issue is more clearly a duplicate - both issues would be fixed by adding that feature.

Shouldn't the package just copy what is written on the template as is, if it does not understand what it means?

No, that would be very complex and subtle. Right now, some of your template bytes are being discarded because they are being parsed as a comment. If you want to work around that before #9200 is fixed, somehow remove the double slash, or instead use single or double quotes.

Something we could likely do in the short term is teach html/template about tokens it doesn't yet support, like backticks. Then your template would get a parse error. I'm not sure if that's better than what we currently have, where these unknown tokens are parsed like any other character.

@golang golang locked and limited conversation to collaborators Dec 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants