Rely on os.Root for checking paths (#2752)

Remove pathIsInRepositoryRoot, and rely on implicit
os.Root operations to check if a path is under the
repository root.

Author: jsoriano

internal/files/linkedfiles.go

@@ -40,7 +40,12 @@ func CreateLinksFSFromPath(workDir string) (*LinksFS, error) {
 		return nil, fmt.Errorf("opening repository root: %w", err)
 	}
 
-	return NewLinksFS(root, workDir)
+	absWorkDir, err := filepath.Abs(workDir)
+	if err != nil {
+		return nil, fmt.Errorf("obtaining absolute path of working directory: %w", err)
+	}
+
+	return NewLinksFS(root, absWorkDir)
 }
 
 var _ fs.FS = (*LinksFS)(nil)
@@ -56,39 +61,28 @@ type LinksFS struct {
 	inner    fs.FS
 }
 
-// NewLinksFS creates a new LinksFS.
+// NewLinksFS creates a new LinksFS. workDir must be an absolute path, or a path relative to
+// the repository root.
 func NewLinksFS(repoRoot *os.Root, workDir string) (*LinksFS, error) {
 	// Ensure workDir is absolute for os.DirFS
 	var absWorkDir string
 	if filepath.IsAbs(workDir) {
 		absWorkDir = workDir
-	} else {
-		// First try: assume workDir is relative to the repository root
-		absWorkDir = filepath.Join(repoRoot.Name(), workDir)
-
-		// Check if path exists
-		if _, err := os.Stat(absWorkDir); os.IsNotExist(err) {
-			// Second try: path might be relative to current working directory
-			currentDir, err := os.Getwd()
-			if err == nil {
-				alternativePath := filepath.Join(currentDir, workDir)
-				if _, err := os.Stat(alternativePath); err == nil {
-					// If this path exists, use it instead
-					absWorkDir = alternativePath
-					logger.Debugf("Using path relative to current working directory: %s", absWorkDir)
-				}
-			}
+		relative, err := filepath.Rel(repoRoot.Name(), absWorkDir)
+		if err != nil {
+			return nil, fmt.Errorf("invalid working directory %s: %w", absWorkDir, err)
 		}
+		workDir = relative
+	} else {
+		absWorkDir = filepath.Clean(filepath.Join(repoRoot.Name(), workDir))
 	}
 
-	// Validate that workDir is within the repository root
-	inRoot, err := pathIsInRepositoryRoot(repoRoot, absWorkDir)
+	info, err := repoRoot.Stat(workDir)
 	if err != nil {
-		return nil, fmt.Errorf("could not validate workDir %s: %w", absWorkDir, err)
+		return nil, fmt.Errorf("invalid working directory %s: %w", absWorkDir, err)
 	}
-
-	if !inRoot {
-		return nil, fmt.Errorf("workDir %s is outside the repository root %s", absWorkDir, repoRoot.Name())
+	if !info.IsDir() {
+		return nil, fmt.Errorf("working directory %s is not a directory", absWorkDir)
 	}
 
 	return &LinksFS{repoRoot: repoRoot, workDir: absWorkDir, inner: os.DirFS(absWorkDir)}, nil
@@ -221,14 +215,6 @@ func newLinkedFile(root *os.Root, linkFilePath string) (Link, error) {
 
 	pathName := filepath.Clean(filepath.Join(l.WorkDir, filepath.FromSlash(l.IncludedFilePath)))
 
-	inRoot, err := pathIsInRepositoryRoot(root, pathName)
-	if err != nil {
-		return Link{}, fmt.Errorf("could not check if path %s is in repository root: %w", pathName, err)
-	}
-	if !inRoot {
-		return Link{}, fmt.Errorf("path %s escapes the repository root", pathName)
-	}
-
 	// Store the original absolute path for package root detection
 	originalAbsPath := pathName
 
@@ -545,28 +531,3 @@ func checksum(b []byte) (string, error) {
 	hash := sha256.Sum256(b)
 	return hex.EncodeToString(hash[:]), nil
 }
-
-// pathIsInRepositoryRoot checks if a path is within the repository root and doesn't escape it.
-func pathIsInRepositoryRoot(root *os.Root, path string) (bool, error) {
-	path = filepath.FromSlash(path)
-	var err error
-	if filepath.IsAbs(path) {
-		path, err = filepath.Rel(root.Name(), path)
-		if err != nil {
-			return false, fmt.Errorf("could not get relative path: %w", err)
-		}
-	}
-
-	// Clean the path to resolve any ".." components
-	cleanPath := filepath.Clean(path)
-
-	// Check if the cleaned path tries to escape the root
-	if strings.HasPrefix(cleanPath, "..") {
-		return false, nil
-	}
-
-	if _, err := root.Stat(cleanPath); err != nil {
-		return false, nil
-	}
-	return true, nil
-}

internal/files/linkedfiles_test.go

@@ -12,6 +12,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -33,7 +34,7 @@ func TestLinkUpdateChecksum(t *testing.T) {
 	require.NoError(t, copyDir(testDataSrc, filepath.Join(tempDir, "testdata")))
 
 	// Set up paths within the temporary directory
-	basePath := filepath.Join(tempDir, "testdata/links")
+	basePath := filepath.Join(tempDir, "testdata", "links")
 
 	// Create an os.Root for secure file operations within tempDir
 	root, err := os.OpenRoot(tempDir)
@@ -82,7 +83,7 @@ func TestListLinkedFiles(t *testing.T) {
 	// Get current working directory to locate test data
 	wd, err := os.Getwd()
 	assert.NoError(t, err)
-	basePath := filepath.Join(wd, filepath.FromSlash("testdata/links"))
+	basePath := filepath.Join(wd, "testdata", "links")
 
 	// Find the repository root to create a secure os.Root context
 	root, err := FindRepositoryRoot()
@@ -193,7 +194,7 @@ func TestUpdateLinkedFilesChecksums(t *testing.T) {
 	require.NoError(t, copyDir(testDataSrc, filepath.Join(tempDir, "testdata")))
 
 	// Set up paths within the temporary directory
-	basePath := filepath.Join(tempDir, "testdata/links")
+	basePath := filepath.Join(tempDir, "testdata", "links")
 
 	// Create an os.Root for secure file operations within tempDir
 	root, err := os.OpenRoot(tempDir)
@@ -227,7 +228,7 @@ func TestLinkedFilesByPackageFrom(t *testing.T) {
 	// Get current working directory to locate test data
 	wd, err := os.Getwd()
 	assert.NoError(t, err)
-	basePath := filepath.Join(wd, filepath.FromSlash("testdata/links"))
+	basePath := filepath.Join(wd, "testdata", "links")
 
 	// Find the repository root to create a secure os.Root context
 	root, err := FindRepositoryRoot()
@@ -269,14 +270,14 @@ func TestIncludeLinkedFiles(t *testing.T) {
 	// Get current working directory to locate test data
 	wd, err := os.Getwd()
 	assert.NoError(t, err)
-	testPkg := filepath.Join(wd, filepath.FromSlash("testdata"))
+	testPkg := filepath.Join(wd, "testdata")
 
 	// Create a temporary directory and copy test data to avoid modifying originals
 	tempDir := t.TempDir()
 	require.NoError(t, copyDir(testPkg, filepath.Join(tempDir, "testdata")))
 
 	// Set up source and destination directories
-	fromDir := filepath.Join(tempDir, "testdata/testpackage")
+	fromDir := filepath.Join(tempDir, "testdata", "testpackage")
 	toDir := filepath.Join(tempDir, "dest")
 
 	// Create an os.Root for secure file operations within tempDir
@@ -769,6 +770,11 @@ func TestLinksFS_ErrorConditions(t *testing.T) {
 	err = os.WriteFile(invalidLinkFile, []byte(""), 0644)
 	require.NoError(t, err)
 
+	// Create link that escapes root
+	outOfRootLinkFile := filepath.Join(workDir, "escapesroot.txt.link")
+	err = os.WriteFile(outOfRootLinkFile, []byte("../../etc/passwd"), 0644)
+	require.NoError(t, err)
+
 	// Setup LinksFS
 	root, err := os.OpenRoot(repoDir)
 	require.NoError(t, err)
@@ -777,6 +783,11 @@ func TestLinksFS_ErrorConditions(t *testing.T) {
 	lfs, err := NewLinksFS(root, workDir)
 	require.NoError(t, err)
 
+	notFoundErrorMsg := "no such file or directory"
+	if runtime.GOOS == "windows" {
+		notFoundErrorMsg = "The system cannot find the file specified"
+	}
+
 	tests := []struct {
 		name     string
 		fileName string
@@ -785,13 +796,18 @@ func TestLinksFS_ErrorConditions(t *testing.T) {
 		{
 			name:     "broken link to non-existent file",
 			fileName: "broken.txt.link",
-			errorMsg: "escapes the repository root",
+			errorMsg: notFoundErrorMsg,
 		},
 		{
 			name:     "invalid link file format",
 			fileName: "invalid.txt.link",
 			errorMsg: "file is empty or first line is missing",
 		},
+		{
+			name:     "escapes root",
+			fileName: "escapesroot.txt.link",
+			errorMsg: "path escapes from parent",
+		},
 	}
 
 	for _, tc := range tests {
@@ -845,13 +861,13 @@ func TestLinksFS_WorkDirValidation(t *testing.T) {
 			name:        "invalid absolute workDir outside repo",
 			workDir:     outsideDir,
 			expectError: true,
-			errorMsg:    "is outside the repository root",
+			errorMsg:    "path escapes from parent",
 		},
 		{
 			name:        "invalid relative workDir escaping repo",
 			workDir:     "../outside",
 			expectError: true,
-			errorMsg:    "is outside the repository root",
+			errorMsg:    "path escapes from parent",
 		},
 	}