From 1ae3b127fc74906f0722caf6486890cf32d23715 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 18 Sep 2024 15:17:25 +0800 Subject: [PATCH] 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 | 1 - routers/web/web.go | 2 + services/context/context.go | 6 +- services/context/csrf.go | 192 ++++++++------------------ tests/integration/attachment_test.go | 3 +- tests/integration/csrf_test.go | 26 +--- tests/integration/repo_branch_test.go | 12 +- 7 files changed, 71 insertions(+), 171 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 1631c90ba2..2512fe8b55 100644 --- a/options/locale/locale_en-US.ini +++ b/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 Codeberg 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 diff --git a/routers/web/web.go b/routers/web/web.go index d174b4e251..39116b882d 100644 --- a/routers/web/web.go +++ b/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) } } diff --git a/services/context/context.go b/services/context/context.go index c0819ab11e..91e7b1849d 100644 --- a/services/context/context.go +++ b/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(``) // 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 diff --git a/services/context/csrf.go b/services/context/csrf.go index 57c55e6550..5890d53f42 100644 --- a/services/context/csrf.go +++ b/services/context/csrf.go @@ -20,64 +20,42 @@ 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 + // sessionKey is the key used for getting the unique ID per user. + sessionKey string + // oldSessionKey saves old value corresponding to sessionKey. + oldSessionKey string } -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 -} - -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, 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()) - 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(``) } 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()) } diff --git a/tests/integration/attachment_test.go b/tests/integration/attachment_test.go index 95c9c9f753..7cbc2545d5 100644 --- a/tests/integration/attachment_test.go +++ b/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) { diff --git a/tests/integration/csrf_test.go b/tests/integration/csrf_test.go index a789859889..fcb9661b8a 100644 --- a/tests/integration/csrf_test.go +++ b/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.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") } diff --git a/tests/integration/repo_branch_test.go b/tests/integration/repo_branch_test.go index 2aa299479a..df9ea9a97c 100644 --- a/tests/integration/repo_branch_test.go +++ b/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) - 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 TestDatabaseMissingABranch(t *testing.T) {