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: ambiguous errors when style tag is not closed #29269

Open
noonien opened this issue Dec 14, 2018 · 3 comments
Open

html/template: ambiguous errors when style tag is not closed #29269

noonien opened this issue Dec 14, 2018 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@noonien
Copy link

noonien commented Dec 14, 2018

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/george/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/george/.go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/george/tmp/resume/go.mod"
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-build336466026=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran the following program: https://play.golang.org/p/BtMWAAxnb3o

What did you expect to see?

Program not returning an error, or at least an error informing that the style tag was not closed.

What did you see instead?

html/template:test:11:17: {{.}} appears in an ambiguous context within a URL
@ALTree ALTree added this to the Unplanned milestone Dec 15, 2018
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 15, 2018
@empijei
Copy link
Contributor

empijei commented Mar 6, 2019

I looked into this and it is more than just ambiguous errors. It seems to be a bug that confuses the escaper into thinking that it is inside a tag and not in CSS state. (I don't think it is related to the style tag being closed or not)

This code:

	t := template.Must(template.New("base").Parse(`<style>
	{{/*This is fine*/}}
	<a href="{{.}}"></a>
	{{if true}}
		{{/*The colon is important*/}}
		<a href=":{{.}}"></a>
		{{if true}}
			{{/*This is fine*/}}
			<a href="{{.}}"></a>
		{{end}}
	{{end}}
	{{if true}}
		{{/*This is not fine*/}}
		<a href="a?{{.}}"></a>
	{{end}}
	</style>`))

	err := t.Execute(os.Stdout, "{")

Produces:

<style>
        <a href="\7b "></a>                 ← Extra trailing space?
                <a href=":\7b "></a>        ← Extra trailing space?
                        <a href="\7b "></a> ← Extra trailing space?
                <a href="a?%7b"></a>        ← Incoherent escaping
</style>

I'll look into this and try to find the root cause.

@mikesamuel how should we escape HTML stuff inside a <style> tag? I believe it should all just be escaped as CSS.

@mikesamuel
Copy link
Contributor

@empijei As you say, <style> has a special content model so there is no "a" tag in <style><a></style>.

  1. The double quotes start a URL context.
  2. The ':' and '?' indicates the end of a protocol / start of a query respectively

I'm unsure why %7b is inappropriate in a URL context which the CSS escaping assumes for quoted strings even though it's not super appropriate for selector:after { font-family: "{{.}}"; content: "{{.}}" }.

The "extra trailing space" is necessary for CSS escaping when you don't know the next character.
CSS allows a space:

EXAMPLE 3: An identifier with the value "&B" could be written as \26 B or \000026B.


how should we escape HTML stuff inside a <style> tag? I believe it should all just be escaped as CSS.

Agreed.
If we were being super-spec-pedantic, then we would treat these two cases differently:

  • <style><a></a></style>
  • <svg><style><a></style></svg>

but that would require an extra context bit for foreign content mode for no apparent benefit.

Biasing towards the second would break

<style>
/* Make <b> blue */ b { color: blue }
/* Make <p> pink */ p { color: pink }
</style>

@empijei
Copy link
Contributor

empijei commented Mar 20, 2019

Thanks @mikesamuel for the response.

I dug a little bit more into this.

About the error

The state of the parser when the reported error is returned is:

{stateCSSDqStr delimNone urlPartUnknown jsCtxRegexp attrNone elementStyle <nil>}
                         ^^^^^^^^^^^^^^

This is why the error is {{.}} appears in an ambiguous context within a URL. This happens before the end of the template is reached, so I don't think we could easily give better error messages here. We never see the unclosed <style> tag.

Some additional doubts

The parser gets in the urlPartUnknown state because it joins two states that are identical except for the urlPart. This can be reproduced with a minimal example:

	t := template.Must(template.New("a").Parse(`
	<style>{{if true}} "a{{.}}" {{end}} "{{.}}"`))
	fmt.Printf("%v",t.Execute(os.Stdout, ""))

This gives the error above because the a before the first {{.}} makes the then branch end in a urlPartPreQuery state. I wonder if the closing quotes should make the parser return into urlPartNone state instead.
Replacing "a{{.}}" with just "a" makes the parser correctly recognize that the urlPart is None.

The code below, instead, returns an error stating that template "ends in a non-text context" and both branches of the join are in urlPartNone state.

	t := template.Must(template.New("a").Parse(`
	<style>{{if true}} "{{.}}" {{end}} "{{.}}"`))
//                          ↑ Removed `a`
	fmt.Printf("%v", t.Execute(os.Stdout, ""))

This is due to the fact that in the initial part of a URL we stay in urlPartNone, and we do not consider the interpolated value to update to a urlPart* state.
This makes sense if we assume all quotes inside the interpolated value will be escaped (which they are), but leaves me puzzled on why don't we do the same for the previous example. We would also escape quotes there.

Why do closing quotes behave differently in these cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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