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

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 <jonnytran@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Kyle D <kdumontnu@gmail.com>

2 files changed, 34 insertions, 5 deletions

diff --git a/modules/session/store.go b/modules/session/store.go
index 4fa4d2848f..2f7ab7760b 100644
--- 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
index 621640895b..0bed726793 100644
--- 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())
 }