code review 4890041: godoc: fix escaping in templates

godoc: fix escaping in templates

- HTML-escape URL paths
- URL-escape URL parameters

[gri, 2011-08-13 00:17:20]

Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-08-13 00:35:22]

LGTM

It all reads correctly with escaping of the right type in the right places,
but I have no first-hand experience using exp/template.

I've always stayed sane by naming by variables with how they're already
escaped ($hfoo for HTML, $ufoo for URL-escaped), but that's a religious or
personal decision.

If the host program is sanity-checking things like package names to be
valid, HTML escaping them isn't necessary (but doesn't hurt).


On Fri, Aug 12, 2011 at 5:17 PM, <gri@golang.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> godoc: fix escaping in templates
>
> - HTML-escape URL paths
> - URL-escape URL parameters
>
> Please review this at
http://codereview.appspot.com/**4890041/<http://codereview.appspot.com/4890041/>
>
> Affected files:
>  M lib/godoc/codewalkdir.html
>  M lib/godoc/dirlist.html
>  M lib/godoc/godoc.html
>  M lib/godoc/search.html
>  M src/cmd/godoc/godoc.go
>
>
> Index: lib/godoc/codewalkdir.html
> ==============================**==============================**=======
> --- a/lib/godoc/codewalkdir.html
> +++ b/lib/godoc/codewalkdir.html
> @@ -7,9 +7,10 @@
>  <table class="layout">
>  {{range .}}
>  <tr>
> -    <td><a href="{{url .Name}}">{{html .Name}}</a></td>
> -    <td width="25">&nbsp;</td>
> -    <td>{{html .Title}}</td>
> +       {{$name := html .Name}}
> +       <td><a href="{{$name}}">{{$name}}</a>**</td>
> +       <td width="25">&nbsp;</td>
> +       <td>{{html .Title}}</td>
>  </tr>
>  {{end}}
>  </table>
> Index: lib/godoc/dirlist.html
> ==============================**==============================**=======
> --- a/lib/godoc/dirlist.html
> +++ b/lib/godoc/dirlist.html
> @@ -18,7 +18,8 @@
>  </tr>
>  {{range .}}
>  <tr>
> -       <td align="left"><a href="{{.|fileInfoName|url}}">**
> {{.|fileInfoName|html}}</a></**td>
> +       {{$name := .|fileInfoName|html}}
> +       <td align="left"><a href="{{$name}}">{{$name}}</a>**</td>
>        <td></td>
>        <td align="right">{{html .Size}}</td>
>        <td></td>
> Index: lib/godoc/godoc.html
> ==============================**==============================**=======
> --- a/lib/godoc/godoc.html
> +++ b/lib/godoc/godoc.html
> @@ -25,7 +25,7 @@
>         <form method="GET" action="/search">
>         {{with .PkgRoots}}
>         {{range .PkgRoots}}
> -        <a href="/pkg/{{url .}}">{{html .}}</a> <span class="sep">|</span>
> +        <a href="/pkg/{{html .}}">{{html .}}</a> <span
> class="sep">|</span>
>         {{end}}
>         {{else}}
>         References:
> Index: lib/godoc/search.html
> ==============================**==============================**=======
> --- a/lib/godoc/search.html
> +++ b/lib/godoc/search.html
> @@ -3,6 +3,7 @@
>        Use of this source code is governed by a BSD-style
>        license that can be found in the LICENSE file.
>  -->
> +{{$query := url .Query}}
>  {{with .Alert}}
>        <p>
>        <span class="alert" style="font-size:120%">{{html .}}</span>
> @@ -20,13 +21,13 @@
>        {{with .Decls}}
>                <h2 id="Global">Package-level declarations</h2>
>                {{range .}}
> -                       {{$pkg := pkgLink .Pak.Path}}
> -                       <h3 id="Global_{{html $pkg}}">package <a
> href="/{{url $pkg}}">{{html .Pak.Name}}</a></h3>
> +                       {{$pkg := pkgLink .Pak.Path | html}}
> +                       <h3 id="Global_{{$pkg}}">package <a
> href="/{{$pkg}}">{{html .Pak.Name}}</a></h3>
>                        {{range .Files}}
> -                               {{$src := srcLink .File.Path}}
> +                               {{$src := srcLink .File.Path | html}}
>                                {{range .Groups}}
>                                        {{range .Infos}}
> -                                               <a href="/{{url
> $src}}?h={{url $.Query}}#L{{infoLine .}}">{{html $src}}:{{infoLine .}}</a>
> +                                               <a
> href="/{{$src}}?h={{$query}}#**L{{infoLine .}}">{{$src}}:{{infoLine
> .}}</a>
>                                                {{infoSnippet_html .}}
>                                        {{end}}
>                                {{end}}
> @@ -36,11 +37,11 @@
>        {{with .Others}}
>                <h2 id="Local">Local declarations and uses</h2>
>                {{range .}}
> -                       {{$pkg := pkgLink .Pak.Path}}
> -                       <h3 id="Local_{{html $pkg}}">package <a
> href="/{{url $pkg}}">{{html .Pak.Name}}</a></h3>
> +                       {{$pkg := pkgLink .Pak.Path | html}}
> +                       <h3 id="Local_{{$pkg}}">package <a
> href="/{{$pkg}}">{{html .Pak.Name}}</a></h3>
>                        {{range .Files}}
> -                               {{$src := srcLink .File.Path}}
> -                               <a href="/{{url $src}}?h={{url
> $.Query}}">{{html $src}}</a>
> +                               {{$src := srcLink .File.Path | html}}
> +                               <a href="/{{$src}}?h={{$query}}">**
> {{$src}}</a>
>                                <table class="layout">
>                                {{range .Groups}}
>                                        <tr>
> @@ -49,7 +50,7 @@
>                                        <td align="left" width="4"></td>
>                                        <td>
>                                        {{range .Infos}}
> -                                               <a href="/{{url
> $src}}?h={{url $.Query}}#L{{infoLine .}}">{{infoLine .}}</a>
> +                                               <a
> href="/{{$src}}?h={{$query}}#**L{{infoLine .}}">{{infoLine .}}</a>
>                                        {{end}}
>                                        </td>
>                                        </tr>
> @@ -71,17 +72,17 @@
>        <p>
>        <table class="layout">
>        {{range .}}
> -               {{$src := srcLink .Filename}}
> +               {{$src := srcLink .Filename | html}}
>                <tr>
>                <td align="left" valign="top">
> -               <a href="/{{url $src}}?h={{url $.Query}}">{{html
> $src}}</a>:
> +               <a href="/{{$src}}?h={{$query}}">**{{$src}}</a>:
>                </td>
>                <td align="left" width="4"></td>
>                <th align="left" valign="top">{{len .Lines}}</th>
>                <td align="left" width="4"></td>
>                <td align="left">
>                {{range .Lines}}
> -                       <a href="/{{url $src}}?h={{url $.Query}}#L{{url
> .}}">{{html .}}</a>
> +                       <a href="/{{$src}}?h={{$query}}#**L{{html
> .}}">{{html .}}</a>
>                {{end}}
>                {{if not $.Complete}}
>                        ...
> Index: src/cmd/godoc/godoc.go
> ==============================**==============================**=======
> --- a/src/cmd/godoc/godoc.go
> +++ b/src/cmd/godoc/godoc.go
> @@ -481,7 +481,7 @@
>        }
>
>        var buf bytes.Buffer
> -       buf.WriteString(http.**URLEscape(relpath))
> +       template.HTMLEscape(&buf, []byte(relpath))
>        // selection ranges are of form "s=low:high"
>        if low < high {
>                fmt.Fprintf(&buf, "?s=%d:%d", low, high) // no need for URL
> escaping
>
>
>

[gri, 2011-08-15 22:15:57]

*** Submitted as http://code.google.com/p/go/source/detail?r=8a9fcd3d510f ***

godoc: fix escaping in templates

- HTML-escape URL paths
- URL-escape URL parameters

R=bradfitz
CC=golang-dev
http://codereview.appspot.com/4890041