fix(security): improve path traversal validation with comprehensive tests

Signed-off-by: Duncan Kibet <[email protected]>

Author: fsdkibe

util/security/path_traversal.go

@@ -2,40 +2,48 @@ package security
 
 import (
 	"fmt"
+	"os"
 	"path/filepath"
 	"strings"
 )
 
-// Ensure that `requestedPath` is on the same directory or any subdirectory of `currentRoot`. Both `currentRoot` and
-// `requestedPath` must be absolute paths. They may contain any number of `./` or `/../` dir changes.
-func EnforceToCurrentRoot(currentRoot, requestedPath string) (string, error) {
-	currentRoot = filepath.Clean(currentRoot)
-	requestedDir, requestedFile := parsePath(requestedPath)
-	if !isRequestedDirUnderCurrentRoot(currentRoot, requestedDir) {
-		return "", fmt.Errorf("requested path %s should be on or under current directory %s", requestedPath, currentRoot)
+// EnforceToCurrentRoot opens the file securely, ensuring it stays within currentRoot.
+// It returns the open *os.File handle. The caller is responsible for closing the file.
+func EnforceToCurrentRoot(currentRoot, requestedPath string) (*os.File, error) {
+	// 1. Create the secure jail (os.Root).
+	// os.OpenRoot guarantees that any operations performed on 'root' are confined to that directory.
+	root, err := os.OpenRoot(currentRoot)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create root at %s: %w", currentRoot, err)
 	}
-	return requestedDir + string(filepath.Separator) + requestedFile, nil
-}
+	// Closing the root handle does NOT close files opened from it.
+	// We can safely close it before returning.
+	defer root.Close()
 
-func isRequestedDirUnderCurrentRoot(currentRoot, requestedPath string) bool {
-	if currentRoot == string(filepath.Separator) {
-		return true
-	} else if currentRoot == requestedPath {
-		return true
-	}
-	if requestedPath[len(requestedPath)-1] != '/' {
-		requestedPath = requestedPath + "/"
+	// 2. Normalize paths for the relative path calculation.
+	// We use filepath.Clean on the string inputs ONLY to calculate the relative path
+	// for the lookup. We do not use these strings for the actual OS-level access.
+	cleanRoot := filepath.Clean(currentRoot)
+	cleanPath := filepath.Clean(requestedPath)
+
+	relPath, err := filepath.Rel(cleanRoot, cleanPath)
+	if err != nil {
+		return nil, fmt.Errorf("requested path %s should be on or under current directory %s", requestedPath, currentRoot)
 	}
-	if currentRoot[len(currentRoot)-1] != '/' {
-		currentRoot = currentRoot + "/"
+
+	// 3. Lexical Check (Defense in Depth).
+	// Fast-fail if the path obviously tries to traverse upwards.
+	if relPath == ".." || strings.HasPrefix(relPath, ".."+string(filepath.Separator)) {
+		return nil, fmt.Errorf("requested path %s should be on or under current directory %s", requestedPath, currentRoot)
 	}
-	return strings.HasPrefix(requestedPath, currentRoot)
-}
 
-func parsePath(path string) (string, string) {
-	directory := filepath.Dir(path)
-	if directory == path {
-		return directory, ""
+	// 4. Secure Open
+	// We open the file relative to the secure root handle.
+	// If 'relPath' attempts to escape the root (even via symlinks), the OS will block it.
+	f, err := root.Open(relPath)
+	if err != nil {
+		return nil, fmt.Errorf("access denied or file not found within root: %w", err)
 	}
-	return directory, filepath.Base(path)
+
+	return f, nil
 }

util/security/path_traversal_test.go

@@ -1,27 +1,171 @@
 package security
 
 import (
+	"io"
+	"os"
+	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 func TestEnforceToCurrentRoot(t *testing.T) {
-	cleanDir, err := EnforceToCurrentRoot("/home/argo/helmapp/", "/home/argo/helmapp/values.yaml")
-	require.NoError(t, err)
-	assert.Equal(t, "/home/argo/helmapp/values.yaml", cleanDir)
-
-	// File is outside current working directory
-	_, err = EnforceToCurrentRoot("/home/argo/helmapp/", "/home/values.yaml")
-	require.Error(t, err)
-
-	// File is outside current working directory
-	_, err = EnforceToCurrentRoot("/home/argo/helmapp/", "/home/argo/helmapp/../differentapp/values.yaml")
-	require.Error(t, err)
-
-	// Goes back and forth, but still legal
-	cleanDir, err = EnforceToCurrentRoot("/home/argo/helmapp/", "/home/argo/helmapp/../../argo/helmapp/values.yaml")
-	require.NoError(t, err)
-	assert.Equal(t, "/home/argo/helmapp/values.yaml", cleanDir)
+	tmpDir := t.TempDir()
+
+	// Setup: Create files with specific content so we can verify we opened the right one.
+	require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "values.yaml"), []byte("root-file"), 0o644))
+	require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "charts"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "charts", "values.yaml"), []byte("subdir-file"), 0o644))
+	require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, "a", "b", "c", "d"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "a", "b", "c", "d", "values.yaml"), []byte("deep-file"), 0o644))
+
+	// Helper to verify we received a valid file handle pointing to the expected data.
+	verifyFileContent := func(t *testing.T, f *os.File, expectedContent string) {
+		t.Helper()
+		defer f.Close()
+		content, err := io.ReadAll(f)
+		require.NoError(t, err)
+		assert.Equal(t, expectedContent, string(content))
+	}
+
+	t.Run("file directly in root", func(t *testing.T) {
+		testFile := filepath.Join(tmpDir, "values.yaml")
+		fileHandle, err := EnforceToCurrentRoot(tmpDir, testFile)
+		require.NoError(t, err)
+		require.NotNil(t, fileHandle)
+		verifyFileContent(t, fileHandle, "root-file")
+	})
+
+	t.Run("file in subdirectory", func(t *testing.T) {
+		testFile := filepath.Join(tmpDir, "charts", "values.yaml")
+		fileHandle, err := EnforceToCurrentRoot(tmpDir, testFile)
+		require.NoError(t, err)
+		require.NotNil(t, fileHandle)
+		verifyFileContent(t, fileHandle, "subdir-file")
+	})
+
+	t.Run("file outside current working directory", func(t *testing.T) {
+		outsidePath := filepath.Join(filepath.Dir(tmpDir), "values.yaml")
+		_, err := EnforceToCurrentRoot(tmpDir, outsidePath)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "should be on or under current directory")
+	})
+
+	t.Run("file escapes using parent directory notation", func(t *testing.T) {
+		escapePath := filepath.Join(tmpDir, "..", "differentapp", "values.yaml")
+		_, err := EnforceToCurrentRoot(tmpDir, escapePath)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "should be on or under current directory")
+	})
+
+	t.Run("goes back and forth but remains within root", func(t *testing.T) {
+		complexPath := filepath.Join(tmpDir, "charts", "..", "values.yaml")
+		fileHandle, err := EnforceToCurrentRoot(tmpDir, complexPath)
+		require.NoError(t, err)
+		require.NotNil(t, fileHandle)
+		verifyFileContent(t, fileHandle, "root-file")
+	})
+
+	t.Run("path equals root directory", func(t *testing.T) {
+		// Opening the directory itself should succeed
+		fileHandle, err := EnforceToCurrentRoot(tmpDir, tmpDir)
+		require.NoError(t, err)
+		require.NotNil(t, fileHandle)
+		defer fileHandle.Close()
+
+		info, err := fileHandle.Stat()
+		require.NoError(t, err)
+		assert.True(t, info.IsDir())
+	})
+
+	t.Run("complex path with multiple . and ..", func(t *testing.T) {
+		complexPath := filepath.Join(tmpDir, ".", "charts", "..", "values.yaml")
+		fileHandle, err := EnforceToCurrentRoot(tmpDir, complexPath)
+		require.NoError(t, err)
+		require.NotNil(t, fileHandle)
+		verifyFileContent(t, fileHandle, "root-file")
+	})
+
+	t.Run("root with trailing slash vs without", func(t *testing.T) {
+		testFile := filepath.Join(tmpDir, "values.yaml")
+
+		// 1. With Slash.
+		rootWithSlash := tmpDir + string(filepath.Separator)
+		f1, err1 := EnforceToCurrentRoot(rootWithSlash, testFile)
+		require.NoError(t, err1)
+		verifyFileContent(t, f1, "root-file")
+
+		// 2. Without Slash.
+		f2, err2 := EnforceToCurrentRoot(tmpDir, testFile)
+		require.NoError(t, err2)
+		verifyFileContent(t, f2, "root-file")
+	})
+
+	t.Run("attempt to escape with multiple parent references", func(t *testing.T) {
+		escapePath := filepath.Join(tmpDir, "..", "..", "..", "etc", "passwd")
+		_, err := EnforceToCurrentRoot(tmpDir, escapePath)
+		require.Error(t, err)
+	})
+
+	t.Run("deep nested subdirectory", func(t *testing.T) {
+		testFile := filepath.Join(tmpDir, "a", "b", "c", "d", "values.yaml")
+		fileHandle, err := EnforceToCurrentRoot(tmpDir, testFile)
+		require.NoError(t, err)
+		require.NotNil(t, fileHandle)
+		verifyFileContent(t, fileHandle, "deep-file")
+	})
+}
+
+func TestEnforceToCurrentRootEdgeCases(t *testing.T) {
+	tmpDir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "values.yaml"), []byte("test"), 0o644))
+
+	t.Run("empty paths", func(t *testing.T) {
+		_, err := EnforceToCurrentRoot("", "/some/path")
+		require.Error(t, err)
+	})
+
+	t.Run("relative root path", func(t *testing.T) {
+		// EnforceToCurrentRoot expects absolute paths for the root (usually)
+		// os.OpenRoot might work with relative paths, but filepath.Rel logic depends on clean comparisons.
+		_, err := EnforceToCurrentRoot("relative/path", "/absolute/path")
+		require.Error(t, err)
+	})
+
+	t.Run("path with double slashes", func(t *testing.T) {
+		doubleSlashPath := tmpDir + string(filepath.Separator) + string(filepath.Separator) + "values.yaml"
+		f, err := EnforceToCurrentRoot(tmpDir, doubleSlashPath)
+		require.NoError(t, err)
+		defer f.Close()
+	})
+
+	t.Run("non-existent file in valid directory", func(t *testing.T) {
+		nonExistentPath := filepath.Join(tmpDir, "does-not-exist.yaml")
+		_, err := EnforceToCurrentRoot(tmpDir, nonExistentPath)
+		// This should fail because root.Open() checks if file exists.
+		require.Error(t, err)
+	})
+}
+
+func TestEnforceToCurrentRootWindowsPaths(t *testing.T) {
+	if runtime.GOOS != "windows" {
+		t.Skip("Skipping Windows-specific tests on non-Windows platform")
+	}
+	tmpDir := t.TempDir()
+	testFile := filepath.Join(tmpDir, "values.yaml")
+	require.NoError(t, os.WriteFile(testFile, []byte("test"), 0o644))
+
+	t.Run("windows path in root", func(t *testing.T) {
+		f, err := EnforceToCurrentRoot(tmpDir, testFile)
+		require.NoError(t, err)
+		defer f.Close()
+	})
+
+	t.Run("windows path escape attempt", func(t *testing.T) {
+		escapePath := filepath.Join(tmpDir, "..", "..", "Windows", "System32", "config")
+		_, err := EnforceToCurrentRoot(tmpDir, escapePath)
+		require.Error(t, err)
+	})
 }