From effad26c0e7348d27f7fdd1c0cbf120d7558cf67 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 30 May 2021 18:25:11 +0800 Subject: [PATCH] Improve assets handler middleware (#15961) * Use route to serve assets but not middleware * Fix build error with bindata tag * convert path to absolute * fix build * reduce function stack * Add tests for assets * Remove test for assets because they are not generated * Use a http function to serve assets * Still use middleware to serve assets then less middleware stack for assets * Move serveContent to original position * remove unnecessary blank line change * Fix bug for /assets* requests * clean code Co-authored-by: zeripath --- integrations/links_test.go | 2 + modules/public/dynamic.go | 9 +- modules/public/public.go | 179 ++++++++++++++----------------------- modules/public/static.go | 14 ++- routers/routes/install.go | 19 ++-- routers/routes/web.go | 52 +++++------ 6 files changed, 108 insertions(+), 167 deletions(-) diff --git a/integrations/links_test.go b/integrations/links_test.go index 3b9c245fc..03229e10e 100644 --- a/integrations/links_test.go +++ b/integrations/links_test.go @@ -35,6 +35,8 @@ func TestLinksNoLogin(t *testing.T) { "/user2/repo1", "/user2/repo1/projects", "/user2/repo1/projects/1", + "/assets/img/404.png", + "/assets/img/500.png", } for _, link := range links { diff --git a/modules/public/dynamic.go b/modules/public/dynamic.go index a57b63636..0bfe38bc3 100644 --- a/modules/public/dynamic.go +++ b/modules/public/dynamic.go @@ -13,12 +13,11 @@ import ( "time" ) -// Static implements the static handler for serving assets. -func Static(opts *Options) func(next http.Handler) http.Handler { - return opts.staticHandler(opts.Directory) +func fileSystem(dir string) http.FileSystem { + return http.Dir(dir) } -// ServeContent serve http content -func ServeContent(w http.ResponseWriter, req *http.Request, fi os.FileInfo, modtime time.Time, content io.ReadSeeker) { +// serveContent serve http content +func serveContent(w http.ResponseWriter, req *http.Request, fi os.FileInfo, modtime time.Time, content io.ReadSeeker) { http.ServeContent(w, req, fi.Name(), modtime, content) } diff --git a/modules/public/public.go b/modules/public/public.go index c68f98035..a58709d86 100644 --- a/modules/public/public.go +++ b/modules/public/public.go @@ -5,85 +5,82 @@ package public import ( - "log" "net/http" + "os" "path" "path/filepath" "strings" "code.gitea.io/gitea/modules/httpcache" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" ) // Options represents the available options to configure the handler. type Options struct { Directory string - IndexFile string - SkipLogging bool - FileSystem http.FileSystem Prefix string + CorsHandler func(http.Handler) http.Handler } -// KnownPublicEntries list all direct children in the `public` directory -var KnownPublicEntries = []string{ - "css", - "fonts", - "img", - "js", - "serviceworker.js", - "vendor", -} - -// Custom implements the static handler for serving custom assets. -func Custom(opts *Options) func(next http.Handler) http.Handler { - return opts.staticHandler(path.Join(setting.CustomPath, "public")) -} - -// staticFileSystem implements http.FileSystem interface. -type staticFileSystem struct { - dir *http.Dir -} - -func newStaticFileSystem(directory string) staticFileSystem { - if !filepath.IsAbs(directory) { - directory = filepath.Join(setting.AppWorkPath, directory) +// AssetsHandler implements the static handler for serving custom or original assets. +func AssetsHandler(opts *Options) func(next http.Handler) http.Handler { + var custPath = filepath.Join(setting.CustomPath, "public") + if !filepath.IsAbs(custPath) { + custPath = filepath.Join(setting.AppWorkPath, custPath) + } + if !filepath.IsAbs(opts.Directory) { + opts.Directory = filepath.Join(setting.AppWorkPath, opts.Directory) + } + if !strings.HasSuffix(opts.Prefix, "/") { + opts.Prefix += "/" } - dir := http.Dir(directory) - return staticFileSystem{&dir} -} -func (fs staticFileSystem) Open(name string) (http.File, error) { - return fs.dir.Open(name) -} - -// StaticHandler sets up a new middleware for serving static files in the -func StaticHandler(dir string, opts *Options) func(next http.Handler) http.Handler { - return opts.staticHandler(dir) -} - -func (opts *Options) staticHandler(dir string) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { - // Defaults - if len(opts.IndexFile) == 0 { - opts.IndexFile = "index.html" - } - // Normalize the prefix if provided - if opts.Prefix != "" { - // Ensure we have a leading '/' - if opts.Prefix[0] != '/' { - opts.Prefix = "/" + opts.Prefix + return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { + if !strings.HasPrefix(req.URL.Path, opts.Prefix) { + next.ServeHTTP(resp, req) + return + } + if req.Method != "GET" && req.Method != "HEAD" { + resp.WriteHeader(http.StatusNotFound) + return } - // Remove any trailing '/' - opts.Prefix = strings.TrimRight(opts.Prefix, "/") - } - if opts.FileSystem == nil { - opts.FileSystem = newStaticFileSystem(dir) - } - return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - if !opts.handle(w, req, opts) { - next.ServeHTTP(w, req) + file := req.URL.Path + file = file[len(opts.Prefix):] + if len(file) == 0 { + resp.WriteHeader(http.StatusNotFound) + return } + if strings.Contains(file, "\\") { + resp.WriteHeader(http.StatusBadRequest) + return + } + file = "/" + file + + var written bool + if opts.CorsHandler != nil { + written = true + opts.CorsHandler(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + written = false + })).ServeHTTP(resp, req) + } + if written { + return + } + + // custom files + if opts.handle(resp, req, http.Dir(custPath), file) { + return + } + + // internal files + if opts.handle(resp, req, fileSystem(opts.Directory), file) { + return + } + + resp.WriteHeader(http.StatusNotFound) }) } } @@ -98,76 +95,36 @@ func parseAcceptEncoding(val string) map[string]bool { return types } -func (opts *Options) handle(w http.ResponseWriter, req *http.Request, opt *Options) bool { - if req.Method != "GET" && req.Method != "HEAD" { - return false - } - - file := req.URL.Path - // if we have a prefix, filter requests by stripping the prefix - if opt.Prefix != "" { - if !strings.HasPrefix(file, opt.Prefix) { - return false - } - file = file[len(opt.Prefix):] - if file != "" && file[0] != '/' { - return false - } - } - - f, err := opt.FileSystem.Open(file) +func (opts *Options) handle(w http.ResponseWriter, req *http.Request, fs http.FileSystem, file string) bool { + // use clean to keep the file is a valid path with no . or .. + f, err := fs.Open(path.Clean(file)) if err != nil { - // 404 requests to any known entries in `public` - if path.Base(opts.Directory) == "public" { - parts := strings.Split(file, "/") - if len(parts) < 2 { - return false - } - for _, entry := range KnownPublicEntries { - if entry == parts[1] { - w.WriteHeader(404) - return true - } - } + if os.IsNotExist(err) { + return false } - return false + w.WriteHeader(http.StatusInternalServerError) + log.Error("[Static] Open %q failed: %v", file, err) + return true } defer f.Close() fi, err := f.Stat() if err != nil { - log.Printf("[Static] %q exists, but fails to open: %v", file, err) + w.WriteHeader(http.StatusInternalServerError) + log.Error("[Static] %q exists, but fails to open: %v", file, err) return true } // Try to serve index file if fi.IsDir() { - // Redirect if missing trailing slash. - if !strings.HasSuffix(req.URL.Path, "/") { - http.Redirect(w, req, path.Clean(req.URL.Path+"/"), http.StatusFound) - return true - } - - f, err = opt.FileSystem.Open(file) - if err != nil { - return false // Discard error. - } - defer f.Close() - - fi, err = f.Stat() - if err != nil || fi.IsDir() { - return false - } - } - - if !opt.SkipLogging { - log.Println("[Static] Serving " + file) + w.WriteHeader(http.StatusNotFound) + return true } if httpcache.HandleFileETagCache(req, w, fi) { return true } - ServeContent(w, req, fi, fi.ModTime(), f) + serveContent(w, req, fi, fi.ModTime(), f) return true } diff --git a/modules/public/static.go b/modules/public/static.go index 36cfdbe44..827dc2a1e 100644 --- a/modules/public/static.go +++ b/modules/public/static.go @@ -20,12 +20,8 @@ import ( "code.gitea.io/gitea/modules/log" ) -// Static implements the static handler for serving assets. -func Static(opts *Options) func(next http.Handler) http.Handler { - opts.FileSystem = Assets - // we don't need to pass the directory, because the directory var is only - // used when in the options there is no FileSystem. - return opts.staticHandler("") +func fileSystem(dir string) http.FileSystem { + return Assets } func Asset(name string) ([]byte, error) { @@ -59,8 +55,8 @@ func AssetIsDir(name string) (bool, error) { } } -// ServeContent serve http content -func ServeContent(w http.ResponseWriter, req *http.Request, fi os.FileInfo, modtime time.Time, content io.ReadSeeker) { +// serveContent serve http content +func serveContent(w http.ResponseWriter, req *http.Request, fi os.FileInfo, modtime time.Time, content io.ReadSeeker) { encodings := parseAcceptEncoding(req.Header.Get("Accept-Encoding")) if encodings["gzip"] { if cf, ok := fi.(*vfsgen۰CompressedFileInfo); ok { @@ -76,7 +72,7 @@ func ServeContent(w http.ResponseWriter, req *http.Request, fi os.FileInfo, modt _, err := rd.Seek(0, io.SeekStart) // rewind to output whole file if err != nil { log.Error("rd.Seek error: %v", err) - http.Error(w, http.StatusText(500), 500) + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) return } } diff --git a/routers/routes/install.go b/routers/routes/install.go index 026d92b13..18e74f005 100644 --- a/routers/routes/install.go +++ b/routers/routes/install.go @@ -81,6 +81,11 @@ func InstallRoutes() *web.Route { r.Use(middle) } + r.Use(public.AssetsHandler(&public.Options{ + Directory: path.Join(setting.StaticRootPath, "public"), + Prefix: "/assets", + })) + r.Use(session.Sessioner(session.Options{ Provider: setting.SessionConfig.Provider, ProviderConfig: setting.SessionConfig.ProviderConfig, @@ -93,20 +98,6 @@ func InstallRoutes() *web.Route { })) r.Use(installRecovery()) - - r.Use(public.Custom( - &public.Options{ - SkipLogging: setting.DisableRouterLog, - }, - )) - r.Use(public.Static( - &public.Options{ - Directory: path.Join(setting.StaticRootPath, "public"), - SkipLogging: setting.DisableRouterLog, - Prefix: "/assets", - }, - )) - r.Use(routers.InstallInit) r.Get("/", routers.Install) r.Post("/", web.Bind(forms.InstallForm{}), routers.InstallPost) diff --git a/routers/routes/web.go b/routers/routes/web.go index 008c745d6..cc65ad6d9 100644 --- a/routers/routes/web.go +++ b/routers/routes/web.go @@ -113,6 +113,8 @@ func commonMiddlewares() []func(http.Handler) http.Handler { return handlers } +var corsHandler func(http.Handler) http.Handler + // NormalRoutes represents non install routes func NormalRoutes() *web.Route { r := web.NewRoute() @@ -120,6 +122,21 @@ func NormalRoutes() *web.Route { r.Use(middle) } + if setting.CORSConfig.Enabled { + corsHandler = 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 + AllowedMethods: setting.CORSConfig.Methods, + AllowCredentials: setting.CORSConfig.AllowCredentials, + MaxAge: int(setting.CORSConfig.MaxAge.Seconds()), + }) + } else { + corsHandler = func(next http.Handler) http.Handler { + return next + } + } + r.Mount("/", WebRoutes()) r.Mount("/api/v1", apiv1.Routes()) r.Mount("/api/internal", private.Routes()) @@ -130,6 +147,12 @@ func NormalRoutes() *web.Route { func WebRoutes() *web.Route { routes := web.NewRoute() + routes.Use(public.AssetsHandler(&public.Options{ + Directory: path.Join(setting.StaticRootPath, "public"), + Prefix: "/assets", + CorsHandler: corsHandler, + })) + routes.Use(session.Sessioner(session.Options{ Provider: setting.SessionConfig.Provider, ProviderConfig: setting.SessionConfig.ProviderConfig, @@ -143,22 +166,6 @@ func WebRoutes() *web.Route { routes.Use(Recovery()) - // 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, - }, - )) - routes.Use(public.Static( - &public.Options{ - Directory: path.Join(setting.StaticRootPath, "public"), - SkipLogging: setting.DisableRouterLog, - Prefix: "/assets", - }, - )) - // 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)) @@ -348,18 +355,7 @@ func RegisterRoutes(m *web.Route) { m.Post("/authorize", bindIgnErr(forms.AuthorizationForm{}), user.AuthorizeOAuth) }, ignSignInAndCsrf, reqSignIn) m.Get("/login/oauth/userinfo", ignSignInAndCsrf, user.InfoOAuth) - if setting.CORSConfig.Enabled { - m.Post("/login/oauth/access_token", 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 - AllowedMethods: setting.CORSConfig.Methods, - AllowCredentials: setting.CORSConfig.AllowCredentials, - MaxAge: int(setting.CORSConfig.MaxAge.Seconds()), - }), bindIgnErr(forms.AccessTokenForm{}), ignSignInAndCsrf, user.AccessTokenOAuth) - } else { - m.Post("/login/oauth/access_token", bindIgnErr(forms.AccessTokenForm{}), ignSignInAndCsrf, user.AccessTokenOAuth) - } + m.Post("/login/oauth/access_token", corsHandler, bindIgnErr(forms.AccessTokenForm{}), ignSignInAndCsrf, user.AccessTokenOAuth) m.Group("/user/settings", func() { m.Get("", userSetting.Profile)