html/template: use the same escaper across multiple template executions

The escaper contains information about which templates have already been
visited and escaped. This information is necessary to prevent templates
that have already been escaped from being over-escaped. However, since we
currently create a new escaper each time we execute a template, this
information does not persist across multiple template executions.

Fix this by saving an escaper in each template name space which is shared by
all templates in that name space.

While there, fix error message formatting for an escaping unit test.

Fixes #20842

Change-Id: Ie392c3e7ce0e0a9947bdf56c99e926e7c7db76e4
Reviewed-on: https://go-review.googlesource.com/47256
Reviewed-by: Mike Samuel <mikesamuel@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

3 files changed, 79 insertions, 32 deletions

diff --git a/src/html/template/escape.go b/src/html/template/escape.go
index 92b1d08677..b51a37039b 100644
--- a/src/html/template/escape.go
+++ b/src/html/template/escape.go
@@ -19,8 +19,7 @@ import (
 // been modified. Otherwise the named templates have been rendered
 // unusable.
 func escapeTemplate(tmpl *Template, node parse.Node, name string) error {
-	e := newEscaper(tmpl)
-	c, _ := e.escapeTree(context{}, node, name, 0)
+	c, _ := tmpl.esc.escapeTree(context{}, node, name, 0)
 	var err error
 	if c.err != nil {
 		err, c.err.Name = c.err, name
@@ -36,7 +35,7 @@ func escapeTemplate(tmpl *Template, node parse.Node, name string) error {
 		}
 		return err
 	}
-	e.commit()
+	tmpl.esc.commit()
 	if t := tmpl.set[name]; t != nil {
 		t.escapeErr = escapeOK
 		t.Tree = t.text.Tree
@@ -81,7 +80,8 @@ var funcMap = template.FuncMap{
 // escaper collects type inferences about templates and changes needed to make
 // templates injection safe.
 type escaper struct {
-	tmpl *Template
+	// ns is the nameSpace that this escaper is associated with.
+	ns *nameSpace
 	// output[templateName] is the output context for a templateName that
 	// has been mangled to include its input context.
 	output map[string]context
@@ -98,10 +98,10 @@ type escaper struct {
 	textNodeEdits     map[*parse.TextNode][]byte
 }
 
-// newEscaper creates a blank escaper for the given set.
-func newEscaper(t *Template) *escaper {
-	return &escaper{
-		t,
+// makeEscaper creates a blank escaper for the given set.
+func makeEscaper(n *nameSpace) escaper {
+	return escaper{
+		n,
 		map[string]context{},
 		map[string]*template.Template{},
 		map[string]bool{},
@@ -491,13 +491,13 @@ func (e *escaper) escapeList(c context, n *parse.ListNode) context {
 // It returns the best guess at an output context, and the result of the filter
 // which is the same as whether e was updated.
 func (e *escaper) escapeListConditionally(c context, n *parse.ListNode, filter func(*escaper, context) bool) (context, bool) {
-	e1 := newEscaper(e.tmpl)
+	e1 := makeEscaper(e.ns)
 	// Make type inferences available to f.
 	for k, v := range e.output {
 		e1.output[k] = v
 	}
 	c = e1.escapeList(c, n)
-	ok := filter != nil && filter(e1, c)
+	ok := filter != nil && filter(&e1, c)
 	if ok {
 		// Copy inferences and edits from e1 back into e.
 		for k, v := range e1.output {
@@ -546,7 +546,7 @@ func (e *escaper) escapeTree(c context, node parse.Node, name string, line int)
 	if t == nil {
 		// Two cases: The template exists but is empty, or has never been mentioned at
 		// all. Distinguish the cases in the error messages.
-		if e.tmpl.set[name] != nil {
+		if e.ns.set[name] != nil {
 			return context{
 				state: stateError,
 				err:   errorf(ErrNoSuchTemplate, node, line, "%q is an incomplete or empty template", name),
@@ -794,8 +794,11 @@ func (e *escaper) commit() {
 	for name := range e.output {
 		e.template(name).Funcs(funcMap)
 	}
+	// Any template from the name space associated with this escaper can be used
+	// to add derived templates to the underlying text/template name space.
+	tmpl := e.arbitraryTemplate()
 	for _, t := range e.derived {
-		if _, err := e.tmpl.text.AddParseTree(t.Name(), t.Tree); err != nil {
+		if _, err := tmpl.text.AddParseTree(t.Name(), t.Tree); err != nil {
 			panic("error adding derived template")
 		}
 	}
@@ -808,17 +811,34 @@ func (e *escaper) commit() {
 	for n, s := range e.textNodeEdits {
 		n.Text = s
 	}
+	// Reset state that is specific to this commit so that the same changes are
+	// not re-applied to the template on subsequent calls to commit.
+	e.called = make(map[string]bool)
+	e.actionNodeEdits = make(map[*parse.ActionNode][]string)
+	e.templateNodeEdits = make(map[*parse.TemplateNode]string)
+	e.textNodeEdits = make(map[*parse.TextNode][]byte)
 }
 
 // template returns the named template given a mangled template name.
 func (e *escaper) template(name string) *template.Template {
-	t := e.tmpl.text.Lookup(name)
+	// Any template from the name space associated with this escaper can be used
+	// to look up templates in the underlying text/template name space.
+	t := e.arbitraryTemplate().text.Lookup(name)
 	if t == nil {
 		t = e.derived[name]
 	}
 	return t
 }
 
+// arbitraryTemplate returns an arbitrary template from the name space
+// associated with e and panics if no templates are found.
+func (e *escaper) arbitraryTemplate() *Template {
+	for _, t := range e.ns.set {
+		return t
+	}
+	panic("no templates in name space")
+}
+
 // Forwarding functions so that clients need only import this package
 // to reach the general escaping functions of text/template.
 
diff --git a/src/html/template/escape_test.go b/src/html/template/escape_test.go
index d61683b8c9..f5a4ce1736 100644
--- a/src/html/template/escape_test.go
+++ b/src/html/template/escape_test.go
@@ -1564,7 +1564,7 @@ func TestEscapeText(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		b, e := []byte(test.input), newEscaper(nil)
+		b, e := []byte(test.input), makeEscaper(nil)
 		c := e.escapeText(context{}, &parse.TextNode{NodeType: parse.NodeText, Text: b})
 		if !test.output.eq(c) {
 			t.Errorf("input %q: want context\n\t%v\ngot\n\t%v", test.input, test.output, c)
@@ -1671,29 +1671,21 @@ func TestEnsurePipelineContains(t *testing.T) {
 			".X | html",
 			[]string{"_html_template_rcdataescaper"},
 		},
-		{
-			"{{.X | html}}",
-			".X | html | html",
-			[]string{"_html_template_htmlescaper", "_html_template_attrescaper"},
-		},
-		{
-			"{{.X | html}}",
-			".X | html | html",
-			[]string{"_html_template_rcdataescaper", "_html_template_attrescaper"},
-		},
 	}
 	for i, test := range tests {
 		tmpl := template.Must(template.New("test").Parse(test.input))
 		action, ok := (tmpl.Tree.Root.Nodes[0].(*parse.ActionNode))
 		if !ok {
-			t.Errorf("#%d: First node is not an action: %s", i, test.input)
+			t.Errorf("First node is not an action: %s", test.input)
 			continue
 		}
 		pipe := action.Pipe
+		originalIDs := make([]string, len(test.ids))
+		copy(originalIDs, test.ids)
 		ensurePipelineContains(pipe, test.ids)
 		got := pipe.String()
 		if got != test.output {
-			t.Errorf("#%d: %s, %v: want\n\t%s\ngot\n\t%s", i, test.input, test.ids, test.output, got)
+			t.Errorf("#%d: %s, %v: want\n\t%s\ngot\n\t%s", i, test.input, originalIDs, test.output, got)
 		}
 	}
 }
@@ -1855,6 +1847,40 @@ func TestErrorOnUndefined(t *testing.T) {
 	}
 }
 
+// This covers issue #20842.
+func TestIdempotentExecute(t *testing.T) {
+	tmpl := Must(New("").
+		Parse(`{{define "main"}}<body>{{template "hello"}}</body>{{end}}`))
+	Must(tmpl.
+		Parse(`{{define "hello"}}Hello, {{"Ladies & Gentlemen!"}}{{end}}`))
+	got := new(bytes.Buffer)
+	var err error
+	// Ensure that "hello" produces the same output when executed twice.
+	want := "Hello, Ladies &amp; Gentlemen!"
+	for i := 0; i < 2; i++ {
+		err = tmpl.ExecuteTemplate(got, "hello", nil)
+		if err != nil {
+			t.Errorf("unexpected error: %s", err)
+		}
+		if got.String() != want {
+			t.Errorf("after executing template \"hello\", got:\n\t%q\nwant:\n\t%q\n", got.String(), want)
+		}
+		got.Reset()
+	}
+	// Ensure that the implicit re-execution of "hello" during the execution of
+	// "main" does not cause the output of "hello" to change.
+	err = tmpl.ExecuteTemplate(got, "main", nil)
+	if err != nil {
+		t.Errorf("unexpected error: %s", err)
+	}
+	// If the HTML escaper is added again to the action {{"Ladies & Gentlemen!"}},
+	// we would expected to see the ampersand overescaped to "&amp;amp;".
+	want = "<body>Hello, Ladies &amp; Gentlemen!</body>"
+	if got.String() != want {
+		t.Errorf("after executing template \"main\", got:\n\t%q\nwant:\n\t%q\n", got.String(), want)
+	}
+}
+
 func BenchmarkEscapedExecute(b *testing.B) {
 	tmpl := Must(New("t").Parse(`<a onclick="alert('{{.}}')">{{.}}</a>`))
 	var buf bytes.Buffer
diff --git a/src/html/template/template.go b/src/html/template/template.go
index 246ef04dbe..6a661bf6e5 100644
--- a/src/html/template/template.go
+++ b/src/html/template/template.go
@@ -36,6 +36,7 @@ type nameSpace struct {
 	mu      sync.Mutex
 	set     map[string]*Template
 	escaped bool
+	esc     escaper
 }
 
 // Templates returns a slice of the templates associated with t, including t
@@ -250,13 +251,13 @@ func (t *Template) Clone() (*Template, error) {
 	if err != nil {
 		return nil, err
 	}
+	ns := &nameSpace{set: make(map[string]*Template)}
+	ns.esc = makeEscaper(ns)
 	ret := &Template{
 		nil,
 		textClone,
 		textClone.Tree,
-		&nameSpace{
-			set: make(map[string]*Template),
-		},
+		ns,
 	}
 	ret.set[ret.Name()] = ret
 	for _, x := range textClone.Templates() {
@@ -279,13 +280,13 @@ func (t *Template) Clone() (*Template, error) {
 
 // New allocates a new HTML template with the given name.
 func New(name string) *Template {
+	ns := &nameSpace{set: make(map[string]*Template)}
+	ns.esc = makeEscaper(ns)
 	tmpl := &Template{
 		nil,
 		template.New(name),
 		nil,
-		&nameSpace{
-			set: make(map[string]*Template),
-		},
+		ns,
 	}
 	tmpl.set[name] = tmpl
 	return tmpl