From 7b79be24ca93cbc4da70891c25b3e82b1815267e Mon Sep 17 00:00:00 2001 From: Sergey Zolotarev Date: Fri, 6 Oct 2023 13:23:14 +0000 Subject: [PATCH] Fix panic in storageHandler (#27446) storageHandler() is written as a middleware but is used as an endpoint handler, and thus `next` is actually `nil`, which causes a null pointer dereference when a request URL does not match the pattern (where it calls `next.ServerHTTP()`). Example CURL command to trigger the panic: ``` curl -I "http://yourhost/gitea//avatars/a" ``` Fixes #27409 --- Note: the diff looks big but it's actually a small change - all I did was to remove the outer closure (and one level of indentation) ~and removed the HTTP method and pattern checks as they seem redundant because go-chi already does those checks~. You might want to check "Hide whitespace" when reviewing it. Alternative solution (a bit simpler): append `, misc.DummyOK` to the route declarations that utilize `storageHandler()` - this makes it return an empty response when the URL is invalid. I've tested this one and it works too. Or maybe it would be better to return a 400 error in that case (?) --- routers/web/base.go | 101 ++++++++++++++++++++++---------------------- 1 file changed, 50 insertions(+), 51 deletions(-) diff --git a/routers/web/base.go b/routers/web/base.go index e4b7d8ce8..78dde57fa 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -19,81 +19,80 @@ import ( "code.gitea.io/gitea/modules/web/routing" ) -func storageHandler(storageSetting *setting.Storage, prefix string, objStore storage.ObjectStorage) func(next http.Handler) http.Handler { +func storageHandler(storageSetting *setting.Storage, prefix string, objStore storage.ObjectStorage) http.HandlerFunc { prefix = strings.Trim(prefix, "/") funcInfo := routing.GetFuncInfo(storageHandler, prefix) - return func(next http.Handler) http.Handler { - if storageSetting.MinioConfig.ServeDirect { - return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - if req.Method != "GET" && req.Method != "HEAD" { - next.ServeHTTP(w, req) - return - } - - if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { - next.ServeHTTP(w, req) - return - } - routing.UpdateFuncInfo(req.Context(), funcInfo) - - rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/") - rPath = util.PathJoinRelX(rPath) - - u, err := objStore.URL(rPath, path.Base(rPath)) - if err != nil { - if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) { - log.Warn("Unable to find %s %s", prefix, rPath) - http.Error(w, "file not found", http.StatusNotFound) - return - } - log.Error("Error whilst getting URL for %s %s. Error: %v", prefix, rPath, err) - http.Error(w, fmt.Sprintf("Error whilst getting URL for %s %s", prefix, rPath), http.StatusInternalServerError) - return - } - - http.Redirect(w, req, u.String(), http.StatusTemporaryRedirect) - }) - } + if storageSetting.MinioConfig.ServeDirect { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { if req.Method != "GET" && req.Method != "HEAD" { - next.ServeHTTP(w, req) + http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed) return } if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { - next.ServeHTTP(w, req) + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) return } routing.UpdateFuncInfo(req.Context(), funcInfo) rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/") rPath = util.PathJoinRelX(rPath) - if rPath == "" || rPath == "." { - http.Error(w, "file not found", http.StatusNotFound) - return - } - fi, err := objStore.Stat(rPath) + u, err := objStore.URL(rPath, path.Base(rPath)) if err != nil { if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) { log.Warn("Unable to find %s %s", prefix, rPath) - http.Error(w, "file not found", http.StatusNotFound) + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) return } - log.Error("Error whilst opening %s %s. Error: %v", prefix, rPath, err) - http.Error(w, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath), http.StatusInternalServerError) + log.Error("Error whilst getting URL for %s %s. Error: %v", prefix, rPath, err) + http.Error(w, fmt.Sprintf("Error whilst getting URL for %s %s", prefix, rPath), http.StatusInternalServerError) return } - fr, err := objStore.Open(rPath) - if err != nil { - log.Error("Error whilst opening %s %s. Error: %v", prefix, rPath, err) - http.Error(w, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath), http.StatusInternalServerError) - return - } - defer fr.Close() - httpcache.ServeContentWithCacheControl(w, req, path.Base(rPath), fi.ModTime(), fr) + http.Redirect(w, req, u.String(), http.StatusTemporaryRedirect) }) } + + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if req.Method != "GET" && req.Method != "HEAD" { + http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed) + return + } + + if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) + return + } + routing.UpdateFuncInfo(req.Context(), funcInfo) + + rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/") + rPath = util.PathJoinRelX(rPath) + if rPath == "" || rPath == "." { + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) + return + } + + fi, err := objStore.Stat(rPath) + if err != nil { + if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) { + log.Warn("Unable to find %s %s", prefix, rPath) + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) + return + } + log.Error("Error whilst opening %s %s. Error: %v", prefix, rPath, err) + http.Error(w, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath), http.StatusInternalServerError) + return + } + + fr, err := objStore.Open(rPath) + if err != nil { + log.Error("Error whilst opening %s %s. Error: %v", prefix, rPath, err) + http.Error(w, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath), http.StatusInternalServerError) + return + } + defer fr.Close() + httpcache.ServeContentWithCacheControl(w, req, path.Base(rPath), fi.ModTime(), fr) + }) }