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
Closed

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

dsnet opened this issue Oct 18, 2016 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dsnet
Copy link
Member

dsnet 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 The path to resolution is known, but the work has not been done. 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
Copy link
Member

@dsnet did you mean CL https://go-review.googlesource.com/#/c/28930?

@dsnet
Copy link
Member Author

dsnet commented Oct 18, 2016

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

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Oct 25, 2017
@rsc rsc unassigned dsnet Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants