Directory paths are rejected from --tarball in the create-release

command

[#140896431](https://www.pivotaltracker.com/story/show/140896431)

Signed-off-by: Tyler Schultz <tschultz@pivotal.io>

cmd/create_release.go

@@ -53,27 +53,19 @@ func (c CreateReleaseCmd) Run(opts CreateReleaseOpts) (boshrel.Release, error) {
 		}
 	}
 
-	var archivePath string
-
-	if opts.Tarball != "" {
-
-		archivePath, err = c.fs.ExpandPath(opts.Tarball)
-		if err != nil {
-			return nil, err
-		}
-
+	if opts.Tarball.ExpandedPath != "" {
 		path, err := c.releaseWriter.Write(release, nil)
 		if err != nil {
 			return nil, err
 		}
 
-		err = boshfu.NewFileMover(c.fs).Move(path, archivePath)
+		err = boshfu.NewFileMover(c.fs).Move(path, opts.Tarball.ExpandedPath)
 		if err != nil {
 			return nil, bosherr.WrapErrorf(err, "Moving release archive to final destination")
 		}
 	}
 
-	ReleaseTables{Release: release, ArchivePath: archivePath}.Print(c.ui)
+	ReleaseTables{Release: release, ArchivePath: opts.Tarball.ExpandedPath}.Print(c.ui)
 
 	return release, nil
 }

cmd/create_release_test.go

@@ -101,7 +101,7 @@ var _ = Describe("CreateReleaseCmd", func() {
 
 			Context("with tarball", func() {
 				BeforeEach(func() {
-					opts.Tarball = "/tarball-destination.tgz"
+					opts.Tarball = FileArg{ExpandedPath: "/tarball-destination.tgz"}
 				})
 
 				It("builds release and release archive based on manifest path", func() {
@@ -155,44 +155,6 @@ var _ = Describe("CreateReleaseCmd", func() {
 					Expect(err).To(HaveOccurred())
 					Expect(err.Error()).To(ContainSubstring("fake-err"))
 				})
-
-				Context("with a path that needs expanding", func() {
-					BeforeEach(func() {
-						opts.Tarball = "~/../tarball-destination.tgz"
-						fakeFS.ExpandPathExpanded = "/tarball-destination.tgz"
-					})
-
-					It("builds the tarball to the expanded path", func() {
-						fakeWriter.WriteStub = func(rel boshrel.Release, skipPkgs []string) (string, error) {
-							Expect(rel).To(Equal(release))
-
-							fakeFS.WriteFileString("/temp-tarball.tgz", "release content blah")
-							return "/temp-tarball.tgz", nil
-						}
-
-						err := act()
-						Expect(err).ToNot(HaveOccurred())
-
-						Expect(fakeFS.ExpandPathPath).To(Equal("~/../tarball-destination.tgz"))
-						Expect(fakeFS.FileExists("/temp-tarball.tgz")).To(BeFalse())
-						content, err := fakeFS.ReadFileString("/tarball-destination.tgz")
-						Expect(err).ToNot(HaveOccurred())
-						Expect(content).To(Equal("release content blah"))
-					})
-
-					Context("when expanding the path fails", func() {
-						BeforeEach(func() {
-							fakeFS.ExpandPathErr = errors.New("bad-expand-path")
-						})
-
-						It("returns the err", func() {
-							err := act()
-							Expect(err).To(HaveOccurred())
-
-							Expect(err.Error()).To(ContainSubstring("bad-expand-path"))
-						})
-					})
-				})
 			})
 		})
 
@@ -347,7 +309,7 @@ var _ = Describe("CreateReleaseCmd", func() {
 
 			It("builds release and archive if building archive is requested", func() {
 				opts.Final = true
-				opts.Tarball = "/archive-path"
+				opts.Tarball = FileArg{ExpandedPath: "/archive-path"}
 
 				releaseDir.DefaultNameReturns("default-rel-name", nil)
 				releaseDir.NextDevVersionReturns(semver.MustNewVersionFromString("next-dev+ver"), nil)
@@ -413,7 +375,7 @@ var _ = Describe("CreateReleaseCmd", func() {
 			})
 
 			It("returns error if building release archive fails", func() {
-				opts.Tarball = "/tarball/dest/path.tgz"
+				opts.Tarball = FileArg{ExpandedPath: "/tarball/dest/path.tgz"}
 
 				fakeWriter.WriteStub = func(rel boshrel.Release, skipPkgs []string) (string, error) {
 					return "", errors.New("fake-err")

cmd/file_arg.go

@@ -0,0 +1,32 @@
+package cmd
+
+import (
+	bosherr "github.com/cloudfoundry/bosh-utils/errors"
+	boshsys "github.com/cloudfoundry/bosh-utils/system"
+)
+
+type FileArg struct {
+	ExpandedPath string
+	FS           boshsys.FileSystem
+}
+
+func (a *FileArg) UnmarshalFlag(data string) error {
+	expandedPath, err := a.FS.ExpandPath(data)
+	if err != nil {
+		return bosherr.WrapErrorf(err, "checking file path")
+	}
+	a.ExpandedPath = expandedPath
+
+	if a.FS.FileExists(expandedPath) {
+		stat, err := a.FS.Stat(expandedPath)
+		if err != nil {
+			return bosherr.WrapErrorf(err, "checking file path")
+		}
+
+		if stat.IsDir() {
+			return bosherr.Errorf("path must not be directory")
+		}
+	}
+
+	return nil
+}

cmd/file_arg_test.go

@@ -0,0 +1,85 @@
+package cmd_test
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	. "github.com/cloudfoundry/bosh-cli/cmd"
+
+	"errors"
+	sysfakes "github.com/cloudfoundry/bosh-utils/system/fakes"
+	"os"
+)
+
+var _ = Describe("FileArg", func() {
+	Describe("UnmarshalFlag", func() {
+		var (
+			arg *FileArg
+			fs  *sysfakes.FakeFileSystem
+		)
+
+		BeforeEach(func() {
+			fs = sysfakes.NewFakeFileSystem()
+			arg = &FileArg{FS: fs}
+
+			fs.MkdirAll("/some/empty/dir", os.ModeDir)
+			fs.MkdirAll("/some/dir", os.ModeDir)
+			fs.WriteFileString("stuff", "/some/dir/contents")
+		})
+
+		Context("when the given path cannot be expanded in the file system", func() {
+			It("reports this as an error", func() {
+				fs.ExpandPathErr = errors.New("can't expand")
+				err := arg.UnmarshalFlag("kaboom")
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(Equal("checking file path: can't expand"))
+			})
+		})
+
+		Context("when the given path can be expanded in the file system", func() {
+			Context("when there is no file at the given path", func() {
+				It("returns with ExpandedPath set", func() {
+					Expect(arg.UnmarshalFlag("/some/dir/newball")).To(Succeed())
+					Expect(arg.ExpandedPath).To(Equal("/some/dir/newball"))
+				})
+
+				It("expands the path before setting", func() {
+					fs.ExpandPathExpanded = "expanded/path"
+					Expect(arg.UnmarshalFlag("newball")).To(Succeed())
+					Expect(arg.ExpandedPath).To(Equal("expanded/path"))
+					Expect(fs.ExpandPathPath).To(Equal("newball"))
+				})
+
+				It("propagates an empty string input path through to ExpandedPath", func() {
+					Expect(arg.UnmarshalFlag("")).To(Succeed())
+					Expect(arg.ExpandedPath).To(Equal(""))
+				})
+			})
+
+			Context("when the filesystem errors while stat'ing the file", func() {
+				It("returns an error", func() {
+					fs.WriteFileString("/some/tarball/path.tgz", "it exists")
+					file := sysfakes.NewFakeFile("/some/tarball/path.tgz", fs)
+					file.StatErr = errors.New("can't stat me!")
+					fs.RegisterOpenFile("/some/tarball/path.tgz", file)
+
+					err := arg.UnmarshalFlag("/some/tarball/path.tgz")
+					Expect(err).To(HaveOccurred())
+					Expect(err.Error()).To(Equal("checking file path: can't stat me!"))
+				})
+			})
+
+			Context("when there is already a file at that location", func() {
+				It("allows paths to existing files", func() {
+					Expect(arg.UnmarshalFlag("/some/dir/contents")).To(Succeed())
+				})
+
+				It("rejects paths to directories", func() {
+					err := arg.UnmarshalFlag("/some/dir")
+					Expect(err).To(HaveOccurred())
+					Expect(err.Error()).To(Equal("path must not be directory"))
+				})
+			})
+		})
+	})
+})

cmd/opts.go

@@ -814,9 +814,9 @@ type CreateReleaseOpts struct {
 	Version          VersionArg `long:"version"            description:"Custom release version (e.g.: 1.0.0, 1.0-beta.2+dev.10)"`
 	TimestampVersion bool       `long:"timestamp-version"  description:"Create release with the timestamp as the dev version (e.g.: 1+dev.TIMESTAMP)"`
 
-	Final   bool   `long:"final"   description:"Make it a final release"`
-	Tarball string `long:"tarball" description:"Create release tarball at path (e.g. /tmp/release.tgz)"`
-	Force   bool   `long:"force"   description:"Ignore Git dirty state check"`
+	Final   bool    `long:"final"   description:"Make it a final release"`
+	Tarball FileArg `long:"tarball" description:"Create release tarball at path (e.g. /tmp/release.tgz)"`
+	Force   bool    `long:"force"   description:"Ignore Git dirty state check"`
 
 	cmd
 }

integration/create_release_test.go

@@ -17,6 +17,7 @@ import (
 	boshrelman "github.com/cloudfoundry/bosh-cli/release/manifest"
 	boshui "github.com/cloudfoundry/bosh-cli/ui"
 	fakeui "github.com/cloudfoundry/bosh-cli/ui/fakes"
+	"os"
 )
 
 var _ = Describe("create-release command", func() {
@@ -234,6 +235,14 @@ license:
 			Expect(fs.ReadFileString(filepath.Join(pkg1.ExtractedPath(), "in-blobs"))).To(Equal("in-blobs"))
 		}
 
+		{ // Check that tarballs will not overwrite a directory
+			directoryPath := filepath.Join(tmpDir, "tarball-collision-dir")
+			Expect(fs.MkdirAll(directoryPath, os.ModeDir)).To(Succeed())
+			_, err := cmdFactory.New([]string{"create-release", "--dir", tmpDir, "--tarball", directoryPath})
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("path must not be directory"))
+		}
+
 		{ // removes unknown blobs, keeping known blobs
 			blobPath := filepath.Join(tmpDir, "blobs", "unknown-blob.tgz")