Skip to content

Commit 7a2786c

Browse files
wxiaoguangwxiaoguang
authored
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
1 parent b258833 commit 7a2786c

File tree

11 files changed

+131
-78
lines changed

11 files changed

+131
-78
lines changed

‎custom/conf/app.example.ini

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1156,15 +1156,9 @@ LEVEL = Info
11561156
;; enable cors headers (disabled by default)
11571157
;ENABLED = false
11581158
;;
1159-
;; scheme of allowed requests
1160-
;SCHEME = http
1161-
;;
1162-
;; list of requesting domains that are allowed
1159+
;; list of requesting origins that are allowed, eg: "https://*.example.com"
11631160
;ALLOW_DOMAIN = *
11641161
;;
1165-
;; allow subdomains of headers listed above to request
1166-
;ALLOW_SUBDOMAIN = false
1167-
;;
11681162
;; list of methods allowed to request
11691163
;METHODS = GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS
11701164
;;

‎docs/content/administration/config-cheat-sheet.en-us.md

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -196,9 +196,7 @@ The following configuration set `Content-Type: application/vnd.android.package-a
196196
## CORS (`cors`)
197197

198198
- `ENABLED`: **false**: enable cors headers (disabled by default)
199-
- `SCHEME`: **http**: scheme of allowed requests
200-
- `ALLOW_DOMAIN`: **\***: list of requesting domains that are allowed
201-
- `ALLOW_SUBDOMAIN`: **false**: allow subdomains of headers listed above to request
199+
- `ALLOW_DOMAIN`: **\***: list of requesting origins that are allowed, eg: "https://*.example.com"
202200
- `METHODS`: **GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS**: list of methods allowed to request
203201
- `MAX_AGE`: **10m**: max time to cache response
204202
- `ALLOW_CREDENTIALS`: **false**: allow request with credentials

‎docs/content/administration/config-cheat-sheet.zh-cn.md

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,9 +195,7 @@ menu:
195195
## 跨域 (`cors`)
196196

197197
- `ENABLED`: **false**: 启用 CORS 头部（默认禁用）
198-
- `SCHEME`: **http**: 允许请求的协议
199198
- `ALLOW_DOMAIN`: **\***: 允许请求的域名列表
200-
- `ALLOW_SUBDOMAIN`: **false**: 允许上述列出的头部的子域名发出请求。
201199
- `METHODS`: **GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS**: 允许发起的请求方式列表
202200
- `MAX_AGE`: **10m**: 缓存响应的最大时间
203201
- `ALLOW_CREDENTIALS`: **false**: 允许带有凭据的请求

‎modules/public/public.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func FileHandlerFunc() http.HandlerFunc {
3333
assetFS := AssetFS()
3434
return func(resp http.ResponseWriter, req *http.Request) {
3535
if req.Method != "GET" && req.Method != "HEAD" {
36-
resp.WriteHeader(http.StatusNotFound)
36+
resp.WriteHeader(http.StatusMethodNotAllowed)
3737
return
3838
}
3939
handleRequest(resp, req, assetFS, req.URL.Path)

‎modules/setting/cors.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,7 @@ import (
1212
// CORSConfig defines CORS settings
1313
var CORSConfig = struct {
1414
Enabled bool
15-
Scheme string
16-
AllowDomain []string
17-
AllowSubdomain bool
15+
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.
1816
Methods []string
1917
MaxAge time.Duration
2018
AllowCredentials bool

‎modules/web/route.go

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -101,16 +101,18 @@ func (r *Route) wrapMiddlewareAndHandler(h []any) ([]func(http.Handler) http.Han
101101
return middlewares, handlerFunc
102102
}
103103

104-
func (r *Route) Methods(method, pattern string, h ...any) {
104+
// Methods adds the same handlers for multiple http "methods" (separated by ",").
105+
// If any method is invalid, the lower level router will panic.
106+
func (r *Route) Methods(methods, pattern string, h ...any) {
105107
middlewares, handlerFunc := r.wrapMiddlewareAndHandler(h)
106108
fullPattern := r.getPattern(pattern)
107-
if strings.Contains(method, ",") {
108-
methods := strings.Split(method, ",")
109+
if strings.Contains(methods, ",") {
110+
methods := strings.Split(methods, ",")
109111
for _, method := range methods {
110112
r.R.With(middlewares...).Method(strings.TrimSpace(method), fullPattern, handlerFunc)
111113
}
112114
} else {
113-
r.R.With(middlewares...).Method(method, fullPattern, handlerFunc)
115+
r.R.With(middlewares...).Method(methods, fullPattern, handlerFunc)
114116
}
115117
}
116118

@@ -136,20 +138,6 @@ func (r *Route) Get(pattern string, h ...any) {
136138
r.Methods("GET", pattern, h...)
137139
}
138140

139-
func (r *Route) Options(pattern string, h ...any) {
140-
r.Methods("OPTIONS", pattern, h...)
141-
}
142-
143-
// GetOptions delegate get and options method
144-
func (r *Route) GetOptions(pattern string, h ...any) {
145-
r.Methods("GET,OPTIONS", pattern, h...)
146-
}
147-
148-
// PostOptions delegate post and options method
149-
func (r *Route) PostOptions(pattern string, h ...any) {
150-
r.Methods("POST,OPTIONS", pattern, h...)
151-
}
152-
153141
// Head delegate head method
154142
func (r *Route) Head(pattern string, h ...any) {
155143
r.Methods("HEAD", pattern, h...)

‎routers/api/v1/api.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -821,9 +821,7 @@ func Routes() *web.Route {
821821
m.Use(securityHeaders())
822822
if setting.CORSConfig.Enabled {
823823
m.Use(cors.Handler(cors.Options{
824-
// Scheme: setting.CORSConfig.Scheme, // FIXME: the cors middleware needs scheme option
825-
AllowedOrigins: setting.CORSConfig.AllowDomain,
826-
// setting.CORSConfig.AllowSubdomain // FIXME: the cors middleware needs allowSubdomain option
824+
AllowedOrigins: setting.CORSConfig.AllowDomain,
827825
AllowedMethods: setting.CORSConfig.Methods,
828826
AllowCredentials: setting.CORSConfig.AllowCredentials,
829827
AllowedHeaders: append([]string{"Authorization", "X-Gitea-OTP"}, setting.CORSConfig.Headers...),

‎routers/web/githttp.go

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -28,16 +28,16 @@ func requireSignIn(ctx *context.Context) {
2828

2929
func gitHTTPRouters(m *web.Route) {
3030
m.Group("", func() {
31-
m.PostOptions("/git-upload-pack", repo.ServiceUploadPack)
32-
m.PostOptions("/git-receive-pack", repo.ServiceReceivePack)
33-
m.GetOptions("/info/refs", repo.GetInfoRefs)
34-
m.GetOptions("/HEAD", repo.GetTextFile("HEAD"))
35-
m.GetOptions("/objects/info/alternates", repo.GetTextFile("objects/info/alternates"))
36-
m.GetOptions("/objects/info/http-alternates", repo.GetTextFile("objects/info/http-alternates"))
37-
m.GetOptions("/objects/info/packs", repo.GetInfoPacks)
38-
m.GetOptions("/objects/info/{file:[^/]*}", repo.GetTextFile(""))
39-
m.GetOptions("/objects/{head:[0-9a-f]{2}}/{hash:[0-9a-f]{38}}", repo.GetLooseObject)
40-
m.GetOptions("/objects/pack/pack-{file:[0-9a-f]{40}}.pack", repo.GetPackFile)
41-
m.GetOptions("/objects/pack/pack-{file:[0-9a-f]{40}}.idx", repo.GetIdxFile)
31+
m.Methods("POST,OPTIONS", "/git-upload-pack", repo.ServiceUploadPack)
32+
m.Methods("POST,OPTIONS", "/git-receive-pack", repo.ServiceReceivePack)
33+
m.Methods("GET,OPTIONS", "/info/refs", repo.GetInfoRefs)
34+
m.Methods("GET,OPTIONS", "/HEAD", repo.GetTextFile("HEAD"))
35+
m.Methods("GET,OPTIONS", "/objects/info/alternates", repo.GetTextFile("objects/info/alternates"))
36+
m.Methods("GET,OPTIONS", "/objects/info/http-alternates", repo.GetTextFile("objects/info/http-alternates"))
37+
m.Methods("GET,OPTIONS", "/objects/info/packs", repo.GetInfoPacks)
38+
m.Methods("GET,OPTIONS", "/objects/info/{file:[^/]*}", repo.GetTextFile(""))
39+
m.Methods("GET,OPTIONS", "/objects/{head:[0-9a-f]{2}}/{hash:[0-9a-f]{38}}", repo.GetLooseObject)
40+
m.Methods("GET,OPTIONS", "/objects/pack/pack-{file:[0-9a-f]{40}}.pack", repo.GetPackFile)
41+
m.Methods("GET,OPTIONS", "/objects/pack/pack-{file:[0-9a-f]{40}}.idx", repo.GetIdxFile)
4242
}, ignSignInAndCsrf, requireSignIn, repo.HTTPGitEnabledHandler, repo.CorsHandler(), context_service.UserAssignmentWeb())
4343
}

‎routers/web/misc/misc.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,6 @@ func DummyOK(w http.ResponseWriter, req *http.Request) {
3333
w.WriteHeader(http.StatusOK)
3434
}
3535

36-
func DummyBadRequest(w http.ResponseWriter, req *http.Request) {
37-
w.WriteHeader(http.StatusBadRequest)
38-
}
39-
4036
func RobotsTxt(w http.ResponseWriter, req *http.Request) {
4137
robotsTxt := util.FilePathJoinAbs(setting.CustomPath, "public/robots.txt")
4238
if ok, _ := util.IsExist(robotsTxt); !ok {

‎routers/web/web.go

Lines changed: 31 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,12 @@ const (
5959
GzipMinSize = 1400
6060
)
6161

62-
// CorsHandler return a http handler who set CORS options if enabled by config
63-
func CorsHandler() func(next http.Handler) http.Handler {
62+
// optionsCorsHandler return a http handler which sets CORS options if enabled by config, it blocks non-CORS OPTIONS requests.
63+
func optionsCorsHandler() func(next http.Handler) http.Handler {
64+
var corsHandler func(next http.Handler) http.Handler
6465
if setting.CORSConfig.Enabled {
65-
return cors.Handler(cors.Options{
66-
// Scheme: setting.CORSConfig.Scheme, // FIXME: the cors middleware needs scheme option
67-
AllowedOrigins: setting.CORSConfig.AllowDomain,
68-
// setting.CORSConfig.AllowSubdomain // FIXME: the cors middleware needs allowSubdomain option
66+
corsHandler = cors.Handler(cors.Options{
67+
AllowedOrigins: setting.CORSConfig.AllowDomain,
6968
AllowedMethods: setting.CORSConfig.Methods,
7069
AllowCredentials: setting.CORSConfig.AllowCredentials,
7170
AllowedHeaders: setting.CORSConfig.Headers,
@@ -74,7 +73,23 @@ func CorsHandler() func(next http.Handler) http.Handler {
7473
}
7574

7675
return func(next http.Handler) http.Handler {
77-
return next
76+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
77+
if r.Method == http.MethodOptions {
78+
if corsHandler != nil && r.Header.Get("Access-Control-Request-Method") != "" {
79+
corsHandler(next).ServeHTTP(w, r)
80+
} else {
81+
// 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
82+
w.WriteHeader(http.StatusMethodNotAllowed)
83+
}
84+
return
85+
}
86+
// for non-OPTIONS requests, call the CORS handler to add some related headers like "Vary"
87+
if corsHandler != nil {
88+
corsHandler(next).ServeHTTP(w, r)
89+
} else {
90+
next.ServeHTTP(w, r)
91+
}
92+
})
7893
}
7994
}
8095

@@ -217,7 +232,7 @@ func Routes() *web.Route {
217232
routes := web.NewRoute()
218233

219234
routes.Head("/", misc.DummyOK) // for health check - doesn't need to be passed through gzip handler
220-
routes.Methods("GET, HEAD", "/assets/*", CorsHandler(), public.FileHandlerFunc())
235+
routes.Methods("GET, HEAD, OPTIONS", "/assets/*", optionsCorsHandler(), public.FileHandlerFunc())
221236
routes.Methods("GET, HEAD", "/avatars/*", storageHandler(setting.Avatar.Storage, "avatars", storage.Avatars))
222237
routes.Methods("GET, HEAD", "/repo-avatars/*", storageHandler(setting.RepoAvatar.Storage, "repo-avatars", storage.RepoAvatars))
223238
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) {
457472
m.Get("/change-password", func(ctx *context.Context) {
458473
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
459474
})
460-
m.Any("/*", CorsHandler(), public.FileHandlerFunc())
461-
}, CorsHandler())
475+
m.Methods("GET, HEAD", "/*", public.FileHandlerFunc())
476+
}, optionsCorsHandler())
462477

463478
m.Group("/explore", func() {
464479
m.Get("", func(ctx *context.Context) {
@@ -531,14 +546,11 @@ func registerRoutes(m *web.Route) {
531546
// TODO manage redirection
532547
m.Post("/authorize", web.Bind(forms.AuthorizationForm{}), auth.AuthorizeOAuth)
533548
}, ignSignInAndCsrf, reqSignIn)
534-
m.Options("/login/oauth/userinfo", CorsHandler(), misc.DummyBadRequest)
535-
m.Get("/login/oauth/userinfo", ignSignInAndCsrf, auth.InfoOAuth)
536-
m.Options("/login/oauth/access_token", CorsHandler(), misc.DummyBadRequest)
537-
m.Post("/login/oauth/access_token", CorsHandler(), web.Bind(forms.AccessTokenForm{}), ignSignInAndCsrf, auth.AccessTokenOAuth)
538-
m.Options("/login/oauth/keys", CorsHandler(), misc.DummyBadRequest)
539-
m.Get("/login/oauth/keys", ignSignInAndCsrf, auth.OIDCKeys)
540-
m.Options("/login/oauth/introspect", CorsHandler(), misc.DummyBadRequest)
541-
m.Post("/login/oauth/introspect", CorsHandler(), web.Bind(forms.IntrospectTokenForm{}), ignSignInAndCsrf, auth.IntrospectOAuth)
549+
550+
m.Methods("GET, OPTIONS", "/login/oauth/userinfo", optionsCorsHandler(), ignSignInAndCsrf, auth.InfoOAuth)
551+
m.Methods("POST, OPTIONS", "/login/oauth/access_token", optionsCorsHandler(), web.Bind(forms.AccessTokenForm{}), ignSignInAndCsrf, auth.AccessTokenOAuth)
552+
m.Methods("GET, OPTIONS", "/login/oauth/keys", optionsCorsHandler(), ignSignInAndCsrf, auth.OIDCKeys)
553+
m.Methods("POST, OPTIONS", "/login/oauth/introspect", optionsCorsHandler(), web.Bind(forms.IntrospectTokenForm{}), ignSignInAndCsrf, auth.IntrospectOAuth)
542554

543555
m.Group("/user/settings", func() {
544556
m.Get("", user_setting.Profile)
@@ -768,7 +780,7 @@ func registerRoutes(m *web.Route) {
768780

769781
m.Group("", func() {
770782
m.Get("/{username}", user.UsernameSubRoute)
771-
m.Get("/attachments/{uuid}", repo.GetAttachment)
783+
m.Methods("GET, OPTIONS", "/attachments/{uuid}", optionsCorsHandler(), repo.GetAttachment)
772784
}, ignSignIn)
773785

774786
m.Post("/{username}", reqSignIn, context_service.UserAssignmentWeb(), user.Action)

0 commit comments

Comments
 (0)