Refactor CORS handler (#28587)

The CORS code has been unmaintained for long time, and the behavior is
not correct.

This PR tries to improve it. The key point is written as comment in
code. And add more tests.

Fix #28515
Fix #27642
Fix #17098

4 files changed, 43 insertions, 37 deletions

diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go
index 0e437bb92e..a4c3d6f444 100644
--- a/routers/api/v1/api.go
+++ b/routers/api/v1/api.go
@@ -822,9 +822,7 @@ func Routes() *web.Route {
 	m.Use(securityHeaders())
 	if setting.CORSConfig.Enabled {
 		m.Use(cors.Handler(cors.Options{
-			// Scheme:           setting.CORSConfig.Scheme, // FIXME: the cors middleware needs scheme option
-			AllowedOrigins: setting.CORSConfig.AllowDomain,
-			// setting.CORSConfig.AllowSubdomain // FIXME: the cors middleware needs allowSubdomain option
+			AllowedOrigins:   setting.CORSConfig.AllowDomain,
 			AllowedMethods:   setting.CORSConfig.Methods,
 			AllowCredentials: setting.CORSConfig.AllowCredentials,
 			AllowedHeaders:   append([]string{"Authorization", "X-Gitea-OTP"}, setting.CORSConfig.Headers...),
diff --git a/routers/web/githttp.go b/routers/web/githttp.go
index b2fb5b472f..8d0d1ce03a 100644
--- a/routers/web/githttp.go
+++ b/routers/web/githttp.go
@@ -28,16 +28,16 @@ func requireSignIn(ctx *context.Context) {
 
 func gitHTTPRouters(m *web.Route) {
 	m.Group("", func() {
-		m.PostOptions("/git-upload-pack", repo.ServiceUploadPack)
-		m.PostOptions("/git-receive-pack", repo.ServiceReceivePack)
-		m.GetOptions("/info/refs", repo.GetInfoRefs)
-		m.GetOptions("/HEAD", repo.GetTextFile("HEAD"))
-		m.GetOptions("/objects/info/alternates", repo.GetTextFile("objects/info/alternates"))
-		m.GetOptions("/objects/info/http-alternates", repo.GetTextFile("objects/info/http-alternates"))
-		m.GetOptions("/objects/info/packs", repo.GetInfoPacks)
-		m.GetOptions("/objects/info/{file:[^/]*}", repo.GetTextFile(""))
-		m.GetOptions("/objects/{head:[0-9a-f]{2}}/{hash:[0-9a-f]{38}}", repo.GetLooseObject)
-		m.GetOptions("/objects/pack/pack-{file:[0-9a-f]{40}}.pack", repo.GetPackFile)
-		m.GetOptions("/objects/pack/pack-{file:[0-9a-f]{40}}.idx", repo.GetIdxFile)
+		m.Methods("POST,OPTIONS", "/git-upload-pack", repo.ServiceUploadPack)
+		m.Methods("POST,OPTIONS", "/git-receive-pack", repo.ServiceReceivePack)
+		m.Methods("GET,OPTIONS", "/info/refs", repo.GetInfoRefs)
+		m.Methods("GET,OPTIONS", "/HEAD", repo.GetTextFile("HEAD"))
+		m.Methods("GET,OPTIONS", "/objects/info/alternates", repo.GetTextFile("objects/info/alternates"))
+		m.Methods("GET,OPTIONS", "/objects/info/http-alternates", repo.GetTextFile("objects/info/http-alternates"))
+		m.Methods("GET,OPTIONS", "/objects/info/packs", repo.GetInfoPacks)
+		m.Methods("GET,OPTIONS", "/objects/info/{file:[^/]*}", repo.GetTextFile(""))
+		m.Methods("GET,OPTIONS", "/objects/{head:[0-9a-f]{2}}/{hash:[0-9a-f]{38}}", repo.GetLooseObject)
+		m.Methods("GET,OPTIONS", "/objects/pack/pack-{file:[0-9a-f]{40}}.pack", repo.GetPackFile)
+		m.Methods("GET,OPTIONS", "/objects/pack/pack-{file:[0-9a-f]{40}}.idx", repo.GetIdxFile)
 	}, ignSignInAndCsrf, requireSignIn, repo.HTTPGitEnabledHandler, repo.CorsHandler(), context_service.UserAssignmentWeb())
 }
diff --git a/routers/web/misc/misc.go b/routers/web/misc/misc.go
index e351994010..54c93763f6 100644
--- a/routers/web/misc/misc.go
+++ b/routers/web/misc/misc.go
@@ -33,10 +33,6 @@ func DummyOK(w http.ResponseWriter, req *http.Request) {
 	w.WriteHeader(http.StatusOK)
 }
 
-func DummyBadRequest(w http.ResponseWriter, req *http.Request) {
-	w.WriteHeader(http.StatusBadRequest)
-}
-
 func RobotsTxt(w http.ResponseWriter, req *http.Request) {
 	robotsTxt := util.FilePathJoinAbs(setting.CustomPath, "public/robots.txt")
 	if ok, _ := util.IsExist(robotsTxt); !ok {
diff --git a/routers/web/web.go b/routers/web/web.go
index 02fb11b1f5..164c137f2a 100644
--- a/routers/web/web.go
+++ b/routers/web/web.go
@@ -60,13 +60,12 @@ const (
 	GzipMinSize = 1400
 )
 
-// CorsHandler return a http handler who set CORS options if enabled by config
-func CorsHandler() func(next http.Handler) http.Handler {
+// optionsCorsHandler return a http handler which sets CORS options if enabled by config, it blocks non-CORS OPTIONS requests.
+func optionsCorsHandler() func(next http.Handler) http.Handler {
+	var corsHandler func(next http.Handler) http.Handler
 	if setting.CORSConfig.Enabled {
-		return cors.Handler(cors.Options{
-			// Scheme:           setting.CORSConfig.Scheme, // FIXME: the cors middleware needs scheme option
-			AllowedOrigins: setting.CORSConfig.AllowDomain,
-			// setting.CORSConfig.AllowSubdomain // FIXME: the cors middleware needs allowSubdomain option
+		corsHandler = cors.Handler(cors.Options{
+			AllowedOrigins:   setting.CORSConfig.AllowDomain,
 			AllowedMethods:   setting.CORSConfig.Methods,
 			AllowCredentials: setting.CORSConfig.AllowCredentials,
 			AllowedHeaders:   setting.CORSConfig.Headers,
@@ -75,7 +74,23 @@ func CorsHandler() func(next http.Handler) http.Handler {
 	}
 
 	return func(next http.Handler) http.Handler {
-		return next
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if r.Method == http.MethodOptions {
+				if corsHandler != nil && r.Header.Get("Access-Control-Request-Method") != "" {
+					corsHandler(next).ServeHTTP(w, r)
+				} else {
+					// it should explicitly deny OPTIONS requests if CORS handler is not executed, to avoid the next GET/POST handler being incorrectly called by the OPTIONS request
+					w.WriteHeader(http.StatusMethodNotAllowed)
+				}
+				return
+			}
+			// for non-OPTIONS requests, call the CORS handler to add some related headers like "Vary"
+			if corsHandler != nil {
+				corsHandler(next).ServeHTTP(w, r)
+			} else {
+				next.ServeHTTP(w, r)
+			}
+		})
 	}
 }
 
@@ -218,7 +233,7 @@ func Routes() *web.Route {
 	routes := web.NewRoute()
 
 	routes.Head("/", misc.DummyOK) // for health check - doesn't need to be passed through gzip handler
-	routes.Methods("GET, HEAD", "/assets/*", CorsHandler(), public.FileHandlerFunc())
+	routes.Methods("GET, HEAD, OPTIONS", "/assets/*", optionsCorsHandler(), public.FileHandlerFunc())
 	routes.Methods("GET, HEAD", "/avatars/*", storageHandler(setting.Avatar.Storage, "avatars", storage.Avatars))
 	routes.Methods("GET, HEAD", "/repo-avatars/*", storageHandler(setting.RepoAvatar.Storage, "repo-avatars", storage.RepoAvatars))
 	routes.Methods("GET, HEAD", "/apple-touch-icon.png", misc.StaticRedirect("/assets/img/apple-touch-icon.png"))
@@ -458,8 +473,8 @@ func registerRoutes(m *web.Route) {
 		m.Get("/change-password", func(ctx *context.Context) {
 			ctx.Redirect(setting.AppSubURL + "/user/settings/account")
 		})
-		m.Any("/*", CorsHandler(), public.FileHandlerFunc())
-	}, CorsHandler())
+		m.Methods("GET, HEAD", "/*", public.FileHandlerFunc())
+	}, optionsCorsHandler())
 
 	m.Group("/explore", func() {
 		m.Get("", func(ctx *context.Context) {
@@ -532,14 +547,11 @@ func registerRoutes(m *web.Route) {
 		// TODO manage redirection
 		m.Post("/authorize", web.Bind(forms.AuthorizationForm{}), auth.AuthorizeOAuth)
 	}, ignSignInAndCsrf, reqSignIn)
-	m.Options("/login/oauth/userinfo", CorsHandler(), misc.DummyBadRequest)
-	m.Get("/login/oauth/userinfo", ignSignInAndCsrf, auth.InfoOAuth)
-	m.Options("/login/oauth/access_token", CorsHandler(), misc.DummyBadRequest)
-	m.Post("/login/oauth/access_token", CorsHandler(), web.Bind(forms.AccessTokenForm{}), ignSignInAndCsrf, auth.AccessTokenOAuth)
-	m.Options("/login/oauth/keys", CorsHandler(), misc.DummyBadRequest)
-	m.Get("/login/oauth/keys", ignSignInAndCsrf, auth.OIDCKeys)
-	m.Options("/login/oauth/introspect", CorsHandler(), misc.DummyBadRequest)
-	m.Post("/login/oauth/introspect", CorsHandler(), web.Bind(forms.IntrospectTokenForm{}), ignSignInAndCsrf, auth.IntrospectOAuth)
+
+	m.Methods("GET, OPTIONS", "/login/oauth/userinfo", optionsCorsHandler(), ignSignInAndCsrf, auth.InfoOAuth)
+	m.Methods("POST, OPTIONS", "/login/oauth/access_token", optionsCorsHandler(), web.Bind(forms.AccessTokenForm{}), ignSignInAndCsrf, auth.AccessTokenOAuth)
+	m.Methods("GET, OPTIONS", "/login/oauth/keys", optionsCorsHandler(), ignSignInAndCsrf, auth.OIDCKeys)
+	m.Methods("POST, OPTIONS", "/login/oauth/introspect", optionsCorsHandler(), web.Bind(forms.IntrospectTokenForm{}), ignSignInAndCsrf, auth.IntrospectOAuth)
 
 	m.Group("/user/settings", func() {
 		m.Get("", user_setting.Profile)
@@ -770,7 +782,7 @@ func registerRoutes(m *web.Route) {
 
 	m.Group("", func() {
 		m.Get("/{username}", user.UsernameSubRoute)
-		m.Get("/attachments/{uuid}", repo.GetAttachment)
+		m.Methods("GET, OPTIONS", "/attachments/{uuid}", optionsCorsHandler(), repo.GetAttachment)
 	}, ignSignIn)
 
 	m.Post("/{username}", reqSignIn, context_service.UserAssignmentWeb(), user.Action)