Skip to content

Commit 42d2949

Browse files
Replace CSRF cookie with CrossOriginProtection (#36183)
Removes the CSRF cookie in favor of [`CrossOriginProtection`](https://pkg.go.dev/net/http#CrossOriginProtection) which relies purely on HTTP headers. Fixes: #11188 Fixes: #30333 Helps: #35107 TODOs: - [x] Fix tests - [ ] Ideally add tests to validates the protection --------- Signed-off-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
1 parent eddf875 commit 42d2949

207 files changed

Lines changed: 178 additions & 1196 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

custom/conf/app.example.ini

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -503,9 +503,6 @@ INTERNAL_TOKEN =
503503
;; Password Hash algorithm, either "argon2", "pbkdf2", "scrypt" or "bcrypt"
504504
;PASSWORD_HASH_ALGO = pbkdf2
505505
;;
506-
;; Set false to allow JavaScript to read CSRF cookie
507-
;CSRF_COOKIE_HTTP_ONLY = true
508-
;;
509506
;; Validate against https://haveibeenpwned.com/Passwords to see if a password has been exposed
510507
;PASSWORD_CHECK_PWN = false
511508
;;

modules/setting/markup.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ func newMarkupRenderer(name string, sec ConfigSection) {
255255
}
256256

257257
// ATTENTION! at the moment, only a safe set like "allow-scripts" are allowed for sandbox mode.
258-
// "allow-same-origin" should never be used, it leads to XSS attack, and it makes the JS in iframe can access parent window's config and CSRF token
258+
// "allow-same-origin" should NEVER be used, it leads to XSS attack: makes the JS in iframe can access parent window's config and send requests with user's credentials.
259259
renderContentSandbox := sec.Key("RENDER_CONTENT_SANDBOX").MustString("allow-scripts allow-popups")
260260
if renderContentSandbox == "disabled" {
261261
renderContentSandbox = ""

modules/setting/oauth2.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ func loadOAuth2From(rootCfg ConfigProvider) {
133133

134134
// FIXME: at the moment, no matter oauth2 is enabled or not, it must generate a "oauth2 JWT_SECRET"
135135
// Because this secret is also used as GeneralTokenSigningSecret (as a quick not-that-breaking fix for some legacy problems).
136-
// Including: CSRF token, account validation token, etc ...
136+
// Including: account validation token, etc ...
137137
// In main branch, the signing token should be refactored (eg: one unique for LFS/OAuth2/etc ...)
138138
jwtSecretBase64 := loadSecret(sec, "JWT_SECRET_URI", "JWT_SECRET")
139139
if InstallLock {

modules/setting/security.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,6 @@ var (
3636
PasswordCheckPwn bool
3737
SuccessfulTokensCacheSize int
3838
DisableQueryAuthToken bool
39-
CSRFCookieName = "_csrf"
40-
CSRFCookieHTTPOnly = true
4139
RecordUserSignupMetadata = false
4240
TwoFactorAuthEnforced = false
4341
)
@@ -139,7 +137,6 @@ func loadSecurityFrom(rootCfg ConfigProvider) {
139137
log.Fatal("The provided password hash algorithm was invalid: %s", sec.Key("PASSWORD_HASH_ALGO").MustString(""))
140138
}
141139

142-
CSRFCookieHTTPOnly = sec.Key("CSRF_COOKIE_HTTP_ONLY").MustBool(true)
143140
PasswordCheckPwn = sec.Key("PASSWORD_CHECK_PWN").MustBool(false)
144141
SuccessfulTokensCacheSize = sec.Key("SUCCESSFUL_TOKENS_CACHE_SIZE").MustInt(20)
145142

routers/common/auth.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ func AuthShared(ctx *context.Base, sessionStore auth_service.SessionStore, authM
3838

3939
// VerifyOptions contains required or check options
4040
type VerifyOptions struct {
41-
SignInRequired bool
42-
SignOutRequired bool
43-
AdminRequired bool
44-
DisableCSRF bool
41+
SignInRequired bool
42+
SignOutRequired bool
43+
AdminRequired bool
44+
DisableCrossOriginProtection bool
4545
}

routers/web/auth/auth.go

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ func autoSignIn(ctx *context.Context) (bool, error) {
102102
return false, err
103103
}
104104

105-
ctx.Csrf.PrepareForSessionUser(ctx)
106105
return true, nil
107106
}
108107

@@ -357,9 +356,6 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRe
357356
ctx.Locale = middleware.Locale(ctx.Resp, ctx.Req)
358357
}
359358

360-
// force to generate a new CSRF token
361-
ctx.Csrf.PrepareForSessionUser(ctx)
362-
363359
// Register last login
364360
if err := user_service.UpdateUser(ctx, u, &user_service.UpdateOptions{SetLastLogin: true}); err != nil {
365361
ctx.ServerError("UpdateUser", err)
@@ -403,7 +399,6 @@ func HandleSignOut(ctx *context.Context) {
403399
_ = ctx.Session.Flush()
404400
_ = ctx.Session.Destroy(ctx.Resp, ctx.Req)
405401
ctx.DeleteSiteCookie(setting.CookieRememberName)
406-
ctx.Csrf.DeleteCookie(ctx)
407402
middleware.DeleteRedirectToCookie(ctx.Resp)
408403
}
409404

@@ -811,8 +806,6 @@ func handleAccountActivation(ctx *context.Context, user *user_model.User) {
811806
return
812807
}
813808

814-
ctx.Csrf.PrepareForSessionUser(ctx)
815-
816809
if err := resetLocale(ctx, user); err != nil {
817810
ctx.ServerError("resetLocale", err)
818811
return

routers/web/auth/oauth.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -393,9 +393,6 @@ func handleOAuth2SignIn(ctx *context.Context, authSource *auth.Source, u *user_m
393393
return
394394
}
395395

396-
// force to generate a new CSRF token
397-
ctx.Csrf.PrepareForSessionUser(ctx)
398-
399396
if err := resetLocale(ctx, u); err != nil {
400397
ctx.ServerError("resetLocale", err)
401398
return

routers/web/githttp.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,5 +22,5 @@ func addOwnerRepoGitHTTPRouters(m *web.Router) {
2222
m.Methods("GET,OPTIONS", "/objects/{head:[0-9a-f]{2}}/{hash:[0-9a-f]{38,62}}", repo.GetLooseObject)
2323
m.Methods("GET,OPTIONS", "/objects/pack/pack-{file:[0-9a-f]{40,64}}.pack", repo.GetPackFile)
2424
m.Methods("GET,OPTIONS", "/objects/pack/pack-{file:[0-9a-f]{40,64}}.idx", repo.GetIdxFile)
25-
}, optSignInIgnoreCsrf, repo.HTTPGitEnabledHandler, repo.CorsHandler(), context.UserAssignmentWeb())
25+
}, repo.HTTPGitEnabledHandler, repo.CorsHandler(), optSignInFromAnyOrigin, context.UserAssignmentWeb())
2626
}

routers/web/user/setting/keys.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ func loadKeysData(ctx *context.Context) {
325325
ctx.Data["GPGKeys"] = gpgkeys
326326
tokenToSign := asymkey_model.VerificationToken(ctx.Doer, 1)
327327

328-
// generate a new aes cipher using the csrfToken
328+
// generate a new aes cipher using the token
329329
ctx.Data["TokenToSign"] = tokenToSign
330330

331331
principals, err := db.Find[asymkey_model.PublicKey](ctx, asymkey_model.FindPublicKeyOptions{

routers/web/web.go

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -129,13 +129,13 @@ func webAuth(authMethod auth_service.Method) func(*context.Context) {
129129
// ensure the session uid is deleted
130130
_ = ctx.Session.Delete("uid")
131131
}
132-
133-
ctx.Csrf.PrepareForSessionUser(ctx)
134132
}
135133
}
136134

137135
// verifyAuthWithOptions checks authentication according to options
138136
func verifyAuthWithOptions(options *common.VerifyOptions) func(ctx *context.Context) {
137+
crossOriginProtection := http.NewCrossOriginProtection()
138+
139139
return func(ctx *context.Context) {
140140
// Check prohibit login users.
141141
if ctx.IsSigned {
@@ -178,9 +178,9 @@ func verifyAuthWithOptions(options *common.VerifyOptions) func(ctx *context.Cont
178178
return
179179
}
180180

181-
if !options.SignOutRequired && !options.DisableCSRF && ctx.Req.Method == http.MethodPost {
182-
ctx.Csrf.Validate(ctx)
183-
if ctx.Written() {
181+
if !options.SignOutRequired && !options.DisableCrossOriginProtection {
182+
if err := crossOriginProtection.Check(ctx.Req); err != nil {
183+
http.Error(ctx.Resp, err.Error(), http.StatusForbidden)
184184
return
185185
}
186186
}
@@ -292,7 +292,12 @@ func Routes() *web.Router {
292292
return routes
293293
}
294294

295-
var optSignInIgnoreCsrf = verifyAuthWithOptions(&common.VerifyOptions{DisableCSRF: true})
295+
// optSignInFromAnyOrigin means that the user can (optionally) be signed in from any origin (no cross-origin protection)
296+
// - With CORS middleware: CORS middleware does the preflight request handling, the requests has Sec-Fetch-Site header.
297+
// The CORS mechanism already protects cross-origin requests, and the CrossOriginProtection has no "allowed origin" list, so disable CrossOriginProtection.
298+
// - For non-browser client requests: git clone via http, no Sec-Fetch-Site header.
299+
// Such requests are not cross-origin requests, so disable CrossOriginProtection.
300+
var optSignInFromAnyOrigin = verifyAuthWithOptions(&common.VerifyOptions{DisableCrossOriginProtection: true})
296301

297302
// registerWebRoutes register routes
298303
func registerWebRoutes(m *web.Router) {
@@ -489,7 +494,7 @@ func registerWebRoutes(m *web.Router) {
489494
m.Post("/-/markup", reqSignIn, web.Bind(structs.MarkupOption{}), misc.Markup)
490495

491496
m.Get("/-/web-theme/list", misc.WebThemeList)
492-
m.Post("/-/web-theme/apply", optSignInIgnoreCsrf, misc.WebThemeApply)
497+
m.Post("/-/web-theme/apply", optSignIn, misc.WebThemeApply)
493498

494499
m.Group("/explore", func() {
495500
m.Get("", func(ctx *context.Context) {
@@ -565,12 +570,14 @@ func registerWebRoutes(m *web.Router) {
565570
m.Post("/grant", web.Bind(forms.GrantApplicationForm{}), auth.GrantApplicationOAuth)
566571
// TODO manage redirection
567572
m.Post("/authorize", web.Bind(forms.AuthorizationForm{}), auth.AuthorizeOAuth)
568-
}, optSignInIgnoreCsrf, reqSignIn)
573+
}, reqSignIn)
569574

570-
m.Methods("GET, POST, OPTIONS", "/userinfo", optionsCorsHandler(), optSignInIgnoreCsrf, auth.InfoOAuth)
571-
m.Methods("POST, OPTIONS", "/access_token", optionsCorsHandler(), web.Bind(forms.AccessTokenForm{}), optSignInIgnoreCsrf, auth.AccessTokenOAuth)
572-
m.Methods("GET, OPTIONS", "/keys", optionsCorsHandler(), optSignInIgnoreCsrf, auth.OIDCKeys)
573-
m.Methods("POST, OPTIONS", "/introspect", optionsCorsHandler(), web.Bind(forms.IntrospectTokenForm{}), optSignInIgnoreCsrf, auth.IntrospectOAuth)
575+
m.Group("", func() {
576+
m.Methods("GET, POST, OPTIONS", "/userinfo", auth.InfoOAuth)
577+
m.Methods("POST, OPTIONS", "/access_token", web.Bind(forms.AccessTokenForm{}), auth.AccessTokenOAuth)
578+
m.Methods("GET, OPTIONS", "/keys", auth.OIDCKeys)
579+
m.Methods("POST, OPTIONS", "/introspect", web.Bind(forms.IntrospectTokenForm{}), auth.IntrospectOAuth)
580+
}, optionsCorsHandler(), optSignInFromAnyOrigin)
574581
}, oauth2Enabled)
575582

576583
m.Group("/user/settings", func() {
@@ -1653,7 +1660,7 @@ func registerWebRoutes(m *web.Router) {
16531660
m.Post("/action/{action:accept_transfer|reject_transfer}", reqSignIn, repo.ActionTransfer)
16541661
}, optSignIn, context.RepoAssignment)
16551662

1656-
common.AddOwnerRepoGitLFSRoutes(m, optSignInIgnoreCsrf, lfsServerEnabled) // "/{username}/{reponame}/{lfs-paths}": git-lfs support
1663+
common.AddOwnerRepoGitLFSRoutes(m, lfsServerEnabled, repo.CorsHandler(), optSignInFromAnyOrigin) // "/{username}/{reponame}/{lfs-paths}": git-lfs support, see also addOwnerRepoGitHTTPRouters
16571664

16581665
addOwnerRepoGitHTTPRouters(m) // "/{username}/{reponame}/{git-paths}": git http support
16591666

0 commit comments

Comments
 (0)