golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123k stars 17.54k forks source link

html/template: script type attribute inside condition causes an error #59112

Open willfaught opened 1 year ago

willfaught commented 1 year ago

What did you do?

Template:

<script {{ if true }} type="" {{ end }}></script>

What did you expect to see?

What did you see instead?

{{if}} branches end in different contexts:
{stateTag delimNone urlPartNone jsCtxRegexp attrNone elementNone <nil>},
{stateTag delimNone urlPartNone jsCtxRegexp attrNone elementScript <nil>}

If you remove the condition around the type attribute, or rename type to something else, then there's no error.

Remarks

The <script> type attribute value should be able to be dynamic to substitute in the various valid values for type:

Note that the original template was:

<script {{ with $type }} type="{{ . }}" {{ end }}></script>

See here for the original context.

This is a valid construct for every other HTML attribute I've ever tried it on.

This issue was already reported in https://github.com/golang/go/issues/57136, however it's clear that the maintainer who closed the issue didn't understand it, as the author explained after it was closed. There was no reply.

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

$ go version
go version go1.20.2 darwin/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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/Will/Library/Caches/go-build"
GOENV="/Users/Will/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/Will/Library/Application Support/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/Will/Library/Application Support/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.20.2/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.20.2/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.20.2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bx/qk0phsxd265fqj512dnnpg080000gn/T/go-build282336036=/tmp/go-build -gno-record-gcc-switches -fno-common"
seankhliao commented 1 year ago

cc @golang/security

ianlancetaylor commented 1 year ago

This is related to #6701 and #12149. The type expression is meaningful within a <script> tag, to say what kind of script it is. The error message is confusing, but I'm not sure there is anything to fix here other than perhaps generating a better error message.

CC @robpike

willfaught commented 1 year ago

@ianlancetaylor I don't understand how this is related to weird semicolons in markup input or invalid quotations in markup output. What is the correct error message in this case?

seankhliao commented 1 year ago

While type="" (or unset type) might equal type="text/javascript", javascript isn't the only allowed type of content in <script> tags. ("Any other value" in the MDN link you provided).

Different content types require different types of escaping, and with a dynamic value for type=, it becomes impossible for html/template to know which sort of escaping (if any) should be applied to the following content.

willfaught commented 1 year ago

Different content types require different types of escaping,

When type=module, there's no inner text, so it's not an issue in that case, but I can see how encoding is an issue for language MIME types.

it becomes impossible for html/template to know which sort of escaping (if any) should be applied to the following content

Then shouldn't constructing a dynamic type attribute value with the printf function be disallowed as well? That is currently allowed, at least in Hugo in combination with Hugo's safeHTMLAttr function.

ianlancetaylor commented 1 year ago

I don't understand how this is related to weird semicolons in markup input or invalid quotations in markup output. What is the correct error message in this case?

I don't know for sure, but perhaps a better error message would be something like "attempt to change script type in conditional context". I don't see how html/template can be expected to correctly handle such a case.

Then shouldn't constructing a dynamic type attribute value with the printf function be disallowed as well? That is currently allowed, at least in Hugo in combination with Hugo's safeHTMLAttr function.

Perhaps I'm missing something, but that seems like an issue with Hugo, not html/template as such.

willfaught commented 1 year ago

I don't see how html/template can be expected to correctly handle such a case.

Just spit balling: Would it be possible to evaluate the tag first, then the inner text once the escaping is known? Then type wouldn't have to be special-cased.

Regardless, the behavior is surprising, and it should be explained in the package doc, along with any other special cases, in my opinion.

that seems like an issue with Hugo, not html/template as such.

Hugo uses html/template under the hood. I assume safeHTMLAttr is a normal custom function that html/template allows, but perhaps that's wrong. @jmooring or @bep might know more.

gopherbot commented 1 year ago

Change https://go.dev/cl/496145 mentions this issue: html/template: example for disallowed script type change