From 882a640421c5ac480d40ae02736c95046fec11aa Mon Sep 17 00:00:00 2001
From: Samuel Tan <samueltan@google.com>
Date: Thu, 11 May 2017 16:56:14 -0700
Subject: [PATCH] html/template: only search identifier nodes for predefined
 escapers

Predefined escapers (i.e. "html" and "urlquery") should only occur in
Identifier nodes, and never in Field or Chain nodes, since these are
global functions that return string values (see inline comments for more
details). Therefore, skip Chain and Field nodes when searching for
predefined escapers in template pipelines.

Also, make a non-functional change two existing test cases to avoid
giving the impression that it is valid to reference a field of a
predefined escaper.

Fixes #20323

Change-Id: I34f722f443c778699fcdd575dc3e0fd1fd6f2eb3
Reviewed-on: https://go-review.googlesource.com/43296
Reviewed-by: Samuel Tan <samueltan@google.com>
Reviewed-by: Mike Samuel <mikesamuel@gmail.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
---
 src/html/template/escape.go      | 40 ++++++++++++++----------------
 src/html/template/escape_test.go | 42 +++++++++++++++++++++++++++++---
 2 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/src/html/template/escape.go b/src/html/template/escape.go
index 3037e07b296..92b1d086778 100644
--- a/src/html/template/escape.go
+++ b/src/html/template/escape.go
@@ -139,20 +139,6 @@ func (e *escaper) escape(c context, n parse.Node) context {
 	panic("escaping " + n.String() + " is unimplemented")
 }
 
-// allIdents returns the names of the identifiers under the Ident field of the node,
-// which might be a singleton (Identifier) or a slice (Field or Chain).
-func allIdents(node parse.Node) []string {
-	switch node := node.(type) {
-	case *parse.IdentifierNode:
-		return []string{node.Ident}
-	case *parse.FieldNode:
-		return node.Ident
-	case *parse.ChainNode:
-		return node.Field
-	}
-	return nil
-}
-
 // escapeAction escapes an action template node.
 func (e *escaper) escapeAction(c context, n *parse.ActionNode) context {
 	if len(n.Pipe.Decl) != 0 {
@@ -162,14 +148,24 @@ func (e *escaper) escapeAction(c context, n *parse.ActionNode) context {
 	c = nudge(c)
 	// Check for disallowed use of predefined escapers in the pipeline.
 	for pos, idNode := range n.Pipe.Cmds {
-		for _, ident := range allIdents(idNode.Args[0]) {
-			if _, ok := predefinedEscapers[ident]; ok {
-				if pos < len(n.Pipe.Cmds)-1 ||
-					c.state == stateAttr && c.delim == delimSpaceOrTagEnd && ident == "html" {
-					return context{
-						state: stateError,
-						err:   errorf(ErrPredefinedEscaper, n, n.Line, "predefined escaper %q disallowed in template", ident),
-					}
+		node, ok := idNode.Args[0].(*parse.IdentifierNode)
+		if !ok {
+			// A predefined escaper "esc" will never be found as an identifier in a
+			// Chain or Field node, since:
+			// - "esc.x ..." is invalid, since predefined escapers return strings, and
+			//   strings do not have methods, keys or fields.
+			// - "... .esc" is invalid, since predefined escapers are global functions,
+			//   not methods or fields of any types.
+			// Therefore, it is safe to ignore these two node types.
+			continue
+		}
+		ident := node.Ident
+		if _, ok := predefinedEscapers[ident]; ok {
+			if pos < len(n.Pipe.Cmds)-1 ||
+				c.state == stateAttr && c.delim == delimSpaceOrTagEnd && ident == "html" {
+				return context{
+					state: stateError,
+					err:   errorf(ErrPredefinedEscaper, n, n.Line, "predefined escaper %q disallowed in template", ident),
 				}
 			}
 		}
diff --git a/src/html/template/escape_test.go b/src/html/template/escape_test.go
index 865226f8551..d61683b8c99 100644
--- a/src/html/template/escape_test.go
+++ b/src/html/template/escape_test.go
@@ -685,6 +685,40 @@ func TestEscape(t *testing.T) {
 	}
 }
 
+func TestEscapeMap(t *testing.T) {
+	data := map[string]string{
+		"html":     `<h1>Hi!</h1>`,
+		"urlquery": `http://www.foo.com/index.html?title=main`,
+	}
+	for _, test := range [...]struct {
+		desc, input, output string
+	}{
+		// covering issue 20323
+		{
+			"field with predefined escaper name 1",
+			`{{.html | print}}`,
+			`&lt;h1&gt;Hi!&lt;/h1&gt;`,
+		},
+		// covering issue 20323
+		{
+			"field with predefined escaper name 2",
+			`{{.urlquery | print}}`,
+			`http://www.foo.com/index.html?title=main`,
+		},
+	} {
+		tmpl := Must(New("").Parse(test.input))
+		b := new(bytes.Buffer)
+		if err := tmpl.Execute(b, data); err != nil {
+			t.Errorf("%s: template execution failed: %s", test.desc, err)
+			continue
+		}
+		if w, g := test.output, b.String(); w != g {
+			t.Errorf("%s: escaped output: want\n\t%q\ngot\n\t%q", test.desc, w, g)
+			continue
+		}
+	}
+}
+
 func TestEscapeSet(t *testing.T) {
 	type dataItem struct {
 		Children []*dataItem
@@ -1595,14 +1629,14 @@ func TestEnsurePipelineContains(t *testing.T) {
 		},
 		{
 			// covering issue 10801
-			"{{.X | js.x }}",
-			".X | js.x | urlquery | html",
+			"{{.X | println.x }}",
+			".X | println.x | urlquery | html",
 			[]string{"urlquery", "html"},
 		},
 		{
 			// covering issue 10801
-			"{{.X | (print 12 | js).x }}",
-			".X | (print 12 | js).x | urlquery | html",
+			"{{.X | (print 12 | println).x }}",
+			".X | (print 12 | println).x | urlquery | html",
 			[]string{"urlquery", "html"},
 		},
 		// The following test cases ensure that the merging of internal escapers