From 91f2afdb546364195ff909186983b94a61ab3181 Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 16 Oct 2020 04:51:06 +0100 Subject: [PATCH] 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 --- modules/storage/local.go | 2 ++ modules/storage/minio.go | 52 +++++++++++++++++++++++++++----------- modules/storage/storage.go | 5 ++++ routers/routes/routes.go | 24 +++++++++++++++--- 4 files changed, 64 insertions(+), 19 deletions(-) diff --git a/modules/storage/local.go b/modules/storage/local.go index e270a40b77..93af13ee33 100644 --- a/modules/storage/local.go +++ b/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 } diff --git a/modules/storage/minio.go b/modules/storage/minio.go index eb43fa96ee..4a2819de5e 100644 --- a/modules/storage/minio.go +++ b/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,19 +183,16 @@ 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. @@ -179,7 +200,8 @@ 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 diff --git a/modules/storage/storage.go b/modules/storage/storage.go index 1fa04119c7..45582f3915 100644 --- a/modules/storage/storage.go +++ b/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 } diff --git a/routers/routes/routes.go b/routers/routes/routes.go index a09e53efc1..adda919857 100644 --- a/routers/routes/routes.go +++ b/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")