code review 26090043: net/http: escape url/html in directory index

net/http: escape url/html in directory index

[mattn, 2013-11-13 14:13:53]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-11-13 15:08:26]

Remove the TODO a line above that, and please add a test.



On Wed, Nov 13, 2013 at 6:13 AM, <mattn.jp@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> http://go.googlecode.com/hg/
>
>
> Description:
> net/http: escape url/html in directory index
>
> Please review this at https://codereview.appspot.com/26090043/
>
> Affected files (+2, -1 lines):
>   M src/pkg/net/http/fs.go
>
>
> Index: src/pkg/net/http/fs.go
> ===================================================================
> --- a/src/pkg/net/http/fs.go
> +++ b/src/pkg/net/http/fs.go
> @@ -13,6 +13,7 @@
>         "mime"
>         "mime/multipart"
>         "net/textproto"
> +       "net/url"
>         "os"
>         "path"
>         "path/filepath"
> @@ -74,7 +75,7 @@
>                                 name += "/"
>                         }
>                         // TODO htmlescape
> -                       fmt.Fprintf(w, "<a href=\"%s\">%s</a>\n", name,
> name)
> +                       fmt.Fprintf(w, "<a href=\"%s\">%s</a>\n",
> url.QueryEscape(name), htmlEscape(name))
>                 }
>         }
>         fmt.Fprintf(w, "</pre>\n")
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[mattn, 2013-11-14 00:36:41]

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

Please take another look.

[mattn, 2013-11-15 08:24:14]

On 2013/11/14 00:36:41, mattn wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:bradfitz@golang.org (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Could you please review this?

[bradfitz, 2013-11-15 14:51:32]

The tree is closed for two weeks anyway. But looks fine.  I haven't looked
at the test in detail.
On Nov 15, 2013 12:24 AM, <mattn.jp@gmail.com> wrote:

> On 2013/11/14 00:36:41, mattn wrote:
>
>> Hello mailto:golang-dev@googlegroups.com, mailto:bradfitz@golang.org
>>
> (cc:
>
>> mailto:golang-dev@googlegroups.com),
>>
>
>  Please take another look.
>>
>
> Could you please review this?
>
> https://codereview.appspot.com/26090043/
>

[r, 2013-12-09 20:58:14]

LGTM but wait for bradfitz

https://codereview.appspot.com/26090043/diff/100001/src/pkg/net/http/fs_test.go
File src/pkg/net/http/fs_test.go (right):

https://codereview.appspot.com/26090043/diff/100001/src/pkg/net/http/fs_test....
src/pkg/net/http/fs_test.go:467: t.Errorf("directory index got link %q, should
be matched %q", link, name)
s/matched //

https://codereview.appspot.com/26090043/diff/100001/src/pkg/net/http/fs_test....
src/pkg/net/http/fs_test.go:474: t.Errorf("directory index of %v not matched as
%v", paths, list)
s/index of //
s/not matched as/should be/

[mattn, 2013-12-10 00:40:48]

https://codereview.appspot.com/26090043/diff/100001/src/pkg/net/http/fs_test.go
File src/pkg/net/http/fs_test.go (right):

https://codereview.appspot.com/26090043/diff/100001/src/pkg/net/http/fs_test....
src/pkg/net/http/fs_test.go:467: t.Errorf("directory index got link %q, should
be matched %q", link, name)
On 2013/12/09 20:58:14, r wrote:
> s/matched //

Done.

https://codereview.appspot.com/26090043/diff/100001/src/pkg/net/http/fs_test....
src/pkg/net/http/fs_test.go:474: t.Errorf("directory index of %v not matched as
%v", paths, list)
On 2013/12/09 20:58:14, r wrote:
> s/index of //
> s/not matched as/should be/

Done.

[bradfitz, 2013-12-10 11:32:04]

https://codereview.appspot.com/26090043/diff/120001/src/pkg/net/http/fs.go
File src/pkg/net/http/fs.go (right):

https://codereview.appspot.com/26090043/diff/120001/src/pkg/net/http/fs.go#ne...
src/pkg/net/http/fs.go:77: fmt.Fprintf(w, "<a href=\"%s\">%s</a>\n",
url.QueryEscape(name), htmlEscape(name))
why url.QueryEscape? This isn't query parameter that you need to %xx escape.  I
think they should both be htmlEscape.

[mattn, 2013-12-10 12:01:25]

https://codereview.appspot.com/26090043/diff/120001/src/pkg/net/http/fs.go
File src/pkg/net/http/fs.go (right):

https://codereview.appspot.com/26090043/diff/120001/src/pkg/net/http/fs.go#ne...
src/pkg/net/http/fs.go:77: fmt.Fprintf(w, "<a href=\"%s\">%s</a>\n",
url.QueryEscape(name), htmlEscape(name))
On 2013/12/10 11:32:04, bradfitz wrote:
> why url.QueryEscape? This isn't query parameter that you need to %xx escape. 
I
> think they should both be htmlEscape.

If you create a directory named '"', it must be escaped with % not &quot;

So it should be:

  <a href="%22">&quot;</a>

If someone create '<', it will occur XSS. (I know they won't publish directory
index for unknown users)

[mattn, 2013-12-18 06:00:48]

ping bradfitz

[gobot, 2013-12-20 16:26:05]

Replacing golang-dev with golang-codereviews.

[mattn, 2013-12-21 15:06:12]

I modified code to percent escape without some ascii characters.
I know this is not same as:
https://dvcs.w3.org/hg/url/raw-file/tip/Overview.html#url-units

But, currently, this should works as same as ap_escape_uri().
Please review again.

[gobot, 2013-12-23 15:01:31]

R=bradfitz@golang.org (assigned by mattn.jp@gmail.com)

[bradfitz, 2013-12-26 19:29:41]

not lgtm

https://codereview.appspot.com/26090043/diff/140002/src/pkg/net/http/fs.go
File src/pkg/net/http/fs.go (right):

https://codereview.appspot.com/26090043/diff/140002/src/pkg/net/http/fs.go#ne...
src/pkg/net/http/fs.go:64: var noEsc =
[]byte(`!$&'()*+,-./0123456789:=@ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz~`)
there are packages that do escaping.  I don't see why HTTP needs its own private
implementation

https://codereview.appspot.com/26090043/diff/140002/src/pkg/net/http/fs.go#ne...
src/pkg/net/http/fs.go:70: for _, c := range []byte(s) {
this allocates

https://codereview.appspot.com/26090043/diff/140002/src/pkg/net/http/fs.go#ne...
src/pkg/net/http/fs.go:79: r += string(c)
this isn't how you build a string

[mattn, 2013-12-27 01:56:14]

https://codereview.appspot.com/26090043/diff/140002/src/pkg/net/http/fs.go
File src/pkg/net/http/fs.go (right):

https://codereview.appspot.com/26090043/diff/140002/src/pkg/net/http/fs.go#ne...
src/pkg/net/http/fs.go:64: var noEsc =
[]byte(`!$&'()*+,-./0123456789:=@ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz~`)
On 2013/12/26 19:29:42, bradfitz wrote:
> there are packages that do escaping.  I don't see why HTTP needs its own
private
> implementation

Do you mean net/url? it doesn't export to use external packages.
And QueryEscape is not same as escape() or ap_escape_uri().

https://codereview.appspot.com/26090043/diff/140002/src/pkg/net/http/fs.go#ne...
src/pkg/net/http/fs.go:70: for _, c := range []byte(s) {
On 2013/12/26 19:29:42, bradfitz wrote:
> this allocates

Done.

https://codereview.appspot.com/26090043/diff/140002/src/pkg/net/http/fs.go#ne...
src/pkg/net/http/fs.go:79: r += string(c)
On 2013/12/26 19:29:42, bradfitz wrote:
> this isn't how you build a string

Which do you mean 'use fmt.Sprintf'?

[bradfitz, 2014-01-14 21:24:15]

R=close

Fixed by b18d37534a92 (net/http: escape contents of the directory indexes
generated by FileServer)