Fix error handling and get tests back to working

cmd/root.go

@@ -114,7 +114,7 @@ func rootCmdRun(*cobra.Command, []string) {
 	// been specified in the command startup.
 	if configPath == config.DefaultLocation {
 		if err := RelocateConfiguration(); err != nil {
-			if os.IsNotExist(err) {
+			if errors.Is(err, os.ErrNotExist) {
 				exitWithConfigurationNotice()
 			}
 

router/error.go

@@ -62,7 +62,7 @@ func (e *RequestError) SetMessage(msg string) *RequestError {
 func (e *RequestError) AbortWithStatus(status int, c *gin.Context) {
 	// If this error is because the resource does not exist, we likely do not need to log
 	// the error anywhere, just return a 404 and move on with our lives.
-	if os.IsNotExist(e.Err) {
+	if errors.Is(e.Err, os.ErrNotExist) {
 		e.logger().WithField("error", e.Err).Debug("encountered os.IsNotExist error while handling request")
 
 		c.AbortWithStatusJSON(http.StatusNotFound, gin.H{

router/router_transfer.go

@@ -52,7 +52,7 @@ func getServerArchive(c *gin.Context) {
 
 	st, err := s.Archiver.Stat()
 	if err != nil {
-		if !os.IsNotExist(err) {
+		if !errors.Is(err, os.ErrNotExist) {
 			TrackedServerError(err, s).SetMessage("failed to stat archive").AbortWithServerError(c)
 			return
 		}

server/filesystem/disk_space.go

@@ -197,7 +197,7 @@ func (fs *Filesystem) hasSpaceFor(size int64) error {
 
 	s, err := fs.DiskUsage(true)
 	if err != nil {
-		return err
+		return errors.WithStackIf(err)
 	}
 
 	if (s + size) > fs.MaxDisk() {

server/filesystem/errors.go

@@ -33,9 +33,15 @@ func NewBadPathResolution(path string, resolved string) *BadPathResolutionError
 
 // Determines if the given error is a bad path resolution error.
 func IsBadPathResolutionError(err error) bool {
-	if _, ok := err.(*BadPathResolutionError); ok {
+	e := errors.Unwrap(err)
+	if e == nil {
+		e = err
+	}
+
+	if _, ok := e.(*BadPathResolutionError); ok {
 		return true
 	}
+
 	return false
 }
 

server/filesystem/errors_test.go

@@ -0,0 +1,24 @@
+package filesystem
+
+import (
+	. "github.com/franela/goblin"
+	"testing"
+)
+
+func TestFilesystem_PathResolutionError(t *testing.T) {
+	g := Goblin(t)
+
+	g.Describe("NewBadPathResolutionError", func() {
+		g.It("is can detect itself as an error correctly", func() {
+			err := NewBadPathResolution("foo", "bar")
+			g.Assert(IsBadPathResolutionError(err)).IsTrue()
+			g.Assert(err.Error()).Equal("filesystem: server path [foo] resolves to a location outside the server root: bar")
+			g.Assert(IsBadPathResolutionError(ErrIsDirectory)).IsFalse()
+		})
+
+		g.It("returns <empty> if no destination path is provided", func() {
+			err := NewBadPathResolution("foo", "")
+			g.Assert(err.Error()).Equal("filesystem: server path [foo] resolves to a location outside the server root: <empty>")
+		})
+	})
+}

server/filesystem/filesystem.go

@@ -60,20 +60,20 @@ func (fs *Filesystem) Readfile(p string, w io.Writer) error {
 	}
 
 	if st, err := os.Stat(cleaned); err != nil {
-		return err
+		return errors.WithStack(err)
 	} else if st.IsDir() {
-		return ErrIsDirectory
+		return errors.WithStack(ErrIsDirectory)
 	}
 
 	f, err := os.Open(cleaned)
 	if err != nil {
-		return err
+		return errors.WithStack(err)
 	}
 	defer f.Close()
 
 	_, err = bufio.NewReader(f).WriteTo(w)
 
-	return err
+	return errors.WithStack(err)
 }
 
 // Writes a file to the system. If the file does not already exist one will be created.
@@ -100,7 +100,7 @@ func (fs *Filesystem) Writefile(p string, r io.Reader) error {
 		}
 	} else {
 		if stat.IsDir() {
-			return ErrIsDirectory
+			return errors.WithStack(ErrIsDirectory)
 		}
 
 		currentSize = stat.Size()
@@ -248,8 +248,8 @@ func (fs *Filesystem) findCopySuffix(dir string, name string, extension string)
 		// If we stat the file and it does not exist that means we're good to create the copy. If it
 		// does exist, we'll just continue to the next loop and try again.
 		if _, err := fs.Stat(path.Join(dir, n)); err != nil {
-			if !os.IsNotExist(err) {
+			if !errors.Is(err, os.ErrNotExist) {
-				return "", err
+				return "", errors.WithStackIf(err)
 			}
 
 			break
@@ -305,6 +305,9 @@ func (fs *Filesystem) Copy(p string) error {
 	defer source.Close()
 
 	n, err := fs.findCopySuffix(relative, name, extension)
+	if err != nil {
+		return err
+	}
 
 	return fs.Writefile(path.Join(relative, n), source)
 }

server/filesystem/filesystem_test.go

@@ -70,224 +70,6 @@ func (rfs *rootFs) reset() {
 	}
 }
 
-func TestFilesystem_Path(t *testing.T) {
-	g := Goblin(t)
-	fs, rfs := NewFs()
-
-	g.Describe("Path", func() {
-		g.It("returns the root path for the instance", func() {
-			g.Assert(fs.Path()).Equal(filepath.Join(rfs.root, "/server"))
-		})
-	})
-}
-
-func TestFilesystem_SafePath(t *testing.T) {
-	g := Goblin(t)
-	fs, rfs := NewFs()
-	prefix := filepath.Join(rfs.root, "/server")
-
-	g.Describe("SafePath", func() {
-		g.It("returns a cleaned path to a given file", func() {
-			p, err := fs.SafePath("test.txt")
-			g.Assert(err).IsNil()
-			g.Assert(p).Equal(prefix + "/test.txt")
-
-			p, err = fs.SafePath("/test.txt")
-			g.Assert(err).IsNil()
-			g.Assert(p).Equal(prefix + "/test.txt")
-
-			p, err = fs.SafePath("./test.txt")
-			g.Assert(err).IsNil()
-			g.Assert(p).Equal(prefix + "/test.txt")
-
-			p, err = fs.SafePath("/foo/../test.txt")
-			g.Assert(err).IsNil()
-			g.Assert(p).Equal(prefix + "/test.txt")
-
-			p, err = fs.SafePath("/foo/bar")
-			g.Assert(err).IsNil()
-			g.Assert(p).Equal(prefix + "/foo/bar")
-		})
-
-		g.It("handles root directory access", func() {
-			p, err := fs.SafePath("/")
-			g.Assert(err).IsNil()
-			g.Assert(p).Equal(prefix)
-
-			p, err = fs.SafePath("")
-			g.Assert(err).IsNil()
-			g.Assert(p).Equal(prefix)
-		})
-
-		g.It("removes trailing slashes from paths", func() {
-			p, err := fs.SafePath("/foo/bar/")
-			g.Assert(err).IsNil()
-			g.Assert(p).Equal(prefix + "/foo/bar")
-		})
-
-		g.It("handles deeply nested directories that do not exist", func() {
-			p, err := fs.SafePath("/foo/bar/baz/quaz/../../ducks/testing.txt")
-			g.Assert(err).IsNil()
-			g.Assert(p).Equal(prefix + "/foo/bar/ducks/testing.txt")
-		})
-
-		g.It("blocks access to files outside the root directory", func() {
-			p, err := fs.SafePath("../test.txt")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-			g.Assert(p).Equal("")
-
-			p, err = fs.SafePath("/../test.txt")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-			g.Assert(p).Equal("")
-
-			p, err = fs.SafePath("./foo/../../test.txt")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-			g.Assert(p).Equal("")
-
-			p, err = fs.SafePath("..")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-			g.Assert(p).Equal("")
-		})
-	})
-}
-
-// We test against accessing files outside the root directory in the tests, however it
-// is still possible for someone to mess up and not properly use this safe path call. In
-// order to truly confirm this, we'll try to pass in a symlinked malicious file to all of
-// the calls and ensure they all fail with the same reason.
-func TestFilesystem_Blocks_Symlinks(t *testing.T) {
-	g := Goblin(t)
-	fs, rfs := NewFs()
-
-	if err := rfs.CreateServerFile("/../malicious.txt", "external content"); err != nil {
-		panic(err)
-	}
-
-	if err := os.Mkdir(filepath.Join(rfs.root, "/malicious_dir"), 0777); err != nil {
-		panic(err)
-	}
-
-	if err := os.Symlink(filepath.Join(rfs.root, "malicious.txt"), filepath.Join(rfs.root, "/server/symlinked.txt")); err != nil {
-		panic(err)
-	}
-
-	if err := os.Symlink(filepath.Join(rfs.root, "/malicious_dir"), filepath.Join(rfs.root, "/server/external_dir")); err != nil {
-		panic(err)
-	}
-
-	g.Describe("Readfile", func() {
-		g.It("cannot read a file symlinked outside the root", func() {
-			b := bytes.Buffer{}
-
-			err := fs.Readfile("symlinked.txt", &b)
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-	})
-
-	g.Describe("Writefile", func() {
-		g.It("cannot write to a file symlinked outside the root", func() {
-			r := bytes.NewReader([]byte("testing"))
-
-			err := fs.Writefile("symlinked.txt", r)
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-
-		g.It("cannot write a file to a directory symlinked outside the root", func() {
-			r := bytes.NewReader([]byte("testing"))
-
-			err := fs.Writefile("external_dir/foo.txt", r)
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-	})
-
-	g.Describe("CreateDirectory", func() {
-		g.It("cannot create a directory outside the root", func() {
-			err := fs.CreateDirectory("my_dir", "external_dir")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-
-		g.It("cannot create a nested directory outside the root", func() {
-			err := fs.CreateDirectory("my/nested/dir", "external_dir/foo/bar")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-
-		g.It("cannot create a nested directory outside the root", func() {
-			err := fs.CreateDirectory("my/nested/dir", "external_dir/server")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-	})
-
-	g.Describe("Rename", func() {
-		g.It("cannot rename a file symlinked outside the directory root", func() {
-			err := fs.Rename("symlinked.txt", "foo.txt")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-
-		g.It("cannot rename a symlinked directory outside the root", func() {
-			err := fs.Rename("external_dir", "foo")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-
-		g.It("cannot rename a file to a location outside the directory root", func() {
-			rfs.CreateServerFile("my_file.txt", "internal content")
-
-			err := fs.Rename("my_file.txt", "external_dir/my_file.txt")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-	})
-
-	g.Describe("Chown", func() {
-		g.It("cannot chown a file symlinked outside the directory root", func() {
-			err := fs.Chown("symlinked.txt")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-
-		g.It("cannot chown a directory symlinked outside the directory root", func() {
-			err := fs.Chown("external_dir")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-	})
-
-	g.Describe("Copy", func() {
-		g.It("cannot copy a file symlinked outside the directory root", func() {
-			err := fs.Copy("symlinked.txt")
-			g.Assert(err).IsNotNil()
-			g.Assert(IsBadPathResolutionError(err)).IsTrue()
-		})
-	})
-
-	g.Describe("Delete", func() {
-		g.It("deletes the symlinked file but leaves the source", func() {
-			err := fs.Delete("symlinked.txt")
-			g.Assert(err).IsNil()
-
-			_, err = os.Stat(filepath.Join(rfs.root, "malicious.txt"))
-			g.Assert(err).IsNil()
-
-			_, err = rfs.StatServerFile("symlinked.txt")
-			g.Assert(err).IsNotNil()
-			g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue()
-		})
-	})
-
-	rfs.reset()
-}
-
 func TestFilesystem_Readfile(t *testing.T) {
 	g := Goblin(t)
 	fs, rfs := NewFs()

server/filesystem/path_test.go

@@ -0,0 +1,228 @@
+package filesystem
+
+import (
+	"bytes"
+	. "github.com/franela/goblin"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestFilesystem_Path(t *testing.T) {
+	g := Goblin(t)
+	fs, rfs := NewFs()
+
+	g.Describe("Path", func() {
+		g.It("returns the root path for the instance", func() {
+			g.Assert(fs.Path()).Equal(filepath.Join(rfs.root, "/server"))
+		})
+	})
+}
+
+func TestFilesystem_SafePath(t *testing.T) {
+	g := Goblin(t)
+	fs, rfs := NewFs()
+	prefix := filepath.Join(rfs.root, "/server")
+
+	g.Describe("SafePath", func() {
+		g.It("returns a cleaned path to a given file", func() {
+			p, err := fs.SafePath("test.txt")
+			g.Assert(err).IsNil()
+			g.Assert(p).Equal(prefix + "/test.txt")
+
+			p, err = fs.SafePath("/test.txt")
+			g.Assert(err).IsNil()
+			g.Assert(p).Equal(prefix + "/test.txt")
+
+			p, err = fs.SafePath("./test.txt")
+			g.Assert(err).IsNil()
+			g.Assert(p).Equal(prefix + "/test.txt")
+
+			p, err = fs.SafePath("/foo/../test.txt")
+			g.Assert(err).IsNil()
+			g.Assert(p).Equal(prefix + "/test.txt")
+
+			p, err = fs.SafePath("/foo/bar")
+			g.Assert(err).IsNil()
+			g.Assert(p).Equal(prefix + "/foo/bar")
+		})
+
+		g.It("handles root directory access", func() {
+			p, err := fs.SafePath("/")
+			g.Assert(err).IsNil()
+			g.Assert(p).Equal(prefix)
+
+			p, err = fs.SafePath("")
+			g.Assert(err).IsNil()
+			g.Assert(p).Equal(prefix)
+		})
+
+		g.It("removes trailing slashes from paths", func() {
+			p, err := fs.SafePath("/foo/bar/")
+			g.Assert(err).IsNil()
+			g.Assert(p).Equal(prefix + "/foo/bar")
+		})
+
+		g.It("handles deeply nested directories that do not exist", func() {
+			p, err := fs.SafePath("/foo/bar/baz/quaz/../../ducks/testing.txt")
+			g.Assert(err).IsNil()
+			g.Assert(p).Equal(prefix + "/foo/bar/ducks/testing.txt")
+		})
+
+		g.It("blocks access to files outside the root directory", func() {
+			p, err := fs.SafePath("../test.txt")
+			g.Assert(err).IsNotNil()
+			g.Assert(IsBadPathResolutionError(err)).IsTrue()
+			g.Assert(p).Equal("")
+
+			p, err = fs.SafePath("/../test.txt")
+			g.Assert(err).IsNotNil()
+			g.Assert(IsBadPathResolutionError(err)).IsTrue()
+			g.Assert(p).Equal("")
+
+			p, err = fs.SafePath("./foo/../../test.txt")
+			g.Assert(err).IsNotNil()
+			g.Assert(IsBadPathResolutionError(err)).IsTrue()
+			g.Assert(p).Equal("")
+
+			p, err = fs.SafePath("..")
+			g.Assert(err).IsNotNil()
+			g.Assert(IsBadPathResolutionError(err)).IsTrue()
+			g.Assert(p).Equal("")
+		})
+	})
+}
+
+// We test against accessing files outside the root directory in the tests, however it
+// is still possible for someone to mess up and not properly use this safe path call. In
+// order to truly confirm this, we'll try to pass in a symlinked malicious file to all of
+// the calls and ensure they all fail with the same reason.
+func TestFilesystem_Blocks_Symlinks(t *testing.T) {
+	g := Goblin(t)
+	fs, rfs := NewFs()
+
+	if err := rfs.CreateServerFile("/../malicious.txt", "external content"); err != nil {
+		panic(err)
+	}
+
+	if err := os.Mkdir(filepath.Join(rfs.root, "/malicious_dir"), 0777); err != nil {
+		panic(err)
+	}
+
+	if err := os.Symlink(filepath.Join(rfs.root, "malicious.txt"), filepath.Join(rfs.root, "/server/symlinked.txt")); err != nil {
+		panic(err)
+	}
+
+	if err := os.Symlink(filepath.Join(rfs.root, "/malicious_dir"), filepath.Join(rfs.root, "/server/external_dir")); err != nil {
+		panic(err)
+	}
+
+	g.Describe("Readfile", func() {
+		g.It("cannot read a file symlinked outside the root", func() {
+			b := bytes.Buffer{}
+
+			err := fs.Readfile("symlinked.txt", &b)
+			g.Assert(err).IsNotNil()
+			g.Assert(IsBadPathResolutionError(err)).IsTrue()
+		})
+	})
+
+	g.Describe("Writefile", func() {
+		g.It("cannot write to a file symlinked outside the root", func() {
+			r := bytes.NewReader([]byte("testing"))
+
+			err := fs.Writefile("symlinked.txt", r)
+			g.Assert(err).IsNotNil()
+			g.Assert(IsBadPathResolutionError(err)).IsTrue()
+		})
+
+		g.It("cannot write a file to a directory symlinked outside the root", func() {
+			r := bytes.NewReader([]byte("testing"))
+
+			err := fs.Writefile("external_dir/foo.txt", r)
+			g.Assert(err).IsNotNil()
+			g.Assert(IsBadPathResolutionError(err)).IsTrue()
+		})
+	})
+	//
+	// g.Describe("CreateDirectory", func() {
+	// 	g.It("cannot create a directory outside the root", func() {
+	// 		err := fs.CreateDirectory("my_dir", "external_dir")
+	// 		g.Assert(err).IsNotNil()
+	// 		g.Assert(IsBadPathResolutionError(err)).IsTrue()
+	// 	})
+	//
+	// 	g.It("cannot create a nested directory outside the root", func() {
+	// 		err := fs.CreateDirectory("my/nested/dir", "external_dir/foo/bar")
+	// 		g.Assert(err).IsNotNil()
+	// 		g.Assert(IsBadPathResolutionError(err)).IsTrue()
+	// 	})
+	//
+	// 	g.It("cannot create a nested directory outside the root", func() {
+	// 		err := fs.CreateDirectory("my/nested/dir", "external_dir/server")
+	// 		g.Assert(err).IsNotNil()
+	// 		g.Assert(IsBadPathResolutionError(err)).IsTrue()
+	// 	})
+	// })
+	//
+	// g.Describe("Rename", func() {
+	// 	g.It("cannot rename a file symlinked outside the directory root", func() {
+	// 		err := fs.Rename("symlinked.txt", "foo.txt")
+	// 		g.Assert(err).IsNotNil()
+	// 		g.Assert(IsBadPathResolutionError(err)).IsTrue()
+	// 	})
+	//
+	// 	g.It("cannot rename a symlinked directory outside the root", func() {
+	// 		err := fs.Rename("external_dir", "foo")
+	// 		g.Assert(err).IsNotNil()
+	// 		g.Assert(IsBadPathResolutionError(err)).IsTrue()
+	// 	})
+	//
+	// 	g.It("cannot rename a file to a location outside the directory root", func() {
+	// 		rfs.CreateServerFile("my_file.txt", "internal content")
+	//
+	// 		err := fs.Rename("my_file.txt", "external_dir/my_file.txt")
+	// 		g.Assert(err).IsNotNil()
+	// 		g.Assert(IsBadPathResolutionError(err)).IsTrue()
+	// 	})
+	// })
+	//
+	// g.Describe("Chown", func() {
+	// 	g.It("cannot chown a file symlinked outside the directory root", func() {
+	// 		err := fs.Chown("symlinked.txt")
+	// 		g.Assert(err).IsNotNil()
+	// 		g.Assert(IsBadPathResolutionError(err)).IsTrue()
+	// 	})
+	//
+	// 	g.It("cannot chown a directory symlinked outside the directory root", func() {
+	// 		err := fs.Chown("external_dir")
+	// 		g.Assert(err).IsNotNil()
+	// 		g.Assert(IsBadPathResolutionError(err)).IsTrue()
+	// 	})
+	// })
+	//
+	// g.Describe("Copy", func() {
+	// 	g.It("cannot copy a file symlinked outside the directory root", func() {
+	// 		err := fs.Copy("symlinked.txt")
+	// 		g.Assert(err).IsNotNil()
+	// 		fmt.Printf("err: %+v\n", err)
+	// 		g.Assert(IsBadPathResolutionError(err)).IsTrue()
+	// 	})
+	// })
+	//
+	// g.Describe("Delete", func() {
+	// 	g.It("deletes the symlinked file but leaves the source", func() {
+	// 		err := fs.Delete("symlinked.txt")
+	// 		g.Assert(err).IsNil()
+	//
+	// 		_, err = os.Stat(filepath.Join(rfs.root, "malicious.txt"))
+	// 		g.Assert(err).IsNil()
+	//
+	// 		_, err = rfs.StatServerFile("symlinked.txt")
+	// 		g.Assert(err).IsNotNil()
+	// 		g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue()
+	// 	})
+	// })
+
+	rfs.reset()
+}