diff options
author | Mike Samuel <mikesamuel@gmail.com> | 2011-09-15 19:05:33 -0700 |
---|---|---|
committer | Mike Samuel <mikesamuel@gmail.com> | 2011-09-15 19:05:33 -0700 |
commit | 8512f4191da7476a2ec7fefc0b7b356a6a7d23ed (patch) | |
tree | f12f00a438827b22f104a9df179a74e0aef2ce02 | |
parent | cd60ce4f9b82195d2444b9def68c23a54350d7af (diff) | |
download | go-8512f4191da7476a2ec7fefc0b7b356a6a7d23ed.tar.gz |
exp/template/html: moved error docs out of package docs onto error codes
This replaces the errStr & errLine members of context with a single err
*Error, and introduces a number of const error codes, one per
escape-time failure mode, that can be separately documented.
The changes to the error documentation moved from doc.go to error.go
are cosmetic.
R=r, nigeltao
CC=golang-dev
http://codereview.appspot.com/5026041
-rw-r--r-- | src/pkg/exp/template/html/Makefile | 1 | ||||
-rw-r--r-- | src/pkg/exp/template/html/context.go | 6 | ||||
-rw-r--r-- | src/pkg/exp/template/html/doc.go | 174 | ||||
-rw-r--r-- | src/pkg/exp/template/html/error.go | 194 | ||||
-rw-r--r-- | src/pkg/exp/template/html/escape.go | 35 | ||||
-rw-r--r-- | src/pkg/exp/template/html/escape_test.go | 10 | ||||
-rw-r--r-- | src/pkg/exp/template/html/transition.go | 33 |
7 files changed, 235 insertions, 218 deletions
diff --git a/src/pkg/exp/template/html/Makefile b/src/pkg/exp/template/html/Makefile index e53270c9c..9032aead8 100644 --- a/src/pkg/exp/template/html/Makefile +++ b/src/pkg/exp/template/html/Makefile @@ -11,6 +11,7 @@ GOFILES=\ context.go\ css.go\ doc.go\ + error.go\ escape.go\ html.go\ js.go\ diff --git a/src/pkg/exp/template/html/context.go b/src/pkg/exp/template/html/context.go index bfe168f64..e8812cf86 100644 --- a/src/pkg/exp/template/html/context.go +++ b/src/pkg/exp/template/html/context.go @@ -21,8 +21,7 @@ type context struct { urlPart urlPart jsCtx jsCtx element element - errLine int - errStr string + err *Error } // eq returns whether two contexts are equal. @@ -32,8 +31,7 @@ func (c context) eq(d context) bool { c.urlPart == d.urlPart && c.jsCtx == d.jsCtx && c.element == d.element && - c.errLine == d.errLine && - c.errStr == d.errStr + c.err == d.err } // mangle produces an identifier that includes a suffix that distinguishes it diff --git a/src/pkg/exp/template/html/doc.go b/src/pkg/exp/template/html/doc.go index 2751ce834..a9b78ca51 100644 --- a/src/pkg/exp/template/html/doc.go +++ b/src/pkg/exp/template/html/doc.go @@ -69,178 +69,8 @@ in this case, Errors -This section describes the errors returned by EscapeSet. Each error is -illustrated by an example that triggers the error, followed by an explanation -of the problem. - -Error: "... appears in an ambiguous URL context" -Example: - <a href=" - {{if .C}} - /path/ - {{else}} - /search?q= - {{end}} - {{.X}} - "> -Discussion: - {{.X}} is in an ambiguous URL context since, depending on {{.C}}, it may be - either a URL suffix or a query parameter. - Moving {{.X}} into the condition removes the ambiguity: - <a href="{{if .C}}/path/{{.X}}{{else}}/search?q={{.X}}"> - - -Error: "... appears inside a comment" -Example: -*/ -// <!-- {{.X}} --> -// <script>/* {{.X}} */</script> -// <style>/* {{.X}} */</style> -/* -Discussion: - {{.X}} appears inside a comment. There is no escaping convention for - comments. To use IE conditional comments, inject the - whole comment as a type string (see below). - To comment out code, break the {{...}}. +See the documentation of ErrorCode for details. -Error: "{{if}} branches end in different contexts" -Example: - {{if .C}}<a href="{{end}}{{.X}} -Discussion: - EscapeSet statically examines each possible path when it encounters a {{if}}, - {{range}}, or {{with}} to escape any following pipelines. The example is - ambiguous since {{.X}} might be an HTML text node, or a URL prefix in an - HTML attribute. EscapeSet needs to understand the context of {{.X}} to escape - it, but that depends on the run-time value of {{.C}}. - - The problem is usually something like missing quotes or angle brackets, or - can be avoided by refactoring to put the two contexts into different - branches of an if, range or with. Adding an {{else}} might help. - - First, look for a bug in your template. Missing quotes or '>' can trigger - this error. - - {{if .C}}<div ... class="foo>{{end}} <- No quote after foo - - Second, try refactoring your template. - - {{if .C}}<script>alert({{end}}{{.X}}{{if .C}})</script>{{end}} - - -> - - {{if .C}}<script>alert({{.X}})</script>{{else}}{{.X}}{{end}} - - Third, check for {{range}}s that have no {{else}} - - <a href="/search - {{range $i, $v := .}} - {{if $i}}&{{else}}?{{end}} - v={{$v}} - {{end}} - &x={{.X}} - "> - - looks good, but if {{.}} is empty then the URL is /search&x=... - where {{.X}} is not guaranteed to be in a URL query. - EscapeSet cannot prove which {{range}} collections are never non-empty, so - add an {{else}} - - <a href="{{range ...}}...{{end}}&x={{X}}"> - - -> - - <a href="{{range ...}}...{{else}}?{{end}}&x={{.X}}"> - - Fourth, contact the mailing list. You may have a useful pattern that - EscapeSet does not yet support, and we can work with you. - - -Error: "... ends in a non-text context: ..." -Examples: - <div - <div title="no close quote> - <script>f() -Discussion: - EscapeSet assumes the ouput is a DocumentFragment of HTML. - Templates that end without closing tags will trigger this warning. - Templates that produce incomplete Fragments should not be named - in the call to EscapeSet. - - -If you have a helper template in your set that is not meant to produce a - document fragment, then do not pass its name to EscapeSet(set, ...names). - - {{define "main"}} <script>{{template "helper"}}</script> {{end}} - {{define "helper"}} document.write(' <div title=" ') {{end}} - - "helper" does not produce a valid document fragment, though it does - produce a valid JavaScript Program. - -"must specify names of top level templates" - - EscapeSet does not assume that all templates in a set produce HTML. - Some may be helpers that produce snippets of other languages. - Passing in no template names is most likely an error, so EscapeSet(set) will - panic. - If you call EscapeSet with a slice of names, guard it with a len check: - - if len(names) != 0 { - set, err := EscapeSet(set, ...names) - } - -Error: "no such template ..." -Examples: - {{define "main"}}<div {{template "attrs"}}>{{end}} - {{define "attrs"}}href="{{.URL}}"{{end}} -Discussion: - EscapeSet looks through template calls to compute the context. - Here the {{.URL}} in "attrs" must be treated as a URL when called from "main", - but if "attrs" is not in set when EscapeSet(&set, "main") is called, this - error will arise. - -Error: "on range loop re-entry: ..." -Example: - {{range .}}<p class={{.}}{{end}} -Discussion: - If an iteration through a range would cause it to end in - a different context than an earlier pass, there is no single context. - In the example, the <p> tag is missing a '>'. - EscapeSet cannot tell whether {{.}} is meant to be an HTML class or the - content of a broken <p> element and complains because the second iteration - would produce something like - - <p class=foo<p class=bar - -Error: "unfinished escape sequence in ..." -Example: - <script>alert("\{{.X}}")</script> -Discussion: - EscapeSet does not support actions following a backslash. - This is usually an error and there are better solutions; for - our example - <script>alert("{{.X}}")</script> - should work, and if {{.X}} is a partial escape sequence such as - "xA0", give it the type ContentTypeJSStr and include the whole - sequence, as in - {`\xA0`, ContentTypeJSStr} - -Error: "unfinished JS regexp charset in ..." -Example: - <script>var pattern = /foo[{{.Chars}}]/</script> -Discussion: - EscapeSet does not support interpolation into regular expression literal - character sets. - -Error: "ZgotmplZ" -Example: - <img src="{{.X}}"> - where {{.X}} evaluates to `javascript:...` -Discussion: - "ZgotmplZ" is a special value that indicates that unsafe content reached - a CSS or URL context at runtime. The output of the example will be - <img src="#ZgotmplZ"> - If the data can be trusted, giving the string type XXX will exempt - it from filtering. A fuller picture @@ -249,8 +79,6 @@ details necessary to understand escaping contexts and error messages. Most users will not need to understand these details. - - Contexts Assuming {{.}} is `O'Reilly: How are <i>you</i>?`, the table below shows diff --git a/src/pkg/exp/template/html/error.go b/src/pkg/exp/template/html/error.go new file mode 100644 index 000000000..5fa235743 --- /dev/null +++ b/src/pkg/exp/template/html/error.go @@ -0,0 +1,194 @@ +// Copyright 2011 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package html + +import ( + "fmt" +) + +// Error describes a problem encountered during template Escaping. +type Error struct { + // ErrorCode describes the kind of error. + ErrorCode ErrorCode + // Name is the name of the template in which the error was encountered. + Name string + // Line is the line number of the error in the template source or 0. + Line int + // Description is a human-readable description of the problem. + Description string +} + +// ErrorCode is a code for a kind of error. +type ErrorCode int + +// We define codes for each error that manifests while escaping templates, but +// escaped templates may also fail at runtime. +// +// Output: "ZgotmplZ" +// Example: +// <img src="{{.X}}"> +// where {{.X}} evaluates to `javascript:...` +// Discussion: +// "ZgotmplZ" is a special value that indicates that unsafe content reached a +// CSS or URL context at runtime. The output of the example will be +// <img src="#ZgotmplZ"> +// If the data comes from a trusted source, use content types to exempt it +// from filtering: URL(`javascript:...`). +const ( + // OK indicates the lack of an error. + OK ErrorCode = iota + + // ErrorAmbigContext: "... appears in an ambiguous URL context" + // Example: + // <a href=" + // {{if .C}} + // /path/ + // {{else}} + // /search?q= + // {{end}} + // {{.X}} + // "> + // Discussion: + // {{.X}} is in an ambiguous URL context since, depending on {{.C}}, + // it may be either a URL suffix or a query parameter. + // Moving {{.X}} into the condition removes the ambiguity: + // <a href="{{if .C}}/path/{{.X}}{{else}}/search?q={{.X}}"> + ErrAmbigContext + + // TODO: document + ErrBadHTML + + // ErrBranchEnd: "{{if}} branches end in different contexts" + // Example: + // {{if .C}}<a href="{{end}}{{.X}} + // Discussion: + // EscapeSet statically examines each possible path when it encounters + // a {{if}}, {{range}}, or {{with}} to escape any following pipelines. + // The example is ambiguous since {{.X}} might be an HTML text node, + // or a URL prefix in an HTML attribute. EscapeSet needs to understand + // the context of {{.X}} to escape it, but that depends on the + // run-time value of {{.C}}. + // + // The problem is usually something like missing quotes or angle + // brackets, or can be avoided by refactoring to put the two contexts + // into different branches of an if, range or with. If the problem + // is in a {{range}} over a collection that should never be empty, + // adding a dummy {{else}} can help. + ErrBranchEnd + + // ErrEndContext: "... ends in a non-text context: ..." + // Examples: + // <div + // <div title="no close quote> + // <script>f() + // Discussion: + // EscapeSet assumes the ouput is a DocumentFragment of HTML. + // Templates that end without closing tags will trigger this error. + // Templates that produce incomplete Fragments should not be named + // in the call to EscapeSet. + // + // If you have a helper template in your set that is not meant to + // produce a document fragment, then do not pass its name to + // EscapeSet(set, ...names). + // + // {{define "main"}} <script>{{template "helper"}}</script> {{end}} + // {{define "helper"}} document.write(' <div title=" ') {{end}} + // + // "helper" does not produce a valid document fragment, though it does + // produce a valid JavaScript Program. + ErrEndContext + + // ErrInsideComment: "... appears inside a comment" + // Example: + // <!-- {{.X}} --> + // <script>/* {{.X}} */</script> + // <style>/* {{.X}} */</style> + // + // Discussion: + // {{.X}} appears inside a comment. There is no escaping convention for + // comments. To use IE conditional comments, inject the whole comment + // as an HTML, JS, or CSS value (see content.go). + // To comment out code, break the {{...}}. + ErrInsideComment + + // ErrNoNames: "must specify names of top level templates" + // + // EscapeSet does not assume that all templates in a set produce HTML. + // Some may be helpers that produce snippets of other languages. + // Passing in no template names is most likely an error, + // so EscapeSet(set) will panic. + // If you call EscapeSet with a slice of names, guard it with len: + // + // if len(names) != 0 { + // set, err := EscapeSet(set, ...names) + // } + ErrNoNames + + // ErrNoSuchTemplate: "no such template ..." + // Examples: + // {{define "main"}}<div {{template "attrs"}}>{{end}} + // {{define "attrs"}}href="{{.URL}}"{{end}} + // Discussion: + // EscapeSet looks through template calls to compute the context. + // Here the {{.URL}} in "attrs" must be treated as a URL when called + // from "main", but if "attrs" is not in set when + // EscapeSet(&set, "main") is called, this error will arise. + ErrNoSuchTemplate + + // TODO: document + ErrOutputContext + + // ErrPartialCharset: "unfinished JS regexp charset in ..." + // Example: + // <script>var pattern = /foo[{{.Chars}}]/</script> + // Discussion: + // EscapeSet does not support interpolation into regular expression + // literal character sets. + ErrPartialCharset + + // ErrPartialEscape: "unfinished escape sequence in ..." + // Example: + // <script>alert("\{{.X}}")</script> + // Discussion: + // EscapeSet does not support actions following a backslash. + // This is usually an error and there are better solutions; for + // our example + // <script>alert("{{.X}}")</script> + // should work, and if {{.X}} is a partial escape sequence such as + // "xA0", mark the whole sequence as safe content: JSStr(`\xA0`) + ErrPartialEscape + + // ErrRangeLoopReentry: "on range loop re-entry: ..." + // Example: + // {{range .}}<p class={{.}}{{end}} + // Discussion: + // If an iteration through a range would cause it to end in a + // different context than an earlier pass, there is no single context. + // In the example, the <p> tag is missing a '>'. + // EscapeSet cannot tell whether {{.}} is meant to be an HTML class or + // the content of a broken <p> element and complains because the + // second iteration would produce something like + // + // <p class=foo<p class=bar + ErrRangeLoopReentry + + // TODO: document + ErrSlashAmbig +) + +func (e *Error) String() string { + if e.Line != 0 { + return fmt.Sprintf("exp/template/html:%s:%d: %s", e.Name, e.Line, e.Description) + } else if e.Name != "" { + return fmt.Sprintf("exp/template/html:%s: %s", e.Name, e.Description) + } + return "exp/template/html: " + e.Description +} + +// errorf creates an error given a format string f and args. +// The template Name still needs to be supplied. +func errorf(k ErrorCode, line int, f string, args ...interface{}) *Error { + return &Error{k, "", line, fmt.Sprintf(f, args...)} +} diff --git a/src/pkg/exp/template/html/escape.go b/src/pkg/exp/template/html/escape.go index b0acf48df..3fa92cc98 100644 --- a/src/pkg/exp/template/html/escape.go +++ b/src/pkg/exp/template/html/escape.go @@ -36,7 +36,7 @@ func EscapeSet(s *template.Set, names ...string) (*template.Set, os.Error) { if len(names) == 0 { // TODO: Maybe add a method to Set to enumerate template names // and use those instead. - return nil, os.NewError("must specify names of top level templates") + return nil, &Error{ErrNoNames, "", 0, "must specify names of top level templates"} } e := escaper{ s, @@ -49,10 +49,10 @@ func EscapeSet(s *template.Set, names ...string) (*template.Set, os.Error) { for _, name := range names { c, _ := e.escapeTree(context{}, name, 0) var err os.Error - if c.errStr != "" { - err = fmt.Errorf("%s:%d: %s", name, c.errLine, c.errStr) + if c.err != nil { + err, c.err.Name = c.err, name } else if c.state != stateText { - err = fmt.Errorf("%s ends in a non-text context: %v", name, c) + err = &Error{ErrEndContext, name, 0, fmt.Sprintf("ends in a non-text context: %v", c)} } if err != nil { // Prevent execution of unsafe templates. @@ -163,9 +163,8 @@ func (e *escaper) escapeAction(c context, n *parse.ActionNode) context { s = append(s, "exp_template_html_urlescaper") case urlPartUnknown: return context{ - state: stateError, - errLine: n.Line, - errStr: fmt.Sprintf("%s appears in an ambiguous URL context", n), + state: stateError, + err: errorf(ErrAmbigContext, n.Line, "%s appears in an ambiguous URL context", n), } default: panic(c.urlPart.String()) @@ -180,9 +179,8 @@ func (e *escaper) escapeAction(c context, n *parse.ActionNode) context { s = append(s, "exp_template_html_jsregexpescaper") case stateComment, stateJSBlockCmt, stateJSLineCmt, stateCSSBlockCmt, stateCSSLineCmt: return context{ - state: stateError, - errLine: n.Line, - errStr: fmt.Sprintf("%s appears inside a comment", n), + state: stateError, + err: errorf(ErrInsideComment, n.Line, "%s appears inside a comment", n), } case stateCSS: s = append(s, "exp_template_html_cssvaluefilter") @@ -319,9 +317,8 @@ func join(a, b context, line int, nodeName string) context { } return context{ - state: stateError, - errLine: line, - errStr: fmt.Sprintf("{{%s}} branches end in different contexts: %v, %v", nodeName, a, b), + state: stateError, + err: errorf(ErrBranchEnd, line, "{{%s}} branches end in different contexts: %v, %v", nodeName, a, b), } } @@ -340,8 +337,8 @@ func (e *escaper) escapeBranch(c context, n *parse.BranchNode, nodeName string) // Make clear that this is a problem on loop re-entry // since developers tend to overlook that branch when // debugging templates. - c0.errLine = n.Line - c0.errStr = "on range loop re-entry: " + c0.errStr + c0.err.Line = n.Line + c0.err.Description = "on range loop re-entry: " + c0.err.Description return c0 } } @@ -386,9 +383,8 @@ func (e *escaper) escapeTree(c context, name string, line int) (context, string) t := e.template(name) if t == nil { return context{ - state: stateError, - errStr: fmt.Sprintf("no such template %s", name), - errLine: line, + state: stateError, + err: errorf(ErrNoSuchTemplate, line, "no such template %s", name), }, dname } if dname != name { @@ -428,8 +424,7 @@ func (e *escaper) computeOutCtx(c context, t *template.Template) context { d = context{ state: stateError, // TODO: Find the first node with a line in t.Tree.Root - errLine: 0, - errStr: fmt.Sprintf("cannot compute output context for template %s", n), + err: errorf(ErrOutputContext, 0, "cannot compute output context for template %s", n), } // TODO: If necessary, compute a fixed point by assuming d // as the input context, and recursing to escapeList with a diff --git a/src/pkg/exp/template/html/escape_test.go b/src/pkg/exp/template/html/escape_test.go index 0ab326ceb..4adf3670e 100644 --- a/src/pkg/exp/template/html/escape_test.go +++ b/src/pkg/exp/template/html/escape_test.go @@ -603,11 +603,11 @@ func TestErrors(t *testing.T) { }, { "<a b=1 c={{.H}}", - "z ends in a non-text context: {stateAttr delimSpaceOrTagEnd", + "z: ends in a non-text context: {stateAttr delimSpaceOrTagEnd", }, { "<script>foo();", - "z ends in a non-text context: {stateJS", + "z: ends in a non-text context: {stateJS", }, { `<a href="{{if .F}}/foo?a={{else}}/bar/{{end}}{{.H}}">`, @@ -656,7 +656,7 @@ func TestErrors(t *testing.T) { // or `/-1\.5/i.test(x)` which is a method call on a // case insensitive regular expression. `<script>{{if false}}var x = 1{{end}}/-{{"1.5"}}/i.test(x)</script>`, - `: '/' could start div or regexp: "/-"`, + `'/' could start div or regexp: "/-"`, }, { `{{template "foo"}}`, @@ -666,7 +666,7 @@ func TestErrors(t *testing.T) { `{{define "z"}}<div{{template "y"}}>{{end}}` + // Illegal starting in stateTag but not in stateText. `{{define "y"}} foo<b{{end}}`, - `z:0: "<" in attribute name: " foo<b"`, + `"<" in attribute name: " foo<b"`, }, { `{{define "z"}}<script>reverseList = [{{template "t"}}]</script>{{end}}` + @@ -701,7 +701,7 @@ func TestErrors(t *testing.T) { continue } if strings.Index(got, test.err) == -1 { - t.Errorf("input=%q: error %q does not contain expected string %q", test.input, got, test.err) + t.Errorf("input=%q: error\n\t%q\ndoes not contain expected string\n\t%q", test.input, got, test.err) continue } } diff --git a/src/pkg/exp/template/html/transition.go b/src/pkg/exp/template/html/transition.go index 117b20a5b..2449a5011 100644 --- a/src/pkg/exp/template/html/transition.go +++ b/src/pkg/exp/template/html/transition.go @@ -6,11 +6,12 @@ package html import ( "bytes" - "fmt" - "os" "strings" ) +// TODO: ensure transition error messages contain template name and ideally +// line info. + // transitionFunc is the array of context transition functions for text nodes. // A transition function takes a context and template text input, and returns // the updated context and any unconsumed text. @@ -82,8 +83,8 @@ func tTag(c context, s []byte) (context, []byte) { i, err := eatAttrName(s, attrStart) if err != nil { return context{ - state: stateError, - errStr: err.String(), + state: stateError, + err: err, }, nil } if i == len(s) { @@ -204,8 +205,8 @@ func tJS(c context, s []byte) (context, []byte) { c.jsCtx = jsCtxRegexp default: return context{ - state: stateError, - errStr: fmt.Sprintf("'/' could start div or regexp: %.32q", s[i:]), + state: stateError, + err: errorf(ErrSlashAmbig, 0, "'/' could start div or regexp: %.32q", s[i:]), }, nil } default: @@ -235,8 +236,8 @@ func tJSStr(c context, s []byte) (context, []byte) { i++ if i == len(b) { return context{ - state: stateError, - errStr: fmt.Sprintf("unfinished escape sequence in JS string: %q", s), + state: stateError, + err: errorf(ErrPartialEscape, 0, "unfinished escape sequence in JS string: %q", s), }, nil } } else { @@ -271,8 +272,8 @@ func tJSRegexp(c context, s []byte) (context, []byte) { i++ if i == len(b) { return context{ - state: stateError, - errStr: fmt.Sprintf("unfinished escape sequence in JS regexp: %q", s), + state: stateError, + err: errorf(ErrPartialEscape, 0, "unfinished escape sequence in JS regexp: %q", s), }, nil } case '[': @@ -289,8 +290,8 @@ func tJSRegexp(c context, s []byte) (context, []byte) { // This can be fixed by making context richer if interpolation // into charsets is desired. return context{ - state: stateError, - errStr: fmt.Sprintf("unfinished JS regexp charset: %q", s), + state: stateError, + err: errorf(ErrPartialCharset, 0, "unfinished JS regexp charset: %q", s), }, nil } @@ -463,8 +464,8 @@ func tCSSStr(c context, s []byte) (context, []byte) { i++ if i == len(b) { return context{ - state: stateError, - errStr: fmt.Sprintf("unfinished escape sequence in CSS string: %q", s), + state: stateError, + err: errorf(ErrPartialEscape, 0, "unfinished escape sequence in CSS string: %q", s), }, nil } } else { @@ -486,7 +487,7 @@ func tError(c context, s []byte) (context, []byte) { // It returns an error if s[i:] does not look like it begins with an // attribute name, such as encountering a quote mark without a preceding // equals sign. -func eatAttrName(s []byte, i int) (int, os.Error) { +func eatAttrName(s []byte, i int) (int, *Error) { for j := i; j < len(s); j++ { switch s[j] { case ' ', '\t', '\n', '\f', '\r', '=', '>': @@ -495,7 +496,7 @@ func eatAttrName(s []byte, i int) (int, os.Error) { // These result in a parse warning in HTML5 and are // indicative of serious problems if seen in an attr // name in a template. - return 0, fmt.Errorf("%q in attribute name: %.32q", s[j:j+1], s) + return -1, errorf(ErrBadHTML, 0, "%q in attribute name: %.32q", s[j:j+1], s) default: // No-op. } |