]> source.dussan.org Git - gitea.git/commitdiff
Refactor CSRF protector (#32057)
authorwxiaoguang <wxiaoguang@gmail.com>
Wed, 18 Sep 2024 07:17:25 +0000 (15:17 +0800)
committerGitHub <noreply@github.com>
Wed, 18 Sep 2024 07:17:25 +0000 (15:17 +0800)
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")

options/locale/locale_en-US.ini
routers/web/web.go
services/context/context.go
services/context/csrf.go
tests/integration/attachment_test.go
tests/integration/csrf_test.go
tests/integration/repo_branch_test.go

index 951994253ac34815bd26ad5cd165bdc78e3c1861..dc85d7c97ca27f7f934f296cb0bcb77e9f2dead8 100644 (file)
@@ -222,8 +222,6 @@ string.desc = Z - A
 [error]
 occurred = An error occurred
 report_message = If you believe that this is a Gitea bug, please search for issues on <a href="%s" target="_blank">GitHub</a> or open a new issue if necessary.
-missing_csrf = Bad Request: no CSRF token present
-invalid_csrf = Bad Request: invalid CSRF token
 not_found = The target couldn't be found.
 network_error = Network error
 
index 41b019e4b59e570967a3b201c1803ef73a5e05be..f1e941a84efcb6e9630970263b304d2f2f601332 100644 (file)
@@ -129,6 +129,8 @@ func webAuth(authMethod auth_service.Method) func(*context.Context) {
                        // ensure the session uid is deleted
                        _ = ctx.Session.Delete("uid")
                }
+
+               ctx.Csrf.PrepareForSessionUser(ctx)
        }
 }
 
index 69b65cbddbc92f64d30c45a5c65b92929d1eb678..42f7c3d9d1d8afa6721c7fd55083bd7fe4528900 100644 (file)
@@ -138,10 +138,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,
@@ -167,7 +165,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)
@@ -204,8 +202,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
index 9b0dc2923b5321b3d13ee9e37ed76bf1289f86f3..9b66d613e3b44c266af305d793f25361f14a8e84 100644 (file)
 package context
 
 import (
-       "encoding/base32"
-       "fmt"
+       "html/template"
        "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.
-       GetHeaderName() string
-       // GetFormName returns form value to search for token.
-       GetFormName() string
-       // GetToken returns the token.
-       GetToken() string
-       // Validate validates the token in http context.
+       // PrepareForSessionUser prepares the csrf protector for the current session user.
+       PrepareForSessionUser(ctx *Context)
+       // Validate validates the csrf 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.
-       Token string
-       // This value must be unique per user.
-       ID 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
+       // id must be unique per user.
+       id string
+       // token is the valid one which wil be used by end user and passed via header, cookie, or hidden form value.
+       token string
 }
 
 // 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.
-       Origin bool
-       // Cookie lifetime. Default is 0
-       CookieLifeTime int
-}
-
-func prepareDefaultCsrfOptions(opt CsrfOptions) CsrfOptions {
-       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
+       // sessionKey is the key used for getting the unique ID per user.
+       sessionKey string
+       // oldSessionKey saves old value corresponding to sessionKey.
+       oldSessionKey string
 }
 
-func newCsrfCookie(c *csrfProtector, value string) *http.Cookie {
+func newCsrfCookie(opt *CsrfOptions, value string) *http.Cookie {
        return &http.Cookie{
-               Name:     c.opt.Cookie,
+               Name:     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,
+               Path:     opt.CookiePath,
+               Domain:   opt.CookieDomain,
+               MaxAge:   int(CsrfTokenTimeout.Seconds()),
+               Secure:   opt.Secure,
+               HttpOnly: opt.CookieHTTPOnly,
+               SameSite: 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{opt: opt}
-
-       if opt.Origin && len(ctx.Req.Header.Get("Origin")) > 0 {
-               return x
+func NewCSRFProtector(opt CsrfOptions) CSRFProtector {
+       if opt.Secret == "" {
+               panic("CSRF secret is empty but it must be set") // it shouldn't happen because it is always set in code
        }
+       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"
-       uidAny := ctx.Session.Get(opt.SessionKey)
-       if uidAny != nil {
+func (c *csrfProtector) PrepareForSessionUser(ctx *Context) {
+       c.id = "0"
+       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)
-       uidChanged := oldUID == nil || oldUID.(string) != x.ID
-       cookieToken := ctx.GetSiteCookie(opt.Cookie)
+       oldUID := ctx.Session.Get(c.opt.oldSessionKey)
+       uidChanged := oldUID == nil || oldUID.(string) != c.id
+       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, re-use 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())
-               if opt.SetCookie {
-                       cookie := newCsrfCookie(x, x.Token)
-                       ctx.Resp.Header().Add("Set-Cookie", cookie.String())
-               }
+               c.token = GenerateCsrfToken(c.opt.Secret, c.id, "POST", time.Now())
+               cookie := newCsrfCookie(&c.opt, c.token)
+               ctx.Resp.Header().Add("Set-Cookie", cookie.String())
        }
 
-       if opt.SetHeader {
-               ctx.Resp.Header().Add(opt.Header, x.Token)
-       }
-       return x
+       ctx.Data["CsrfToken"] = c.token
+       ctx.Data["CsrfTokenHtml"] = template.HTML(`<input type="hidden" name="_csrf" value="` + template.HTMLEscapeString(c.token) + `">`)
 }
 
 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.
-                       http.Error(ctx.Resp, "Invalid CSRF token.", http.StatusBadRequest)
-               } else {
-                       ctx.Flash.Error(ctx.Tr("error.invalid_csrf"))
-                       ctx.Redirect(setting.AppSubURL + "/")
-               }
+               // 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)
        }
 }
 
 // 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 neither a header nor form value is found, http.StatusBadRequest is sent.
+// If this validation fails, 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, "")
-               cookie.MaxAge = -1
-               ctx.Resp.Header().Add("Set-Cookie", cookie.String())
-       }
+       cookie := newCsrfCookie(&c.opt, "")
+       cookie.MaxAge = -1
+       ctx.Resp.Header().Add("Set-Cookie", cookie.String())
 }
index 8206d8f4dc0d58108d68417c8ab8ace984819b33..40969d26f2a22cce44e77e9f20bd7e69350ccbe7 100644 (file)
@@ -59,7 +59,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) {
index a789859889b81cab2af7904fa6e5aaf8cd722b64..fcb9661b8a84da2ccc37a4c10420a4bda76b44e3 100644 (file)
@@ -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.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()),
-       )
+       resp := session.MakeRequest(t, req, http.StatusBadRequest)
+       assert.Contains(t, resp.Body.String(), "Invalid CSRF token")
 
        // 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.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()),
-       )
+       resp = session.MakeRequest(t, req, http.StatusBadRequest)
+       assert.Contains(t, resp.Body.String(), "Invalid CSRF token")
 }
index d1bc9198c32f49064aa412317603404e9548bcc2..f5217374b00f958d34e5bd7255da0127ecf7af78 100644 (file)
@@ -17,7 +17,6 @@ import (
        repo_model "code.gitea.io/gitea/models/repo"
        "code.gitea.io/gitea/models/unit"
        "code.gitea.io/gitea/models/unittest"
-       "code.gitea.io/gitea/modules/setting"
        api "code.gitea.io/gitea/modules/structs"
        "code.gitea.io/gitea/modules/test"
        "code.gitea.io/gitea/modules/translation"
@@ -146,15 +145,8 @@ func TestCreateBranchInvalidCSRF(t *testing.T) {
                "_csrf":           "fake_csrf",
                "new_branch_name": "test",
        })
-       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()),
-       )
+       resp := session.MakeRequest(t, req, http.StatusBadRequest)
+       assert.Contains(t, resp.Body.String(), "Invalid CSRF token")
 }
 
 func prepareBranch(t *testing.T, session *TestSession, repo *repo_model.Repository) {