diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index dfe23addd..fa75cbf88 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 f97f021c3..b7cae8dc5 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 bfb8845ab..7c56f8222 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 5fbfe30a8..abc6b4615 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 bafbbab64..63daaad60 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 86b83dd72..805fcb441 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 6a8832288..09fde1e05 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 b2fb5b472..8d0d1ce03 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 e35199401..54c93763f 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 d67f9cf90..103c03f45 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 e4151d1c3..2b995e590 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") + }) + }) }