Repository: hairyhenderson/gomplate
Pull request #1915 (2023-11-19 18:24:29)

Fix the wdFS behaviour due to Go 1.21.4's changes to IsAbs
Handling of relatively obscure forms of Windows file paths was broken by the security fixes in Go 1.21.4. This PR fixes the logic to be more in-line with how Windows itself categorizes and parses paths.

There's probably an opportunity for more refactoring, but this'll do for now.

This also updates the CI to expose bugs when dealing with non-current volumes on Windows.


Patch:
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index daa98bdb..2fb03af9 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -42,18 +42,20 @@ jobs:
   windows-build:
     runs-on: windows-latest
     env:
-      TMP: D:\a\tmp
+      # note: the tmp dir is set to C: so that it's not on the same drive as the
+      # repo, which is on D: - this will expose bugs with path handling!
+      TMP: C:\tmp
     steps:
       - run: pwd
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
       - uses: actions/setup-go@v4
         with:
-          go-version: '1.21'
+          go-version: '1.21.4'
       - run: |
           git config --global user.email "bogus@example.com"
           git config --global user.name "Someone"
-      - uses: actions/checkout@v4
-        with:
-          fetch-depth: 0
       - run: make build
       - name: Save binary
         uses: actions/upload-artifact@v3
diff --git a/data/datasource_git.go b/data/datasource_git.go
index 02df14be..3859d344 100644
--- a/data/datasource_git.go
+++ b/data/datasource_git.go
@@ -241,7 +241,7 @@ func (g gitsource) clone(ctx context.Context, repoURL *url.URL, depth int) (bill
 		return g.clone(ctx, u, depth)
 	}
 	if err != nil {
-		return nil, nil, fmt.Errorf("git clone for %v failed: %w", repoURL, err)
+		return nil, nil, fmt.Errorf("git clone %s: %w", u, err)
 	}
 	return fs, repo, nil
 }
diff --git a/internal/datafs/fsys.go b/internal/datafs/fsys.go
index ac4c22c5..fb88b0da 100644
--- a/internal/datafs/fsys.go
+++ b/internal/datafs/fsys.go
@@ -7,6 +7,7 @@ import (
 	"net/url"
 	"path"
 	"path/filepath"
+	"strings"
 
 	"github.com/hairyhenderson/go-fsimpl"
 )
@@ -81,7 +82,21 @@ func FSysForPath(ctx context.Context, path string) (fs.FS, error) {
 		return nil, fmt.Errorf("no filesystem provider in context")
 	}
 
-	fsys, err := fsp.New(&url.URL{Scheme: u.Scheme, Path: "/"})
+	// default to "/" so we have a rooted filesystem for all schemes, but also
+	// support volumes on Windows
+	origPath := u.Path
+	if u.Scheme == "file" || strings.HasSuffix(u.Scheme, "+file") || u.Scheme == "" {
+		u.Path, _, err = ResolveLocalPath(origPath)
+		if err != nil {
+			return nil, fmt.Errorf("resolve local path %q: %w", origPath, err)
+		}
+		// if this is a drive letter, add a trailing slash
+		if u.Path[0] != '/' {
+			u.Path += "/"
+		}
+	}
+
+	fsys, err := fsp.New(u)
 	if err != nil {
 		return nil, fmt.Errorf("filesystem provider for %q unavailable: %w", path, err)
 	}
diff --git a/internal/datafs/wdfs.go b/internal/datafs/wdfs.go
index 79c2b0ef..e696e7d1 100644
--- a/internal/datafs/wdfs.go
+++ b/internal/datafs/wdfs.go
@@ -6,6 +6,7 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/hack-pad/hackpadfs"
 	osfs "github.com/hack-pad/hackpadfs/os"
@@ -20,15 +21,15 @@ import (
 //
 // TODO: maybe take fsys as an argument, and if it's a wdFS, use its vol instead
 // of calling os.Getwd?
-func ResolveLocalPath(name string) (root, resolved string) {
+func ResolveLocalPath(name string) (root, resolved string, err error) {
 	// ignore empty names
 	if len(name) == 0 {
-		return "", ""
+		return "", "", nil
 	}
 
 	wd, err := os.Getwd()
 	if err != nil {
-		panic(err)
+		return "", "", fmt.Errorf("getwd: %w", err)
 	}
 
 	vol := filepath.VolumeName(wd)
@@ -40,10 +41,10 @@ func ResolveLocalPath(name string) (root, resolved string) {
 	return f.resolveLocalPath(name)
 }
 
-func (w *wdFS) resolveLocalPath(name string) (root, resolved string) {
+func (w *wdFS) resolveLocalPath(name string) (root, resolved string, err error) {
 	// ignore empty names
 	if len(name) == 0 {
-		return "", ""
+		return "", "", nil
 	}
 
 	// we want to assume a / separator regardless of the OS
@@ -51,17 +52,22 @@ func (w *wdFS) resolveLocalPath(name string) (root, resolved string) {
 
 	// special-case for (Windows) paths that start with '/' but have no volume
 	// name (e.g. "/foo/bar"). UNC paths (beginning with "//") are ignored.
-	if name[0] == '/' && (len(name) == 1 || name[1] != '/') {
+	if name[0] == '/' && (len(name) == 1 || (name[1] != '/' && name[1] != '?')) {
 		name = filepath.Join(w.vol, name)
+		// TODO: maybe this can be reduced to just '!filepath.IsAbs(name)'?
 	} else if name[0] != '/' && !filepath.IsAbs(name) {
-		wd, err := os.Getwd()
+		abs := ""
+		abs, err = filepath.Abs(name)
 		if err != nil {
-			panic(err)
+			return "", "", fmt.Errorf("abs %q: %w", name, err)
 		}
-		name = filepath.Join(wd, name)
+		name = abs
 	}
 
-	name = filepath.ToSlash(name)
+	name, err = normalizeWindowsPath(name)
+	if err != nil {
+		return "", "", fmt.Errorf("normalize %q: %w", name, err)
+	}
 
 	vol := filepath.VolumeName(name)
 	if vol != "" && name != vol {
@@ -80,15 +86,124 @@ func (w *wdFS) resolveLocalPath(name string) (root, resolved string) {
 		name = "."
 	}
 
-	return root, name
+	return root, name, nil
+}
+
+// normalizeWindowsPath - converts the various types of Windows paths to either
+// a rooted or relative path, depending on the type of path.
+func normalizeWindowsPath(name string) (string, error) {
+	name = strings.ReplaceAll(name, `\`, "/")
+
+	switch win32PathType(name) {
+	case winPathUnknown, winPathRootLocalDevice, winPathDriveRelative, winPathNT:
+		return "", fmt.Errorf("unsupported path %q: %w", name, fs.ErrInvalid)
+	case winPathDriveAbsolute, winPathRelative, winPathRooted:
+		// absolute/relative returned as-is
+		return name, nil
+	case winPathUncAbsolute:
+		// UNC paths are returned as-is
+		return name, nil
+	case winPathLocalDevice:
+		// local device paths have the prefix stripped
+		return name[4:], nil
+	default:
+		return "", fmt.Errorf("unknown path type %q: %w", name, fs.ErrInvalid)
+	}
+}
+
+type winPathtype int
+
+// There are 8 types of "DOS" paths in Windows (as opposed to NT paths):
+//
+// NT paths begin with a "\??\" prefix, and are implicitly absolute.
+const (
+	// - Unknown - e.g. "" or some other invalid path
+	winPathUnknown winPathtype = iota
+	// - Drive Absolute - e.g. C:\foo\bar
+	winPathDriveAbsolute
+	// - Drive Relative - e.g. C:foo\bar
+	winPathDriveRelative
+	// - Rooted - e.g. \foo\bar
+	winPathRooted
+	// - Relative - e.g. foo\bar, .\foo\bar, ..\foo\bar
+	winPathRelative
+	// - UNC Absolute - e.g. \\foo\bar
+	winPathUncAbsolute
+	// - Local Device - e.g. \\.\C:\foo\bar, \\.\COM1, \\?\C:\foo\bar
+	winPathLocalDevice
+	// - Root Local Device - e.g. \\. or \\?
+	winPathRootLocalDevice
+	// - NT path - e.g. \??\C:\foo\bar or \??\UNC\foo\bar
+	winPathNT
+)
+
+// win32PathType - returns the type of path, as defined by the win32Path enum
+// See https://googleprojectzero.blogspot.com/2016/02/the-definitive-guide-on-win32-to-nt.html
+// for details on the different types
+func win32PathType(name string) winPathtype {
+	if name == "" {
+		return winPathUnknown
+	}
+
+	// not using filepath.ToSlash here, because we want to be able to test this
+	// on non-Windows systems too
+	name = strings.ReplaceAll(name, `\`, "/")
+
+	// if the first character is a slash, it's either rooted, a UNC, a local device, or root local device path
+	if name[0] == '/' {
+		switch {
+		case len(name) == 1 || (name[1] != '/' && name[1] != '?'):
+			return winPathRooted
+		case len(name) == 2 || (name[2] != '.' && name[2] != '?'):
+			return winPathUncAbsolute
+		case len(name) >= 4 && name[1:4] == "??/":
+			return winPathNT
+		case len(name) >= 4 && name[3] == '/':
+			return winPathLocalDevice
+		default:
+			return winPathRootLocalDevice
+		}
+	}
+
+	switch {
+	case len(name) == 1 || name[1] != ':':
+		return winPathRelative
+	case len(name) == 2 || name[2] != '/':
+		return winPathDriveRelative
+	default:
+		return winPathDriveAbsolute
+	}
+}
+
+func isSupportedPath(name string) bool {
+	switch win32PathType(name) {
+	case winPathUnknown, winPathRootLocalDevice, winPathDriveRelative, winPathNT:
+		return false
+	default:
+		return true
+	}
 }
 
 // WdFS is a filesystem provider that creates local filesystems which support
 // absolute paths beginning with '/', and interpret relative paths as relative
-// to the current working directory (as reported by [os.Getwd])
+// to the current working directory (as reported by [os.Getwd]).
+//
+// On Windows, certain types of paths are not supported, and will return an
+// error. These are:
+// - Drive Relative - e.g. C:foo\bar
+// - Root Local - e.g. \\. or \\?
+// - non-drive Local Devices - e.g. \\.\COM1, \\.\pipe\foo
+// - NT Paths - e.g. \??\C:\foo\bar or \??\UNC\foo\bar
 var WdFS = fsimpl.FSProviderFunc(
 	func(u *url.URL) (fs.FS, error) {
-		vol, _ := ResolveLocalPath(u.Path)
+		if !isSupportedPath(u.Path) {
+			return nil, fmt.Errorf("unsupported path %q: %w", u.Path, fs.ErrInvalid)
+		}
+
+		vol, _, err := ResolveLocalPath(u.Path)
+		if err != nil {
+			return nil, fmt.Errorf("resolve %q: %w", u.Path, err)
+		}
 
 		var fsys fs.FS
 		if vol == "" || vol == "/" {
@@ -165,7 +280,10 @@ func (w *wdFS) fsysFor(vol string) (fs.FS, error) {
 }
 
 func (w *wdFS) Open(name string) (fs.File, error) {
-	root, resolved := w.resolveLocalPath(name)
+	root, resolved, err := w.resolveLocalPath(name)
+	if err != nil {
+		return nil, fmt.Errorf("resolve: %w", err)
+	}
 	fsys, err := w.fsysFor(root)
 	if err != nil {
 		return nil, err
@@ -174,7 +292,10 @@ func (w *wdFS) Open(name string) (fs.File, error) {
 }
 
 func (w *wdFS) Stat(name string) (fs.FileInfo, error) {
-	root, resolved := w.resolveLocalPath(name)
+	root, resolved, err := w.resolveLocalPath(name)
+	if err != nil {
+		return nil, fmt.Errorf("resolve: %w", err)
+	}
 	fsys, err := w.fsysFor(root)
 	if err != nil {
 		return nil, err
@@ -183,7 +304,10 @@ func (w *wdFS) Stat(name string) (fs.FileInfo, error) {
 }
 
 func (w *wdFS) ReadFile(name string) ([]byte, error) {
-	root, resolved := w.resolveLocalPath(name)
+	root, resolved, err := w.resolveLocalPath(name)
+	if err != nil {
+		return nil, fmt.Errorf("resolve: %w", err)
+	}
 	fsys, err := w.fsysFor(root)
 	if err != nil {
 		return nil, err
@@ -192,7 +316,10 @@ func (w *wdFS) ReadFile(name string) ([]byte, error) {
 }
 
 func (w *wdFS) ReadDir(name string) ([]fs.DirEntry, error) {
-	root, resolved := w.resolveLocalPath(name)
+	root, resolved, err := w.resolveLocalPath(name)
+	if err != nil {
+		return nil, fmt.Errorf("resolve: %w", err)
+	}
 	fsys, err := w.fsysFor(root)
 	if err != nil {
 		return nil, err
@@ -217,7 +344,10 @@ func (w *wdFS) Glob(_ string) ([]string, error) {
 }
 
 func (w *wdFS) Create(name string) (fs.File, error) {
-	root, resolved := w.resolveLocalPath(name)
+	root, resolved, err := w.resolveLocalPath(name)
+	if err != nil {
+		return nil, fmt.Errorf("resolve: %w", err)
+	}
 	fsys, err := w.fsysFor(root)
 	if err != nil {
 		return nil, err
@@ -226,7 +356,10 @@ func (w *wdFS) Create(name string) (fs.File, error) {
 }
 
 func (w *wdFS) OpenFile(name string, flag int, perm fs.FileMode) (fs.File, error) {
-	root, resolved := w.resolveLocalPath(name)
+	root, resolved, err := w.resolveLocalPath(name)
+	if err != nil {
+		return nil, fmt.Errorf("resolve: %w", err)
+	}
 	fsys, err := w.fsysFor(root)
 	if err != nil {
 		return nil, err
@@ -235,7 +368,10 @@ func (w *wdFS) OpenFile(name string, flag int, perm fs.FileMode) (fs.File, error
 }
 
 func (w *wdFS) Mkdir(name string, perm fs.FileMode) error {
-	root, resolved := w.resolveLocalPath(name)
+	root, resolved, err := w.resolveLocalPath(name)
+	if err != nil {
+		return fmt.Errorf("resolve: %w", err)
+	}
 	fsys, err := w.fsysFor(root)
 	if err != nil {
 		return err
@@ -248,7 +384,10 @@ func (w *wdFS) Mkdir(name string, perm fs.FileMode) error {
 }
 
 func (w *wdFS) MkdirAll(name string, perm fs.FileMode) error {
-	root, resolved := w.resolveLocalPath(name)
+	root, resolved, err := w.resolveLocalPath(name)
+	if err != nil {
+		return fmt.Errorf("resolve: %w", err)
+	}
 	fsys, err := w.fsysFor(root)
 	if err != nil {
 		return err
@@ -257,7 +396,10 @@ func (w *wdFS) MkdirAll(name string, perm fs.FileMode) error {
 }
 
 func (w *wdFS) Remove(name string) error {
-	root, resolved := w.resolveLocalPath(name)
+	root, resolved, err := w.resolveLocalPath(name)
+	if err != nil {
+		return fmt.Errorf("resolve: %w", err)
+	}
 	fsys, err := w.fsysFor(root)
 	if err != nil {
 		return err
@@ -266,7 +408,10 @@ func (w *wdFS) Remove(name string) error {
 }
 
 func (w *wdFS) Chmod(name string, mode fs.FileMode) error {
-	root, resolved := w.resolveLocalPath(name)
+	root, resolved, err := w.resolveLocalPath(name)
+	if err != nil {
+		return fmt.Errorf("resolve: %w", err)
+	}
 	fsys, err := w.fsysFor(root)
 	if err != nil {
 		return err
diff --git a/template.go b/template.go
index 7aad0694..66fc4e54 100644
--- a/template.go
+++ b/template.go
@@ -90,7 +90,12 @@ func parseNestedTemplates(ctx context.Context, nested config.Templates, tmpl *te
 		}
 
 		// TODO: maybe need to do something with root here?
-		if _, reldir := datafs.ResolveLocalPath(u.Path); reldir != "" && reldir != "." {
+		_, reldir, err := datafs.ResolveLocalPath(u.Path)
+		if err != nil {
+			return fmt.Errorf("resolveLocalPath: %w", err)
+		}
+
+		if reldir != "" && reldir != "." {
 			fsys, err = fs.Sub(fsys, reldir)
 			if err != nil {
 				return fmt.Errorf("sub filesystem for %q unavailable: %w", &u, err)
@@ -220,7 +225,10 @@ func walkDir(ctx context.Context, cfg *config.Config, dir string, outFileNamer f
 
 	// we need dir to be relative to the root of fsys
 	// TODO: maybe need to do something with root here?
-	_, reldir := datafs.ResolveLocalPath(dir)
+	_, reldir, err := datafs.ResolveLocalPath(dir)
+	if err != nil {
+		return nil, fmt.Errorf("resolveLocalPath: %w", err)
+	}
 
 	subfsys, err := fs.Sub(fsys, reldir)
 	if err != nil {


Test patch:
diff --git a/internal/datafs/wdfs_test.go b/internal/datafs/wdfs_test.go
index 610920af..768ec386 100644
--- a/internal/datafs/wdfs_test.go
+++ b/internal/datafs/wdfs_test.go
@@ -170,12 +170,17 @@ func TestResolveLocalPath_NonWindows(t *testing.T) {
 		{"tmp/foo", wd + "/tmp/foo"},
 		{"./tmp/foo", wd + "/tmp/foo"},
 		{"tmp/../foo", wd + "/foo"},
+		{"/", "."},
 	}
 
 	for _, td := range testdata {
-		root, path := ResolveLocalPath(td.path)
-		assert.Equal(t, "/", root)
-		assert.Equal(t, td.expected, path)
+		td := td
+		t.Run(td.path, func(t *testing.T) {
+			root, path, err := ResolveLocalPath(td.path)
+			require.NoError(t, err)
+			assert.Equal(t, "/", root)
+			assert.Equal(t, td.expected, path)
+		})
 	}
 }
 
@@ -203,7 +208,8 @@ func TestResolveLocalPath_Windows(t *testing.T) {
 	for _, td := range testdata {
 		td := td
 		t.Run(td.path, func(t *testing.T) {
-			root, path := ResolveLocalPath(td.path)
+			root, path, err := ResolveLocalPath(td.path)
+			require.NoError(t, err)
 			assert.Equal(t, td.expRoot, root)
 			assert.Equal(t, td.expected, path)
 		})
@@ -224,12 +230,14 @@ func TestWdFS_ResolveLocalPath_NonWindows(t *testing.T) {
 		{"tmp/foo", wd + "/tmp/foo"},
 		{"./tmp/foo", wd + "/tmp/foo"},
 		{"tmp/../foo", wd + "/foo"},
+		{"/", "."},
 	}
 
 	fsys := &wdFS{}
 
 	for _, td := range testdata {
-		root, path := fsys.resolveLocalPath(td.path)
+		root, path, err := fsys.resolveLocalPath(td.path)
+		require.NoError(t, err)
 		assert.Equal(t, "/", root)
 		assert.Equal(t, td.expected, path)
 	}
@@ -249,14 +257,14 @@ func TestWdFS_ResolveLocalPath_Windows(t *testing.T) {
 		expected string
 	}{
 		{"C:/tmp/foo", "C:", "tmp/foo"},
-		{"D:\\tmp\\foo", "D:", "tmp/foo"},
+		{`D:\tmp\foo`, "D:", "tmp/foo"},
 		{"/tmp/foo", volname, "tmp/foo"},
 		{"tmp/foo", volname, wd + "/tmp/foo"},
 		{"./tmp/foo", volname, wd + "/tmp/foo"},
 		{"tmp/../foo", volname, wd + "/foo"},
-		{`\\?\C:\tmp\foo`, "//?/C:", "tmp/foo"},
+		{`\\?\C:\tmp\foo`, "C:", "tmp/foo"},
 		{`\\somehost\share\foo\bar`, "//somehost/share", "foo/bar"},
-		{`//?/C:/tmp/foo`, "//?/C:", "tmp/foo"},
+		{`//?/C:/tmp/foo`, "C:", "tmp/foo"},
 		{`//somehost/share/foo/bar`, "//somehost/share", "foo/bar"},
 	}
 
@@ -265,9 +273,45 @@ func TestWdFS_ResolveLocalPath_Windows(t *testing.T) {
 	for _, td := range testdata {
 		td := td
 		t.Run(td.path, func(t *testing.T) {
-			root, path := fsys.resolveLocalPath(td.path)
+			root, path, err := fsys.resolveLocalPath(td.path)
+			require.NoError(t, err)
 			assert.Equal(t, td.expRoot, root)
 			assert.Equal(t, td.expected, path)
 		})
 	}
 }
+
+func TestWin32PathType(t *testing.T) {
+	testdata := []struct {
+		path     string
+		expected winPathtype
+	}{
+		{"", winPathUnknown},
+		{`\`, winPathRooted},
+		{`\\`, winPathUncAbsolute},
+		{`x`, winPathRelative},
+		{`x:`, winPathDriveRelative},
+		{"C:/tmp/foo", winPathDriveAbsolute},
+		{`D:\tmp\foo`, winPathDriveAbsolute},
+		{"/tmp/foo", winPathRooted},
+		{"tmp/foo", winPathRelative},
+		{"./tmp/foo", winPathRelative},
+		{"tmp/../foo", winPathRelative},
+		{`\\?\C:\tmp\foo`, winPathLocalDevice},
+		{`\\somehost\share\foo\bar`, winPathUncAbsolute},
+		{`//./C:/tmp/foo`, winPathLocalDevice},
+		{`//./pipe/foo`, winPathLocalDevice},
+		{`//./COM2`, winPathLocalDevice},
+		{`\\.`, winPathRootLocalDevice},
+		{`//?`, winPathRootLocalDevice},
+		{`/??/C:/`, winPathNT},
+		{`/??/UNC/server/foo`, winPathNT},
+	}
+
+	for _, td := range testdata {
+		td := td
+		t.Run(td.path, func(t *testing.T) {
+			assert.Equal(t, td.expected, win32PathType(td.path))
+		})
+	}
+}
diff --git a/internal/tests/integration/datasources_git_test.go b/internal/tests/integration/datasources_git_test.go
index 236e35f9..fbc8e85d 100644
--- a/internal/tests/integration/datasources_git_test.go
+++ b/internal/tests/integration/datasources_git_test.go
@@ -33,8 +33,8 @@ func setupDatasourcesGitTest(t *testing.T) *fs.Dir {
 
 	repoPath := tmpDir.Join("repo")
 
-	icmd.RunCommand("git", "init", repoPath).
-		Assert(t, icmd.Expected{Out: "Initialized empty Git repository"})
+	icmd.RunCmd(icmd.Command("git", "init", repoPath), icmd.Dir(repoPath)).
+		Assert(t, icmd.Expected{Out: "Initialized empty Git repository in "})
 	icmd.RunCmd(icmd.Command("git", "add", "config.json"), icmd.Dir(repoPath)).Assert(t, icmd.Expected{})
 	icmd.RunCmd(icmd.Command("git", "add", "jsonfile"), icmd.Dir(repoPath)).Assert(t, icmd.Expected{})
 	icmd.RunCmd(icmd.Command("git", "add", "subdir"), icmd.Dir(repoPath)).Assert(t, icmd.Expected{})