Minor improvements to logic around decompression

router/middleware/middleware.go

@@ -130,7 +130,7 @@ func (re *RequestError) asFilesystemError() (int, string) {
 		return http.StatusBadRequest, "Cannot perform that action: file is a directory."
 	}
 	if filesystem.IsErrorCode(err, filesystem.ErrCodeDiskSpace) || strings.Contains(err.Error(), "filesystem: not enough disk space") {
-		return http.StatusBadRequest, "Cannot perform that action: file is a directory."
+		return http.StatusBadRequest, "There is not enough disk space available to perform that action."
 	}
 	if strings.HasSuffix(err.Error(), "file name too long") {
 		return http.StatusBadRequest, "Cannot perform that action: file name is too long."

router/router_server_files.go

@@ -380,7 +380,8 @@ func postServerCompressFiles(c *gin.Context) {
 // of unpacking an archive that exists on the server into the provided RootPath
 // for the server.
 func postServerDecompressFiles(c *gin.Context) {
-	s := ExtractServer(c)
+	s := middleware.ExtractServer(c)
+	lg := middleware.ExtractLogger(c)
 	var data struct {
 		RootPath string `json:"root"`
 		File     string `json:"file"`
@@ -389,12 +390,12 @@ func postServerDecompressFiles(c *gin.Context) {
 		return
 	}
 
-	lg := s.Log().WithFields(log.Fields{"root_path": data.RootPath, "file": data.File})
+	lg = lg.WithFields(log.Fields{"root_path": data.RootPath, "file": data.File})
-	lg.Debug("checking if space is available for decompression")
+	lg.Debug("checking if space is available for file decompression")
-	hasSpace, err := s.Filesystem().SpaceAvailableForDecompression(data.RootPath, data.File)
+	err := s.Filesystem().SpaceAvailableForDecompression(data.RootPath, data.File)
 	if err != nil {
 		if filesystem.IsErrorCode(err, filesystem.ErrCodeUnknownArchive) {
-			s.Log().WithField("path", data.RootPath).WithField("file", data.File).WithField("error", err).Warn("failed to decompress file due to unknown format")
+			lg.WithField("error", err).Warn("failed to decompress file: unknown archive format")
 			c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{"error": "The archive provided is in a format Wings does not understand."})
 			return
 		}
@@ -402,30 +403,21 @@ func postServerDecompressFiles(c *gin.Context) {
 		return
 	}
 
-	if !hasSpace {
+	lg.Info("starting file decompression")
-		c.AbortWithStatusJSON(http.StatusConflict, gin.H{
-			"error": "This server does not have enough available disk space to decompress this archive.",
-		})
-		return
-	}
-
 	if err := s.Filesystem().DecompressFile(data.RootPath, data.File); err != nil {
 		// If the file is busy for some reason just return a nicer error to the user since there is not
 		// much we specifically can do. They'll need to stop the running server process in order to overwrite
 		// a file like this.
 		if strings.Contains(err.Error(), "text file busy") {
-			s.Log().WithField("error", err).Warn("failed to decompress file due to busy text file")
+			lg.WithField("error", err).Warn("failed to decompress file: text file busy")
-
 			c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{
 				"error": "One or more files this archive is attempting to overwrite are currently in use by another process. Please try again.",
 			})
 			return
 		}
-
+		middleware.CaptureAndAbort(c, err)
-		NewServerError(err, s).Abort(c)
 		return
 	}
-
 	c.Status(http.StatusNoContent)
 }
 

server/filesystem/decompress.go

@@ -4,28 +4,29 @@ import (
 	"archive/tar"
 	"archive/zip"
 	"compress/gzip"
-	"emperror.dev/errors"
 	"fmt"
-	"github.com/mholt/archiver/v3"
 	"os"
 	"path/filepath"
 	"reflect"
 	"strings"
 	"sync/atomic"
+
+	"emperror.dev/errors"
+	"github.com/mholt/archiver/v3"
 )
 
-// Look through a given archive and determine if decompressing it would put the server over
+// SpaceAvailableForDecompression looks through a given archive and determines
-// its allocated disk space limit.
+// if decompressing it would put the server over its allocated disk space limit.
-func (fs *Filesystem) SpaceAvailableForDecompression(dir string, file string) (bool, error) {
+func (fs *Filesystem) SpaceAvailableForDecompression(dir string, file string) error {
 	// Don't waste time trying to determine this if we know the server will have the space for
 	// it since there is no limit.
 	if fs.MaxDisk() <= 0 {
-		return true, nil
+		return nil
 	}
 
 	source, err := fs.SafePath(filepath.Join(dir, file))
 	if err != nil {
-		return false, err
+		return err
 	}
 
 	// Get the cached size in a parallel process so that if it is not cached we are not
@@ -38,32 +39,28 @@ func (fs *Filesystem) SpaceAvailableForDecompression(dir string, file string) (b
 		if atomic.AddInt64(&size, f.Size())+dirSize > fs.MaxDisk() {
 			return &Error{code: ErrCodeDiskSpace}
 		}
-
 		return nil
 	})
-
 	if err != nil {
 		if strings.HasPrefix(err.Error(), "format ") {
-			return false, &Error{code: ErrCodeUnknownArchive}
+			return &Error{code: ErrCodeUnknownArchive}
+		}
+		return err
+	}
+	return err
 }
 
-		return false, err
+// DecompressFile will decompress a file in a given directory by using the
-	}
+// archiver tool to infer the file type and go from there. This will walk over
-
+// all of the files within the given archive and ensure that there is not a
-	return true, err
+// zip-slip attack being attempted by validating that the final path is within
-}
+// the server data directory.
-
-// Decompress a file in a given directory by using the archiver tool to infer the file
-// type and go from there. This will walk over all of the files within the given archive
-// and ensure that there is not a zip-slip attack being attempted by validating that the
-// final path is within the server data directory.
 func (fs *Filesystem) DecompressFile(dir string, file string) error {
 	source, err := fs.SafePath(filepath.Join(dir, file))
 	if err != nil {
 		return err
 	}
-
+	// Ensure that the source archive actually exists on the system.
-	// Make sure the file exists basically.
 	if _, err := os.Stat(source); err != nil {
 		return err
 	}
@@ -79,7 +76,6 @@ func (fs *Filesystem) DecompressFile(dir string, file string) error {
 		}
 
 		var name string
-
 		switch s := f.Sys().(type) {
 		case *tar.Header:
 			name = s.Name
@@ -88,7 +84,11 @@ func (fs *Filesystem) DecompressFile(dir string, file string) error {
 		case *zip.FileHeader:
 			name = s.Name
 		default:
-			return errors.New(fmt.Sprintf("could not parse underlying data source with type %s", reflect.TypeOf(s).String()))
+			return &Error{
+				code:     ErrCodeUnknownError,
+				resolved: filepath.Join(dir, f.Name()),
+				err:      errors.New(fmt.Sprintf("could not parse underlying data source with type: %s", reflect.TypeOf(s).String())),
+			}
 		}
 
 		p := filepath.Join(dir, name)
@@ -96,15 +96,16 @@ func (fs *Filesystem) DecompressFile(dir string, file string) error {
 		if err := fs.IsIgnored(p); err != nil {
 			return nil
 		}
-		return errors.WithMessage(fs.Writefile(p, f), "could not extract file from archive")
+		if err := fs.Writefile(p, f); err != nil {
+			return &Error{code: ErrCodeUnknownError, err: err, resolved: source}
+		}
+		return nil
 	})
 	if err != nil {
 		if strings.HasPrefix(err.Error(), "format ") {
 			return &Error{code: ErrCodeUnknownArchive}
 		}
-
 		return err
 	}
-
 	return nil
 }

server/filesystem/errors.go

@@ -17,19 +17,33 @@ const (
 	ErrCodeUnknownArchive ErrorCode = "E_UNKNFMT"
 	ErrCodePathResolution ErrorCode = "E_BADPATH"
 	ErrCodeDenylistFile   ErrorCode = "E_DENYLIST"
+	ErrCodeUnknownError   ErrorCode = "E_UNKNOWN"
 )
 
 type Error struct {
 	code ErrorCode
-	path     string
+	// Contains the underlying error leading to this. This value may or may not be
+	// present, it is entirely dependent on how this error was triggered.
+	err error
+	// This contains the value of the final destination that triggered this specific
+	// error event.
 	resolved string
+	// This value is generally only present on errors stemming from a path resolution
+	// error. For everything else you should be setting and reading the resolved path
+	// value which will be far more useful.
+	path string
+}
+
+// Code returns the ErrorCode for this specific error instance.
+func (e *Error) Code() ErrorCode {
+	return e.code
 }
 
 // Returns a human-readable error string to identify the Error by.
 func (e *Error) Error() string {
 	switch e.code {
 	case ErrCodeIsDirectory:
-		return "filesystem: is a directory"
+		return fmt.Sprintf("filesystem: cannot perform action: [%s] is a directory", e.resolved)
 	case ErrCodeDiskSpace:
 		return "filesystem: not enough disk space"
 	case ErrCodeUnknownArchive:
@@ -46,36 +60,17 @@ func (e *Error) Error() string {
 			r = "<empty>"
 		}
 		return fmt.Sprintf("filesystem: server path [%s] resolves to a location outside the server root: %s", e.path, r)
+	case ErrCodeUnknownError:
+		fallthrough
+	default:
+		return fmt.Sprintf("filesystem: an error occurred: %s", e.Cause())
 	}
-	return "filesystem: unhandled error type"
 }
 
-// Returns the ErrorCode for this specific error instance.
+// Cause returns the underlying cause of this filesystem error. In some causes
-func (e *Error) Code() ErrorCode {
+// there may not be a cause present, in which case nil will be returned.
-	return e.code
+func (e *Error) Cause() error {
-}
+	return e.err
-
-// Checks if the given error is one of the Filesystem errors.
-func IsFilesystemError(err error) (*Error, bool) {
-	var fserr *Error
-	if errors.As(err, &fserr) {
-		return fserr, true
-	}
-	return nil, false
-}
-
-// Checks if "err" is a filesystem Error type. If so, it will then drop in and check
-// that the error code is the same as the provided ErrorCode passed in "code".
-func IsErrorCode(err error, code ErrorCode) bool {
-	if e, ok := IsFilesystemError(err); ok {
-		return e.code == code
-	}
-	return false
-}
-
-// Returns a new BadPathResolution error.
-func NewBadPathResolution(path string, resolved string) *Error {
-	return &Error{code: ErrCodePathResolution, path: path, resolved: resolved}
 }
 
 // Generates an error logger instance with some basic information.
@@ -92,10 +87,46 @@ func (fs *Filesystem) handleWalkerError(err error, f os.FileInfo) error {
 	if !IsErrorCode(err, ErrCodePathResolution) {
 		return err
 	}
-
 	if f != nil && f.IsDir() {
 		return filepath.SkipDir
 	}
-
 	return nil
 }
+
+// IsFilesystemError checks if the given error is one of the Filesystem errors.
+func IsFilesystemError(err error) bool {
+	var fserr *Error
+	if err != nil && errors.As(err, &fserr) {
+		return true
+	}
+	return false
+}
+
+// IsErrorCode checks if "err" is a filesystem Error type. If so, it will then
+// drop in and check that the error code is the same as the provided ErrorCode
+// passed in "code".
+func IsErrorCode(err error, code ErrorCode) bool {
+	var fserr *Error
+	if err != nil && errors.As(err, &fserr) {
+		return fserr.code == code
+	}
+	return false
+}
+
+// NewBadPathResolution returns a new BadPathResolution error.
+func NewBadPathResolution(path string, resolved string) *Error {
+	return &Error{code: ErrCodePathResolution, path: path, resolved: resolved}
+}
+
+// WrapError wraps the provided error as a Filesystem error and attaches the
+// provided resolved source to it. If the error is already a Filesystem error
+// no action is taken.
+func WrapError(err error, resolved string) *Error {
+	if err == nil {
+		return nil
+	}
+	if IsFilesystemError(err) {
+		return err.(*Error)
+	}
+	return &Error{code: ErrCodeUnknownError, err: err, resolved: resolved}
+}

server/filesystem/filesystem.go

@@ -123,7 +123,8 @@ func (fs *Filesystem) Readfile(p string, w io.Writer) error {
 	return err
 }
 
-// Writes a file to the system. If the file does not already exist one will be created.
+// Writefile writes a file to the system. If the file does not already exist one
+// will be created.
 func (fs *Filesystem) Writefile(p string, r io.Reader) error {
 	cleaned, err := fs.SafePath(p)
 	if err != nil {
@@ -138,7 +139,7 @@ func (fs *Filesystem) Writefile(p string, r io.Reader) error {
 		return err
 	} else if err == nil {
 		if stat.IsDir() {
-			return &Error{code: ErrCodeIsDirectory}
+			return &Error{code: ErrCodeIsDirectory, resolved: cleaned}
 		}
 		currentSize = stat.Size()
 	}