diff options
| author | Giteabot <teabot@gitea.io> | 2023-10-06 22:51:26 +0800 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2023-10-06 16:51:26 +0200 |
| commit | 5b670d83e1a2edc05c3f94b59bc99b9acd2c1023 (patch) | |
| tree | 4afdc51e5d18f81dd77b1ded77f5204dcee478a8 | |
| parent | 9207331f4dad9ac767915b8abc7a119d5d6bce1c (diff) | |
| download | gitea-5b670d83e1a2edc05c3f94b59bc99b9acd2c1023.tar.gz gitea-5b670d83e1a2edc05c3f94b59bc99b9acd2c1023.zip | |
Fix panic in storageHandler (#27446) (#27479)v1.21.0-rc1
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>
| -rw-r--r-- | routers/web/base.go | 101 |
1 files changed, 50 insertions, 51 deletions
diff --git a/routers/web/base.go b/routers/web/base.go index e4b7d8ce8d..78dde57fa6 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) + }) } |