net/http: fix redirect logic to handle mutations of cookies

In the situation where the Client.Jar is set and the Request.Header
has cookies manually inserted, the redirect logic needs to be
able to apply changes to cookies from "Set-Cookie" headers to both
the Jar and the manually inserted Header cookies.

Since Header cookies lack information about the original domain
and path, the logic in this CL simply removes cookies from the
initial Header if any subsequent "Set-Cookie" matches. Thus,
in the event of cookie conflicts, the logic preserves the behavior
prior to change made in golang.org/cl/28930.

Fixes #17494
Updates #4800

Change-Id: I645194d9f97ff4d95bd07ca36de1d6cdf2f32429
Reviewed-on: https://go-review.googlesource.com/31435
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

1 files changed, 96 insertions, 0 deletions

diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go
index 77498b3913..c86ae19c86 100644
--- a/src/net/http/client_test.go
+++ b/src/net/http/client_test.go
@@ -19,6 +19,7 @@ import (
 	"log"
 	"net"
 	. "net/http"
+	"net/http/cookiejar"
 	"net/http/httptest"
 	"net/url"
 	"reflect"
@@ -1296,6 +1297,101 @@ func TestClientCopyHeadersOnRedirect(t *testing.T) {
 	}
 }
 
+// Issue 17494: cookies should be altered when Client follows redirects.
+func TestClientAltersCookiesOnRedirect(t *testing.T) {
+	cookieMap := func(cs []*Cookie) map[string][]string {
+		m := make(map[string][]string)
+		for _, c := range cs {
+			m[c.Name] = append(m[c.Name], c.Value)
+		}
+		return m
+	}
+
+	ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+		var want map[string][]string
+		got := cookieMap(r.Cookies())
+
+		c, _ := r.Cookie("Cycle")
+		switch c.Value {
+		case "0":
+			want = map[string][]string{
+				"Cookie1": []string{"OldValue1a", "OldValue1b"},
+				"Cookie2": []string{"OldValue2"},
+				"Cookie3": []string{"OldValue3a", "OldValue3b"},
+				"Cookie4": []string{"OldValue4"},
+				"Cycle":   []string{"0"},
+			}
+			SetCookie(w, &Cookie{Name: "Cycle", Value: "1", Path: "/"})
+			SetCookie(w, &Cookie{Name: "Cookie2", Path: "/", MaxAge: -1}) // Delete cookie from Header
+			Redirect(w, r, "/", StatusFound)
+		case "1":
+			want = map[string][]string{
+				"Cookie1": []string{"OldValue1a", "OldValue1b"},
+				"Cookie3": []string{"OldValue3a", "OldValue3b"},
+				"Cookie4": []string{"OldValue4"},
+				"Cycle":   []string{"1"},
+			}
+			SetCookie(w, &Cookie{Name: "Cycle", Value: "2", Path: "/"})
+			SetCookie(w, &Cookie{Name: "Cookie3", Value: "NewValue3", Path: "/"}) // Modify cookie in Header
+			SetCookie(w, &Cookie{Name: "Cookie4", Value: "NewValue4", Path: "/"}) // Modify cookie in Jar
+			Redirect(w, r, "/", StatusFound)
+		case "2":
+			want = map[string][]string{
+				"Cookie1": []string{"OldValue1a", "OldValue1b"},
+				"Cookie3": []string{"NewValue3"},
+				"Cookie4": []string{"NewValue4"},
+				"Cycle":   []string{"2"},
+			}
+			SetCookie(w, &Cookie{Name: "Cycle", Value: "3", Path: "/"})
+			SetCookie(w, &Cookie{Name: "Cookie5", Value: "NewValue5", Path: "/"}) // Insert cookie into Jar
+			Redirect(w, r, "/", StatusFound)
+		case "3":
+			want = map[string][]string{
+				"Cookie1": []string{"OldValue1a", "OldValue1b"},
+				"Cookie3": []string{"NewValue3"},
+				"Cookie4": []string{"NewValue4"},
+				"Cookie5": []string{"NewValue5"},
+				"Cycle":   []string{"3"},
+			}
+			// Don't redirect to ensure the loop ends.
+		default:
+			t.Errorf("unexpected redirect cycle")
+			return
+		}
+
+		if !reflect.DeepEqual(got, want) {
+			t.Errorf("redirect %s, Cookie = %v, want %v", c.Value, got, want)
+		}
+	}))
+	defer ts.Close()
+
+	tr := &Transport{}
+	defer tr.CloseIdleConnections()
+	jar, _ := cookiejar.New(nil)
+	c := &Client{
+		Transport: tr,
+		Jar:       jar,
+	}
+
+	u, _ := url.Parse(ts.URL)
+	req, _ := NewRequest("GET", ts.URL, nil)
+	req.AddCookie(&Cookie{Name: "Cookie1", Value: "OldValue1a"})
+	req.AddCookie(&Cookie{Name: "Cookie1", Value: "OldValue1b"})
+	req.AddCookie(&Cookie{Name: "Cookie2", Value: "OldValue2"})
+	req.AddCookie(&Cookie{Name: "Cookie3", Value: "OldValue3a"})
+	req.AddCookie(&Cookie{Name: "Cookie3", Value: "OldValue3b"})
+	jar.SetCookies(u, []*Cookie{&Cookie{Name: "Cookie4", Value: "OldValue4", Path: "/"}})
+	jar.SetCookies(u, []*Cookie{&Cookie{Name: "Cycle", Value: "0", Path: "/"}})
+	res, err := c.Do(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+	if res.StatusCode != 200 {
+		t.Fatal(res.Status)
+	}
+}
+
 // Part of Issue 4800
 func TestShouldCopyHeaderOnRedirect(t *testing.T) {
 	tests := []struct {