Fix panic in storageHandler (#27446) (#27479)

Backport #27446 by @sryze

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 (?)

Co-authored-by: Sergey Zolotarev <sryze@outlook.com>

diff --git a/routers/web/base.go b/routers/web/base.go
index e4b7d8ce8db2ea94b105dd4ea095e529a7804019..78dde57fa6f859af35b67aaf011660fe9a623987 100644 (file)
--- 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)
+       })
 }