html/template: fix two unrelated bugs

1) The code to catch an exception marked the template as escaped when it was not yet, which caused subsequent executions of the template to not escape properly. 2) ensurePipelineContains needs to handled Field as well as Identifier nodes. Fixes #7379. LGTM=mikesamuel R=mikesamuel CC=golang-codereviews https://golang.org/cl/85240043

html/template: fix two unrelated bugs
1) The code to catch an exception marked the template as escaped when it was not yet, which caused subsequent executions of the template to not escape properly. 2) ensurePipelineContains needs to handled Field as well as Identifier nodes. Fixes #7379. LGTM=mikesamuel R=mikesamuel CC=golang-codereviews https://golang.org/cl/85240043
51fba7d8 · Rob Pike · 56294f4a · 51fba7d8 · 51fba7d8
Commit 51fba7d8 authored Apr 09, 2014 by Rob Pike
Hide whitespace changes
Inline Side-by-side

Showing with 68 additions and 15 deletions

src/pkg/html/template/escape.go src/pkg/html/template/escape.go +36 -15

src/pkg/html/template/escape_test.go src/pkg/html/template/escape_test.go +32 -0

No files found.
--- a/src/pkg/html/template/escape.go
+++ b/src/pkg/html/template/escape.go
@@ -40,10 +40,14 @@ func escapeTemplates(tmpl *Template, names ...string) error {
 			}
 			return err
 		}
-		tmpl.escaped = true
-		tmpl.Tree = tmpl.text.Tree
 	}
 	e.commit()
+	for _, name := range names {
+		if t := tmpl.set[name]; t != nil {
+			t.escaped = true
+			t.Tree = t.text.Tree
+		}
+	}
 	return nil
 }

@@ -207,6 +211,19 @@ func (e *escaper) escapeAction(c context, n *parse.ActionNode) context {
 	return c
 }

+// allIdents returns the names of the identifiers under the Ident field of the node,
+// which might be a singleton (Identifier) or a slice (Field).
+func allIdents(node parse.Node) []string {
+	switch node := node.(type) {
+	case *parse.IdentifierNode:
+		return []string{node.Ident}
+	case *parse.FieldNode:
+		return node.Ident
+	}
+	panic("unidentified node type in allIdents")
+	return nil
+}
+
 // ensurePipelineContains ensures that the pipeline has commands with
 // the identifiers in s in order.
 // If the pipeline already has some of the sanitizers, do not interfere.
@@ -229,27 +246,31 @@ func ensurePipelineContains(p *parse.PipeNode, s []string) {
 		idents = p.Cmds[i+1:]
 	}
 	dups := 0
-	for _, id := range idents {
-		if escFnsEq(s[dups], (id.Args[0].(*parse.IdentifierNode)).Ident) {
-			dups++
-			if dups == len(s) {
-				return
+	for _, idNode := range idents {
+		for _, ident := range allIdents(idNode.Args[0]) {
+			if escFnsEq(s[dups], ident) {
+				dups++
+				if dups == len(s) {
+					return
+				}
 			}
 		}
 	}
 	newCmds := make([]*parse.CommandNode, n-len(idents), n+len(s)-dups)
 	copy(newCmds, p.Cmds)
 	// Merge existing identifier commands with the sanitizers needed.
-	for _, id := range idents {
-		pos := id.Args[0].Position()
-		i := indexOfStr((id.Args[0].(*parse.IdentifierNode)).Ident, s, escFnsEq)
-		if i != -1 {
-			for _, name := range s[:i] {
-				newCmds = appendCmd(newCmds, newIdentCmd(name, pos))
+	for _, idNode := range idents {
+		pos := idNode.Args[0].Position()
+		for _, ident := range allIdents(idNode.Args[0]) {
+			i := indexOfStr(ident, s, escFnsEq)
+			if i != -1 {
+				for _, name := range s[:i] {
+					newCmds = appendCmd(newCmds, newIdentCmd(name, pos))
+				}
+				s = s[i+1:]
 			}
-			s = s[i+1:]
 		}
-		newCmds = appendCmd(newCmds, id)
+		newCmds = appendCmd(newCmds, idNode)
 	}
 	// Create any remaining sanitizers.
 	for _, name := range s {

--- a/src/pkg/html/template/escape_test.go
+++ b/src/pkg/html/template/escape_test.go
@@ -1649,6 +1649,38 @@ func TestEmptyTemplate(t *testing.T) {
 	}
 }

+type Issue7379 int
+
+func (Issue7379) SomeMethod(x int) string {
+	return fmt.Sprintf("<%d>", x)
+}
+
+// This is a test for issue 7379: type assertion error caused panic, and then
+// the code to handle the panic breaks escaping. It's hard to see the second
+// problem once the first is fixed, but its fix is trivial so we let that go. See
+// the discussion for issue 7379.
+func TestPipeToMethodIsEscaped(t *testing.T) {
+	tmpl := Must(New("x").Parse("<html>{{0 | .SomeMethod}}</html>\n"))
+	tryExec := func() string {
+		defer func() {
+			panicValue := recover()
+			if panicValue != nil {
+				t.Errorf("panicked: %v\n", panicValue)
+			}
+		}()
+		var b bytes.Buffer
+		tmpl.Execute(&b, Issue7379(0))
+		return b.String()
+	}
+	for i := 0; i < 3; i++ {
+		str := tryExec()
+		const expect = "<html>&lt;0&gt;</html>\n"
+		if str != expect {
+			t.Errorf("expected %q got %q", expect, str)
+		}
+	}
+}
+
 func BenchmarkEscapedExecute(b *testing.B) {
 	tmpl := Must(New("t").Parse(`<a onclick="alert('{{.}}')">{{.}}</a>`))
 	var buf bytes.Buffer