Skip to content

Commit d88b98c

Browse files
wxiaoguangkatsusan
wxiaoguang
authored and
katsusan
committed
Refactor CORS handler (go-gitea#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 go-gitea#28515 Fix go-gitea#27642 Fix go-gitea#17098
1 parent d0f24ff commit d88b98c

File tree

11 files changed

+131
-78
lines changed

11 files changed

+131
-78
lines changed

‎custom/conf/app.example.ini

+1-7
Original file line numberDiff line numberDiff line change
@@ -1158,15 +1158,9 @@ LEVEL = Info
11581158
;; enable cors headers (disabled by default)
11591159
;ENABLED = false
11601160
;;
1161-
;; scheme of allowed requests
1162-
;SCHEME = http
1163-
;;
1164-
;; list of requesting domains that are allowed
1161+
;; list of requesting origins that are allowed, eg: "https://*.example.com"
11651162
;ALLOW_DOMAIN = *
11661163
;;
1167-
;; allow subdomains of headers listed above to request
1168-
;ALLOW_SUBDOMAIN = false
1169-
;;
11701164
;; list of methods allowed to request
11711165
;METHODS = GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS
11721166
;;

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

+1-3
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

-2
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

+1-1
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

+1-3
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

+6-18
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

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

‎routers/web/githttp.go

+11-11
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

-4
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

+31-19
Original file line numberDiff line numberDiff line change
@@ -60,13 +60,12 @@ const (
6060
GzipMinSize = 1400
6161
)
6262

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

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

@@ -218,7 +233,7 @@ func Routes() *web.Route {
218233
routes := web.NewRoute()
219234

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

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

544556
m.Group("/user/settings", func() {
545557
m.Get("", user_setting.Profile)
@@ -770,7 +782,7 @@ func registerRoutes(m *web.Route) {
770782

771783
m.Group("", func() {
772784
m.Get("/{username}", user.UsernameSubRoute)
773-
m.Get("/attachments/{uuid}", repo.GetAttachment)
785+
m.Methods("GET, OPTIONS", "/attachments/{uuid}", optionsCorsHandler(), repo.GetAttachment)
774786
}, ignSignIn)
775787

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

‎tests/integration/cors_test.go

+78-7
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,88 @@ import (
77
"net/http"
88
"testing"
99

10+
"code.gitea.io/gitea/modules/setting"
11+
"code.gitea.io/gitea/modules/test"
12+
"code.gitea.io/gitea/routers"
1013
"code.gitea.io/gitea/tests"
1114

1215
"github.com/stretchr/testify/assert"
1316
)
1417

15-
func TestCORSNotSet(t *testing.T) {
18+
func TestCORS(t *testing.T) {
1619
defer tests.PrepareTestEnv(t)()
17-
req := NewRequest(t, "GET", "/api/v1/version")
18-
session := loginUser(t, "user2")
19-
resp := session.MakeRequest(t, req, http.StatusOK)
20-
assert.Equal(t, resp.Code, http.StatusOK)
21-
corsHeader := resp.Header().Get("Access-Control-Allow-Origin")
22-
assert.Empty(t, corsHeader, "Access-Control-Allow-Origin: generated header should match") // header not set
20+
t.Run("CORS enabled", func(t *testing.T) {
21+
defer test.MockVariableValue(&setting.CORSConfig.Enabled, true)()
22+
defer test.MockVariableValue(&testWebRoutes, routers.NormalRoutes())()
23+
24+
t.Run("API with CORS", func(t *testing.T) {
25+
// GET api with no CORS header
26+
req := NewRequest(t, "GET", "/api/v1/version")
27+
resp := MakeRequest(t, req, http.StatusOK)
28+
assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin"))
29+
assert.Contains(t, resp.Header().Values("Vary"), "Origin")
30+
31+
// OPTIONS api for CORS
32+
req = NewRequest(t, "OPTIONS", "/api/v1/version").
33+
SetHeader("Origin", "https://example.com").
34+
SetHeader("Access-Control-Request-Method", "GET")
35+
resp = MakeRequest(t, req, http.StatusOK)
36+
assert.NotEmpty(t, resp.Header().Get("Access-Control-Allow-Origin"))
37+
assert.Contains(t, resp.Header().Values("Vary"), "Origin")
38+
})
39+
40+
t.Run("Web with CORS", func(t *testing.T) {
41+
// GET userinfo with no CORS header
42+
req := NewRequest(t, "GET", "/#/oauth/userinfo")
43+
resp := MakeRequest(t, req, http.StatusUnauthorized)
44+
assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin"))
45+
assert.Contains(t, resp.Header().Values("Vary"), "Origin")
46+
47+
// OPTIONS userinfo for CORS
48+
req = NewRequest(t, "OPTIONS", "/#/oauth/userinfo").
49+
SetHeader("Origin", "https://example.com").
50+
SetHeader("Access-Control-Request-Method", "GET")
51+
resp = MakeRequest(t, req, http.StatusOK)
52+
assert.NotEmpty(t, resp.Header().Get("Access-Control-Allow-Origin"))
53+
assert.Contains(t, resp.Header().Values("Vary"), "Origin")
54+
55+
// OPTIONS userinfo for non-CORS
56+
req = NewRequest(t, "OPTIONS", "/#/oauth/userinfo")
57+
resp = MakeRequest(t, req, http.StatusMethodNotAllowed)
58+
assert.NotContains(t, resp.Header().Values("Vary"), "Origin")
59+
})
60+
})
61+
62+
t.Run("CORS disabled", func(t *testing.T) {
63+
defer test.MockVariableValue(&setting.CORSConfig.Enabled, false)()
64+
defer test.MockVariableValue(&testWebRoutes, routers.NormalRoutes())()
65+
66+
t.Run("API without CORS", func(t *testing.T) {
67+
req := NewRequest(t, "GET", "/api/v1/version")
68+
resp := MakeRequest(t, req, http.StatusOK)
69+
assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin"))
70+
assert.Empty(t, resp.Header().Values("Vary"))
71+
72+
req = NewRequest(t, "OPTIONS", "/api/v1/version").
73+
SetHeader("Origin", "https://example.com").
74+
SetHeader("Access-Control-Request-Method", "GET")
75+
resp = MakeRequest(t, req, http.StatusMethodNotAllowed)
76+
assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin"))
77+
assert.Empty(t, resp.Header().Values("Vary"))
78+
})
79+
80+
t.Run("Web without CORS", func(t *testing.T) {
81+
req := NewRequest(t, "GET", "/#/oauth/userinfo")
82+
resp := MakeRequest(t, req, http.StatusUnauthorized)
83+
assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin"))
84+
assert.NotContains(t, resp.Header().Values("Vary"), "Origin")
85+
86+
req = NewRequest(t, "OPTIONS", "/#/oauth/userinfo").
87+
SetHeader("Origin", "https://example.com").
88+
SetHeader("Access-Control-Request-Method", "GET")
89+
resp = MakeRequest(t, req, http.StatusMethodNotAllowed)
90+
assert.Empty(t, resp.Header().Get("Access-Control-Allow-Origin"))
91+
assert.NotContains(t, resp.Header().Values("Vary"), "Origin")
92+
})
93+
})
2394
}

0 commit comments

Comments
 (0)