From 3e3ae4a9dee56f9b15fd77930a389f372d4489fe Mon Sep 17 00:00:00 2001 From: Sergey Zolotarev Date: Thu, 5 Oct 2023 16:31:59 +0600 Subject: [PATCH 1/2] Fix panic in storageHandler 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 --- routers/web/base.go | 99 ++++++++++++++++++++++----------------------- 1 file changed, 49 insertions(+), 50 deletions(-) diff --git a/routers/web/base.go b/routers/web/base.go index e4b7d8ce8db2..9da4ff218e8f 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, "method not allowed", http.StatusMethodNotAllowed) return } if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { - next.ServeHTTP(w, req) + http.Error(w, "file not found", 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) 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, "method not allowed", http.StatusMethodNotAllowed) + return + } + + if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { + http.Error(w, "file not found", 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) + 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 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) + }) } From 87925702221384c26a509dfe9b576ce63c24f177 Mon Sep 17 00:00:00 2001 From: Sergey Zolotarev Date: Thu, 5 Oct 2023 17:01:51 +0600 Subject: [PATCH 2/2] Replace hardcoded HTTP error messages with StatusText() in storageHandler --- routers/web/base.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/routers/web/base.go b/routers/web/base.go index 9da4ff218e8f..78dde57fa6f8 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -26,12 +26,12 @@ func storageHandler(storageSetting *setting.Storage, prefix string, objStore sto if storageSetting.MinioConfig.ServeDirect { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { if req.Method != "GET" && req.Method != "HEAD" { - http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed) return } if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { - http.Error(w, "file not found", http.StatusNotFound) + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) return } routing.UpdateFuncInfo(req.Context(), funcInfo) @@ -43,7 +43,7 @@ func storageHandler(storageSetting *setting.Storage, prefix string, objStore sto 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 getting URL for %s %s. Error: %v", prefix, rPath, err) @@ -57,12 +57,12 @@ func storageHandler(storageSetting *setting.Storage, prefix string, objStore sto return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { if req.Method != "GET" && req.Method != "HEAD" { - http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed) return } if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") { - http.Error(w, "file not found", http.StatusNotFound) + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) return } routing.UpdateFuncInfo(req.Context(), funcInfo) @@ -70,7 +70,7 @@ func storageHandler(storageSetting *setting.Storage, prefix string, objStore sto rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/") rPath = util.PathJoinRelX(rPath) if rPath == "" || rPath == "." { - http.Error(w, "file not found", http.StatusNotFound) + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) return } @@ -78,7 +78,7 @@ func storageHandler(storageSetting *setting.Storage, prefix string, objStore sto 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)