Prevent panics with missing storage (#13164)

* The `.Use` of storageHandler before setting up the template renderer
causes a panic if there is an error to log.
* The error passed to `ctx.Error` in that case may contain sensitive
information and should not be rendered to the end user. We should
instead log the error and render a simple error message.
* There is no handling of missing avatars and this needs a 404. Minio
errors need to be mapped to standard golang errors such as
os.ErrNotExist.
* There is no logging when storage is set up.

Related #13159

Signed-off-by: Andrew Thornton <[email protected]>

Author: zeripath

modules/storage/local.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"path/filepath"
 
+	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/util"
 )
 
@@ -40,6 +41,7 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
 	}
 	config := configInterface.(LocalStorageConfig)
 
+	log.Info("Creating new Local Storage at %s", config.Path)
 	if err := os.MkdirAll(config.Path, os.ModePerm); err != nil {
 		return nil, err
 	}

modules/storage/minio.go

@@ -13,6 +13,8 @@ import (
 	"strings"
 	"time"
 
+	"code.gitea.io/gitea/modules/log"
+
 	"github.com/minio/minio-go/v7"
 	"github.com/minio/minio-go/v7/pkg/credentials"
 )
@@ -58,20 +60,42 @@ type MinioStorage struct {
 	basePath string
 }
 
+func convertMinioErr(err error) error {
+	if err == nil {
+		return nil
+	}
+	errResp, ok := err.(minio.ErrorResponse)
+	if !ok {
+		return err
+	}
+
+	// Convert two responses to standard analogues
+	switch errResp.Code {
+	case "NoSuchKey":
+		return os.ErrNotExist
+	case "AccessDenied":
+		return os.ErrPermission
+	}
+
+	return err
+}
+
 // NewMinioStorage returns a minio storage
 func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error) {
 	configInterface, err := toConfig(MinioStorageConfig{}, cfg)
 	if err != nil {
-		return nil, err
+		return nil, convertMinioErr(err)
 	}
 	config := configInterface.(MinioStorageConfig)
 
+	log.Info("Creating Minio storage at %s:%s with base path %s", config.Endpoint, config.Bucket, config.BasePath)
+
 	minioClient, err := minio.New(config.Endpoint, &minio.Options{
 		Creds:  credentials.NewStaticV4(config.AccessKeyID, config.SecretAccessKey, ""),
 		Secure: config.UseSSL,
 	})
 	if err != nil {
-		return nil, err
+		return nil, convertMinioErr(err)
 	}
 
 	if err := minioClient.MakeBucket(ctx, config.Bucket, minio.MakeBucketOptions{
@@ -80,7 +104,7 @@ func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
 		// Check to see if we already own this bucket (which happens if you run this twice)
 		exists, errBucketExists := minioClient.BucketExists(ctx, config.Bucket)
 		if !exists || errBucketExists != nil {
-			return nil, err
+			return nil, convertMinioErr(err)
 		}
 	}
 
@@ -101,7 +125,7 @@ func (m *MinioStorage) Open(path string) (Object, error) {
 	var opts = minio.GetObjectOptions{}
 	object, err := m.client.GetObject(m.ctx, m.bucket, m.buildMinioPath(path), opts)
 	if err != nil {
-		return nil, err
+		return nil, convertMinioErr(err)
 	}
 	return &minioObject{object}, nil
 }
@@ -117,7 +141,7 @@ func (m *MinioStorage) Save(path string, r io.Reader) (int64, error) {
 		minio.PutObjectOptions{ContentType: "application/octet-stream"},
 	)
 	if err != nil {
-		return 0, err
+		return 0, convertMinioErr(err)
 	}
 	return uploadInfo.Size, nil
 }
@@ -159,27 +183,25 @@ func (m *MinioStorage) Stat(path string) (os.FileInfo, error) {
 		minio.StatObjectOptions{},
 	)
 	if err != nil {
-		if errResp, ok := err.(minio.ErrorResponse); ok {
-			if errResp.Code == "NoSuchKey" {
-				return nil, os.ErrNotExist
-			}
-		}
-		return nil, err
+		return nil, convertMinioErr(err)
 	}
 	return &minioFileInfo{info}, nil
 }
 
 // Delete delete a file
 func (m *MinioStorage) Delete(path string) error {
-	return m.client.RemoveObject(m.ctx, m.bucket, m.buildMinioPath(path), minio.RemoveObjectOptions{})
+	err := m.client.RemoveObject(m.ctx, m.bucket, m.buildMinioPath(path), minio.RemoveObjectOptions{})
+
+	return convertMinioErr(err)
 }
 
 // URL gets the redirect URL to a file. The presigned link is valid for 5 minutes.
 func (m *MinioStorage) URL(path, name string) (*url.URL, error) {
 	reqParams := make(url.Values)
 	// TODO it may be good to embed images with 'inline' like ServeData does, but we don't want to have to read the file, do we?
 	reqParams.Set("response-content-disposition", "attachment; filename=\""+quoteEscaper.Replace(name)+"\"")
-	return m.client.PresignedGetObject(m.ctx, m.bucket, m.buildMinioPath(path), 5*time.Minute, reqParams)
+	u, err := m.client.PresignedGetObject(m.ctx, m.bucket, m.buildMinioPath(path), 5*time.Minute, reqParams)
+	return u, convertMinioErr(err)
 }
 
 // IterateObjects iterates across the objects in the miniostorage
@@ -193,13 +215,13 @@ func (m *MinioStorage) IterateObjects(fn func(path string, obj Object) error) er
 	}) {
 		object, err := m.client.GetObject(lobjectCtx, m.bucket, mObjInfo.Key, opts)
 		if err != nil {
-			return err
+			return convertMinioErr(err)
 		}
 		if err := func(object *minio.Object, fn func(path string, obj Object) error) error {
 			defer object.Close()
 			return fn(strings.TrimPrefix(m.basePath, mObjInfo.Key), &minioObject{object})
 		}(object, fn); err != nil {
-			return err
+			return convertMinioErr(err)
 		}
 	}
 	return nil

modules/storage/storage.go

@@ -12,6 +12,7 @@ import (
 	"net/url"
 	"os"
 
+	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 )
 
@@ -141,21 +142,25 @@ func NewStorage(typStr string, cfg interface{}) (ObjectStorage, error) {
 }
 
 func initAvatars() (err error) {
+	log.Info("Initialising Avatar storage with type: %s", setting.Avatar.Storage.Type)
 	Avatars, err = NewStorage(setting.Avatar.Storage.Type, setting.Avatar.Storage)
 	return
 }
 
 func initAttachments() (err error) {
+	log.Info("Initialising Attachment storage with type: %s", setting.Attachment.Storage.Type)
 	Attachments, err = NewStorage(setting.Attachment.Storage.Type, setting.Attachment.Storage)
 	return
 }
 
 func initLFS() (err error) {
+	log.Info("Initialising LFS storage with type: %s", setting.LFS.Storage.Type)
 	LFS, err = NewStorage(setting.LFS.Storage.Type, setting.LFS.Storage)
 	return
 }
 
 func initRepoAvatars() (err error) {
+	log.Info("Initialising Repository Avatar storage with type: %s", setting.RepoAvatar.Storage.Type)
 	RepoAvatars, err = NewStorage(setting.RepoAvatar.Storage.Type, setting.RepoAvatar.Storage)
 	return
 }

routers/routes/routes.go

@@ -7,8 +7,10 @@ package routes
 import (
 	"bytes"
 	"encoding/gob"
+	"fmt"
 	"io"
 	"net/http"
+	"os"
 	"path"
 	"strings"
 	"text/template"
@@ -125,7 +127,13 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
 			rPath := strings.TrimPrefix(req.RequestURI, "/"+prefix)
 			u, err := objStore.URL(rPath, path.Base(rPath))
 			if err != nil {
-				ctx.Error(500, err.Error())
+				if err == os.ErrNotExist {
+					log.Warn("Unable to find %s %s", prefix, rPath)
+					ctx.Error(404, "file not found")
+					return
+				}
+				log.Error("Error whilst getting URL for %s %s. Error: %v", prefix, rPath, err)
+				ctx.Error(500, fmt.Sprintf("Error whilst getting URL for %s %s", prefix, rPath))
 				return
 			}
 			http.Redirect(
@@ -152,14 +160,21 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
 		//If we have matched and access to release or issue
 		fr, err := objStore.Open(rPath)
 		if err != nil {
-			ctx.Error(500, err.Error())
+			if err == os.ErrNotExist {
+				log.Warn("Unable to find %s %s", prefix, rPath)
+				ctx.Error(404, "file not found")
+				return
+			}
+			log.Error("Error whilst opening %s %s. Error: %v", prefix, rPath, err)
+			ctx.Error(500, fmt.Sprintf("Error whilst opening %s %s", prefix, rPath))
 			return
 		}
 		defer fr.Close()
 
 		_, err = io.Copy(ctx.Resp, fr)
 		if err != nil {
-			ctx.Error(500, err.Error())
+			log.Error("Error whilst rendering %s %s. Error: %v", prefix, rPath, err)
+			ctx.Error(500, fmt.Sprintf("Error whilst rendering %s %s", prefix, rPath))
 			return
 		}
 	}
@@ -208,10 +223,11 @@ func NewMacaron() *macaron.Macaron {
 		},
 	))
 
+	m.Use(templates.HTMLRenderer())
+
 	m.Use(storageHandler(setting.Avatar.Storage, "avatars", storage.Avatars))
 	m.Use(storageHandler(setting.RepoAvatar.Storage, "repo-avatars", storage.RepoAvatars))
 
-	m.Use(templates.HTMLRenderer())
 	mailer.InitMailRender(templates.Mailer())
 
 	localeNames, err := options.Dir("locale")