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