code review 7369045: net/http, net/url: deal with URL.Opaque beginning with //

net/http, net/url: deal with URL.Opaque beginning with //

Update issue 4860

[bradfitz, 2013-02-21 04:20:05]

Hello golang-dev@googlegroups.com,

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

[adg, 2013-02-21 04:33:13]

https://codereview.appspot.com/7369045/diff/2001/src/pkg/net/http/requestwrit...
File src/pkg/net/http/requestwrite_test.go (right):

https://codereview.appspot.com/7369045/diff/2001/src/pkg/net/http/requestwrit...
src/pkg/net/http/requestwrite_test.go:390: WantWrite: "GET
http://y.google.com/%2F/%2F/ HTTP/1.1\r\n" +
wow, this is correct?

[bradfitz, 2013-02-21 04:42:23]

On Wed, Feb 20, 2013 at 8:33 PM, <adg@golang.org> wrote:

>
> http://y.google.com/%2F/%2F/ HTTP/1.1\r\n" +
> wow, this is correct?
>

That's up for debate.

Arguably, it's what the user told us they wanted (they gave us conflicting
information), and it's easier to do what they asked for, rather than try to
invent heuristics and precedence rules.  I don't really want to be parsing
their Opaque value and trying to match it against Host, and I don't want
the HTTP layer to use req.URL.Host sometimes but use
parseHostOutOf(req.URL.Opaque) in other cases.  Seems best to do what we're
told if they're getting this low-level.

The only thing this attempts to do is follow the docs on
(*net.URL).RequestURI, which says it returns what you need to write to do
an HTTP request.

Given that you can't do an HTTP request with "//foo" (see RFC 2616 5.1.2
Request-URI), it seems best to prepend the scheme to make it an absoluteURI.

[adg, 2013-02-21 04:44:09]

LGTM

Shrug.


On 21 February 2013 15:42, Brad Fitzpatrick <bradfitz@golang.org> wrote:

>
>
> On Wed, Feb 20, 2013 at 8:33 PM, <adg@golang.org> wrote:
>
>>
>> http://y.google.com/%2F/%2F/ HTTP/1.1\r\n" +
>> wow, this is correct?
>>
>
> That's up for debate.
>
> Arguably, it's what the user told us they wanted (they gave us conflicting
> information), and it's easier to do what they asked for, rather than try to
> invent heuristics and precedence rules.  I don't really want to be parsing
> their Opaque value and trying to match it against Host, and I don't want
> the HTTP layer to use req.URL.Host sometimes but use
> parseHostOutOf(req.URL.Opaque) in other cases.  Seems best to do what we're
> told if they're getting this low-level.
>
> The only thing this attempts to do is follow the docs on
> (*net.URL).RequestURI, which says it returns what you need to write to do
> an HTTP request.
>
> Given that you can't do an HTTP request with "//foo" (see RFC 2616 5.1.2
> Request-URI), it seems best to prepend the scheme to make it an
> absoluteURI.
>
>

[bradfitz, 2013-02-21 04:55:24]

I can wait for another shrug. :)
On Feb 20, 2013 8:44 PM, "Andrew Gerrand" <adg@golang.org> wrote:

> LGTM
>
> Shrug.
>
>
> On 21 February 2013 15:42, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>>
>>
>> On Wed, Feb 20, 2013 at 8:33 PM, <adg@golang.org> wrote:
>>
>>>
>>> http://y.google.com/%2F/%2F/ HTTP/1.1\r\n" +
>>> wow, this is correct?
>>>
>>
>> That's up for debate.
>>
>> Arguably, it's what the user told us they wanted (they gave us
>> conflicting information), and it's easier to do what they asked for, rather
>> than try to invent heuristics and precedence rules.  I don't really want to
>> be parsing their Opaque value and trying to match it against Host, and I
>> don't want the HTTP layer to use req.URL.Host sometimes but use
>> parseHostOutOf(req.URL.Opaque) in other cases.  Seems best to do what we're
>> told if they're getting this low-level.
>>
>> The only thing this attempts to do is follow the docs on
>> (*net.URL).RequestURI, which says it returns what you need to write to do
>> an HTTP request.
>>
>> Given that you can't do an HTTP request with "//foo" (see RFC 2616 5.1.2
>> Request-URI), it seems best to prepend the scheme to make it an
>> absoluteURI.
>>
>>
>

[rsc, 2013-02-21 18:43:15]

what's the motivation for case #2?

https://codereview.appspot.com/7369045/diff/2001/src/pkg/net/http/requestwrit...
File src/pkg/net/http/requestwrite_test.go (right):

https://codereview.appspot.com/7369045/diff/2001/src/pkg/net/http/requestwrit...
src/pkg/net/http/requestwrite_test.go:383: Opaque: "//y.google.com/%2F/%2F/",
Not at all sure about this. I think Opaque should be used as is. Otherwise it's
not "opaque".

[rsc, 2013-02-21 18:46:37]

Still not sure about the motivation, even after rereading the thread and
the issue, but I suggest that instead people can just set Opaque = "
http://y.google.com/asdf" if that's what they want the GET to say.

[bradfitz, 2013-02-21 18:54:04]

On Thu, Feb 21, 2013 at 10:46 AM, Russ Cox <rsc@golang.org> wrote:

> Still not sure about the motivation, even after rereading the thread and
> the issue, but I suggest that instead people can just set Opaque = "
> http://y.google.com/asdf" if that's what they want the GET to say.
>

Oh, without a Scheme.  Hm, that could work.  Just &net.URL{Opaque: "
http://foo.com/"}.

The original motivation was that we thought there existed no *net.URL using
Opaque (to contain %2F) which would work with both App Engine (which uses
(*URL).String) and net/http.Transport, which uses (*net.URL).RequestURI().
 So I made RequestURI return something valid for that case.

I still think this CL makes sense, since RequestURI is documented to return
something for an HTTP Request-URI line, and // isn't valid.  But if a
just-Opaque URL works, I care less.

Francesc, want to try?

[rsc, 2013-02-21 18:55:56]

I'd rather document that RequestURI returns Opaque unmodified if set. That
gives people complete control and keeps us out of the business of looking
at ostensibly opaque fields.

[bradfitz, 2013-02-21 19:13:41]

On Thu, Feb 21, 2013 at 10:54 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> On Thu, Feb 21, 2013 at 10:46 AM, Russ Cox <rsc@golang.org> wrote:
>
>> Still not sure about the motivation, even after rereading the thread and
>> the issue, but I suggest that instead people can just set Opaque = "
>> http://y.google.com/asdf" if that's what they want the GET to say.
>>
>
> Oh, without a Scheme.  Hm, that could work.  Just &net.URL{Opaque: "
> http://foo.com/"}.
>

Actually that doesn't work.

The *http.Transport refuses it:

        ts := httptest.NewServer(http.HandlerFunc(func(w
http.ResponseWriter, r *http.Request) {
                log.Printf("Got request for %q", r.RequestURI)
        }))
        defer ts.Close()
        req, _ := http.NewRequest("GET", ts.URL, nil)
        *req.URL = url.URL{Opaque: ts.URL + "/why-hello-there/%2F/%2F/%2F/"}
        _, err = http.DefaultClient.Do(req)
        ....

Do: Get http://127.0.0.1:42887/why-hello-there/%2F/%2F/%2F/: unsupported
protocol scheme ""

[campoy, 2013-02-21 19:29:14]

Hi team,

I'm not sure I fully understand the subtle differences between RequestURI
and String.

My only concern here is to be able to make APIs needing "%2F" in the URLs
they request work both on and off App Engine.

The fix I added on the App Engine side solves the problem in a pretty
straight forward way.

I'm not sure changing the behavior of the url.URL methods is a good idea.
Wouldn't that be a non backward compatible change? It sounds dangerous to
me.

 Francesc Campoy Flores | Go Developer Relations | campoy@google.com |
 415-990-4126


On Thu, Feb 21, 2013 at 10:55 AM, Russ Cox <rsc@golang.org> wrote:

> I'd rather document that RequestURI returns Opaque unmodified if set. That
> gives people complete control and keeps us out of the business of looking
> at ostensibly opaque fields.
>
>

[rsc, 2013-02-21 19:29:33]

Why not &net.URL{Scheme:"http", Host:"foo.com", Opaque:"http://fo.com/"} ?

The scheme and host are needed for Get to know who to dial and how. The
opaque is the GET line.

Russ

[bradfitz, 2013-02-21 19:39:48]

On Thu, Feb 21, 2013 at 11:29 AM, Russ Cox <rsc@golang.org> wrote:

> Why not &net.URL{Scheme:"http", Host:"foo.com", Opaque:"http://fo.com/"} ?
>
> The scheme and host are needed for Get to know who to dial and how. The
> opaque is the GET line.
>

Because:

http://play.golang.org/p/IYexHouPVH

  u := &url.URL{Scheme:"http", Host:"foo.com", Opaque:"http://foo.com/%2F/"}
println(u.String())

"http:http://foo.com/%2F/"

That's why it's failing on App Engine.

[rsc, 2013-02-21 19:45:42]

LGTM

Sorry, I thought the general form was {scheme}://{host}{opaque}, but I reread
the docs and see that it is {scheme}:{opaque}. This CL seems fine now. Weird,
but fine.

[bradfitz, 2013-02-21 20:01:50]

*** Submitted as https://code.google.com/p/go/source/detail?r=67ff484a1801 ***

net/http, net/url: deal with URL.Opaque beginning with //

Update issue 4860

R=adg, rsc, campoy
CC=golang-dev
https://codereview.appspot.com/7369045