Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protection against CSRF added #22077

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ require (
github.com/prometheus/procfs v0.8.0 // indirect
github.com/rivo/uniseg v0.4.2 // indirect
github.com/rogpeppe/go-internal v1.9.0 // indirect
github.com/rs/cors v1.8.2 // indirect
github.com/rs/xid v1.4.0 // indirect
github.com/russross/blackfriday/v2 v2.1.0 // indirect
github.com/shopspring/decimal v1.2.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -1297,6 +1297,8 @@ github.com/rogpeppe/go-internal v1.8.1/go.mod h1:JeRgkft04UBgHMgCIwADu4Pn6Mtm5d4
github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8=
github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs=
github.com/rs/cors v1.7.0/go.mod h1:gFx+x8UowdsKA9AchylcLynDq+nNFfI8FkUZdN/jGCU=
github.com/rs/cors v1.8.2 h1:KCooALfAYGs415Cwu5ABvv9n9509fSiG5SQJn/AQo4U=
github.com/rs/cors v1.8.2/go.mod h1:XyqrcTp5zjWr1wsJ8PIRZssZ8b/WMcMf71DJnit4EMU=
github.com/rs/xid v1.2.1/go.mod h1:+uKXf+4Djp6Md1KODXJxgGQPKngRmWyn10oCKFzNHOQ=
github.com/rs/xid v1.4.0 h1:qd7wPTDkN6KQx2VmMBLrpHkiyQwgFXRnkOLacUiaSNY=
github.com/rs/xid v1.4.0/go.mod h1:trrq9SKmegXys3aeAKXMUTdJsYXVwGY3RLcfgqegfbg=
Expand Down
14 changes: 13 additions & 1 deletion modules/setting/cors.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2019 The Gitea Authors. All rights reserved.
// Copyright 2022 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package setting
Expand All @@ -7,8 +7,13 @@ import (
"time"

"code.gitea.io/gitea/modules/log"
"github.com/rs/cors"
)

// Cors handles CORS requests and allows other middlewares
// to check whetcher request marches CORS allowed origins.
var Cors *cors.Cors

// CORSConfig defines CORS settings
var CORSConfig = struct {
Enabled bool
Expand All @@ -34,6 +39,13 @@ func newCORSService() {
}

if CORSConfig.Enabled {
Cors = cors.New(cors.Options{
AllowedOrigins: CORSConfig.AllowDomain,
AllowedMethods: CORSConfig.Methods,
AllowCredentials: CORSConfig.AllowCredentials,
MaxAge: int(CORSConfig.MaxAge.Seconds()),
})

log.Info("CORS Service Enabled")
}
}
40 changes: 39 additions & 1 deletion routers/common/middleware.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2021 The Gitea Authors. All rights reserved.
// Copyright 2022 The Gitea Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, updating the copyright year is kinda useless nowadays, as using git is sufficient to establish who/when the work was done (which is why there is "the gitea authors" and not the detail). That's the opinion of the Linux Foundation ( https://www.linuxfoundation.org/blog/blog/copyright-notices-in-open-source-software-projects , the author is the Vice President of Compliance and Legal ). I also got the same answer from our legal department, even if I haven't been able to convince them to publish something outside our intranet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

// SPDX-License-Identifier: MIT

package common
Expand Down Expand Up @@ -78,5 +78,43 @@ func Middlewares() []func(http.Handler) http.Handler {
next.ServeHTTP(resp, req)
})
})

// Add CSRF handler.
handlers = append(handlers, csrfHandler())

return handlers
}

// csfrHandler blocks recognized CSRF attempts.
// WARNING: for this proctection to work, web browser compatible with
// Fetch Metadata Request Headers (https://w3c.github.io/webappsec-fetch-metadata)
// must be used.
func csrfHandler() func(next http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {

// Put header names we use for CSRF recognition into Vary response header.
if setting.CORSConfig.Enabled {
resp.Header().Set("Vary", "Origin, Sec-Fetch-Site")
} else {
resp.Header().Set("Vary", "Sec-Fetch-Site")
}

// Allow requests not recognized as CSRF.
secFetchSite := strings.ToLower(req.Header.Get("Sec-Fetch-Site"))
if req.Method == "GET" || // GET must not be used for changing state (CSRF resistant).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it also cover HEAD ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added HEAD and OPTIONS also.

secFetchSite == "" || // Accept requests from clients without Fetch Metadata Request Headers support.
secFetchSite == "same-origin" || // Accept requests from own origin.
secFetchSite == "none" || // Accept requests initiated by user (i.e. using bookmark).
((secFetchSite == "same-site" || secFetchSite == "cross-site") && // Accept cross site requests allowed by CORS.
setting.CORSConfig.Enabled && setting.Cors.OriginAllowed(req)) {
next.ServeHTTP(resp, req)
return
}

// Forbid and log other requests as CSRF.
log.Error("CSRF rejected: METHOD=\"%s\", Origin=\"%s\", Sec-Fetch-Site=\"%s\"", req.Method, req.Header.Get("Origin"), secFetchSite)
http.Error(resp, http.StatusText(http.StatusForbidden), http.StatusForbidden)
})
}
}