Add tests for NormalizePath and IsAbs

Signed-off-by: Gabriel Adrian Samfira <[email protected]>

util/system/path.go

@@ -82,3 +82,20 @@ func NormalizeWorkdir(current, wd string) (string, error) {
 	// slashes in CWD.
 	return filepath.FromSlash(wd), nil
 }
+
+// IsAbs returns a boolean value indicating whether or not the path
+// is absolute. On Linux, this is just a wrapper for filepath.IsAbs().
+// On Windows, we strip away the drive letter (if any), clean the path,
+// and check whether or not the path starts with a filepath.Separator.
+// This function is meant to check if a path is absolute, in the context
+// of a COPY, ADD or WORKDIR, which have their root set in the mount point
+// of the writable layer we are mutating. The filepath.IsAbs() function on
+// Windows will not work in these scenatios, as it will return true for paths
+// that:
+//   - Begin with drive letter (DOS style paths)
+//   - Are volume paths \\?\Volume{UUID}
+//   - Are UNC paths
+//   - Are a reserved name (COM, AUX, NUL, etc)
+func IsAbs(pth string) bool {
+	return isAbs(pth)
+}

util/system/path_unix.go

@@ -13,6 +13,6 @@ func CheckSystemDriveAndRemoveDriveLetter(path string) (string, error) {
 	return path, nil
 }
 
-func IsAbs(pth string) bool {
+func isAbs(pth string) bool {
 	return filepath.IsAbs(pth)
 }

util/system/path_unix_test.go

@@ -0,0 +1,90 @@
+//go:build !windows
+// +build !windows
+
+package system
+
+import (
+	"testing"
+
+	"github.com/pkg/errors"
+	"github.com/stretchr/testify/require"
+)
+
+func TestNormalizeWorkdir(t *testing.T) {
+	testCases := []struct {
+		name           string
+		currentWorkdir string
+		newWorkDir     string
+		desiredResult  string
+		err            string
+	}{
+		{
+			name:           "no current wd with relative wd",
+			currentWorkdir: "",
+			newWorkDir:     "test",
+			desiredResult:  `/test`,
+			err:            "",
+		},
+		{
+			name:           "no current wd with absolute wd",
+			currentWorkdir: "",
+			newWorkDir:     `/strippedWd`,
+			desiredResult:  `/strippedWd`,
+			err:            "",
+		},
+		{
+			name:           "current wd is absolute, new wd is relative",
+			currentWorkdir: "/test",
+			newWorkDir:     `subdir`,
+			desiredResult:  `/test/subdir`,
+			err:            "",
+		},
+		{
+			name:           "current wd is absolute, new wd is relative one folder up",
+			currentWorkdir: "/test",
+			newWorkDir:     `../subdir`,
+			desiredResult:  `/subdir`,
+			err:            "",
+		},
+		{
+			name:           "current wd is absolute, new wd is absolute",
+			currentWorkdir: "/test",
+			newWorkDir:     `/current`,
+			desiredResult:  `/current`,
+			err:            "",
+		},
+		{
+			name:           "current wd is relative, new wd is relative",
+			currentWorkdir: "test",
+			newWorkDir:     `current`,
+			desiredResult:  `/test/current`,
+			err:            "",
+		},
+		{
+			name:           "current wd is relative, no new wd",
+			currentWorkdir: "test",
+			newWorkDir:     "",
+			desiredResult:  `/test`,
+			err:            "",
+		},
+		{
+			name:           "current wd is absolute, no new wd",
+			currentWorkdir: "/test",
+			newWorkDir:     "",
+			desiredResult:  `/test`,
+			err:            "",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result, err := NormalizeWorkdir(tc.currentWorkdir, tc.newWorkDir)
+			if tc.err != "" {
+				require.EqualError(t, errors.Cause(err), tc.err)
+			} else {
+				require.NoError(t, err)
+			}
+			require.Equal(t, tc.desiredResult, result)
+		})
+	}
+}

util/system/path_windows.go

@@ -25,16 +25,55 @@ func CheckSystemDriveAndRemoveDriveLetter(path string) (string, error) {
 	if len(path) == 2 && string(path[1]) == ":" {
 		return "", errors.Errorf("No relative path specified in %q", path)
 	}
-	if !filepath.IsAbs(path) || len(path) < 2 {
+	// On Windows, filepath.IsAbs() will return true for paths that:
+	//   * Begin with drive letter (DOS style paths)
+	//   * Are volume paths \\?\Volume{UUID}
+	//   * Are UNC paths
+	//   * Are a reserved name (COM, AUX, NUL, etc)
+	// Using filepath.IsAbs() here, will yield undesirable results, as a proper, valid absolute
+	// path that is a UNC path (for example), will skip any !filepath.IsAbs() check, and reach
+	// the end of this function, where we would simply strip the first two characters (which in
+	// a UNC path are "\\") and return a string that makes no sense. In the context of what we're
+	// using this function for, we only care about DOS style absolute paths.
+	//
+	// On Windows, the ":" character is illegal inside a folder or fie name, mainly because
+	// it's used to delimit drive letters from the rest of the path that resides inside that
+	// DOS path.
+	// Thus, it's easier to simply split the path by ":", and regard the first element as the drive
+	// letter, and the second one as the path.
+	// NOTE: This function only cares about DOS style absolute paths. If the path is any other kind of
+	// absolute path, we will return it here, without mangling it, aside from cleaning it and
+	// making sure the windows path delimiter is used (which should be fine).
+	//
+	// By not mangling the path, we allow any subsequent check to determine if the path is suitable for
+	// whatever need exists.
+	// Alternatively, we could further validate the path and error out here, but that seems outside the
+	// scope of what this function should do.
+	parts := strings.SplitN(path, ":", 2)
+	if len(parts) == 1 || len(path) < 2 {
 		return filepath.FromSlash(filepath.Clean(path)), nil
 	}
-	if string(path[1]) == ":" && !strings.EqualFold(string(path[0]), "c") {
+	if !strings.EqualFold(parts[0], "c") {
 		return "", errors.New("The specified path is not on the system drive (C:)")
 	}
-	return filepath.FromSlash(filepath.Clean(path[2:])), nil
+
+	// Apparently, a path of the form F:somepath, seems to be valid in a cmd shell:
+	// C:\>mkdir F:somepath
+	// C:\>dir F:\
+	// Volume in drive F is New Volume
+	// Volume Serial Number is 86E5-AB64
+	//
+	// Directory of F:\
+	//
+	// 11/27/2022  02:22 PM    <DIR>          somepath
+	// 			0 File(s)              0 bytes
+	// 			1 Dir(s)   1,052,876,800 bytes free
+	//
+	// Join with "/" and return it
+	return filepath.Join("/", filepath.Clean(parts[1])), nil
 }
 
-func IsAbs(pth string) bool {
+func isAbs(pth string) bool {
 	cleanedPath, err := CheckSystemDriveAndRemoveDriveLetter(pth)
 	if err != nil {
 		return false

util/system/path_windows_test.go

@@ -3,7 +3,12 @@
 
 package system
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/pkg/errors"
+	"github.com/stretchr/testify/require"
+)
 
 // TestCheckSystemDriveAndRemoveDriveLetter tests CheckSystemDriveAndRemoveDriveLetter
 func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) {
@@ -79,3 +84,202 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) {
 		t.Fatalf(path, err)
 	}
 }
+
+// TestNormalizeWorkdir tests NormalizeWorkdir
+func TestNormalizeWorkdir(t *testing.T) {
+	testCases := []struct {
+		name           string
+		currentWorkdir string
+		newWorkDir     string
+		desiredResult  string
+		err            string
+	}{
+		{
+			name:           "no current wd with relative wd",
+			currentWorkdir: "",
+			newWorkDir:     "test",
+			desiredResult:  `\test`,
+			err:            "",
+		},
+		{
+			name:           "no current wd with stripped absolute wd",
+			currentWorkdir: "",
+			newWorkDir:     `\strippedWd`,
+			desiredResult:  `\strippedWd`,
+			err:            "",
+		},
+		{
+			name:           "no current wd with absolute wd",
+			currentWorkdir: "",
+			newWorkDir:     `C:\withDriveLetter`,
+			desiredResult:  `\withDriveLetter`,
+			err:            "",
+		},
+		{
+			name:           "no current wd with absolute wd with forward slash",
+			currentWorkdir: "",
+			newWorkDir:     `C:/withDriveLetterAndForwardSlash`,
+			desiredResult:  `\withDriveLetterAndForwardSlash`,
+			err:            "",
+		},
+		{
+			name:           "no current wd with absolute wd with mixed slashes",
+			currentWorkdir: "",
+			newWorkDir:     `C:/first\second/third`,
+			desiredResult:  `\first\second\third`,
+			err:            "",
+		},
+		{
+			name:           "current wd is relative no wd",
+			currentWorkdir: "testing",
+			newWorkDir:     "",
+			desiredResult:  `\testing`,
+			err:            "",
+		},
+		{
+			name:           "current wd is relative with relative wd",
+			currentWorkdir: "testing",
+			newWorkDir:     "newTesting",
+			desiredResult:  `\testing\newTesting`,
+			err:            "",
+		},
+		{
+			name:           "current wd is relative withMixedSlashes and relative new wd",
+			currentWorkdir: `testing/with\mixed/slashes`,
+			newWorkDir:     "newTesting",
+			desiredResult:  `\testing\with\mixed\slashes\newTesting`,
+			err:            "",
+		},
+		{
+			name:           "current wd is absolute withMixedSlashes and relative new wd",
+			currentWorkdir: `C:\testing/with\mixed/slashes`,
+			newWorkDir:     "newTesting",
+			desiredResult:  `\testing\with\mixed\slashes\newTesting`,
+			err:            "",
+		},
+		{
+			name:           "current wd is absolute withMixedSlashes and no new wd",
+			currentWorkdir: `C:\testing/with\mixed/slashes`,
+			newWorkDir:     "",
+			desiredResult:  `\testing\with\mixed\slashes`,
+			err:            "",
+		},
+		{
+			name:           "current wd is absolute path to non C drive",
+			currentWorkdir: `D:\IWillErrorOut`,
+			newWorkDir:     "doesNotMatter",
+			desiredResult:  "",
+			err:            "The specified path is not on the system drive (C:)",
+		},
+		{
+			name:           "new WD is an absolute path to illegal drive",
+			currentWorkdir: `C:\testing`,
+			newWorkDir:     `D:\testing`,
+			desiredResult:  "",
+			err:            "The specified path is not on the system drive (C:)",
+		},
+		{
+			name:           "current WD has no relative path to drive",
+			currentWorkdir: `C:`,
+			newWorkDir:     `testing`,
+			desiredResult:  "",
+			err:            `No relative path specified in "C:"`,
+		},
+		{
+			name:           "new WD has no relative path to drive",
+			currentWorkdir: `/test`,
+			newWorkDir:     `C:`,
+			desiredResult:  "",
+			err:            `No relative path specified in "C:"`,
+		},
+		{
+			name:           "new WD has no slash after drive letter",
+			currentWorkdir: `/test`,
+			newWorkDir:     `C:testing`,
+			desiredResult:  `\testing`,
+			err:            "",
+		},
+		{
+			name:           "current WD is an unlikely absolute path",
+			currentWorkdir: `C:\..\test\..\`,
+			newWorkDir:     ``,
+			desiredResult:  `\`,
+			err:            "",
+		},
+		{
+			name:           "linux style paths should work",
+			currentWorkdir: "/test",
+			newWorkDir:     "relative/path",
+			desiredResult:  `\test\relative\path`,
+			err:            "",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result, err := NormalizeWorkdir(tc.currentWorkdir, tc.newWorkDir)
+			if tc.err != "" {
+				require.EqualError(t, errors.Cause(err), tc.err)
+			} else {
+				require.NoError(t, err)
+			}
+			require.Equal(t, tc.desiredResult, result)
+		})
+	}
+}
+
+func TestIsAbs(t *testing.T) {
+	testCases := []struct {
+		name          string
+		path          string
+		desiredResult bool
+	}{
+		{
+			name:          "path with drive letter is absolute",
+			path:          `C:\test`,
+			desiredResult: true,
+		},
+		{
+			name:          "path with drive letter but no slash is absolute",
+			path:          `C:test`,
+			desiredResult: true,
+		},
+		{
+			name:          "path with drive letter and linux style slashes is absolute",
+			path:          `C:/test`,
+			desiredResult: true,
+		},
+		{
+			name:          "path without drive letter but with leading slash is absolute",
+			path:          `\test`,
+			desiredResult: true,
+		},
+		{
+			name:          "path without drive letter but with leading forward slash is absolute",
+			path:          `/test`,
+			desiredResult: true,
+		},
+		{
+			name:          "simple relative path",
+			path:          `test`,
+			desiredResult: false,
+		},
+		{
+			name:          "deeper relative path",
+			path:          `test/nested`,
+			desiredResult: false,
+		},
+		{
+			name:          "one level up relative path",
+			path:          `../test`,
+			desiredResult: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := IsAbs(tc.path)
+			require.Equal(t, tc.desiredResult, result)
+		})
+	}
+}