Fix session cookie issues (#626, #549)

go.mod

@@ -7,7 +7,7 @@ require (
 	github.com/golang-jwt/jwt/v5 v5.2.2
 	github.com/gorilla/mux v1.6.2
 	github.com/gorilla/pat v0.0.0-20180118222023-199c85a7f6d1
-	github.com/gorilla/sessions v1.1.1
+	github.com/gorilla/sessions v1.4.0
 	github.com/jarcoal/httpmock v0.0.0-20180424175123-9c70cfe4a1da
 	github.com/lestrrat-go/jwx v1.2.29
 	github.com/markbates/going v1.0.0
@@ -22,7 +22,7 @@ require (
 	github.com/decred/dcrd/dcrec/secp256k1/v4 v4.2.0 // indirect
 	github.com/goccy/go-json v0.10.2 // indirect
 	github.com/gorilla/context v1.1.1 // indirect
-	github.com/gorilla/securecookie v1.1.1 // indirect
+	github.com/gorilla/securecookie v1.1.2 // indirect
 	github.com/kr/pretty v0.3.1 // indirect
 	github.com/lestrrat-go/backoff/v2 v2.0.8 // indirect
 	github.com/lestrrat-go/blackmagic v1.0.2 // indirect

go.sum

@@ -13,18 +13,18 @@ github.com/goccy/go-json v0.10.2 h1:CrxCmQqYDkv1z7lO7Wbh2HN93uovUHgrECaO5ZrCXAU=
 github.com/goccy/go-json v0.10.2/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I=
 github.com/golang-jwt/jwt/v5 v5.2.2 h1:Rl4B7itRWVtYIHFrSNd7vhTiz9UpLdi6gZhZ3wEeDy8=
 github.com/golang-jwt/jwt/v5 v5.2.2/go.mod h1:pqrtFR0X4osieyHYxtmOUWsAWrfe1Q5UVIyoH402zdk=
-github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
-github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
+github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0=
+github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/gorilla/context v1.1.1 h1:AWwleXJkX/nhcU9bZSnZoi3h/qGYqQAGhq6zZe/aQW8=
 github.com/gorilla/context v1.1.1/go.mod h1:kBGZzfjB9CEq2AlWe17Uuf7NDRt0dE0s8S51q0aT7Yg=
 github.com/gorilla/mux v1.6.2 h1:Pgr17XVTNXAk3q/r4CpKzC5xBM/qW1uVLV+IhRZpIIk=
 github.com/gorilla/mux v1.6.2/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs=
 github.com/gorilla/pat v0.0.0-20180118222023-199c85a7f6d1 h1:LqbZZ9sNMWVjeXS4NN5oVvhMjDyLhmA1LG86oSo+IqY=
 github.com/gorilla/pat v0.0.0-20180118222023-199c85a7f6d1/go.mod h1:YeAe0gNeiNT5hoiZRI4yiOky6jVdNvfO2N6Kav/HmxY=
-github.com/gorilla/securecookie v1.1.1 h1:miw7JPhV+b/lAHSXz4qd/nN9jRiAFV5FwjeKyCS8BvQ=
-github.com/gorilla/securecookie v1.1.1/go.mod h1:ra0sb63/xPlUeL+yeDciTfxMRAA+MP+HVt/4epWDjd4=
-github.com/gorilla/sessions v1.1.1 h1:YMDmfaK68mUixINzY/XjscuJ47uXFWSSHzFbBQM0PrE=
-github.com/gorilla/sessions v1.1.1/go.mod h1:8KCfur6+4Mqcc6S0FEfKuN15Vl5MgXW92AE8ovaJD0w=
+github.com/gorilla/securecookie v1.1.2 h1:YCIWL56dvtr73r6715mJs5ZvhtnY73hBvEF8kXD8ePA=
+github.com/gorilla/securecookie v1.1.2/go.mod h1:NfCASbcHqRSY+3a8tlWJwsQap2VX5pwzwo4h3eOamfo=
+github.com/gorilla/sessions v1.4.0 h1:kpIYOp/oi6MG/p5PgxApU8srsSw9tuFbt46Lt7auzqQ=
+github.com/gorilla/sessions v1.4.0/go.mod h1:FLWm50oby91+hl7p/wRxDth9bWSuk0qVL2emc7lT5ik=
 github.com/jarcoal/httpmock v0.0.0-20180424175123-9c70cfe4a1da h1:FjHUJJ7oBW4G/9j1KzlHaXL09LyMVM9rupS39lncbXk=
 github.com/jarcoal/httpmock v0.0.0-20180424175123-9c70cfe4a1da/go.mod h1:ks+b9deReOc7jgqp+e7LuFiCBH6Rm5hL32cLcEAArb4=
 github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI=
@@ -85,8 +85,6 @@ golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug
 golang.org/x/net v0.6.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs=
 golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg=
 golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44=
-golang.org/x/oauth2 v0.27.0 h1:da9Vo7/tDv5RH/7nZDz1eMGS/q1Vv1N/7FCrBhI9I3M=
-golang.org/x/oauth2 v0.27.0/go.mod h1:onh5ek6nERTohokkhCD/y2cV4Do3fxFHFuAejCkRWT8=
 golang.org/x/oauth2 v0.30.0 h1:dnDm7JmhM45NNpd8FDDeLhK6FwqbOf4MLCM9zb1BOHI=
 golang.org/x/oauth2 v0.30.0/go.mod h1:B++QgG3ZKulg6sRPGD/mqlHQs5rB3Ml9erfeDY7xKlU=
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=

gothic/gothic.go

@@ -204,11 +204,11 @@ var CompleteUserAuth = func(res http.ResponseWriter, req *http.Request) (goth.Us
 		return goth.User{}, err
 	}
 
-	err = StoreInSession(providerName, sess.Marshal(), req, res)
-
-	if err != nil {
-		return goth.User{}, err
-	}
+	// Note: We intentionally do NOT call StoreInSession here.
+	// The session is only used during the OAuth flow and is cleared by
+	// the deferred Logout call. Storing it would create a duplicate
+	// Set-Cookie header that gets immediately invalidated.
+	// See: https://github.com/markbates/goth/issues/626
 
 	gu, err := provider.FetchUser(sess)
 	return gu, err

gothic/gothic_test.go

@@ -101,113 +101,113 @@
 	req, err := http.NewRequest("GET", "/auth?provider=faux", nil)
 	a.NoError(err)
 
 	u, err := GetAuthURL(res, req)
 	a.NoError(err)
 
 	// Check that we get the correct auth URL with a state parameter
 	parsed, err := url.Parse(u)
 	a.NoError(err)
 	a.Equal("http", parsed.Scheme)
 	a.Equal("example.com", parsed.Host)
 	q := parsed.Query()
 	a.Contains(q, "client_id")
 	a.Equal("code", q.Get("response_type"))
 	a.NotZero(q, "state")
 
 	// Check that if we run GetAuthURL on another request, that request's
 	// auth URL has a different state from the previous one.
 	req2, err := http.NewRequest("GET", "/auth?provider=faux", nil)
 	a.NoError(err)
 	url2, err := GetAuthURL(httptest.NewRecorder(), req2)
 	a.NoError(err)
 	parsed2, err := url.Parse(url2)
 	a.NoError(err)
 	a.NotEqual(parsed.Query().Get("state"), parsed2.Query().Get("state"))
 }
 
 func Test_CompleteUserAuth(t *testing.T) {
 	a := assert.New(t)
 
 	res := httptest.NewRecorder()
 	req, err := http.NewRequest("GET", "/auth/callback?provider=faux", nil)
 	a.NoError(err)
 
 	sess := faux.Session{Name: "Homer Simpson", Email: "homer@example.com"}
 	session, _ := Store.Get(req, SessionName)
 	session.Values["faux"] = gzipString(sess.Marshal())
 	err = session.Save(req, res)
 	a.NoError(err)
 
 	user, err := CompleteUserAuth(res, req)
 	a.NoError(err)
 
 	a.Equal(user.Name, "Homer Simpson")
 	a.Equal(user.Email, "homer@example.com")
 }
 
 func Test_CompleteUserAuthWithSessionDeducedProvider(t *testing.T) {
 	a := assert.New(t)
 
 	res := httptest.NewRecorder()
 	// Intentionally omit a provider argument, force looking in session.
 	req, err := http.NewRequest("GET", "/auth/callback", nil)
 	a.NoError(err)
 
 	sess := faux.Session{Name: "Homer Simpson", Email: "homer@example.com"}
 	session, _ := Store.Get(req, SessionName)
 	session.Values["faux"] = gzipString(sess.Marshal())
 	err = session.Save(req, res)
 	a.NoError(err)
 
 	user, err := CompleteUserAuth(res, req)
 	a.NoError(err)
 
 	a.Equal(user.Name, "Homer Simpson")
 	a.Equal(user.Email, "homer@example.com")
 }
 
 func Test_CompleteUserAuthWithContextParamProvider(t *testing.T) {
 	a := assert.New(t)
 
 	res := httptest.NewRecorder()
 	req, err := http.NewRequest("GET", "/auth/callback", nil)
 	a.NoError(err)
 
 	req = GetContextWithProvider(req, "faux")
 
 	sess := faux.Session{Name: "Homer Simpson", Email: "homer@example.com"}
 	session, _ := Store.Get(req, SessionName)
 	session.Values["faux"] = gzipString(sess.Marshal())
 	err = session.Save(req, res)
 	a.NoError(err)
 
 	user, err := CompleteUserAuth(res, req)
 	a.NoError(err)
 
 	a.Equal(user.Name, "Homer Simpson")
 	a.Equal(user.Email, "homer@example.com")
 }
 
 func Test_Logout(t *testing.T) {
 	a := assert.New(t)
 
 	res := httptest.NewRecorder()
 	req, err := http.NewRequest("GET", "/auth/callback?provider=faux", nil)
 	a.NoError(err)
 
 	sess := faux.Session{Name: "Homer Simpson", Email: "homer@example.com"}
 	session, _ := Store.Get(req, SessionName)
 	session.Values["faux"] = gzipString(sess.Marshal())
 	err = session.Save(req, res)
 	a.NoError(err)
 
 	user, err := CompleteUserAuth(res, req)
 	a.NoError(err)
 
 	a.Equal(user.Name, "Homer Simpson")
 	a.Equal(user.Email, "homer@example.com")
 	err = Logout(res, req)
 	a.NoError(err)
 	session, _ = Store.Get(req, SessionName)
 	a.Equal(session.Values, make(map[interface{}]interface{}))
 	a.Equal(session.Options.MaxAge, -1)
@@ -290,3 +290,122 @@
 
 	return string(s)
 }
+
+// Test_StoreInSession_ReturnsErrorOnSaveFailure verifies that StoreInSession
+// properly propagates errors from session.Save().
+// See: https://github.com/markbates/goth/issues/549
+func Test_StoreInSession_ReturnsErrorOnSaveFailure(t *testing.T) {
+	a := assert.New(t)
+
+	// Use a store that always fails on save
+	originalStore := Store
+	Store = &failingStore{err: fmt.Errorf("session save failed: hash key is not set")}
+	defer func() { Store = originalStore }()
+
+	res := httptest.NewRecorder()
+	req, err := http.NewRequest("GET", "/auth?provider=faux", nil)
+	a.NoError(err)
+
+	// StoreInSession should return the error from the failing store
+	err = StoreInSession("faux", "test-value", req, res)
+	if a.Error(err, "Expected error from failing store") {
+		a.Contains(err.Error(), "session save failed", "Error should be propagated from store")
+	}
+}
+
+// Test_GetAuthURL_PropagatesSessionErrors verifies that GetAuthURL returns
+// errors from session operations, such as when securecookie fails due to
+// missing hash key.
+// See: https://github.com/markbates/goth/issues/549
+func Test_GetAuthURL_PropagatesSessionErrors(t *testing.T) {
+	a := assert.New(t)
+
+	// Use a store that fails on save (simulating securecookie with no hash key)
+	originalStore := Store
+	Store = &failingStore{err: fmt.Errorf("securecookie: hash key is not set")}
+	defer func() { Store = originalStore }()
+
+	res := httptest.NewRecorder()
+	req, err := http.NewRequest("GET", "/auth?provider=faux", nil)
+	a.NoError(err)
+
+	// GetAuthURL should propagate the session error
+	_, err = GetAuthURL(res, req)
+	if a.Error(err, "Expected error when session save fails") {
+		a.Contains(err.Error(), "hash key is not set", "Original error should be propagated")
+	}
+}
+
+// failingStore is a test store that always fails on Save
+type failingStore struct {
+	err error
+}
+
+func (f *failingStore) Get(r *http.Request, name string) (*sessions.Session, error) {
+	return sessions.NewSession(f, name), nil
+}
+
+func (f *failingStore) New(r *http.Request, name string) (*sessions.Session, error) {
+	return sessions.NewSession(f, name), nil
+}
+
+func (f *failingStore) Save(r *http.Request, w http.ResponseWriter, s *sessions.Session) error {
+	return f.err
+}
+
+// Test_CompleteUserAuth_SingleSetCookie verifies that CompleteUserAuth only
+// produces a single Set-Cookie header (the logout cookie), not two headers
+// where one is valid and one is expired.
+// See: https://github.com/markbates/goth/issues/626
+func Test_CompleteUserAuth_SingleSetCookie(t *testing.T) {
+	a := assert.New(t)
+
+	// Use a real cookie store to test Set-Cookie header behavior
+	cookieStore := sessions.NewCookieStore([]byte("test-secret-key-32-bytes-long!!"))
+	cookieStore.Options.MaxAge = 86400 * 30
+	originalStore := Store
+	Store = cookieStore
+	defer func() { Store = originalStore }()
+
+	// Create request with session containing auth data
+	req, err := http.NewRequest("GET", "/auth/callback?provider=faux", nil)
+	a.NoError(err)
+
+	// Set up session with provider data (simulating after BeginAuth)
+	sess := faux.Session{Name: "Homer Simpson", Email: "homer@example.com"}
+	session, _ := Store.New(req, SessionName)
+	session.Values["faux"] = gzipString(sess.Marshal())
+
+	// Save session and get the cookie to include in request
+	setupRes := httptest.NewRecorder()
+	err = session.Save(req, setupRes)
+	a.NoError(err)
+
+	// Extract the session cookie from setup response
+	cookies := setupRes.Result().Cookies()
+	a.NotEmpty(cookies, "Expected session cookie to be set")
+
+	// Create new request with the session cookie
+	req, err = http.NewRequest("GET", "/auth/callback?provider=faux", nil)
+	a.NoError(err)
+	for _, c := range cookies {
+		req.AddCookie(c)
+	}
+
+	// Now call CompleteUserAuth
+	res := httptest.NewRecorder()
+	user, err := CompleteUserAuth(res, req)
+	a.NoError(err)
+	a.Equal("Homer Simpson", user.Name)
+
+	// Count Set-Cookie headers - should be exactly 1 (the logout cookie)
+	setCookieHeaders := res.Result().Header["Set-Cookie"]
+	a.Len(setCookieHeaders, 1, "Expected exactly 1 Set-Cookie header, got %d: %v", len(setCookieHeaders), setCookieHeaders)
+
+	// The single cookie should be the logout cookie (MaxAge=-1 or expires in past)
+	if len(setCookieHeaders) == 1 {
+		cookie := setCookieHeaders[0]
+		// Logout sets MaxAge=-1 which results in immediate expiry
+		a.Contains(cookie, "Max-Age=0", "Expected logout cookie with Max-Age=0")
+	}
+}