Refactor cookie (go-gitea#24107)

Close go-gitea#24062

At the beginning, I just wanted to fix the warning mentioned by go-gitea#24062

But, the cookie code really doesn't look good to me, so clean up them.

Complete the TODO on `SetCookie`: 

> TODO: Copied from gitea.com/macaron/macaron and should be improved
after macaron removed.

Author: wxiaoguang

modules/context/auth.go

@@ -67,7 +67,7 @@ func Toggle(options *ToggleOptions) func(ctx *Context) {
 		}
 
 		if !options.SignOutRequired && !options.DisableCSRF && ctx.Req.Method == "POST" {
-			ctx.csrf.Validate(ctx)
+			ctx.Csrf.Validate(ctx)
 			if ctx.Written() {
 				return
 			}
@@ -89,7 +89,7 @@ func Toggle(options *ToggleOptions) func(ctx *Context) {
 
 		// Redirect to log in page if auto-signin info is provided and has not signed in.
 		if !options.SignOutRequired && !ctx.IsSigned &&
-			len(ctx.GetCookie(setting.CookieUserName)) > 0 {
+			len(ctx.GetSiteCookie(setting.CookieUserName)) > 0 {
 			if ctx.Req.URL.Path != "/user/events" {
 				middleware.SetRedirectToCookie(ctx.Resp, setting.AppSubURL+ctx.Req.URL.RequestURI())
 			}

modules/context/context.go

@@ -45,6 +45,8 @@ import (
 	"golang.org/x/crypto/pbkdf2"
 )
 
+const CookieNameFlash = "gitea_flash"
+
 // Render represents a template render
 type Render interface {
 	TemplateLookup(tmpl string) (*template.Template, error)
@@ -60,7 +62,7 @@ type Context struct {
 	Render   Render
 	translation.Locale
 	Cache   cache.Cache
-	csrf    CSRFProtector
+	Csrf    CSRFProtector
 	Flash   *middleware.Flash
 	Session session.Store
 
@@ -478,38 +480,26 @@ func (ctx *Context) Redirect(location string, status ...int) {
 	http.Redirect(ctx.Resp, ctx.Req, location, code)
 }
 
-// SetCookie convenience function to set most cookies consistently
+// SetSiteCookie convenience function to set most cookies consistently
 // CSRF and a few others are the exception here
-func (ctx *Context) SetCookie(name, value string, expiry int) {
-	middleware.SetCookie(ctx.Resp, name, value,
-		expiry,
-		setting.AppSubURL,
-		setting.SessionConfig.Domain,
-		setting.SessionConfig.Secure,
-		true,
-		middleware.SameSite(setting.SessionConfig.SameSite))
+func (ctx *Context) SetSiteCookie(name, value string, maxAge int) {
+	middleware.SetSiteCookie(ctx.Resp, name, value, maxAge)
 }
 
-// DeleteCookie convenience function to delete most cookies consistently
+// DeleteSiteCookie convenience function to delete most cookies consistently
 // CSRF and a few others are the exception here
-func (ctx *Context) DeleteCookie(name string) {
-	middleware.SetCookie(ctx.Resp, name, "",
-		-1,
-		setting.AppSubURL,
-		setting.SessionConfig.Domain,
-		setting.SessionConfig.Secure,
-		true,
-		middleware.SameSite(setting.SessionConfig.SameSite))
+func (ctx *Context) DeleteSiteCookie(name string) {
+	middleware.SetSiteCookie(ctx.Resp, name, "", -1)
 }
 
-// GetCookie returns given cookie value from request header.
-func (ctx *Context) GetCookie(name string) string {
-	return middleware.GetCookie(ctx.Req, name)
+// GetSiteCookie returns given cookie value from request header.
+func (ctx *Context) GetSiteCookie(name string) string {
+	return middleware.GetSiteCookie(ctx.Req, name)
 }
 
 // GetSuperSecureCookie returns given cookie value from request header with secret string.
 func (ctx *Context) GetSuperSecureCookie(secret, name string) (string, bool) {
-	val := ctx.GetCookie(name)
+	val := ctx.GetSiteCookie(name)
 	return ctx.CookieDecrypt(secret, val)
 }
 
@@ -530,10 +520,9 @@ func (ctx *Context) CookieDecrypt(secret, val string) (string, bool) {
 }
 
 // SetSuperSecureCookie sets given cookie value to response header with secret string.
-func (ctx *Context) SetSuperSecureCookie(secret, name, value string, expiry int) {
+func (ctx *Context) SetSuperSecureCookie(secret, name, value string, maxAge int) {
 	text := ctx.CookieEncrypt(secret, value)
-
-	ctx.SetCookie(name, text, expiry)
+	ctx.SetSiteCookie(name, text, maxAge)
 }
 
 // CookieEncrypt encrypts a given value using the provided secret
@@ -549,19 +538,19 @@ func (ctx *Context) CookieEncrypt(secret, value string) string {
 
 // GetCookieInt returns cookie result in int type.
 func (ctx *Context) GetCookieInt(name string) int {
-	r, _ := strconv.Atoi(ctx.GetCookie(name))
+	r, _ := strconv.Atoi(ctx.GetSiteCookie(name))
 	return r
 }
 
 // GetCookieInt64 returns cookie result in int64 type.
 func (ctx *Context) GetCookieInt64(name string) int64 {
-	r, _ := strconv.ParseInt(ctx.GetCookie(name), 10, 64)
+	r, _ := strconv.ParseInt(ctx.GetSiteCookie(name), 10, 64)
 	return r
 }
 
 // GetCookieFloat64 returns cookie result in float64 type.
 func (ctx *Context) GetCookieFloat64(name string) float64 {
-	v, _ := strconv.ParseFloat(ctx.GetCookie(name), 64)
+	v, _ := strconv.ParseFloat(ctx.GetSiteCookie(name), 64)
 	return v
 }
 
@@ -659,7 +648,10 @@ func WithContext(req *http.Request, ctx *Context) *http.Request {
 
 // GetContext retrieves install context from request
 func GetContext(req *http.Request) *Context {
-	return req.Context().Value(contextKey).(*Context)
+	if ctx, ok := req.Context().Value(contextKey).(*Context); ok {
+		return ctx
+	}
+	return nil
 }
 
 // GetContextUser returns context user
@@ -726,54 +718,32 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler {
 			ctx.Data["Context"] = &ctx
 
 			ctx.Req = WithContext(req, &ctx)
-			ctx.csrf = PrepareCSRFProtector(csrfOpts, &ctx)
+			ctx.Csrf = PrepareCSRFProtector(csrfOpts, &ctx)
 
-			// Get flash.
-			flashCookie := ctx.GetCookie("macaron_flash")
-			vals, _ := url.ParseQuery(flashCookie)
-			if len(vals) > 0 {
-				f := &middleware.Flash{
+			// Get the last flash message from cookie
+			lastFlashCookie := middleware.GetSiteCookie(ctx.Req, CookieNameFlash)
+			if vals, _ := url.ParseQuery(lastFlashCookie); len(vals) > 0 {
+				// store last Flash message into the template data, to render it
+				ctx.Data["Flash"] = &middleware.Flash{
 					DataStore:  &ctx,
 					Values:     vals,
 					ErrorMsg:   vals.Get("error"),
 					SuccessMsg: vals.Get("success"),
 					InfoMsg:    vals.Get("info"),
 					WarningMsg: vals.Get("warning"),
 				}
-				ctx.Data["Flash"] = f
 			}
 
-			f := &middleware.Flash{
-				DataStore:  &ctx,
-				Values:     url.Values{},
-				ErrorMsg:   "",
-				WarningMsg: "",
-				InfoMsg:    "",
-				SuccessMsg: "",
-			}
+			// prepare an empty Flash message for current request
+			ctx.Flash = &middleware.Flash{DataStore: &ctx, Values: url.Values{}}
 			ctx.Resp.Before(func(resp ResponseWriter) {
-				if flash := f.Encode(); len(flash) > 0 {
-					middleware.SetCookie(resp, "macaron_flash", flash, 0,
-						setting.SessionConfig.CookiePath,
-						middleware.Domain(setting.SessionConfig.Domain),
-						middleware.HTTPOnly(true),
-						middleware.Secure(setting.SessionConfig.Secure),
-						middleware.SameSite(setting.SessionConfig.SameSite),
-					)
-					return
+				if val := ctx.Flash.Encode(); val != "" {
+					middleware.SetSiteCookie(ctx.Resp, CookieNameFlash, val, 0)
+				} else if lastFlashCookie != "" {
+					middleware.SetSiteCookie(ctx.Resp, CookieNameFlash, "", -1)
 				}
-
-				middleware.SetCookie(ctx.Resp, "macaron_flash", "", -1,
-					setting.SessionConfig.CookiePath,
-					middleware.Domain(setting.SessionConfig.Domain),
-					middleware.HTTPOnly(true),
-					middleware.Secure(setting.SessionConfig.Secure),
-					middleware.SameSite(setting.SessionConfig.SameSite),
-				)
 			})
 
-			ctx.Flash = f
-
 			// If request sends files, parse them here otherwise the Query() can't be parsed and the CsrfToken will be invalid.
 			if ctx.Req.Method == "POST" && strings.Contains(ctx.Req.Header.Get("Content-Type"), "multipart/form-data") {
 				if err := ctx.Req.ParseMultipartForm(setting.Attachment.MaxSize << 20); err != nil && !strings.Contains(err.Error(), "EOF") { // 32MB max size
@@ -785,7 +755,7 @@ func Contexter(ctx context.Context) func(next http.Handler) http.Handler {
 			httpcache.SetCacheControlInHeader(ctx.Resp.Header(), 0, "no-transform")
 			ctx.Resp.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions)
 
-			ctx.Data["CsrfToken"] = ctx.csrf.GetToken()
+			ctx.Data["CsrfToken"] = ctx.Csrf.GetToken()
 			ctx.Data["CsrfTokenHtml"] = template.HTML(`<input type="hidden" name="_csrf" value="` + ctx.Data["CsrfToken"].(string) + `">`)
 
 			// FIXME: do we really always need these setting? There should be someway to have to avoid having to always set these

modules/context/csrf.go

@@ -42,37 +42,26 @@ type CSRFProtector interface {
 	GetToken() string
 	// Validate validates the token in http context.
 	Validate(ctx *Context)
+	// DeleteCookie deletes the cookie
+	DeleteCookie(ctx *Context)
 }
 
 type csrfProtector struct {
-	// Header name value for setting and getting csrf token.
-	Header string
-	// Form name value for setting and getting csrf token.
-	Form string
-	// Cookie name value for setting and getting csrf token.
-	Cookie string
-	// Cookie domain
-	CookieDomain string
-	// Cookie path
-	CookiePath string
-	// Cookie HttpOnly flag value used for the csrf token.
-	CookieHTTPOnly bool
+	opt CsrfOptions
 	// Token generated to pass via header, cookie, or hidden form value.
 	Token string
 	// This value must be unique per user.
 	ID string
-	// Secret used along with the unique id above to generate the Token.
-	Secret string
 }
 
 // GetHeaderName returns the name of the HTTP header for csrf token.
 func (c *csrfProtector) GetHeaderName() string {
-	return c.Header
+	return c.opt.Header
 }
 
 // GetFormName returns the name of the form value for csrf token.
 func (c *csrfProtector) GetFormName() string {
-	return c.Form
+	return c.opt.Form
 }
 
 // GetToken returns the current token. This is typically used
@@ -138,23 +127,32 @@ func prepareDefaultCsrfOptions(opt CsrfOptions) CsrfOptions {
 	if opt.SessionKey == "" {
 		opt.SessionKey = "uid"
 	}
+	if opt.CookieLifeTime == 0 {
+		opt.CookieLifeTime = int(CsrfTokenTimeout.Seconds())
+	}
+
 	opt.oldSessionKey = "_old_" + opt.SessionKey
 	return opt
 }
 
+func newCsrfCookie(c *csrfProtector, value string) *http.Cookie {
+	return &http.Cookie{
+		Name:     c.opt.Cookie,
+		Value:    value,
+		Path:     c.opt.CookiePath,
+		Domain:   c.opt.CookieDomain,
+		MaxAge:   c.opt.CookieLifeTime,
+		Secure:   c.opt.Secure,
+		HttpOnly: c.opt.CookieHTTPOnly,
+		SameSite: c.opt.SameSite,
+	}
+}
+
 // PrepareCSRFProtector returns a CSRFProtector to be used for every request.
 // Additionally, depending on options set, generated tokens will be sent via Header and/or Cookie.
 func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector {
 	opt = prepareDefaultCsrfOptions(opt)
-	x := &csrfProtector{
-		Secret:         opt.Secret,
-		Header:         opt.Header,
-		Form:           opt.Form,
-		Cookie:         opt.Cookie,
-		CookieDomain:   opt.CookieDomain,
-		CookiePath:     opt.CookiePath,
-		CookieHTTPOnly: opt.CookieHTTPOnly,
-	}
+	x := &csrfProtector{opt: opt}
 
 	if opt.Origin && len(ctx.Req.Header.Get("Origin")) > 0 {
 		return x
@@ -175,7 +173,7 @@ func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector {
 
 	oldUID := ctx.Session.Get(opt.oldSessionKey)
 	uidChanged := oldUID == nil || oldUID.(string) != x.ID
-	cookieToken := ctx.GetCookie(opt.Cookie)
+	cookieToken := ctx.GetSiteCookie(opt.Cookie)
 
 	needsNew := true
 	if uidChanged {
@@ -193,21 +191,10 @@ func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector {
 
 	if needsNew {
 		// FIXME: actionId.
-		x.Token = GenerateCsrfToken(x.Secret, x.ID, "POST", time.Now())
+		x.Token = GenerateCsrfToken(x.opt.Secret, x.ID, "POST", time.Now())
 		if opt.SetCookie {
-			var expires interface{}
-			if opt.CookieLifeTime == 0 {
-				expires = time.Now().Add(CsrfTokenTimeout)
-			}
-			middleware.SetCookie(ctx.Resp, opt.Cookie, x.Token,
-				opt.CookieLifeTime,
-				opt.CookiePath,
-				opt.CookieDomain,
-				opt.Secure,
-				opt.CookieHTTPOnly,
-				expires,
-				middleware.SameSite(opt.SameSite),
-			)
+			cookie := newCsrfCookie(x, x.Token)
+			ctx.Resp.Header().Add("Set-Cookie", cookie.String())
 		}
 	}
 
@@ -218,8 +205,8 @@ func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector {
 }
 
 func (c *csrfProtector) validateToken(ctx *Context, token string) {
-	if !ValidCsrfToken(token, c.Secret, c.ID, "POST", time.Now()) {
-		middleware.DeleteCSRFCookie(ctx.Resp)
+	if !ValidCsrfToken(token, c.opt.Secret, c.ID, "POST", time.Now()) {
+		c.DeleteCookie(ctx)
 		if middleware.IsAPIPath(ctx.Req) {
 			// currently, there should be no access to the APIPath with CSRF token. because templates shouldn't use the `/api/` endpoints.
 			http.Error(ctx.Resp, "Invalid CSRF token.", http.StatusBadRequest)
@@ -245,3 +232,11 @@ func (c *csrfProtector) Validate(ctx *Context) {
 	}
 	c.validateToken(ctx, "") // no csrf token, use an empty token to respond error
 }
+
+func (c *csrfProtector) DeleteCookie(ctx *Context) {
+	if c.opt.SetCookie {
+		cookie := newCsrfCookie(c, "")
+		cookie.MaxAge = -1
+		ctx.Resp.Header().Add("Set-Cookie", cookie.String())
+	}
+}

modules/setting/session.go

@@ -21,7 +21,7 @@ var SessionConfig = struct {
 	ProviderConfig string
 	// Cookie name to save session ID. Default is "MacaronSession".
 	CookieName string
-	// Cookie path to store. Default is "/".
+	// Cookie path to store. Default is "/". HINT: there was a bug, the old value doesn't have trailing slash, and could be empty "".
 	CookiePath string
 	// GC interval time in seconds. Default is 3600.
 	Gclifetime int64
@@ -49,7 +49,7 @@ func loadSessionFrom(rootCfg ConfigProvider) {
 		SessionConfig.ProviderConfig = path.Join(AppWorkPath, SessionConfig.ProviderConfig)
 	}
 	SessionConfig.CookieName = sec.Key("COOKIE_NAME").MustString("i_like_gitea")
-	SessionConfig.CookiePath = AppSubURL
+	SessionConfig.CookiePath = AppSubURL + "/" // there was a bug, old code only set CookePath=AppSubURL, no trailing slash
 	SessionConfig.Secure = sec.Key("COOKIE_SECURE").MustBool(false)
 	SessionConfig.Gclifetime = sec.Key("GC_INTERVAL_TIME").MustInt64(86400)
 	SessionConfig.Maxlifetime = sec.Key("SESSION_LIFE_TIME").MustInt64(86400)