From 68bdcb3cbcd096f7d49038ae7deb4f6c63f7d069 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sun, 8 Nov 2020 15:15:39 -0800 Subject: [PATCH] Fix error handling and get tests back to working --- cmd/root.go | 2 +- router/error.go | 2 +- router/router_transfer.go | 2 +- server/filesystem/disk_space.go | 2 +- server/filesystem/errors.go | 8 +- server/filesystem/errors_test.go | 24 +++ server/filesystem/filesystem.go | 17 +- server/filesystem/filesystem_test.go | 218 ------------------------- server/filesystem/path_test.go | 228 +++++++++++++++++++++++++++ 9 files changed, 273 insertions(+), 230 deletions(-) create mode 100644 server/filesystem/errors_test.go create mode 100644 server/filesystem/path_test.go diff --git a/cmd/root.go b/cmd/root.go index 61533b2..34a79a5 100644 --- a/cmd/root.go +++ b/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() } diff --git a/router/error.go b/router/error.go index 63369de..1196101 100644 --- a/router/error.go +++ b/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{ diff --git a/router/router_transfer.go b/router/router_transfer.go index f2df212..94a3890 100644 --- a/router/router_transfer.go +++ b/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 } diff --git a/server/filesystem/disk_space.go b/server/filesystem/disk_space.go index 8c0a7ff..2f233d7 100644 --- a/server/filesystem/disk_space.go +++ b/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() { diff --git a/server/filesystem/errors.go b/server/filesystem/errors.go index e44ad53..4e8f90f 100644 --- a/server/filesystem/errors.go +++ b/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 } diff --git a/server/filesystem/errors_test.go b/server/filesystem/errors_test.go new file mode 100644 index 0000000..e860cc6 --- /dev/null +++ b/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 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: ") + }) + }) +} diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index be1cdc0..e58cb5e 100644 --- a/server/filesystem/filesystem.go +++ b/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) { - return "", err + if !errors.Is(err, os.ErrNotExist) { + 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) } diff --git a/server/filesystem/filesystem_test.go b/server/filesystem/filesystem_test.go index 49ab4f5..c39eacc 100644 --- a/server/filesystem/filesystem_test.go +++ b/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() diff --git a/server/filesystem/path_test.go b/server/filesystem/path_test.go new file mode 100644 index 0000000..143268d --- /dev/null +++ b/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() +}