fix(filesystem): improve path traversal detection checks (#1152)

* fix(filesystem): improve path traversal detection by centralizing trusted root checks

* fix(filesystem): remove duplicate code

* fix(filesystem): return absPath on path traversal error for improved error handling

* refactor(filesystem): rename InTrustedRoot to InBasePath for clarity and update references

Author: kimdre

internal/docker/compose.go

@@ -16,6 +16,7 @@ import (
 	"time"
 
 	"github.com/kimdre/doco-cd/internal/encryption"
+	"github.com/kimdre/doco-cd/internal/filesystem"
 	"github.com/kimdre/doco-cd/internal/utils/module"
 
 	"github.com/kimdre/doco-cd/internal/docker/swarm"
@@ -630,26 +631,6 @@ func getPaths(changedFiles []gitInternal.ChangedFile, basePath string) []string
 	return slice.Unique(absPaths)
 }
 
-// checkPathAffected checks if a changed file is affected by a used file.
-func checkPathAffected(changed string, used string) bool {
-	used = filepath.Clean(used)
-	changed = filepath.Clean(changed)
-
-	rel, err := filepath.Rel(used, changed)
-	if err != nil {
-		// It' share same reporoot, so it should not happen
-		slog.Debug("checkPathAffected ",
-			slog.String("used", used),
-			slog.String("changed", changed),
-			slog.Any("error", err),
-		)
-
-		return false
-	}
-
-	return !strings.HasPrefix(rel, "..")
-}
-
 // HasChangedConfigs checks if any files used in docker compose `configs:` definitions have changed using the Git status.
 func HasChangedConfigs(paths []string, project *types.Project) ([]string, error) {
 	configToServicesMap := make(map[string][]string)
@@ -669,7 +650,7 @@ func HasChangedConfigs(paths []string, project *types.Project) ([]string, error)
 		}
 
 		for _, p := range paths {
-			if checkPathAffected(p, c.File) {
+			if filesystem.InBasePath(c.File, p) {
 				changedServices = append(changedServices, configToServicesMap[name]...)
 			}
 		}
@@ -696,7 +677,7 @@ func HasChangedSecrets(paths []string, project *types.Project) ([]string, error)
 		}
 
 		for _, p := range paths {
-			if checkPathAffected(p, s.File) {
+			if filesystem.InBasePath(s.File, p) {
 				changedServices = append(changedServices, secretsToServicesMap[name]...)
 			}
 		}
@@ -714,7 +695,7 @@ func HasChangedBindMounts(paths []string, project *types.Project) ([]string, err
 		for _, v := range s.Volumes {
 			if v.Type == "bind" && v.Source != "" {
 				for _, p := range paths {
-					if checkPathAffected(p, v.Source) {
+					if filesystem.InBasePath(v.Source, p) {
 						changedServices = append(changedServices, s.Name)
 						break out
 					}
@@ -734,7 +715,7 @@ func HasChangedEnvFiles(paths []string, project *types.Project) ([]string, error
 	out:
 		for _, envFile := range s.EnvFiles {
 			for _, p := range paths {
-				if checkPathAffected(p, envFile.Path) {
+				if filesystem.InBasePath(envFile.Path, p) {
 					changedServices = append(changedServices, s.Name)
 					break out
 				}
@@ -794,7 +775,7 @@ func HasChangedBuildFiles(paths []string, project *types.Project) ([]string, err
 			}
 
 			for _, p := range paths {
-				if checkPathAffected(p, ctxFile) {
+				if filesystem.InBasePath(ctxFile, p) {
 					changedServices = append(changedServices, s.Name)
 					break out
 				}
@@ -1090,7 +1071,7 @@ func DecryptProjectFiles(repoPath string, p *types.Project) ([]string, error) {
 				if info.IsDir() {
 					decryptedFiles, err = encryption.DecryptFilesInDirectory(repoPath, v.Source)
 					if err != nil {
-						if errors.Is(err, encryption.ErrPathTraversal) {
+						if errors.Is(err, filesystem.ErrPathTraversal) {
 							continue
 						}
 

internal/docker/compose_test.go

@@ -1569,7 +1569,7 @@ func TestGetProjects(t *testing.T) {
 	t.Logf("Found %d projects", len(projects))
 }
 
-func Test_checkPathAffected(t *testing.T) {
+func TestCheckPathAffected(t *testing.T) {
 	const repoRoot = "/data/reporoot"
 
 	tests := []struct {
@@ -1678,7 +1678,7 @@ func Test_checkPathAffected(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := checkPathAffected(tt.changed, tt.used)
+			got := filesystem.InBasePath(tt.used, tt.changed)
 			if tt.want != got {
 				t.Errorf("checkPathAffected(used=%q, changed=%q) = %v, want %v", tt.used, tt.changed, got, tt.want)
 			}

internal/encryption/decrypt.go

@@ -27,10 +27,7 @@ var IgnoreDirs = []string{
 	"node_modules",
 }
 
-var (
-	ErrSopsKeyNotSet = errors.New("SOPS secret key is not set")
-	ErrPathTraversal = errors.New("path traversal detected")
-)
+var ErrSopsKeyNotSet = errors.New("SOPS secret key is not set")
 
 func GetFileFormat(path string) string {
 	var format string
@@ -66,25 +63,10 @@ func DecryptContent(content []byte, format string) ([]byte, error) {
 	return decrypt.Data(content, format)
 }
 
-// Add a check to ensure files/directories are within the repository root.
-func isWithinRepoRoot(repoPath, targetPath string) bool {
-	absRepoPath, err := filepath.Abs(repoPath)
-	if err != nil {
-		return false
-	}
-
-	absTargetPath, err := filepath.Abs(targetPath)
-	if err != nil {
-		return false
-	}
-
-	return strings.HasPrefix(absTargetPath, absRepoPath)
-}
-
 // DecryptFilesInDirectory walks through the specified directory and decrypts all SOPS-encrypted files.
 func DecryptFilesInDirectory(repoPath, dirPath string) ([]string, error) {
-	if !isWithinRepoRoot(repoPath, dirPath) {
-		return nil, fmt.Errorf("%w: %s is outside the repository root %s", ErrPathTraversal, dirPath, repoPath)
+	if !filesystem.InBasePath(repoPath, dirPath) {
+		return nil, fmt.Errorf("%w: %s is outside the repository root %s", filesystem.ErrPathTraversal, dirPath, repoPath)
 	}
 
 	var decryptedFiles []string
@@ -138,7 +120,7 @@ func DecryptFilesInDirectory(repoPath, dirPath string) ([]string, error) {
 
 			// Recursively walk the symlink target
 			_, err = DecryptFilesInDirectory(repoPath, absTarget)
-			if errors.Is(err, ErrPathTraversal) {
+			if errors.Is(err, filesystem.ErrPathTraversal) {
 				return nil
 			}
 

internal/filesystem/filesystem.go

@@ -31,13 +31,27 @@ func VerifyAndSanitizePath(path, trustedRoot string) (string, error) {
 
 	trustedRoot = filepath.Clean(trustedRoot) + string(os.PathSeparator)
 
-	if !strings.HasPrefix(absPath, trustedRoot) {
-		return absPath, ErrPathTraversal
+	if !InBasePath(trustedRoot, absPath) {
+		return absPath, fmt.Errorf("%w: %s is outside of trusted root %s", ErrPathTraversal, absPath, trustedRoot)
 	}
 
 	return absPath, nil
 }
 
+// InBasePath checks if the given path is within the base path,
+// preventing path traversal attacks.
+func InBasePath(basePath, path string) bool {
+	basePath = filepath.Clean(basePath)
+	path = filepath.Clean(path)
+
+	rel, err := filepath.Rel(basePath, path)
+	if err != nil {
+		return false
+	}
+
+	return !strings.HasPrefix(rel, "..")
+}
+
 // IsSocket checks if the given path is a socket file.
 func IsSocket(path string) bool {
 	info, err := os.Stat(path)

internal/filesystem/filesystem_test.go

@@ -62,3 +62,78 @@ func TestVerifyAndSanitizePath(t *testing.T) {
 		})
 	}
 }
+
+func TestInBasePath(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name        string
+		path        string
+		trustedRoot string
+		expected    bool
+	}{
+		{
+			name:        "Same path",
+			path:        "/valid/path",
+			trustedRoot: "/valid/path",
+			expected:    true,
+		},
+		{
+			name:        "Path within trusted root",
+			path:        "/valid/path",
+			trustedRoot: "/valid",
+			expected:    true,
+		},
+		{
+			name:        "Path outside trusted root",
+			path:        "/invalid/path",
+			trustedRoot: "/valid",
+			expected:    false,
+		},
+		{
+			name:        "Absolute Path with traversal",
+			path:        "/valid/../../invalid/path",
+			trustedRoot: "/valid",
+			expected:    false,
+		},
+		{
+			name:        "Relative path",
+			path:        "valid/path",
+			trustedRoot: "valid",
+			expected:    true,
+		},
+		{
+			name:        "Relative path with traversal",
+			path:        "valid/../../invalid/path",
+			trustedRoot: "valid",
+			expected:    false,
+		},
+		{
+			name:        "Relative path outside trusted root",
+			path:        "invalid/path",
+			trustedRoot: "valid",
+			expected:    false,
+		},
+		{
+			name:        "Path in base outside trusted root",
+			path:        "/base/invalid/path",
+			trustedRoot: "/base/valid",
+			expected:    false,
+		},
+		{
+			name:        "Path in base in trusted root",
+			path:        "/base/valid/path",
+			trustedRoot: "/base/valid",
+			expected:    true,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := InBasePath(tc.trustedRoot, tc.path)
+
+			if result != tc.expected {
+				t.Fatalf("expected %v, got %v", tc.expected, result)
+			}
+		})
+	}
+}