Issue 5992071: code review 5992071: html, exp/html: escape ' and " as ' and ", sinc...

Issue 5992071: code review 5992071: html, exp/html: escape ' and " as ' and ", sinc... (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years ago by nigeltao

Modified:
12 years ago

Reviewers:

CC:
rsc, MikeSamuel, dsymonds, golang-dev

Visibility:
Public.

Description

html, exp/html: escape ' and " as ' and ", since IE8 and below do not support '. This makes package html consistent with package text/template's HTMLEscape function. Fixes issue 3489.

Patch Set 1 #

Patch Set 2 : diff -r 734071724054 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 734071724054 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 734071724054 https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r b9cbd32246e3 https://go.googlecode.com/hg/ #

Created: 12 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+22 lines, -15 lines)			Patch
M	src/pkg/exp/html/escape.go	View	1 2 3 4	2 chunks	+5 lines, -3 lines	0 comments	Download
M	src/pkg/exp/html/render_test.go	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	src/pkg/exp/html/token_test.go	View	1	4 chunks	+6 lines, -5 lines	0 comments	Download
M	src/pkg/html/escape.go	View	1 2 3 4	2 chunks	+5 lines, -3 lines	0 comments	Download
M	src/pkg/net/http/server.go	View	1 2 3 4	1 chunk	+4 lines, -2 lines	0 comments	Download
M	src/pkg/text/template/funcs.go	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 5

Expand All Messages | Collapse All Messages

nigeltao

Hello rsc@golang.org, mikesamuel@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/

12 years ago (2012-04-09 01:10:06 UTC) #1

MikeSamuel

2012/4/8 <nigeltao@golang.org>: > Reviewers: rsc, MikeSamuel, > > Message: > Hello rsc@golang.org, mikesamuel@gmail.com (cc: > ...

12 years ago (2012-04-09 15:00:55 UTC) #3

2012/4/8  <nigeltao@golang.org>:
> Reviewers: rsc, MikeSamuel,
>
> Message:
> Hello rsc@golang.org, mikesamuel@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> html, exp/html: escape ' and " as &#39; and &#34;, since IE8 and
> below do not support &apos;.
>
> This makes package html consistent with package text/template's
> HTMLEscape function.
>
> Fixes issue 3489.
>
> Please review this at http://codereview.appspot.com/5992071/
>
> Affected files:
>  src/pkg/exp/html/escape.go
>  src/pkg/exp/html/render_test.go
>  src/pkg/exp/html/token_test.go
>  src/pkg/html/escape.go
>  src/pkg/text/template/funcs.go
>
>
> Index: src/pkg/exp/html/escape.go
> ===================================================================
> --- a/src/pkg/exp/html/escape.go
> +++ b/src/pkg/exp/html/escape.go
> @@ -205,13 +205,15 @@
>                case '&':
>                        esc = "&amp;"
>                case '\'':
> -                       esc = "&apos;"
> +                       // "&#39;" is shorter than "&apos;" and IE8 and
> below do not support "&apos;"
> +                       esc = "&#39;"
>                case '<':
>                        esc = "&lt;"
>                case '>':
>                        esc = "&gt;"
>                case '"':
> -                       esc = "&quot;"
> +                       // "&#34;" is shorter than "&quot;".
> +                       esc = "&#34;"
>                default:
>                        panic("unrecognized escape character")
>                }
> @@ -226,7 +228,7 @@
>  }
>
>  // EscapeString escapes special characters like "<" to become "&lt;". It
> -// escapes only five such characters: amp, apos, lt, gt and quot.
> +// escapes only five such characters: <, >, &, ' and ".
>  // UnescapeString(EscapeString(s)) == s always holds, but the converse
> isn't
>  // always true.
>  func EscapeString(s string) string {
> Index: src/pkg/exp/html/render_test.go
> ===================================================================
> --- a/src/pkg/exp/html/render_test.go
> +++ b/src/pkg/exp/html/render_test.go
> @@ -98,7 +98,7 @@
>                        },
>                },
>        }
> -       want := `<html><head></head><body>0&lt;1<p id="A"
> foo="abc&quot;def">` +
> +       want := `<html><head></head><body>0&lt;1<p id="A"
> foo="abc&#34;def">` +
>                `2<b empty="">3</b><i backslash="\">&amp;4</i></p>` +
>                `5<blockquote></blockquote><br/>6</body></html>`
>        b := new(bytes.Buffer)
> Index: src/pkg/exp/html/token_test.go
> ===================================================================
> --- a/src/pkg/exp/html/token_test.go
> +++ b/src/pkg/exp/html/token_test.go
> @@ -359,7 +359,7 @@
>        {
>                "tricky",
>                "<p \t\n iD=\"a&quot;B\"
>  foo=\"bar\"><EM>te&lt;&amp;;xt</em></p>",
> -               `<p id="a&quot;B"
> foo="bar">$<em>$te&lt;&amp;;xt$</em>$</p>`,
> +               `<p id="a&#34;B" foo="bar">$<em>$te&lt;&amp;;xt$</em>$</p>`,
>        },
>        // A nonexistent entity. Tokenizing and converting back to a string
> should
>        // escape the "&" to become "&amp;".
> @@ -421,7 +421,7 @@
>        {
>                "Double-quoted attribute value",
>                `<input value="I'm an attribute" FOO="BAR">`,
> -               `<input value="I&apos;m an attribute" foo="BAR">`,
> +               `<input value="I&#39;m an attribute" foo="BAR">`,
>        },
>        {
>                "Attribute name characters",
> @@ -436,7 +436,7 @@
>        {
>                "Attributes with a solitary single quote",
>                `<p id=can't><p id=won't>`,
> -               `<p id="can&apos;t">$<p id="won&apos;t">`,
> +               `<p id="can&#39;t">$<p id="won&#39;t">`,
>        },
>  }
>
> @@ -545,10 +545,11 @@
>                `"<&>"`,
>                `&quot;&lt;&amp;&gt;&quot;`,
>                `3&5==1 && 0<1, "0&lt;1", a+acute=&aacute;`,
> +               `The special characters are: <, >, &, ' and "`,
>        }
>        for _, s := range ss {
> -               if s != UnescapeString(EscapeString(s)) {
> -                       t.Errorf("s != UnescapeString(EscapeString(s)),
> s=%q", s)
> +               if got := UnescapeString(EscapeString(s)); got != s {
> +                       t.Errorf("got %q want %q", got, s)
>                }
>        }
>  }
> Index: src/pkg/html/escape.go
> ===================================================================
> --- a/src/pkg/html/escape.go
> +++ b/src/pkg/html/escape.go
> @@ -210,13 +210,15 @@
>                case '&':
>                        esc = "&amp;"
>                case '\'':
> -                       esc = "&apos;"
> +                       // "&#39;" is shorter than "&apos;" and IE8 and
> below do not support "&apos;"
> +                       esc = "&#39;"
>                case '<':
>                        esc = "&lt;"
>                case '>':
>                        esc = "&gt;"
>                case '"':
> -                       esc = "&quot;"
> +                       // "&#34;" is shorter than "&quot;".
> +                       esc = "&#34;"
>                default:
>                        panic("unrecognized escape character")
>                }
> @@ -231,7 +233,7 @@
>  }
>
>  // EscapeString escapes special characters like "<" to become "&lt;". It
> -// escapes only five such characters: amp, apos, lt, gt and quot.
> +// escapes only five such characters: <, >, &, ' and ".
>  // UnescapeString(EscapeString(s)) == s always holds, but the converse
> isn't
>  // always true.
>  func EscapeString(s string) string {
> Index: src/pkg/text/template/funcs.go
> ===================================================================
> --- a/src/pkg/text/template/funcs.go
> +++ b/src/pkg/text/template/funcs.go
> @@ -246,7 +246,7 @@
>
>  var (
>        htmlQuot = []byte("&#34;") // shorter than "&quot;"
> -       htmlApos = []byte("&#39;") // shorter than "&apos;"
> +       htmlApos = []byte("&#39;") // shorter than "&apos;" and IE8 and
> below do not support "&apos;"
>        htmlAmp  = []byte("&amp;")
>        htmlLt   = []byte("&lt;")
>        htmlGt   = []byte("&gt;")
>
>

LGTM

rsc

Please note that this is more than an IE8 issue. ' did not exist in ...

12 years ago (2012-04-09 19:01:56 UTC) #4