Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: regression caused by copying headers in redirects #17494

Closed
dsnet opened this issue Oct 18, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@dsnet
Copy link
Member

commented Oct 18, 2016

http://golang.org/cl/28930 caused a regression. The CL in question fixes #4800 by automatically copying the headers when following a redirect. In the situation where the initial request has some cookie set and one of the redirects sets the cookie to a different value, we should change the cookies sent in subsequent requests to use newly set cookies.

This pattern is used by some login logic where credentials are stored in the cookies. Suppose the client has a stale auth cookie and makes a request. The server redirects the client to some other page to obtain a new auth cookie (and gets redirected back to the original page). When the client redirects back to original page, it presents both the stale and new auth cookie, which confuses the server. Since the server still thinks the client is not logged in, it sends it through the redirect loop again.

Minimum reproducing case:

func main() {
    // Start a trivial server.
    go func() {
        http.HandleFunc("/", func(resp http.ResponseWriter, req *http.Request) {
            fmt.Println("Got cookie:", req.Header.Get("Cookie"))

            // Set the cookie to a new value.
            http.SetCookie(resp, &http.Cookie{
                Name:  "YumYumCookie",
                Value: "NewValue",
                Path:  "/",
            })

            // Keep redirecting to yourself until you see the new cookie value.
            ck, _ := req.Cookie("YumYumCookie")
            if ck.Value != "NewValue" {
                http.Redirect(resp, req, "/", http.StatusFound)
            }
        })
        http.ListenAndServe("localhost:8888", nil)
    }()

    time.Sleep(time.Second)

    // Make a request to the server. Initialize the request with an old value for the cookie.
    jar, _ := cookiejar.New(nil)
    client := &http.Client{Jar: jar}
    req, _ := http.NewRequest("GET", "http://localhost:8888/", nil)
    req.AddCookie(&http.Cookie{
        Name:  "YumYumCookie",
        Value: "OldValue",
        Path:  "/",
    })
    req.Header.Add("HeaderKey", "HeaderValue")
    fmt.Println(client.Do(req))
}

On go1.7, I see:

Got cookie: YumYumCookie=OldValue
Got cookie: YumYumCookie=NewValue
&{200 OK 200 HTTP/1.1 1 1 map[Content-Type:[text/plain; charset=utf-8] Set-Cookie:[YumYumCookie=NewValue; Path=/] Date:[Tue, 18 Oct 2016 01:38:34 GMT] Content-Length:[0]] 0x784480 0 [] false false map[] 0xc4201261e0 <nil>} <nil>

On go1.8dev, I see:

Got cookie: YumYumCookie=OldValue
Got cookie: YumYumCookie=OldValue; YumYumCookie=NewValue
Got cookie: YumYumCookie=OldValue; YumYumCookie=NewValue
Got cookie: YumYumCookie=OldValue; YumYumCookie=NewValue
Got cookie: YumYumCookie=OldValue; YumYumCookie=NewValue
Got cookie: YumYumCookie=OldValue; YumYumCookie=NewValue
Got cookie: YumYumCookie=OldValue; YumYumCookie=NewValue
Got cookie: YumYumCookie=OldValue; YumYumCookie=NewValue
Got cookie: YumYumCookie=OldValue; YumYumCookie=NewValue
Got cookie: YumYumCookie=OldValue; YumYumCookie=NewValue
&{302 Found 302 HTTP/1.1 1 1 map[Location:[/] Set-Cookie:[YumYumCookie=NewValue; Path=/] Date:[Tue, 18 Oct 2016 01:38:54 GMT] Content-Length:[24] Content-Type:[text/html; charset=utf-8]] 0xc42004e640 24 [] false false map[] 0xc42012c3c0 <nil>} Get /: stopped after 10 redirects

/cc @bradfitz

@dsnet dsnet added the NeedsFix label Oct 18, 2016

@dsnet dsnet added this to the Go1.8 milestone Oct 18, 2016

@dsnet dsnet self-assigned this Oct 18, 2016

@odeke-em

This comment has been minimized.

Copy link
Member

commented Oct 18, 2016

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2016

@odeke-em, that is correct. Thank you.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 18, 2016

CL https://golang.org/cl/31435 mentions this issue.

@gopherbot gopherbot closed this in c60d9a3 Oct 25, 2016

@golang golang locked and limited conversation to collaborators Oct 25, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.