code review 98320044: net/http: don't change host header when following redirects

net/http: don't change host header when following redirects

If a client used the feature of having a different host from the value in req.URL.Host, the Client would change the host when following redirects.

If we're following a local redirect, keep sending the same Host header.

Fixes issue 8027.

[DMorsing, 2014-05-19 18:35:34]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-05-19 18:45:25]

I don't even want to spend the mental energy reviewing this while we're in
code freeze unless you tell me it's an important regression from Go 1.2.

Can this wait?



On Mon, May 19, 2014 at 11:35 AM, <daniel.morsing@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> net/http: don't change host header when following redirects
>
> If a client used the feature of having a different host from the value
> in req.URL.Host, the Client would change the host when following
> redirects.
>
> If we're following a local redirect, keep sending the same Host header.
>
> Fixes issue 8027.
>
> Please review this at https://codereview.appspot.com/98320044/
>
> Affected files (+66, -1 lines):
>   M src/pkg/net/http/client.go
>   M src/pkg/net/http/client_test.go
>
>
> Index: src/pkg/net/http/client.go
> ===================================================================
> --- a/src/pkg/net/http/client.go
> +++ b/src/pkg/net/http/client.go
> @@ -317,10 +317,17 @@
>                                 nreq.Method = "GET"
>                         }
>                         nreq.Header = make(Header)
> -                       nreq.URL, err = base.Parse(urlStr)
> +                       var relurl *url.URL
> +                       relurl, err = url.Parse(urlStr)
>                         if err != nil {
>                                 break
>                         }
> +                       // if someone used the trick of setting a
> different Host and URL.Host
> +                       // we need to make sure that we don't change the
> Host
> +                       if !relurl.IsAbs() {
> +                               nreq.Host = req.Host
> +                       }
> +                       nreq.URL = base.ResolveReference(relurl)
>                         if len(via) > 0 {
>                                 // Add the Referer header.
>                                 lastReq := via[len(via)-1]
> Index: src/pkg/net/http/client_test.go
> ===================================================================
> --- a/src/pkg/net/http/client_test.go
> +++ b/src/pkg/net/http/client_test.go
> @@ -1036,3 +1036,61 @@
>                 t.Errorf("Response trailers = %#v; want %#v", res.Trailer,
> want)
>         }
>  }
> +
> +func TestClientChangesHost(t *testing.T) {
> +       defer afterTest(t)
> +       saw := make(chan string, 3)
> +       var ts *httptest.Server
> +       ts = httptest.NewServer(HandlerFunc(func(w ResponseWriter, r
> *Request) {
> +               saw <- r.Host
> +               switch r.URL.Path {
> +               case "/":
> +                       Redirect(w, r, "/foo", StatusFound)
> +               case "/foo":
> +                       Redirect(w, r, ts.URL+"/bar", StatusFound)
> +               }
> +       }))
> +       defer ts.Close()
> +
> +       req, err := NewRequest("GET", ts.URL, nil)
> +       if err != nil {
> +               t.Fatalf("expected successful request build, got %q", err)
> +       }
> +       req.Host = "example.com"
> +
> +       c := &Client{}
> +       _, err = c.Do(req)
> +       if err != nil {
> +               t.Fatalf("expected successful request, got %q", err)
> +       }
> +       select {
> +       case host := <-saw:
> +               if host != "example.com" {
> +                       t.Fatalf("expected host to be 'example.com', got
> %q", host)
> +               }
> +       default:
> +               t.Fatal("server didn't see a request")
> +       }
> +
> +       select {
> +       case host := <-saw:
> +               if host != "example.com" {
> +                       t.Fatalf("expected host to be 'example.com', got
> %q", host)
> +               }
> +       default:
> +               t.Fatal("server didn't see a second request")
> +       }
> +
> +       select {
> +       case host := <-saw:
> +               tsurl, err := url.Parse(ts.URL)
> +               if err != nil {
> +                       t.Fatalf("expected successful url parse, got %q",
> err)
> +               }
> +               if host != tsurl.Host {
> +                       t.Fatalf("expected host to be %q, got %q",
> tsurl.Host, host)
> +               }
> +       default:
> +               t.Fatal("server didn't see the third request")
> +       }
> +}
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[DMorsing, 2014-05-19 18:47:09]

On 2014/05/19 18:45:25, bradfitz wrote:
> I don't even want to spend the mental energy reviewing this while we're in
> code freeze unless you tell me it's an important regression from Go 1.2.
> 
> Can this wait?

Yes.

[DMorsing, 2014-07-20 12:43:18]

ping.

[bradfitz, 2014-07-23 18:04:35]

I'm not crazy about this change, but if you make the code sufficiently readable
and paranoid I'd accept it.

https://codereview.appspot.com/98320044/diff/70001/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (right):

https://codereview.appspot.com/98320044/diff/70001/src/pkg/net/http/client.go...
src/pkg/net/http/client.go:320: var relurl *url.URL
why is this named rel? It's not necessarily relative.

https://codereview.appspot.com/98320044/diff/70001/src/pkg/net/http/client.go...
src/pkg/net/http/client.go:325: // if someone used the trick of setting a
different Host and URL.Host
setting Host explicitly and different than URL.Host

But I want to see that check spelled out explicitly here before going into any
different behavior in general.

In fact, I'd make a bool up above the direct loop like:

   // hostsDiffer is whether the user set an explicit Hostname on the request
   // and blah blah blah...
   hostsDiffer := ireq.Hostname != "" && ireq.Hostname != ireq.URL.Host

and then down here use 

    if hostsDiffer && compare ireq.URL.Host to target host ... {
           ...
    } else {
           ... old logic unchanged
    }

https://codereview.appspot.com/98320044/diff/70001/src/pkg/net/http/client.go...
src/pkg/net/http/client.go:330: nreq.URL = base.ResolveReference(relurl)
in particular, I don't want to ever call this function unless we're in the
hostsDiffer mode and still on the same host.

[DMorsing, 2014-07-27 15:33:42]

Beginning to think that if you need this level of control over redirects, you
can use the transport directly.

[gobot, 2014-12-19 05:19:37]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/98320044 is best) in the description in your
new CL.

Thanks very much.