]> Cypherpunks repositories - gostls13.git/commitdiff
html/template: use the same escaper across multiple template executions
authorSamuel Tan <samueltan@google.com>
Wed, 5 Jul 2017 23:28:41 +0000 (16:28 -0700)
committerBrad Fitzpatrick <bradfitz@golang.org>
Fri, 14 Jul 2017 18:19:04 +0000 (18:19 +0000)
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>

src/html/template/escape.go
src/html/template/escape_test.go
src/html/template/template.go

index 92b1d086778e5da614fa0168a5ddf836e249f950..b51a37039bde7d1f391135b95202bdd0fc2622fd 100644 (file)
@@ -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.
 
index d61683b8c99c5eb6b4bdf8008f85ffbbc794ced7..f5a4ce173644159dcc3702214ff534bb9d007088 100644 (file)
@@ -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
index 246ef04dbefda0f0c16728724b505c24191767f3..6a661bf6e5d61e5fc7342fd8f2645fef54a19d04 100644 (file)
@@ -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