From fc6fcaa4e2c97d4817f715bc9e0989ab38c9ba12 Mon Sep 17 00:00:00 2001
From: Tom Payne <twpayne@gmail.com>
Date: Mon, 9 Dec 2024 23:13:56 +0000
Subject: [PATCH] chore: Print error if external paths are empty or in parent

---
 internal/chezmoi/sourcestate.go      |  13 +++-
 internal/chezmoi/sourcestate_test.go | 104 +++++++++++++++++++++++++++
 2 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/internal/chezmoi/sourcestate.go b/internal/chezmoi/sourcestate.go
index 2b79051ff0f..bd34c277023 100644
--- a/internal/chezmoi/sourcestate.go
+++ b/internal/chezmoi/sourcestate.go
@@ -1356,9 +1356,20 @@ func (s *SourceState) addExternal(sourceAbsPath, parentAbsPath AbsPath) error {
 	s.mutex.Lock()
 	defer s.mutex.Unlock()
 	for path, external := range externals {
-		if strings.HasPrefix(path, "/") || filepath.IsAbs(path) {
+		switch {
+		case path == "":
+			return fmt.Errorf("%s: empty path", sourceAbsPath)
+		case strings.HasPrefix(path, "/") || filepath.IsAbs(path):
 			return fmt.Errorf("%s: %s: path is not relative", sourceAbsPath, path)
 		}
+		switch relPath, err := filepath.Rel(".", path); {
+		case err != nil:
+			return fmt.Errorf("%s: %s: %w", sourceAbsPath, path, err)
+		case relPath == ".":
+			return fmt.Errorf("%s: %s: empty relative path", sourceAbsPath, path)
+		case relPath == "..", strings.HasPrefix(relPath, "../"):
+			return fmt.Errorf("%s: %s: relative path in parent", sourceAbsPath, path)
+		}
 		targetRelPath := parentTargetSourceRelPath.JoinString(path)
 		external.sourceAbsPath = sourceAbsPath
 		s.externals[targetRelPath] = append(s.externals[targetRelPath], &external)
diff --git a/internal/chezmoi/sourcestate_test.go b/internal/chezmoi/sourcestate_test.go
index e13d0d3ad88..84912064d52 100644
--- a/internal/chezmoi/sourcestate_test.go
+++ b/internal/chezmoi/sourcestate_test.go
@@ -1967,6 +1967,110 @@ func TestTemplateOptionsParseDirectives(t *testing.T) {
 	}
 }
 
+func TestSourceStateExternalErrors(t *testing.T) {
+	for _, tc := range []struct {
+		name        string
+		shareDir    map[string]any
+		expectedErr string
+	}{
+		{
+			name: "missing_type",
+			shareDir: map[string]any{
+				".chezmoiexternal.toml": chezmoitest.JoinLines(
+					`["dir"]`,
+					`    url = "http://example.com/"`,
+				),
+			},
+			expectedErr: "dir: missing external type",
+		},
+		{
+			name: "empty_rel_path",
+			shareDir: map[string]any{
+				".chezmoiexternal.toml": chezmoitest.JoinLines(
+					`[""]`,
+					`    type = "file"`,
+					`    url = "http://example.com/"`,
+				),
+			},
+			expectedErr: "/home/user/.local/share/chezmoi/.chezmoiexternal.toml: empty path",
+		},
+		{
+			name: "relative_empty_rel_path",
+			shareDir: map[string]any{
+				".chezmoiexternal.toml": chezmoitest.JoinLines(
+					`["."]`,
+					`    type = "file"`,
+					`    url = "http://example.com/"`,
+				),
+			},
+			expectedErr: "/home/user/.local/share/chezmoi/.chezmoiexternal.toml: .: empty relative path",
+		},
+		{
+			name: "parent_root_rel_path",
+			shareDir: map[string]any{
+				".chezmoiexternal.toml": chezmoitest.JoinLines(
+					`[".."]`,
+					`    type = "file"`,
+					`    url = "http://example.com/"`,
+				),
+			},
+			expectedErr: "/home/user/.local/share/chezmoi/.chezmoiexternal.toml: ..: relative path in parent",
+		},
+		{
+			name: "relative_parent_root_rel_path",
+			shareDir: map[string]any{
+				".chezmoiexternal.toml": chezmoitest.JoinLines(
+					`["./.."]`,
+					`    type = "file"`,
+					`    url = "http://example.com/"`,
+				),
+			},
+			expectedErr: "/home/user/.local/share/chezmoi/.chezmoiexternal.toml: ./..: relative path in parent",
+		},
+		{
+			name: "relative_empty",
+			shareDir: map[string]any{
+				".chezmoiexternal.toml": chezmoitest.JoinLines(
+					`["a/../b/.."]`,
+					`    type = "file"`,
+					`    url = "http://example.com/"`,
+				),
+			},
+			expectedErr: "/home/user/.local/share/chezmoi/.chezmoiexternal.toml: a/../b/..: empty relative path",
+		},
+		{
+			name: "relative_parent",
+			shareDir: map[string]any{
+				".chezmoiexternal.toml": chezmoitest.JoinLines(
+					`["a/../b/../.."]`,
+					`    type = "file"`,
+					`    url = "http://example.com/"`,
+				),
+			},
+			expectedErr: "/home/user/.local/share/chezmoi/.chezmoiexternal.toml: a/../b/../..: relative path in parent",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			chezmoitest.WithTestFS(t, map[string]any{
+				"/home/user/.local/share/chezmoi": tc.shareDir,
+			}, func(fileSystem vfs.FS) {
+				ctx := context.Background()
+				system := NewRealSystem(fileSystem)
+				s := NewSourceState(
+					WithBaseSystem(system),
+					WithCacheDir(NewAbsPath("/home/user/.cache/chezmoi")),
+					WithDestDir(NewAbsPath("/home/user")),
+					WithSourceDir(NewAbsPath("/home/user/.local/share/chezmoi")),
+					WithSystem(system),
+				)
+				err := s.Read(ctx, nil)
+				assert.Error(t, err)
+				assert.Equal(t, err.Error(), tc.expectedErr)
+			})
+		})
+	}
+}
+
 // applyAll updates targetDirAbsPath in targetSystem to match s.
 func (s *SourceState) applyAll(
 	targetSystem, destSystem System,