From 94d8474edf3a01e79295e3c2aa834d25f36528f6 Mon Sep 17 00:00:00 2001
From: Marco Molteni <marco.molteni@mailbox.org>
Date: Wed, 25 Mar 2020 18:39:31 +0100
Subject: [PATCH] review: rework findInParentTree logic

Signed-off-by: Marco Molteni <marco.molteni@mailbox.org>
---
 internal/command/action.go      | 35 +++++++++++++++++++++------------
 internal/command/action_test.go | 16 +++++++++++++--
 2 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/internal/command/action.go b/internal/command/action.go
index e2d5bbe..6a2e9db 100644
--- a/internal/command/action.go
+++ b/internal/command/action.go
@@ -199,26 +199,35 @@ func joinEnv(env []string) string {
 // If found, returns the absolute path to the file.
 func findInParentTree(secretsFile string, leafDir string) (string, error) {
 	if filepath.IsAbs(secretsFile) {
-		return "", fmt.Errorf("file specified (%s) is an absolute path: will not recurse up", secretsFile)
+		return "", fmt.Errorf(
+			"file specified (%s) is an absolute path: will not recurse up", secretsFile)
 	}
 
 	for {
 		joinedPath := filepath.Join(leafDir, secretsFile)
-		if _, err := os.Stat(joinedPath); err == nil {
-			// File found -- return the current filepath
-			return joinedPath, nil
-		} else if os.IsNotExist(err) {
-			upOne := filepath.Dir(leafDir)
-			if upOne == leafDir {
-				return "", fmt.Errorf(
-					"unable to locate file specified (%s): reached root of file system", secretsFile)
+
+		_, err := os.Stat(joinedPath)
+
+		if err != nil {
+			// If the file is not present, we just move up one level and run the next loop
+			// iteration
+			if os.IsNotExist(err) {
+				upOne := filepath.Dir(leafDir)
+				if upOne == leafDir {
+					return "", fmt.Errorf(
+						"unable to locate file specified (%s): reached root of file system", secretsFile)
+				}
+
+				leafDir = upOne
+				continue
 			}
-			// Move up to parent dir
-			leafDir = upOne
-		} else {
-			// Any other error
+
+			// If we have an unexpected error, we fail-fast
 			return "", fmt.Errorf("unable to locate file specified (%s): %s", secretsFile, err)
 		}
+
+		// If there's no error, we found the file so we return it
+		return joinedPath, nil
 	}
 }
 
diff --git a/internal/command/action_test.go b/internal/command/action_test.go
index 7b845af..f690a43 100644
--- a/internal/command/action_test.go
+++ b/internal/command/action_test.go
@@ -302,10 +302,22 @@ func TestLocateFileRecurseUp(t *testing.T) {
 		So(err, ShouldBeNil)
 		defer os.RemoveAll(topDir)
 
-		badFileName := "/foo/bar/baz"
+		absFileName := "/foo/bar/baz"
 		wantErrMsg := "file specified (/foo/bar/baz) is an absolute path: will not recurse up"
 
-		_, err = findInParentTree(badFileName, topDir)
+		_, err = findInParentTree(absFileName, topDir)
 		So(err.Error(), ShouldEqual, wantErrMsg)
 	})
+
+	Convey("returns a friendly error in unexpected circumstances (100% coverage)", t, func() {
+		topDir, err := ioutil.TempDir("", "summon")
+		So(err, ShouldBeNil)
+		defer os.RemoveAll(topDir)
+
+		fileNameWithNulByte := "pizza\x00margherita"
+		wantErrMsg := "unable to locate file specified (pizza\x00margherita): stat"
+
+		_, err = findInParentTree(fileNameWithNulByte, topDir)
+		So(err.Error(), ShouldStartWith, wantErrMsg)
+	})
 }