From eca72769c3736fbe733fae0bfd6437c9ff21bf34 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 5 Apr 2021 21:13:03 +0100 Subject: [PATCH 1/6] Move FCGI req.URL.Path fix-up to the FCGI listener Simplify the web.go FCGI path by moving the req.URL.Path fix-up to listener Signed-off-by: Andrew Thornton --- cmd/web_graceful.go | 9 ++++++++- routers/routes/web.go | 9 --------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/cmd/web_graceful.go b/cmd/web_graceful.go index 5db065818a19f..91ac024ddb42f 100644 --- a/cmd/web_graceful.go +++ b/cmd/web_graceful.go @@ -9,9 +9,11 @@ import ( "net" "net/http" "net/http/fcgi" + "strings" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" ) func runHTTP(network, listenAddr, name string, m http.Handler) error { @@ -48,7 +50,12 @@ func runFCGI(network, listenAddr, name string, m http.Handler) error { fcgiServer := graceful.NewServer(network, listenAddr, name) err := fcgiServer.ListenAndServe(func(listener net.Listener) error { - return fcgi.Serve(listener, m) + return fcgi.Serve(listener, http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { + if setting.AppSubURL != "" { + req.URL.Path = strings.TrimPrefix(req.URL.Path, setting.AppSubURL) + } + m.ServeHTTP(resp, req) + })) }) if err != nil { log.Fatal("Failed to start FCGI main server: %v", err) diff --git a/routers/routes/web.go b/routers/routes/web.go index 5b382ecccba9b..a6d2311d1a981 100644 --- a/routers/routes/web.go +++ b/routers/routes/web.go @@ -168,15 +168,6 @@ func WebRoutes() *web.Route { r.Use(h) } - if (setting.Protocol == setting.FCGI || setting.Protocol == setting.FCGIUnix) && setting.AppSubURL != "" { - r.Use(func(next http.Handler) http.Handler { - return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - req.URL.Path = strings.TrimPrefix(req.URL.Path, setting.AppSubURL) - next.ServeHTTP(resp, req) - }) - }) - } - mailer.InitMailRender(templates.Mailer()) if setting.Service.EnableCaptcha { From 50b9f77aacf27188791795a82345e3cd6c0f2f90 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 6 Apr 2021 09:05:43 +0100 Subject: [PATCH 2/6] move access logger to common as it is a logger Signed-off-by: Andrew Thornton --- routers/routes/web.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/routers/routes/web.go b/routers/routes/web.go index a6d2311d1a981..e15bc1c4bfedf 100644 --- a/routers/routes/web.go +++ b/routers/routes/web.go @@ -89,6 +89,9 @@ func commonMiddlewares() []func(http.Handler) http.Handler { handlers = append(handlers, LoggerHandler(setting.RouterLogLevel)) } } + if setting.EnableAccessLog { + handlers = append(handlers, context.AccessLogger()) + } handlers = append(handlers, func(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { @@ -178,10 +181,6 @@ func WebRoutes() *web.Route { // GetHead allows a HEAD request redirect to GET if HEAD method is not defined for that route r.Use(middleware.GetHead) - if setting.EnableAccessLog { - r.Use(context.AccessLogger()) - } - r.Use(user.GetNotificationCount) r.Use(repo.GetActiveStopwatch) r.Use(func(ctx *context.Context) { From 83dc5c033e9196eb5791e69828cd5d54d287ef5d Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 6 Apr 2021 09:30:36 +0100 Subject: [PATCH 3/6] Use routes.Route not routes.Use to reduce the number of calls on the stack Signed-off-by: Andrew Thornton --- modules/context/context.go | 6 +++ routers/routes/web.go | 88 ++++++++++++++++++++------------------ 2 files changed, 52 insertions(+), 42 deletions(-) diff --git a/modules/context/context.go b/modules/context/context.go index b876487d5e004..e5feb546348bc 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -692,6 +692,7 @@ func Contexter() func(next http.Handler) http.Handler { log.Debug("Session ID: %s", ctx.Session.ID()) log.Debug("CSRF Token: %v", ctx.Data["CsrfToken"]) + // FIXME: do we really always need these setting? There should be someway to have to avoid having to always set these ctx.Data["IsLandingPageHome"] = setting.LandingPageURL == setting.LandingPageHome ctx.Data["IsLandingPageExplore"] = setting.LandingPageURL == setting.LandingPageExplore ctx.Data["IsLandingPageOrganizations"] = setting.LandingPageURL == setting.LandingPageOrganizations @@ -707,6 +708,11 @@ func Contexter() func(next http.Handler) http.Handler { ctx.Data["ManifestData"] = setting.ManifestData + ctx.Data["UnitWikiGlobalDisabled"] = models.UnitTypeWiki.UnitGlobalDisabled() + ctx.Data["UnitIssuesGlobalDisabled"] = models.UnitTypeIssues.UnitGlobalDisabled() + ctx.Data["UnitPullsGlobalDisabled"] = models.UnitTypePullRequests.UnitGlobalDisabled() + ctx.Data["UnitProjectsGlobalDisabled"] = models.UnitTypeProjects.UnitGlobalDisabled() + ctx.Data["i18n"] = locale ctx.Data["Tr"] = i18n.Tr ctx.Data["Lang"] = locale.Language() diff --git a/routers/routes/web.go b/routers/routes/web.go index e15bc1c4bfedf..4089aa3a33ce9 100644 --- a/routers/routes/web.go +++ b/routers/routes/web.go @@ -30,7 +30,6 @@ import ( "code.gitea.io/gitea/routers" "code.gitea.io/gitea/routers/admin" apiv1 "code.gitea.io/gitea/routers/api/v1" - "code.gitea.io/gitea/routers/api/v1/misc" "code.gitea.io/gitea/routers/dev" "code.gitea.io/gitea/routers/events" "code.gitea.io/gitea/routers/org" @@ -131,9 +130,9 @@ func NormalRoutes() *web.Route { // WebRoutes returns all web routes func WebRoutes() *web.Route { - r := web.NewRoute() + routes := web.NewRoute() - r.Use(session.Sessioner(session.Options{ + routes.Use(session.Sessioner(session.Options{ Provider: setting.SessionConfig.Provider, ProviderConfig: setting.SessionConfig.ProviderConfig, CookieName: setting.SessionConfig.CookieName, @@ -144,88 +143,93 @@ func WebRoutes() *web.Route { Domain: setting.SessionConfig.Domain, })) - r.Use(Recovery()) + routes.Use(Recovery()) - r.Use(public.Custom( + // TODO: we should consider if there is a way to mount these using r.Route as at present + // these two handlers mean that every request has to hit these "filesystems" twice + // before finally getting to the router. It allows them to override any matching router below. + routes.Use(public.Custom( &public.Options{ SkipLogging: setting.DisableRouterLog, }, )) - r.Use(public.Static( + routes.Use(public.Static( &public.Options{ Directory: path.Join(setting.StaticRootPath, "public"), SkipLogging: setting.DisableRouterLog, }, )) - r.Use(storageHandler(setting.Avatar.Storage, "avatars", storage.Avatars)) - r.Use(storageHandler(setting.RepoAvatar.Storage, "repo-avatars", storage.RepoAvatars)) + // We use r.Route here over r.Use because this prevents requests that are not for avatars having to go through this additional handler + routes.Route("/avatars", "GET, HEAD", storageHandler(setting.Avatar.Storage, "avatars", storage.Avatars)) + routes.Route("/repo-avatars", "GET, HEAD", storageHandler(setting.RepoAvatar.Storage, "repo-avatars", storage.RepoAvatars)) + + // for health check - doeesn't need to be passed through gzip handler + routes.Head("/", func(w http.ResponseWriter, req *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + // this png is very likely to always be below the limit for gzip so it doesn't need to pass through gzip + routes.Get("/apple-touch-icon.png", func(w http.ResponseWriter, req *http.Request) { + http.Redirect(w, req, path.Join(setting.StaticURLPrefix, "img/apple-touch-icon.png"), 301) + }) gob.Register(&u2f.Challenge{}) + common := []interface{}{} + if setting.EnableGzip { h, err := gziphandler.GzipHandlerWithOpts(gziphandler.MinSize(GzipMinSize)) if err != nil { log.Fatal("GzipHandlerWithOpts failed: %v", err) } - r.Use(h) + common = append(common, h) } mailer.InitMailRender(templates.Mailer()) if setting.Service.EnableCaptcha { - r.Use(captcha.Captchaer(context.GetImageCaptcha())) + // The captcha http.Handler should only fire on /captcha/* so we can just mount this on that url + routes.Route("/captcha/*", "GET,HEAD", append(common, captcha.Captchaer(context.GetImageCaptcha()))...) } - // Removed: toolbox.Toolboxer middleware will provide debug informations which seems unnecessary - r.Use(context.Contexter()) - // GetHead allows a HEAD request redirect to GET if HEAD method is not defined for that route - r.Use(middleware.GetHead) - - r.Use(user.GetNotificationCount) - r.Use(repo.GetActiveStopwatch) - r.Use(func(ctx *context.Context) { - ctx.Data["UnitWikiGlobalDisabled"] = models.UnitTypeWiki.UnitGlobalDisabled() - ctx.Data["UnitIssuesGlobalDisabled"] = models.UnitTypeIssues.UnitGlobalDisabled() - ctx.Data["UnitPullsGlobalDisabled"] = models.UnitTypePullRequests.UnitGlobalDisabled() - ctx.Data["UnitProjectsGlobalDisabled"] = models.UnitTypeProjects.UnitGlobalDisabled() - }) - - // for health check - r.Head("/", func(w http.ResponseWriter, req *http.Request) { - w.WriteHeader(http.StatusOK) - }) if setting.HasRobotsTxt { - r.Get("/robots.txt", func(w http.ResponseWriter, req *http.Request) { + routes.Get("/robots.txt", append(common, func(w http.ResponseWriter, req *http.Request) { filePath := path.Join(setting.CustomPath, "robots.txt") fi, err := os.Stat(filePath) if err == nil && httpcache.HandleTimeCache(req, w, fi) { return } http.ServeFile(w, req, filePath) - }) + })...) } - r.Get("/apple-touch-icon.png", func(w http.ResponseWriter, req *http.Request) { - http.Redirect(w, req, path.Join(setting.StaticURLPrefix, "img/apple-touch-icon.png"), 301) - }) - - // prometheus metrics endpoint + // prometheus metrics endpoint - do not need to go through contexter if setting.Metrics.Enabled { c := metrics.NewCollector() prometheus.MustRegister(c) - r.Get("/metrics", routers.Metrics) + routes.Get("/metrics", append(common, routers.Metrics)...) } - if setting.API.EnableSwagger { - // Note: The route moved from apiroutes because it's in fact want to render a web page - r.Get("/api/swagger", misc.Swagger) // Render V1 by default - } + // Removed: toolbox.Toolboxer middleware will provide debug informations which seems unnecessary + common = append(common, context.Contexter()) - RegisterRoutes(r) + // GetHead allows a HEAD request redirect to GET if HEAD method is not defined for that route + common = append(common, middleware.GetHead) - return r + // TODO: These really seem like things that could be folded into Contexter or as helper functions + common = append(common, user.GetNotificationCount) + common = append(common, repo.GetActiveStopwatch) + + others := web.NewRoute() + for _, middle := range common { + others.Use(middle) + } + + RegisterRoutes(others) + routes.Mount("", others) + return routes } func goGet(ctx *context.Context) { From ed44d2e997cc56927465cc98be8619e74e703ec6 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 6 Apr 2021 10:01:02 +0100 Subject: [PATCH 4/6] remove duplicate access log call Signed-off-by: Andrew Thornton --- routers/api/v1/api.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 88355fb2b34ed..b46f7e159f352 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -572,10 +572,6 @@ func Routes() *web.Route { } m.Use(context.APIContexter()) - if setting.EnableAccessLog { - m.Use(context.AccessLogger()) - } - m.Use(context.ToggleAPI(&context.ToggleOptions{ SignInRequired: setting.Service.RequireSignInView, })) From 56ff032e84fc9eb51f22bdd77ecd741a3affa73f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 7 Apr 2021 09:48:21 +0100 Subject: [PATCH 5/6] add api/swagger back in Signed-off-by: Andrew Thornton --- routers/routes/web.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/routers/routes/web.go b/routers/routes/web.go index 2c507d7374c11..42e2bb55065a6 100644 --- a/routers/routes/web.go +++ b/routers/routes/web.go @@ -29,6 +29,7 @@ import ( "code.gitea.io/gitea/routers" "code.gitea.io/gitea/routers/admin" apiv1 "code.gitea.io/gitea/routers/api/v1" + "code.gitea.io/gitea/routers/api/v1/misc" "code.gitea.io/gitea/routers/dev" "code.gitea.io/gitea/routers/events" "code.gitea.io/gitea/routers/org" @@ -218,6 +219,11 @@ func WebRoutes() *web.Route { // GetHead allows a HEAD request redirect to GET if HEAD method is not defined for that route common = append(common, middleware.GetHead) + if setting.API.EnableSwagger { + // Note: The route moved from apiroutes because it's in fact want to render a web page + r.Get("/api/swagger", append(common, misc.Swagger)...) // Render V1 by default + } + // TODO: These really seem like things that could be folded into Contexter or as helper functions common = append(common, user.GetNotificationCount) common = append(common, repo.GetActiveStopwatch) From d9e1432f75d852f35ce3a23c9a98511e98f150f6 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 7 Apr 2021 09:51:21 +0100 Subject: [PATCH 6/6] add api/swagger back in Signed-off-by: Andrew Thornton --- routers/routes/web.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/routes/web.go b/routers/routes/web.go index 42e2bb55065a6..588787b010ce8 100644 --- a/routers/routes/web.go +++ b/routers/routes/web.go @@ -221,7 +221,7 @@ func WebRoutes() *web.Route { if setting.API.EnableSwagger { // Note: The route moved from apiroutes because it's in fact want to render a web page - r.Get("/api/swagger", append(common, misc.Swagger)...) // Render V1 by default + routes.Get("/api/swagger", append(common, misc.Swagger)...) // Render V1 by default } // TODO: These really seem like things that could be folded into Contexter or as helper functions