exp/template/html: autoescape actions in HTML style attributes
This does not wire up <style> elements as that is pending support
for raw text content in CL http://codereview.appspot.com/4964045/
This CL allows actions to appear in contexts like
selectors: {{.Tag}}{{.Class}}{{.Id}}
property names: border-{{.BidiLeadingEdge}}
property values: color: {{.Color}}
strings: font-family: "{{font-name}}"
URL strings: background: "/foo?image={{.ImgQuery}}"
URL literals: background: url("{{.Image}}")
but disallows actions inside CSS comments and disallows
embedding of JS in CSS entirely.
It is based on the CSS3 lexical grammar with affordances for
common browser extensions including line comments.
http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/css.go File src/pkg/exp/template/html/css.go (right): http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/css.go#newcode56 src/pkg/exp/template/html/css.go:56: // decodeCSS decodes CSS3 escapes given a sequence of ...
12 years, 8 months ago
(2011-09-06 04:28:16 UTC)
#5
http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
File src/pkg/exp/template/html/css.go (right):
http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/cs...
src/pkg/exp/template/html/css.go:56: // decodeCSS decodes CSS3 escapes given a
sequence of stringchars.
On 2011/09/06 01:54:08, nigeltao wrote:
> On 2011/09/02 17:07:43, MikeSamuel wrote:
> > On 2011/09/02 09:11:37, nigeltao wrote:
> > > What's a stringchar?
> >
> > http://www.w3.org/TR/css3-syntax/#SUBTOK-stringchar
>
> Can you incorporate that link into the doc comment? Future maintainers might
> also not know what a stringchar is.
Done.
http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
File src/pkg/exp/template/html/html.go (right):
http://codereview.appspot.com/4968058/diff/21001/src/pkg/exp/template/html/ht...
src/pkg/exp/template/html/html.go:86: b.WriteString("&#x")
On 2011/09/06 01:54:08, nigeltao wrote:
> On 2011/09/02 17:07:43, MikeSamuel wrote:
> > On 2011/09/02 09:11:37, nigeltao wrote:
> > > fmt.Fprintf(b, "&#x%04x;", r)
> > >
> > > Similarly in url.go.
> >
> > Done.
> >
> > I saw the below in template/funcs.go
> >
> > // TODO(dsymonds): Do this without fmt?
> > fmt.Fprintf(w, "\\u%04X", rune)
> >
> > and thought that format strings were frowned upon for this sort of thing.
> >
> > Changing urlProcessor to use
> > fmt.Fprintf(&b, "%%%02x", c)
> > instead of
> > b.WriteByte('%')
> > b.WriteByte("0123456789abcdef"[c>>4])
> > b.WriteByte("0123456789abcdef"[c&0xf])
> > causes the benchmarks to take treble the time.
> >
> > html.BenchmarkURLEscaper 200000 8377 ns/op
> > html.BenchmarkURLNormalizer 500000 6453 ns/op
> >
> > vs
> >
> > html.BenchmarkURLEscaper 1000000 2553 ns/op
> > html.BenchmarkURLNormalizer 1000000 1852 ns/op
>
> Since the benchmarks show 3x, I'm happy to go back to what you had before.
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css.go
File src/pkg/exp/template/html/css.go (right):
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:33: return
bytes.HasSuffix(bytes.ToLower(b[len(b)-len(kw):]), kw)
On 2011/09/06 01:54:08, nigeltao wrote:
> Can you use bytes.Equal instead of bytes.HasSuffix?
>
> Also, it might read better if you take the "b[len(b)-len(kw):]" out of here
and
> put
> b = b[len(b)-len(kw):]
> inside the if block above? It might help to start the function with:
> i := len(b) - len(kw)
>
> Finally, if kw is going to be constant (and not untrusted input), it might be
> more appropriate for kw to be a string instead of a []byte. The final line
could
> be:
> return string(bytes.ToLower(b)) == kw
>
> The compiler might not currently optimize away the []byte to string
conversion,
> but it should be able to.
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:69: copy(b[len(b):len(b)+i], s[:i])
On 2011/09/06 01:54:08, nigeltao wrote:
> This could be:
> b, s = append(b, s[:i]...), s[i:]
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:95: copy(b[len(b):len(b)+n], s[1:])
On 2011/09/06 01:54:08, nigeltao wrote:
> This could be:
> b, s = append(b, s[1:1+n]...), s[1+n:]
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:113: n = (n << 4) | (int(c - '0'))
On 2011/09/06 01:54:08, nigeltao wrote:
> If you lifted the "n <<= 4" outside the switch, these could be written with
> fewer parens:
> n |= int(c) - '0'
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css.go:244: if c < 0x80 && isCSSNmchar(int(c)) {
On 2011/09/06 01:54:08, nigeltao wrote:
> Is the c < 0x80 guard because CSS is limited to ASCII only or is it because
you
> are iterating over bytes instead of runes?
Normally I'd never blacklist, but browser vendors have largely agreed that
JavaScript in CSS was a horribly bad idea and have moved away from allowing
that. IE 7 was the last to allow expression(...) so this is only to guard
against JS in CSS on older but still widespread browsers.
The loop is over runes.
I'm trying to prevent any mechanisms for embedding non-standard syntactic
constructs to break a keyword.. All CSS keywords are ASCII only so I'm
collecting only ASCII nmchars because I don't trust browsers not to ignore
combining marks, bidi-embeddings, and non-standard syntactic constructs.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
File src/pkg/exp/template/html/css_test.go (right):
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css_test.go:76: {`\`, ``},
On 2011/09/06 01:54:08, nigeltao wrote:
> Can you add a test case for `foo\`?
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css_test.go:94: {`The \3c
i\3equick\3c/i\3e,\d\A\3cspan style=\27 color:brown\27\3e brown\3c/span\3e fox
jumps\2028over the \3c canine class=\22lazy\22 \3e dog\3c/canine\3e`,
On 2011/09/06 01:54:08, nigeltao wrote:
> Since this struct literal spans multiple lines, I'd format it as:
> {
> `The \3c etc`,
> "The <i> etc",
> },
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/css...
src/pkg/exp/template/html/css_test.go:166:
On 2011/09/06 01:54:08, nigeltao wrote:
> I'd nix the blank line.
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/esc...
File src/pkg/exp/template/html/escape.go (right):
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/esc...
src/pkg/exp/template/html/escape.go:78: case stateURL, stateCSSDqStr,
stateCSSSqStr,
On 2011/09/06 01:54:08, nigeltao wrote:
> I think it'd be clearer if the case was all on one line.
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/esc...
src/pkg/exp/template/html/escape.go:608:
On 2011/09/06 01:54:08, nigeltao wrote:
> As these three sections make one big comment, I'd make this line a "\t//"
> instead of a blank line, and similarly at line 611.
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/htm...
File src/pkg/exp/template/html/html_test.go (right):
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/htm...
src/pkg/exp/template/html/html_test.go:37:
On 2011/09/06 01:54:08, nigeltao wrote:
> I'd nix the blank line.
Done.
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/url...
File src/pkg/exp/template/html/url_test.go (right):
http://codereview.appspot.com/4968058/diff/8003/src/pkg/exp/template/html/url...
src/pkg/exp/template/html/url_test.go:84: t.Errorf("%s once:
want\n\t%q\ngot\n\t%q", test.name, test.escaped, s)
On 2011/09/06 01:54:08, nigeltao wrote:
> Drop the "once"?
Done.
*** Submitted as http://code.google.com/p/go/source/detail?r=504f4e9b079c *** exp/template/html: autoescape actions in HTML style attributes. This does not ...
12 years, 8 months ago
(2011-09-08 21:18:26 UTC)
#6
*** Submitted as http://code.google.com/p/go/source/detail?r=504f4e9b079c ***
exp/template/html: autoescape actions in HTML style attributes.
This does not wire up <style> elements as that is pending support
for raw text content in CL http://codereview.appspot.com/4964045/
This CL allows actions to appear in contexts like
selectors: {{.Tag}}{{.Class}}{{.Id}}
property names: border-{{.BidiLeadingEdge}}
property values: color: {{.Color}}
strings: font-family: "{{font-name}}"
URL strings: background: "/foo?image={{.ImgQuery}}"
URL literals: background: url("{{.Image}}")
but disallows actions inside CSS comments and disallows
embedding of JS in CSS entirely.
It is based on the CSS3 lexical grammar with affordances for
common browser extensions including line comments.
R=nigeltao
CC=golang-dev
http://codereview.appspot.com/4968058
Committer: Nigel Tao <nigeltao@golang.org>
Issue 4968058: code review 4968058: exp/template/html: autoescape actions in HTML style att...
(Closed)
Created 12 years, 8 months ago by MikeSamuel
Modified 12 years, 8 months ago
Reviewers:
Base URL:
Comments: 62