Refactor CSRF protector (#32057)

Remove unused CSRF options, decouple "new csrf protector" and "prepare"
logic, do not redirect to home page if CSRF validation falis (it
shouldn't happen in daily usage, if it happens, redirecting to home
doesn't help either but just makes the problem more complex for "fetch")

(cherry picked from commit 1fede04b83288d8a91304a83b7601699bb5cba04)

Conflicts:
	options/locale/locale_en-US.ini
	tests/integration/repo_branch_test.go
  trivial context conflicts

options/locale/locale_en-US.ini

@@ -231,7 +231,6 @@ string.desc = Z - A
 [error]
 occurred = An error occurred
 report_message = If you believe that this is a Forgejo bug, please search for issues on <a href="%s" target="_blank">Codeberg</a> or open a new issue if necessary.
-invalid_csrf = Bad Request: invalid CSRF token
 not_found = The target couldn't be found.
 network_error = Network error
 server_internal = Internal server error

routers/web/web.go

@@ -132,6 +132,8 @@ func webAuth(authMethod auth_service.Method) func(*context.Context) {
 			// ensure the session uid is deleted
 			_ = ctx.Session.Delete("uid")
 		}
+
+		ctx.Csrf.PrepareForSessionUser(ctx)
 	}
 }
 

services/context/context.go

@@ -127,10 +127,8 @@ func Contexter() func(next http.Handler) http.Handler {
 	csrfOpts := CsrfOptions{
 		Secret:         hex.EncodeToString(setting.GetGeneralTokenSigningSecret()),
 		Cookie:         setting.CSRFCookieName,
-		SetCookie:      true,
 		Secure:         setting.SessionConfig.Secure,
 		CookieHTTPOnly: setting.CSRFCookieHTTPOnly,
-		Header:         "X-Csrf-Token",
 		CookieDomain:   setting.SessionConfig.Domain,
 		CookiePath:     setting.SessionConfig.CookiePath,
 		SameSite:       setting.SessionConfig.SameSite,
@@ -156,7 +154,7 @@ func Contexter() func(next http.Handler) http.Handler {
 			ctx.Base.AppendContextValue(WebContextKey, ctx)
 			ctx.Base.AppendContextValueFunc(gitrepo.RepositoryContextKey, func() any { return ctx.Repo.GitRepo })
 
-			ctx.Csrf = PrepareCSRFProtector(csrfOpts, ctx)
+			ctx.Csrf = NewCSRFProtector(csrfOpts)
 
 			// Get the last flash message from cookie
 			lastFlashCookie := middleware.GetSiteCookie(ctx.Req, CookieNameFlash)
@@ -193,8 +191,6 @@ func Contexter() func(next http.Handler) http.Handler {
 			ctx.Resp.Header().Set(`X-Frame-Options`, setting.CORSConfig.XFrameOptions)
 
 			ctx.Data["SystemConfig"] = setting.Config()
-			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
 			ctx.Data["DisableMigrations"] = setting.Repository.DisableMigrations

services/context/csrf.go

@@ -20,64 +20,42 @@
 package context
 
 import (
-	"encoding/base32"
+	"html/template"
-	"fmt"
 	"net/http"
 	"strconv"
 	"time"
 
 	"code.gitea.io/gitea/modules/log"
-	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/util"
-	"code.gitea.io/gitea/modules/web/middleware"
+)
+
+const (
+	CsrfHeaderName = "X-Csrf-Token"
+	CsrfFormName   = "_csrf"
 )
 
 // CSRFProtector represents a CSRF protector and is used to get the current token and validate the token.
 type CSRFProtector interface {
-	// GetHeaderName returns HTTP header to search for token.
+	// PrepareForSessionUser prepares the csrf protector for the current session user.
-	GetHeaderName() string
+	PrepareForSessionUser(ctx *Context)
-	// GetFormName returns form value to search for token.
+	// Validate validates the csrf token in http context.
-	GetFormName() string
-	// GetToken returns the token.
-	GetToken() string
-	// Validate validates the token in http context.
 	Validate(ctx *Context)
-	// DeleteCookie deletes the cookie
+	// DeleteCookie deletes the csrf cookie
 	DeleteCookie(ctx *Context)
 }
 
 type csrfProtector struct {
 	opt CsrfOptions
-	// Token generated to pass via header, cookie, or hidden form value.
+	// id must be unique per user.
-	Token string
+	id string
-	// This value must be unique per user.
+	// token is the valid one which wil be used by end user and passed via header, cookie, or hidden form value.
-	ID string
+	token string
-}
-
-// GetHeaderName returns the name of the HTTP header for csrf token.
-func (c *csrfProtector) GetHeaderName() string {
-	return c.opt.Header
-}
-
-// GetFormName returns the name of the form value for csrf token.
-func (c *csrfProtector) GetFormName() string {
-	return c.opt.Form
-}
-
-// GetToken returns the current token. This is typically used
-// to populate a hidden form in an HTML template.
-func (c *csrfProtector) GetToken() string {
-	return c.Token
 }
 
 // CsrfOptions maintains options to manage behavior of Generate.
 type CsrfOptions struct {
 	// The global secret value used to generate Tokens.
 	Secret string
-	// HTTP header used to set and get token.
-	Header string
-	// Form value used to set and get token.
-	Form string
 	// Cookie value used to set and get token.
 	Cookie string
 	// Cookie domain.
@@ -87,103 +65,64 @@ type CsrfOptions struct {
 	CookieHTTPOnly bool
 	// SameSite set the cookie SameSite type
 	SameSite http.SameSite
-	// Key used for getting the unique ID per user.
-	SessionKey string
-	// oldSessionKey saves old value corresponding to SessionKey.
-	oldSessionKey string
-	// If true, send token via X-Csrf-Token header.
-	SetHeader bool
-	// If true, send token via _csrf cookie.
-	SetCookie bool
 	// Set the Secure flag to true on the cookie.
 	Secure bool
-	// Disallow Origin appear in request header.
+	// sessionKey is the key used for getting the unique ID per user.
-	Origin bool
+	sessionKey string
-	// Cookie lifetime. Default is 0
+	// oldSessionKey saves old value corresponding to sessionKey.
-	CookieLifeTime int
+	oldSessionKey string
 }
 
-func prepareDefaultCsrfOptions(opt CsrfOptions) CsrfOptions {
+func newCsrfCookie(opt *CsrfOptions, value string) *http.Cookie {
-	if opt.Secret == "" {
-		randBytes, err := util.CryptoRandomBytes(8)
-		if err != nil {
-			// this panic can be handled by the recover() in http handlers
-			panic(fmt.Errorf("failed to generate random bytes: %w", err))
-		}
-		opt.Secret = base32.StdEncoding.EncodeToString(randBytes)
-	}
-	if opt.Header == "" {
-		opt.Header = "X-Csrf-Token"
-	}
-	if opt.Form == "" {
-		opt.Form = "_csrf"
-	}
-	if opt.Cookie == "" {
-		opt.Cookie = "_csrf"
-	}
-	if opt.CookiePath == "" {
-		opt.CookiePath = "/"
-	}
-	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,
+		Name:     opt.Cookie,
 		Value:    value,
-		Path:     c.opt.CookiePath,
+		Path:     opt.CookiePath,
-		Domain:   c.opt.CookieDomain,
+		Domain:   opt.CookieDomain,
-		MaxAge:   c.opt.CookieLifeTime,
+		MaxAge:   int(CsrfTokenTimeout.Seconds()),
-		Secure:   c.opt.Secure,
+		Secure:   opt.Secure,
-		HttpOnly: c.opt.CookieHTTPOnly,
+		HttpOnly: opt.CookieHTTPOnly,
-		SameSite: c.opt.SameSite,
+		SameSite: opt.SameSite,
 	}
 }
 
-// PrepareCSRFProtector returns a CSRFProtector to be used for every request.
+func NewCSRFProtector(opt CsrfOptions) CSRFProtector {
-// Additionally, depending on options set, generated tokens will be sent via Header and/or Cookie.
+	if opt.Secret == "" {
-func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector {
+		panic("CSRF secret is empty but it must be set") // it shouldn't happen because it is always set in code
-	opt = prepareDefaultCsrfOptions(opt)
-	x := &csrfProtector{opt: opt}
-
-	if opt.Origin && len(ctx.Req.Header.Get("Origin")) > 0 {
-		return x
 	}
+	opt.Cookie = util.IfZero(opt.Cookie, "_csrf")
+	opt.CookiePath = util.IfZero(opt.CookiePath, "/")
+	opt.sessionKey = "uid"
+	opt.oldSessionKey = "_old_" + opt.sessionKey
+	return &csrfProtector{opt: opt}
+}
 
-	x.ID = "0"
+func (c *csrfProtector) PrepareForSessionUser(ctx *Context) {
-	uidAny := ctx.Session.Get(opt.SessionKey)
+	c.id = "0"
-	if uidAny != nil {
+	if uidAny := ctx.Session.Get(c.opt.sessionKey); uidAny != nil {
 		switch uidVal := uidAny.(type) {
 		case string:
-			x.ID = uidVal
+			c.id = uidVal
 		case int64:
-			x.ID = strconv.FormatInt(uidVal, 10)
+			c.id = strconv.FormatInt(uidVal, 10)
 		default:
 			log.Error("invalid uid type in session: %T", uidAny)
 		}
 	}
 
-	oldUID := ctx.Session.Get(opt.oldSessionKey)
+	oldUID := ctx.Session.Get(c.opt.oldSessionKey)
-	uidChanged := oldUID == nil || oldUID.(string) != x.ID
+	uidChanged := oldUID == nil || oldUID.(string) != c.id
-	cookieToken := ctx.GetSiteCookie(opt.Cookie)
+	cookieToken := ctx.GetSiteCookie(c.opt.Cookie)
 
 	needsNew := true
 	if uidChanged {
-		_ = ctx.Session.Set(opt.oldSessionKey, x.ID)
+		_ = ctx.Session.Set(c.opt.oldSessionKey, c.id)
 	} else if cookieToken != "" {
 		// If cookie token presents, reuse existing unexpired token, else generate a new one.
 		if issueTime, ok := ParseCsrfToken(cookieToken); ok {
 			dur := time.Since(issueTime) // issueTime is not a monotonic-clock, the server time may change a lot to an early time.
 			if dur >= -CsrfTokenRegenerationInterval && dur <= CsrfTokenRegenerationInterval {
-				x.Token = cookieToken
+				c.token = cookieToken
 				needsNew = false
 			}
 		}
@@ -191,42 +130,33 @@ func PrepareCSRFProtector(opt CsrfOptions, ctx *Context) CSRFProtector {
 
 	if needsNew {
 		// FIXME: actionId.
-		x.Token = GenerateCsrfToken(x.opt.Secret, x.ID, "POST", time.Now())
+		c.token = GenerateCsrfToken(c.opt.Secret, c.id, "POST", time.Now())
-		if opt.SetCookie {
+		cookie := newCsrfCookie(&c.opt, c.token)
-			cookie := newCsrfCookie(x, x.Token)
+		ctx.Resp.Header().Add("Set-Cookie", cookie.String())
-			ctx.Resp.Header().Add("Set-Cookie", cookie.String())
-		}
 	}
 
-	if opt.SetHeader {
+	ctx.Data["CsrfToken"] = c.token
-		ctx.Resp.Header().Add(opt.Header, x.Token)
+	ctx.Data["CsrfTokenHtml"] = template.HTML(`<input type="hidden" name="_csrf" value="` + template.HTMLEscapeString(c.token) + `">`)
-	}
-	return x
 }
 
 func (c *csrfProtector) validateToken(ctx *Context, token string) {
-	if !ValidCsrfToken(token, c.opt.Secret, c.ID, "POST", time.Now()) {
+	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.
-			// currently, there should be no access to the APIPath with CSRF token. because templates shouldn't use the `/api/` endpoints.
+		// FIXME: distinguish what the response is for: HTML (web page) or JSON (fetch)
-			http.Error(ctx.Resp, "Invalid CSRF token.", http.StatusBadRequest)
+		http.Error(ctx.Resp, "Invalid CSRF token.", http.StatusBadRequest)
-		} else {
-			ctx.Flash.Error(ctx.Tr("error.invalid_csrf"))
-			ctx.Redirect(setting.AppSubURL + "/")
-		}
 	}
 }
 
 // Validate should be used as a per route middleware. It attempts to get a token from an "X-Csrf-Token"
 // HTTP header and then a "_csrf" form value. If one of these is found, the token will be validated.
-// If this validation fails, custom Error is sent in the reply.
+// If this validation fails, http.StatusBadRequest is sent.
-// If neither a header nor form value is found, http.StatusBadRequest is sent.
 func (c *csrfProtector) Validate(ctx *Context) {
-	if token := ctx.Req.Header.Get(c.GetHeaderName()); token != "" {
+	if token := ctx.Req.Header.Get(CsrfHeaderName); token != "" {
 		c.validateToken(ctx, token)
 		return
 	}
-	if token := ctx.Req.FormValue(c.GetFormName()); token != "" {
+	if token := ctx.Req.FormValue(CsrfFormName); token != "" {
 		c.validateToken(ctx, token)
 		return
 	}
@@ -234,9 +164,7 @@ func (c *csrfProtector) Validate(ctx *Context) {
 }
 
 func (c *csrfProtector) DeleteCookie(ctx *Context) {
-	if c.opt.SetCookie {
+	cookie := newCsrfCookie(&c.opt, "")
-		cookie := newCsrfCookie(c, "")
+	cookie.MaxAge = -1
-		cookie.MaxAge = -1
+	ctx.Resp.Header().Add("Set-Cookie", cookie.String())
-		ctx.Resp.Header().Add("Set-Cookie", cookie.String())
-	}
 }

tests/integration/attachment_test.go

@@ -60,7 +60,8 @@ func createAttachment(t *testing.T, session *TestSession, repoURL, filename stri
 func TestCreateAnonymousAttachment(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	session := emptyTestSession(t)
-	createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusSeeOther)
+	// this test is not right because it just doesn't pass the CSRF validation
+	createAttachment(t, session, "user2/repo1", "image.png", generateImg(), http.StatusBadRequest)
 }
 
 func TestCreateIssueAttachment(t *testing.T) {

tests/integration/csrf_test.go

@@ -5,12 +5,10 @@ package integration
 
 import (
 	"net/http"
-	"strings"
 	"testing"
 
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
-	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
@@ -25,28 +23,12 @@ func TestCsrfProtection(t *testing.T) {
 	req := NewRequestWithValues(t, "POST", "/user/settings", map[string]string{
 		"_csrf": "fake_csrf",
 	})
-	session.MakeRequest(t, req, http.StatusSeeOther)
+	resp := session.MakeRequest(t, req, http.StatusBadRequest)
-
+	assert.Contains(t, resp.Body.String(), "Invalid CSRF token")
-	resp := session.MakeRequest(t, req, http.StatusSeeOther)
-	loc := resp.Header().Get("Location")
-	assert.Equal(t, setting.AppSubURL+"/", loc)
-	resp = session.MakeRequest(t, NewRequest(t, "GET", loc), http.StatusOK)
-	htmlDoc := NewHTMLParser(t, resp.Body)
-	assert.Equal(t, "Bad Request: invalid CSRF token",
-		strings.TrimSpace(htmlDoc.doc.Find(".ui.message").Text()),
-	)
 
 	// test web form csrf via header. TODO: should use an UI api to test
 	req = NewRequest(t, "POST", "/user/settings")
 	req.Header.Add("X-Csrf-Token", "fake_csrf")
-	session.MakeRequest(t, req, http.StatusSeeOther)
+	resp = session.MakeRequest(t, req, http.StatusBadRequest)
-
+	assert.Contains(t, resp.Body.String(), "Invalid CSRF token")
-	resp = session.MakeRequest(t, req, http.StatusSeeOther)
-	loc = resp.Header().Get("Location")
-	assert.Equal(t, setting.AppSubURL+"/", loc)
-	resp = session.MakeRequest(t, NewRequest(t, "GET", loc), http.StatusOK)
-	htmlDoc = NewHTMLParser(t, resp.Body)
-	assert.Equal(t, "Bad Request: invalid CSRF token",
-		strings.TrimSpace(htmlDoc.doc.Find(".ui.message").Text()),
-	)
 }

tests/integration/repo_branch_test.go

@@ -18,7 +18,6 @@ import (
 	"code.gitea.io/gitea/models/unittest"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/graceful"
-	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/test"
 	"code.gitea.io/gitea/modules/translation"
 	repo_service "code.gitea.io/gitea/services/repository"
@@ -157,15 +156,8 @@ func TestCreateBranchInvalidCSRF(t *testing.T) {
 		"_csrf":           "fake_csrf",
 		"new_branch_name": "test",
 	})
-	resp := session.MakeRequest(t, req, http.StatusSeeOther)
+	resp := session.MakeRequest(t, req, http.StatusBadRequest)
-	loc := resp.Header().Get("Location")
+	assert.Contains(t, resp.Body.String(), "Invalid CSRF token")
-	assert.Equal(t, setting.AppSubURL+"/", loc)
-	resp = session.MakeRequest(t, NewRequest(t, "GET", loc), http.StatusOK)
-	htmlDoc := NewHTMLParser(t, resp.Body)
-	assert.Equal(t,
-		"Bad Request: invalid CSRF token",
-		strings.TrimSpace(htmlDoc.doc.Find(".ui.message").Text()),
-	)
 }
 
 func TestDatabaseMissingABranch(t *testing.T) {