From c4fb54ce5e8ae43e89f55dffe4b606f390559f15 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Tue, 9 Apr 2024 12:23:35 -0400 Subject: [PATCH 01/10] fix: Fix to delete cookie when AppSubURL is non-empty --- modules/web/middleware/cookie.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index 621640895b95..ce37362ea4f2 100644 --- a/modules/web/middleware/cookie.go +++ b/modules/web/middleware/cookie.go @@ -50,5 +50,10 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) { // 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()) + // Ensure that we delete the cookie when AppSubURL is non-empty. + if cookie.Path != "/" { + cookie.Path = "/" + resp.Header().Add("Set-Cookie", cookie.String()) + } } } From 35fd915232ad5231d1979e8d16d0e12501a052dc Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Tue, 9 Apr 2024 16:50:36 -0400 Subject: [PATCH 02/10] Fix to clear the cookie both by stripping the slash and adding it --- modules/web/middleware/cookie.go | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index ce37362ea4f2..f470daa686b6 100644 --- a/modules/web/middleware/cookie.go +++ b/modules/web/middleware/cookie.go @@ -48,12 +48,23 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) { 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()) - // Ensure that we delete the cookie when AppSubURL is non-empty. - if cookie.Path != "/" { - cookie.Path = "/" - resp.Header().Add("Set-Cookie", cookie.String()) + // The code was updated, but it behaves differently depending on the + // value of AppSubURL. When AppSubURL is non-empty, the cookie with a + // trailing slash must be deleted. + withoutTrailingSlash := strings.TrimSuffix(setting.SessionConfig.CookiePath, "/") + withTrailingSlash := ensureSuffix(setting.SessionConfig.CookiePath, "/") + for _, path := range []string{withoutTrailingSlash, withTrailingSlash} { + if setting.SessionConfig.CookiePath != path { + cookie.Path = path + resp.Header().Add("Set-Cookie", cookie.String()) + } } } } + +func ensureSuffix(s, suffix string) string { + if strings.HasSuffix(s, suffix) { + return s + } + return s + suffix +} From 475eac8fea0cd39bbdcaa59d576df8cc9f00bec6 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Tue, 9 Apr 2024 22:55:58 -0400 Subject: [PATCH 03/10] Change to not use helper function --- modules/web/middleware/cookie.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index f470daa686b6..da04daaa0c1a 100644 --- a/modules/web/middleware/cookie.go +++ b/modules/web/middleware/cookie.go @@ -52,7 +52,7 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) { // value of AppSubURL. When AppSubURL is non-empty, the cookie with a // trailing slash must be deleted. withoutTrailingSlash := strings.TrimSuffix(setting.SessionConfig.CookiePath, "/") - withTrailingSlash := ensureSuffix(setting.SessionConfig.CookiePath, "/") + withTrailingSlash := withoutTrailingSlash + "/" for _, path := range []string{withoutTrailingSlash, withTrailingSlash} { if setting.SessionConfig.CookiePath != path { cookie.Path = path @@ -61,10 +61,3 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) { } } } - -func ensureSuffix(s, suffix string) string { - if strings.HasSuffix(s, suffix) { - return s - } - return s + suffix -} From e13a93f3825c1e9858e021bd74d8d6f534a18d73 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Tue, 9 Apr 2024 23:09:35 -0400 Subject: [PATCH 04/10] Change to consolidate the add-header line --- modules/web/middleware/cookie.go | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index da04daaa0c1a..5cf789a9c158 100644 --- a/modules/web/middleware/cookie.go +++ b/modules/web/middleware/cookie.go @@ -38,26 +38,23 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) { Name: name, Value: url.QueryEscape(value), MaxAge: maxAge, - Path: setting.SessionConfig.CookiePath, + Path: "", // Filled in below. Domain: setting.SessionConfig.Domain, Secure: setting.SessionConfig.Secure, HttpOnly: true, 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="". - // The code was updated, but it behaves differently depending on the - // value of AppSubURL. When AppSubURL is non-empty, the cookie with a - // trailing slash must be deleted. - withoutTrailingSlash := strings.TrimSuffix(setting.SessionConfig.CookiePath, "/") - withTrailingSlash := withoutTrailingSlash + "/" - for _, path := range []string{withoutTrailingSlash, withTrailingSlash} { - if setting.SessionConfig.CookiePath != path { - cookie.Path = path - resp.Header().Add("Set-Cookie", cookie.String()) - } + // 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="". + // The code was updated, but it behaves differently depending on the value + // of AppSubURL. When AppSubURL is non-empty, the cookie with a trailing + // slash must be deleted. + withoutTrailingSlash := strings.TrimSuffix(setting.SessionConfig.CookiePath, "/") + withTrailingSlash := withoutTrailingSlash + "/" + for _, path := range []string{withoutTrailingSlash, withTrailingSlash} { + if maxAge < 0 || setting.SessionConfig.CookiePath == path { + cookie.Path = path + resp.Header().Add("Set-Cookie", cookie.String()) } } } From fcc77fcb98fad49aca2b346b3059f2f124f2ee05 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Tue, 9 Apr 2024 23:31:38 -0400 Subject: [PATCH 05/10] Move condition based on PR feedback Co-authored-by: wxiaoguang --- modules/web/middleware/cookie.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index 5cf789a9c158..1aaae70b2e23 100644 --- a/modules/web/middleware/cookie.go +++ b/modules/web/middleware/cookie.go @@ -49,12 +49,15 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) { // The code was updated, but it behaves differently depending on the value // of AppSubURL. When AppSubURL is non-empty, the cookie with a trailing // slash must be deleted. - withoutTrailingSlash := strings.TrimSuffix(setting.SessionConfig.CookiePath, "/") - withTrailingSlash := withoutTrailingSlash + "/" - for _, path := range []string{withoutTrailingSlash, withTrailingSlash} { - if maxAge < 0 || setting.SessionConfig.CookiePath == path { + if maxAge < 0 { + withoutTrailingSlash := strings.TrimSuffix(setting.SessionConfig.CookiePath, "/") + withTrailingSlash := withoutTrailingSlash + "/" + for _, path := range []string{withoutTrailingSlash, withTrailingSlash} { cookie.Path = path resp.Header().Add("Set-Cookie", cookie.String()) } + } else { + cookie.Path = setting.SessionConfig.CookiePath + resp.Header().Add("Set-Cookie", cookie.String()) } } From 8bd0cb58b674e8e558a3dd9eede79e841210db08 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Tue, 9 Apr 2024 23:37:35 -0400 Subject: [PATCH 06/10] Move comment back to original location --- modules/web/middleware/cookie.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index 1aaae70b2e23..a3fd4c81efa7 100644 --- a/modules/web/middleware/cookie.go +++ b/modules/web/middleware/cookie.go @@ -44,12 +44,12 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) { HttpOnly: true, SameSite: setting.SessionConfig.SameSite, } - // 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="". - // The code was updated, but it behaves differently depending on the value - // of AppSubURL. When AppSubURL is non-empty, the cookie with a trailing - // slash must be deleted. 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="". + // The code was updated, but it behaves differently depending on the + // value of AppSubURL. When AppSubURL is non-empty, the cookie with a + // trailing slash must be deleted. withoutTrailingSlash := strings.TrimSuffix(setting.SessionConfig.CookiePath, "/") withTrailingSlash := withoutTrailingSlash + "/" for _, path := range []string{withoutTrailingSlash, withTrailingSlash} { From d2af373570d698bb7b44380ce7c4758f99a36d61 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Wed, 10 Apr 2024 13:23:24 -0400 Subject: [PATCH 07/10] fix: Fix to clear old cookies when session is regenerated The old cookies with trailing slash are more specific, so browsers prefer them over the new cookies. This change attempts to delete the old cookies whenever cookies are set. Adjust the middleware for cookies to do this automatically whenever SetSiteCookie is called. Expose a new function DeleteSiteCookieWithTrailingSlash that is used whenever we update the session cookie. --- modules/web/middleware/cookie.go | 42 +++++++++++++++++----------- routers/web/auth/auth.go | 4 +++ services/auth/auth.go | 3 ++ services/auth/source/oauth2/store.go | 3 ++ 4 files changed, 36 insertions(+), 16 deletions(-) diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index a3fd4c81efa7..62afd0a3a4b4 100644 --- a/modules/web/middleware/cookie.go +++ b/modules/web/middleware/cookie.go @@ -38,26 +38,36 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) { Name: name, Value: url.QueryEscape(value), MaxAge: maxAge, - Path: "", // Filled in below. + Path: setting.SessionConfig.CookiePath, Domain: setting.SessionConfig.Domain, Secure: setting.SessionConfig.Secure, HttpOnly: true, SameSite: setting.SessionConfig.SameSite, } - 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="". - // The code was updated, but it behaves differently depending on the - // value of AppSubURL. When AppSubURL is non-empty, the cookie with a - // trailing slash must be deleted. - withoutTrailingSlash := strings.TrimSuffix(setting.SessionConfig.CookiePath, "/") - withTrailingSlash := withoutTrailingSlash + "/" - for _, path := range []string{withoutTrailingSlash, withTrailingSlash} { - cookie.Path = path - resp.Header().Add("Set-Cookie", cookie.String()) - } - } else { - cookie.Path = setting.SessionConfig.CookiePath - resp.Header().Add("Set-Cookie", cookie.String()) + 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. + DeleteSiteCookieWithTrailingSlash(resp, name) +} + +// DeleteSiteCookieWithTrailingSlash will delete cookies that have a path with a trailing / +func DeleteSiteCookieWithTrailingSlash(resp http.ResponseWriter, name string) { + // If the cookie path is empty or ends with /, cookies with trailing / + // will not take precedence, so do nothing + if setting.SessionConfig.CookiePath == "" || strings.HasSuffix(setting.SessionConfig.CookiePath, "/") { + 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/routers/web/auth/auth.go b/routers/web/auth/auth.go index 8b5cd986b81a..283c98effe36 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -844,6 +844,10 @@ func ActivateEmail(ctx *context.Context) { } func updateSession(ctx *context.Context, deletes []string, updates map[string]any) error { + // Ensure that a cookie with a trail slash does not take + // precedence over the cookie written by the middleware + middleware.DeleteSiteCookieWithTrailingSlash(ctx.Resp, setting.SessionConfig.CookieName) + if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { return fmt.Errorf("regenerate session: %w", err) } diff --git a/services/auth/auth.go b/services/auth/auth.go index a2523a2452e9..b94d7dcd8c80 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -63,6 +63,9 @@ func isArchivePath(req *http.Request) bool { // handleSignIn clears existing session variables and stores new ones for the specified user object func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore, user *user_model.User) { + // Ensure that a cookie with a trail slash does not take + // precedence over the cookie written by the middleware + middleware.DeleteSiteCookieWithTrailingSlash(resp, setting.SessionConfig.CookieName) // We need to regenerate the session... newSess, err := session.RegenerateSession(resp, req) if err != nil { diff --git a/services/auth/source/oauth2/store.go b/services/auth/source/oauth2/store.go index 394bf9946359..67da28fdf8aa 100644 --- a/services/auth/source/oauth2/store.go +++ b/services/auth/source/oauth2/store.go @@ -9,6 +9,8 @@ import ( "net/http" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/web/middleware" chiSession "gitea.com/go-chi/session" "github.com/gorilla/sessions" @@ -65,6 +67,7 @@ func (st *SessionsStore) Save(r *http.Request, w http.ResponseWriter, session *s chiStore := chiSession.GetSession(r) if session.IsNew { + middleware.DeleteSiteCookieWithTrailingSlash(w, setting.SessionConfig.CookieName) _, _ = chiSession.RegenerateSession(w, r) session.IsNew = false } From 93d9a3c036df62393414cc5540d674285dca4572 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Thu, 11 Apr 2024 13:38:47 -0400 Subject: [PATCH 08/10] Fix to also delete cookies with no path set --- modules/web/middleware/cookie.go | 24 +++++++++++++++++------- routers/web/auth/auth.go | 2 +- services/auth/auth.go | 2 +- services/auth/source/oauth2/store.go | 2 +- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index 62afd0a3a4b4..7e63a9de7710 100644 --- a/modules/web/middleware/cookie.go +++ b/modules/web/middleware/cookie.go @@ -48,22 +48,32 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) { // 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. - DeleteSiteCookieWithTrailingSlash(resp, name) + DeleteLegacySiteCookie(resp, name) } -// DeleteSiteCookieWithTrailingSlash will delete cookies that have a path with a trailing / -func DeleteSiteCookieWithTrailingSlash(resp http.ResponseWriter, name string) { - // If the cookie path is empty or ends with /, cookies with trailing / - // will not take precedence, so do nothing - if setting.SessionConfig.CookiePath == "" || strings.HasSuffix(setting.SessionConfig.CookiePath, "/") { +// DeleteLegacySiteCookie deletes the cookie with the given name at the cookie +// path with a trailing / or no path at all, which would unintentionally +// override the cookie. +func DeleteLegacySiteCookie(resp http.ResponseWriter, name string) { + var path string + if setting.SessionConfig.CookiePath == "/" { + // If the cookie path is /, we need to delete the cookie with no path + // because that could take precedence. + path = "" + } else if strings.HasSuffix(setting.SessionConfig.CookiePath, "/") { + // If the cookie path is not / and ends with /, no legacy cookies will + // take precedence, so do nothing. return + } else { + // Delete the cookie with a trailing /. + path = setting.SessionConfig.CookiePath + "/" } cookie := &http.Cookie{ Name: name, Value: "", MaxAge: -1, - Path: setting.SessionConfig.CookiePath + "/", + Path: path, Domain: setting.SessionConfig.Domain, Secure: setting.SessionConfig.Secure, HttpOnly: true, diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index 283c98effe36..e913b95c8898 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -846,7 +846,7 @@ func ActivateEmail(ctx *context.Context) { func updateSession(ctx *context.Context, deletes []string, updates map[string]any) error { // Ensure that a cookie with a trail slash does not take // precedence over the cookie written by the middleware - middleware.DeleteSiteCookieWithTrailingSlash(ctx.Resp, setting.SessionConfig.CookieName) + middleware.DeleteLegacySiteCookie(ctx.Resp, setting.SessionConfig.CookieName) if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { return fmt.Errorf("regenerate session: %w", err) diff --git a/services/auth/auth.go b/services/auth/auth.go index b94d7dcd8c80..39934019e7c5 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -65,7 +65,7 @@ func isArchivePath(req *http.Request) bool { func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore, user *user_model.User) { // Ensure that a cookie with a trail slash does not take // precedence over the cookie written by the middleware - middleware.DeleteSiteCookieWithTrailingSlash(resp, setting.SessionConfig.CookieName) + middleware.DeleteLegacySiteCookie(resp, setting.SessionConfig.CookieName) // We need to regenerate the session... newSess, err := session.RegenerateSession(resp, req) if err != nil { diff --git a/services/auth/source/oauth2/store.go b/services/auth/source/oauth2/store.go index 67da28fdf8aa..09a74ebb6f1c 100644 --- a/services/auth/source/oauth2/store.go +++ b/services/auth/source/oauth2/store.go @@ -67,7 +67,7 @@ func (st *SessionsStore) Save(r *http.Request, w http.ResponseWriter, session *s chiStore := chiSession.GetSession(r) if session.IsNew { - middleware.DeleteSiteCookieWithTrailingSlash(w, setting.SessionConfig.CookieName) + middleware.DeleteLegacySiteCookie(w, setting.SessionConfig.CookieName) _, _ = chiSession.RegenerateSession(w, r) session.IsNew = false } From 1b83a2d77ad7cab3be7258ee92888f414f1fa5b9 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Thu, 11 Apr 2024 13:59:39 -0400 Subject: [PATCH 09/10] Push legacy cookie deletion call into session module --- modules/session/store.go | 7 +++++++ routers/web/auth/auth.go | 4 ---- services/auth/auth.go | 3 --- services/auth/source/oauth2/store.go | 6 ++---- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/modules/session/store.go b/modules/session/store.go index 4fa4d2848f1b..2f7ab7760b59 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/routers/web/auth/auth.go b/routers/web/auth/auth.go index e913b95c8898..8b5cd986b81a 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -844,10 +844,6 @@ func ActivateEmail(ctx *context.Context) { } func updateSession(ctx *context.Context, deletes []string, updates map[string]any) error { - // Ensure that a cookie with a trail slash does not take - // precedence over the cookie written by the middleware - middleware.DeleteLegacySiteCookie(ctx.Resp, setting.SessionConfig.CookieName) - if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil { return fmt.Errorf("regenerate session: %w", err) } diff --git a/services/auth/auth.go b/services/auth/auth.go index 39934019e7c5..a2523a2452e9 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -63,9 +63,6 @@ func isArchivePath(req *http.Request) bool { // handleSignIn clears existing session variables and stores new ones for the specified user object func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore, user *user_model.User) { - // Ensure that a cookie with a trail slash does not take - // precedence over the cookie written by the middleware - middleware.DeleteLegacySiteCookie(resp, setting.SessionConfig.CookieName) // We need to regenerate the session... newSess, err := session.RegenerateSession(resp, req) if err != nil { diff --git a/services/auth/source/oauth2/store.go b/services/auth/source/oauth2/store.go index 09a74ebb6f1c..90fa965602a7 100644 --- a/services/auth/source/oauth2/store.go +++ b/services/auth/source/oauth2/store.go @@ -9,8 +9,7 @@ import ( "net/http" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/web/middleware" + session_module "code.gitea.io/gitea/modules/session" chiSession "gitea.com/go-chi/session" "github.com/gorilla/sessions" @@ -67,8 +66,7 @@ func (st *SessionsStore) Save(r *http.Request, w http.ResponseWriter, session *s chiStore := chiSession.GetSession(r) if session.IsNew { - middleware.DeleteLegacySiteCookie(w, setting.SessionConfig.CookieName) - _, _ = chiSession.RegenerateSession(w, r) + _, _ = session_module.RegenerateSession(w, r) session.IsNew = false } From 926dcb13491f04d31e898c6f493ced0db2ef50a6 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Fri, 12 Apr 2024 12:12:45 -0400 Subject: [PATCH 10/10] Revert to not do anything when there's an empty cookie path --- modules/web/middleware/cookie.go | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/modules/web/middleware/cookie.go b/modules/web/middleware/cookie.go index 7e63a9de7710..0bed7267930c 100644 --- a/modules/web/middleware/cookie.go +++ b/modules/web/middleware/cookie.go @@ -52,28 +52,21 @@ func SetSiteCookie(resp http.ResponseWriter, name, value string, maxAge int) { } // DeleteLegacySiteCookie deletes the cookie with the given name at the cookie -// path with a trailing / or no path at all, which would unintentionally -// override the cookie. +// path with a trailing /, which would unintentionally override the cookie. func DeleteLegacySiteCookie(resp http.ResponseWriter, name string) { - var path string - if setting.SessionConfig.CookiePath == "/" { - // If the cookie path is /, we need to delete the cookie with no path - // because that could take precedence. - path = "" - } else if strings.HasSuffix(setting.SessionConfig.CookiePath, "/") { - // If the cookie path is not / and ends with /, no legacy cookies will - // take precedence, so do nothing. + 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 - } else { - // Delete the cookie with a trailing /. - path = setting.SessionConfig.CookiePath + "/" } cookie := &http.Cookie{ Name: name, Value: "", MaxAge: -1, - Path: path, + Path: setting.SessionConfig.CookiePath + "/", Domain: setting.SessionConfig.Domain, Secure: setting.SessionConfig.Secure, HttpOnly: true,