code review 31400043: net/url: correctly escape URL as RFC 3986

net/url: correctly escape URL as RFC 3986

Include '!', '\'', '(', ')', '*'
in the reserved case, as RFC 3986 2.2 defined.
Fixes issue 6784.

[Hong Ruiqi, 2013-11-23 14:10:57]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-11-23 15:19:54]

This would need updates to the tests too.
 On Nov 23, 2013 7:18 AM, <hongruiqi@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net/url: correctly escape URL as RFC 3986
>
> Include '[',']', '!', '\'', '(', ')', '*'
> in the reserved case, as RFC 3986 2.2 defined.
> Fixes issue 6784.
>
> Please review this at https://codereview.appspot.com/31400043/
>
> Affected files (+1, -1 lines):
>   M src/pkg/net/url/url.go
>
>
> Index: src/pkg/net/url/url.go
> ===================================================================
> --- a/src/pkg/net/url/url.go
> +++ b/src/pkg/net/url/url.go
> @@ -75,7 +75,7 @@
>         case '-', '_', '.', '~': // §2.3 Unreserved characters (mark)
>                 return false
>
> -       case '$', '&', '+', ',', '/', ':', ';', '=', '?', '@': // §2.2
> Reserved characters (reserved)
> +       case '[', ']', '!', '\'', '(', ')', '*', '$', '&', '+', ',', '/',
> ':', ';', '=', '?', '@': // §2.2 Reserved characters (reserved)
>                 // Different sections of the URL allow a few of
>                 // the reserved characters to appear unescaped.
>                 switch mode {
>
>
> --
>
> ---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.
>

[Hong Ruiqi, 2013-11-23 16:19:08]

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

Please take another look.

[minux1, 2013-11-23 21:23:03]

https://codereview.appspot.com/31400043/diff/60001/src/pkg/net/url/url_test.go
File src/pkg/net/url/url_test.go (right):

https://codereview.appspot.com/31400043/diff/60001/src/pkg/net/url/url_test.g...
src/pkg/net/url/url_test.go:882: "/action!go",
if you include '[', ']', '!', '\'', '(', ')', '*', then you probably should test
them all
instead of only '!'.

Also, I'd like to know why we leave off those reserved characters in the
first place. Is is simply an oversight or something security related?
Russ?

[Hong Ruiqi, 2013-11-24 03:10:04]

On 2013/11/23 21:23:03, minux wrote:
> Also, I'd like to know why we leave off those reserved characters in the
> first place. Is is simply an oversight or something security related?

It seems that the net/url package was origin implemented on rfc 2396, 
this rfc doesn't have those reserved characters, and some of them belong to
unreserved characters.
CL 5970050 updated shouldEscaple function to obey rfc 3986, and remove some
unreserved
characters that not in rfc 3986, but did not put the newly added to reserved.
I don't know whether there is something security consideration.

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

Replacing golang-dev with golang-codereviews.

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

Replacing golang-dev with golang-codereviews.

[andybalholm, 2014-01-08 17:18:17]

PIng.

[andybalholm, 2014-01-08 21:05:43]

I had a user complaining about some URLs not working. I applied this patch and
now they’re working for him.

[bradfitz, 2014-01-08 21:11:13]

The risk with patches like this is you fix one user but might break two
others.

This might be correct, but it requires some thought to know who if anybody
would be affected negatively.



On Wed, Jan 8, 2014 at 1:05 PM, Andy Balholm <andybalholm@gmail.com> wrote:

> I had a user complaining about some URLs not working. I applied this patch
> and now they’re working for him.

[andybalholm, 2014-01-08 21:26:42]

On Jan 8, 2014, at 1:11 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> The risk with patches like this is you fix one user but might break two
others.
> 
> This might be correct, but it requires some thought to know who if anybody
would be affected negatively.

What investigation needs to be done?

[bradfitz, 2014-01-15 21:00:47]

On Wed, Jan 8, 2014 at 1:26 PM, Andy Balholm <andybalholm@gmail.com> wrote:

>
> On Jan 8, 2014, at 1:11 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
> > The risk with patches like this is you fix one user but might break two
> others.
> >
> > This might be correct, but it requires some thought to know who if
> anybody would be affected negatively.
>
> What investigation needs to be done?
>

As Russ said in issue 5684 (which I dup'd 6784 into), please check the
source history of all this first.  Just verify we haven't already done this
before.

[andybalholm, 2014-01-15 21:30:43]

On Jan 15, 2014, at 1:00 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> As Russ said in issue 5684 (which I dup'd 6784 into), please check the source
history of all this first.  Just verify we haven't already done this before.

He’s right; we have been through this before:

https://code.google.com/p/go/issues/detail?id=3433
https://codereview.appspot.com/5970050

That issue was about a URL which wouldn’t work if the parentheses weren’t
escaped.

If I understand the RFC correctly (though that seems unlikely considering how
much trouble everyone implementing web servers seems to have with it), there are
certain characters which may be escaped, but which are not required to be
escaped. This includes the parentheses and bangs. A web server that
distinguishes between the escaped and unescaped forms is broken. So it shouldn’t
matter what we do here. But we have now proved that real web servers are broken
both ways. So what can we do to make Go work with them?

It seems like about the only way to make this mess work would be to add a
RawPath field to url.URL. It would be populated by url.Parse; when the URL is
converted to a string, RawPath is used instead of unescaping Path—if RawPath is
non-empty and it is equivalent to Path except for percent-encoding. Does anyone
have a better idea…?

[rsc, 2014-01-22 20:44:09]

I am unconvinced we should keep fiddling with this code. I believe it is a
valid reading of the spec as is, and I don't believe it helps to make
different versions of Go do different things. People interacting with wacky
servers already have a documented workaround. From the net/url package docs:

    Note that the Path field is stored in decoded form: /%47%6f%2f becomes
    /Go/. A consequence is that it is impossible to tell which slashes in
    the Path were slashes in the raw URL and which were %2f. This
    distinction is rarely important, but when it is a client must use other
    routines to parse the raw URL or construct the parsed URL. For example,
    an HTTP server can consult req.RequestURI, and an HTTP client can use
    URL{Host: "example.com", Opaque: "//example.com/Go%2f"} instead of
    URL{Host: "example.com", Path: "/Go/"}.

Russ

[andybalholm, 2014-01-22 22:47:38]

On Jan 22, 2014, at 12:44 PM, Russ Cox <rsc@golang.org> wrote:

> I am unconvinced we should keep fiddling with this code. I believe it is a
valid reading of the spec as is, and I don't believe it helps to make different
versions of Go do different things. People interacting with wacky servers
already have a documented workaround. From the net/url package docs:

Thanks, Russ. That’s just what I needed. My proxy server works with the example
URLs from both of the conflicting issues now.

[rsc, 2014-03-05 20:03:58]

R=close