fix: Fix to delete cookie when AppSubURL is non-empty (#30375) (#30469)

Backport #30375 by @jtran Cookies may exist on "/subpath" and "/subpath/" for some legacy reasons (eg: changed CookiePath behavior in code). The legacy cookie should be removed correctly. Co-authored-by: Jonathan Tran <[email protected]> Co-authored-by: wxiaoguang <[email protected]> Co-authored-by: Kyle D <[email protected]>
go-gitea · Apr 14, 2024 · e64926c · e64926c
1 parent 0352b99
commit e64926c
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 6 deletions.
diff --git a/modules/session/store.go b/modules/session/store.go
@@ -6,6 +6,9 @@ package session
 import (
 	"net/http"
 
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/web/middleware"
+
 	"gitea.com/go-chi/session"
 )
 
@@ -18,6 +21,10 @@ type Store interface {
 
 // RegenerateSession regenerates the underlying session and returns the new store
 func RegenerateSession(resp http.ResponseWriter, req *http.Request) (Store, error) {
+	// Ensure that a cookie with a trailing slash does not take precedence over
+	// the cookie written by the middleware.
+	middleware.DeleteLegacySiteCookie(resp, setting.SessionConfig.CookieName)
+
 	s, err := session.RegenerateSession(resp, req)
 	return s, err
 }
diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go
@@ -45,10 +45,32 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) {
 		SameSite: setting.SessionConfig.SameSite,
 	}
 	resp.Header().Add("Set-Cookie", cookie.String())
-	if maxAge < 0 {
-		// There was a bug in "setting.SessionConfig.CookiePath" code, the old default value of it was empty "".
-		// So we have to delete the cookie on path="" again, because some old code leaves cookies on path="".
-		cookie.Path = strings.TrimSuffix(setting.SessionConfig.CookiePath, "/")
-		resp.Header().Add("Set-Cookie", cookie.String())
+	// Previous versions would use a cookie path with a trailing /.
+	// These are more specific than cookies without a trailing /, so
+	// we need to delete these if they exist.
+	DeleteLegacySiteCookie(resp, name)
+}
+
+// DeleteLegacySiteCookie deletes the cookie with the given name at the cookie
+// path with a trailing /, which would unintentionally override the cookie.
+func DeleteLegacySiteCookie(resp http.ResponseWriter, name string) {
+	if setting.SessionConfig.CookiePath == "" || strings.HasSuffix(setting.SessionConfig.CookiePath, "/") {
+		// If the cookie path ends with /, no legacy cookies will take
+		// precedence, so do nothing.  The exception is that cookies with no
+		// path could override other cookies, but it's complicated and we don't
+		// currently handle that.
+		return
 	}
+
+	cookie := &http.Cookie{
+		Name:     name,
+		Value:    "",
+		MaxAge:   -1,
+		Path:     setting.SessionConfig.CookiePath + "/",
+		Domain:   setting.SessionConfig.Domain,
+		Secure:   setting.SessionConfig.Secure,
+		HttpOnly: true,
+		SameSite: setting.SessionConfig.SameSite,
+	}
+	resp.Header().Add("Set-Cookie", cookie.String())
 }
diff --git a/services/auth/source/oauth2/store.go b/services/auth/source/oauth2/store.go
@@ -9,6 +9,7 @@ import (
 	"net/http"
 
 	"code.gitea.io/gitea/modules/log"
+	session_module "code.gitea.io/gitea/modules/session"
 
 	chiSession "gitea.com/go-chi/session"
 	"github.com/gorilla/sessions"
@@ -65,7 +66,7 @@ func (st *SessionsStore) Save(r *http.Request, w http.ResponseWriter, session *s
 	chiStore := chiSession.GetSession(r)
 
 	if session.IsNew {
-		_, _ = chiSession.RegenerateSession(w, r)
+		_, _ = session_module.RegenerateSession(w, r)
 		session.IsNew = false
 	}