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: Redefining a template inhibits escaping #22780

Closed
alin-amana opened this issue Nov 17, 2017 · 4 comments
Closed

html/template: Redefining a template inhibits escaping #22780

alin-amana opened this issue Nov 17, 2017 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@alin-amana
Copy link

I have a collection of templates t for generating a small set of HTML pages. I made the mistake of reusing the template name across all page templates (and not use Clone() to create independent sets of templates for each) and the result is no HTML escaping.

Essentially I would expect that if I use something like:

t, err := template.New("t").Parse(`{{define "T"}}Hello, {{.}}!{{end}}`)
page, err := t.New("page").Parse(`{{template "T" .}}`)
...
err = page.Execute(out, "<script>alert('you have been pwned')</script>")

I will get properly escaped HTML:

Hello, &lt;script&gt;alert(&#39;you have been pwned&#39;)&lt;/script&gt;!

Which is exactly what happens. But if I add a second t.New().Parse() call after the first one:

t, err := template.New("t").Parse(`{{define "T"}}Hello, {{.}}!{{end}}`)
page1, err := t.New("page").Parse(`{{template "T" .}}`)
page2, err := t.New("page").Parse(`something else`)
...
err = page1.Execute(out, "<script>alert('you have been pwned')</script>")

I get unescaped text:

Hello, <script>alert('you have been pwned')</script>!

Working example at https://play.golang.org/p/iqskRxgJOa . Comment out line 13 and get properly escaped HTML output.

Depending on the contents of the second redefinition, sometimes HTML escaping automagically works (it works on the playground if you use the same content for page2 as for page1, but for some reason not in my server. I'm pretty sure this has to do with the weird way associated templates share their namespace: the second template replaces the "page" template in the namespace, but page1 is now in an inconsistent state in that it is different from the "page" template from its own namespace. At this point an execution should either return with an error or automagically replace text and Tree with those in page2 (grabbing them from the namespace map). Or work properly, producing escaped HTML.

Using go version go1.9.1 linux/amd64 on Ubuntu 17.10.

@ianlancetaylor
Copy link
Contributor

Related to #21844?

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 17, 2017
@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Nov 17, 2017
@ianlancetaylor
Copy link
Contributor

CC @stjj89 @bep

@gopherbot
Copy link

Change https://golang.org/cl/78542 mentions this issue: html/template: reset templates orphaned by (*Template).New

@stjj89
Copy link
Contributor

stjj89 commented Nov 17, 2017

The problem here is that the template (page1) overwritten by the second call to t.New still holds a pointer to the t's template set. When we call Execute on the orphaned page1, the escaper uses this pointer to look up the template named "page". The underlying template set, which has been updated, returns the template "something else". This causes the escaper to escape "something else" instead of {{template "T" .}}.

Here's a minimal example demonstrating the issue: https://play.golang.org/p/WACVAFn8UO

I can reproduce this bug on commit 5c5a106, before any of my escaping changes were introduced, so this is not a regression. It isn't exactly related to #21844, which is a regression in 1.9.

The html/template docs don't really say anything about what happens to these orphaned templates, so this might just be undefined behavior that is working as intended. If we decide to do something about this, perhaps we should reset the orphaned template so that any attempt to execute it later fails? See my suggested fix at https://golang.org/cl/78542.

@golang golang locked and limited conversation to collaborators Dec 8, 2018
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