From 7a2786ca6cd84633784a2c9986da65a9c4d79c78 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 25 Dec 2023 21:01:24 +0800 Subject: [PATCH] Refactor CORS handler (#28587) (#28611) Backport #28587, the only conflict is the test file. 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 --- custom/conf/app.example.ini | 8 +- .../config-cheat-sheet.en-us.md | 4 +- .../config-cheat-sheet.zh-cn.md | 2 - modules/public/public.go | 2 +- modules/setting/cors.go | 4 +- modules/web/route.go | 24 ++---- routers/api/v1/api.go | 4 +- routers/web/githttp.go | 22 ++--- routers/web/misc/misc.go | 4 - routers/web/web.go | 50 ++++++----- tests/integration/cors_test.go | 85 +++++++++++++++++-- 11 files changed, 131 insertions(+), 78 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index dfe23adddb51c..fa75cbf88c78a 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1156,15 +1156,9 @@ LEVEL = Info ;; enable cors headers (disabled by default) ;ENABLED = false ;; -;; scheme of allowed requests -;SCHEME = http -;; -;; list of requesting domains that are allowed +;; list of requesting origins that are allowed, eg: "https://*.example.com" ;ALLOW_DOMAIN = * ;; -;; allow subdomains of headers listed above to request -;ALLOW_SUBDOMAIN = false -;; ;; list of methods allowed to request ;METHODS = GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS ;; diff --git a/docs/content/administration/config-cheat-sheet.en-us.md b/docs/content/administration/config-cheat-sheet.en-us.md index f97f021c3700e..b7cae8dc5e90b 100644 --- a/docs/content/administration/config-cheat-sheet.en-us.md +++ b/docs/content/administration/config-cheat-sheet.en-us.md @@ -196,9 +196,7 @@ The following configuration set `Content-Type: application/vnd.android.package-a ## CORS (`cors`) - `ENABLED`: **false**: enable cors headers (disabled by default) -- `SCHEME`: **http**: scheme of allowed requests -- `ALLOW_DOMAIN`: **\***: list of requesting domains that are allowed -- `ALLOW_SUBDOMAIN`: **false**: allow subdomains of headers listed above to request +- `ALLOW_DOMAIN`: **\***: list of requesting origins that are allowed, eg: "https://*.example.com" - `METHODS`: **GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS**: list of methods allowed to request - `MAX_AGE`: **10m**: max time to cache response - `ALLOW_CREDENTIALS`: **false**: allow request with credentials diff --git a/docs/content/administration/config-cheat-sheet.zh-cn.md b/docs/content/administration/config-cheat-sheet.zh-cn.md index bfb8845ab42bc..7c56f8222c9f1 100644 --- a/docs/content/administration/config-cheat-sheet.zh-cn.md +++ b/docs/content/administration/config-cheat-sheet.zh-cn.md @@ -195,9 +195,7 @@ menu: ## 跨域 (`cors`) - `ENABLED`: **false**: 启用 CORS 头部（默认禁用） -- `SCHEME`: **http**: 允许请求的协议 - `ALLOW_DOMAIN`: **\***: 允许请求的域名列表 -- `ALLOW_SUBDOMAIN`: **false**: 允许上述列出的头部的子域名发出请求。 - `METHODS`: **GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS**: 允许发起的请求方式列表 - `MAX_AGE`: **10m**: 缓存响应的最大时间 - `ALLOW_CREDENTIALS`: **false**: 允许带有凭据的请求 diff --git a/modules/public/public.go b/modules/public/public.go index 5fbfe30a81c7e..abc6b46158027 100644 --- a/modules/public/public.go +++ b/modules/public/public.go @@ -33,7 +33,7 @@ func FileHandlerFunc() http.HandlerFunc { assetFS := AssetFS() return func(resp http.ResponseWriter, req *http.Request) { if req.Method != "GET" && req.Method != "HEAD" { - resp.WriteHeader(http.StatusNotFound) + resp.WriteHeader(http.StatusMethodNotAllowed) return } handleRequest(resp, req, assetFS, req.URL.Path) diff --git a/modules/setting/cors.go b/modules/setting/cors.go index bafbbab64f990..63daaad60ba75 100644 --- a/modules/setting/cors.go +++ b/modules/setting/cors.go @@ -12,9 +12,7 @@ import ( // CORSConfig defines CORS settings var CORSConfig = struct { Enabled bool - Scheme string - AllowDomain []string - AllowSubdomain bool + AllowDomain []string // FIXME: this option is from legacy code, it actually works as "AllowedOrigins". When refactoring in the future, the config option should also be renamed together. Methods []string MaxAge time.Duration AllowCredentials bool diff --git a/modules/web/route.go b/modules/web/route.go index 86b83dd72362c..805fcb4411529 100644 --- a/modules/web/route.go +++ b/modules/web/route.go @@ -101,16 +101,18 @@ func (r *Route) wrapMiddlewareAndHandler(h []any) ([]func(http.Handler) http.Han return middlewares, handlerFunc } -func (r *Route) Methods(method, pattern string, h ...any) { +// Methods adds the same handlers for multiple http "methods" (separated by ","). +// If any method is invalid, the lower level router will panic. +func (r *Route) Methods(methods, pattern string, h ...any) { middlewares, handlerFunc := r.wrapMiddlewareAndHandler(h) fullPattern := r.getPattern(pattern) - if strings.Contains(method, ",") { - methods := strings.Split(method, ",") + if strings.Contains(methods, ",") { + methods := strings.Split(methods, ",") for _, method := range methods { r.R.With(middlewares...).Method(strings.TrimSpace(method), fullPattern, handlerFunc) } } else { - r.R.With(middlewares...).Method(method, fullPattern, handlerFunc) + r.R.With(middlewares...).Method(methods, fullPattern, handlerFunc) } } @@ -136,20 +138,6 @@ func (r *Route) Get(pattern string, h ...any) { r.Methods("GET", pattern, h...) } -func (r *Route) Options(pattern string, h ...any) { - r.Methods("OPTIONS", pattern, h...) -} - -// GetOptions delegate get and options method -func (r *Route) GetOptions(pattern string, h ...any) { - r.Methods("GET,OPTIONS", pattern, h...) -} - -// PostOptions delegate post and options method -func (r *Route) PostOptions(pattern string, h ...any) { - r.Methods("POST,OPTIONS", pattern, h...) -} - // Head delegate head method func (r *Route) Head(pattern string, h ...any) { r.Methods("HEAD", pattern, h...) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 6a8832288f81b..09fde1e05b90a 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -821,9 +821,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 b2fb5b472f772..8d0d1ce03a3d4 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 e35199401074d..54c93763f6a00 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 d67f9cf90c972..103c03f45a2ec 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -59,13 +59,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, @@ -74,7 +73,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) + } + }) } } @@ -217,7 +232,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")) @@ -457,8 +472,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) { @@ -531,14 +546,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) @@ -768,7 +780,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) diff --git a/tests/integration/cors_test.go b/tests/integration/cors_test.go index e4151d1c32e75..2b995e5906c6c 100644 --- a/tests/integration/cors_test.go +++ b/tests/integration/cors_test.go @@ -7,17 +7,88 @@ import ( "net/http" "testing" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/routers" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" ) -func TestCORSNotSet(t *testing.T) { +func TestCORS(t *testing.T) { defer tests.PrepareTestEnv(t)() - req := NewRequestf(t, "GET", "/api/v1/version") - session := loginUser(t, "user2") - resp := session.MakeRequest(t, req, http.StatusOK) - assert.Equal(t, resp.Code, http.StatusOK) - corsHeader := resp.Header().Get("Access-Control-Allow-Origin") - assert.Empty(t, corsHeader, "Access-Control-Allow-Origin: generated header should match") // header not set + t.Run("CORS enabled", func(t *testing.T) { + defer test.MockVariableValue(&setting.CORSConfig.Enabled, true)() + defer test.MockVariableValue(&testWebRoutes, routers.NormalRoutes())() + + t.Run("API with CORS", func(t *testing.T) { + // GET api with no CORS header + req := NewRequest(t, "GET", "/api/v1/version") + resp := MakeRequest(t, req, http.StatusOK) + assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin")) + assert.Contains(t, resp.Header().Values("Vary"), "Origin") + + // OPTIONS api for CORS + req = NewRequest(t, "OPTIONS", "/api/v1/version") + req.Header.Set("Origin", "https://example.com") + req.Header.Set("Access-Control-Request-Method", "GET") + resp = MakeRequest(t, req, http.StatusOK) + assert.NotEmpty(t, resp.Header().Get("Access-Control-Allow-Origin")) + assert.Contains(t, resp.Header().Values("Vary"), "Origin") + }) + + t.Run("Web with CORS", func(t *testing.T) { + // GET userinfo with no CORS header + req := NewRequest(t, "GET", "/login/oauth/userinfo") + resp := MakeRequest(t, req, http.StatusUnauthorized) + assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin")) + assert.Contains(t, resp.Header().Values("Vary"), "Origin") + + // OPTIONS userinfo for CORS + req = NewRequest(t, "OPTIONS", "/login/oauth/userinfo") + req.Header.Set("Origin", "https://example.com") + req.Header.Set("Access-Control-Request-Method", "GET") + resp = MakeRequest(t, req, http.StatusOK) + assert.NotEmpty(t, resp.Header().Get("Access-Control-Allow-Origin")) + assert.Contains(t, resp.Header().Values("Vary"), "Origin") + + // OPTIONS userinfo for non-CORS + req = NewRequest(t, "OPTIONS", "/login/oauth/userinfo") + resp = MakeRequest(t, req, http.StatusMethodNotAllowed) + assert.NotContains(t, resp.Header().Values("Vary"), "Origin") + }) + }) + + t.Run("CORS disabled", func(t *testing.T) { + defer test.MockVariableValue(&setting.CORSConfig.Enabled, false)() + defer test.MockVariableValue(&testWebRoutes, routers.NormalRoutes())() + + t.Run("API without CORS", func(t *testing.T) { + req := NewRequest(t, "GET", "/api/v1/version") + resp := MakeRequest(t, req, http.StatusOK) + assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin")) + assert.Empty(t, resp.Header().Values("Vary")) + + req = NewRequest(t, "OPTIONS", "/api/v1/version") + req.Header.Set("Origin", "https://example.com") + req.Header.Set("Access-Control-Request-Method", "GET") + resp = MakeRequest(t, req, http.StatusMethodNotAllowed) + assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin")) + assert.Empty(t, resp.Header().Values("Vary")) + }) + + t.Run("Web without CORS", func(t *testing.T) { + req := NewRequest(t, "GET", "/login/oauth/userinfo") + resp := MakeRequest(t, req, http.StatusUnauthorized) + assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin")) + assert.NotContains(t, resp.Header().Values("Vary"), "Origin") + + req = NewRequest(t, "OPTIONS", "/login/oauth/userinfo") + req.Header.Set("Origin", "https://example.com") + req.Header.Set("Access-Control-Request-Method", "GET") + resp = MakeRequest(t, req, http.StatusMethodNotAllowed) + assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin")) + assert.NotContains(t, resp.Header().Values("Vary"), "Origin") + }) + }) }