feat(commit-server): use os.Root for traversal protection (#22956)

Signed-off-by: Michael Crenshaw <[email protected]>

commitserver/commit/commit.go

@@ -12,6 +12,7 @@ import (
 	"github.com/argoproj/argo-cd/v3/commitserver/apiclient"
 	"github.com/argoproj/argo-cd/v3/commitserver/metrics"
 	"github.com/argoproj/argo-cd/v3/util/git"
+	"github.com/argoproj/argo-cd/v3/util/io"
 	"github.com/argoproj/argo-cd/v3/util/io/files"
 )
 
@@ -97,6 +98,12 @@ func (s *Service) handleCommitRequest(logCtx *log.Entry, r *apiclient.CommitHydr
 	}
 	defer cleanup()
 
+	root, err := os.OpenRoot(dirPath)
+	if err != nil {
+		return "", "", fmt.Errorf("failed to open root dir: %w", err)
+	}
+	defer io.Close(root)
+
 	logCtx.Debugf("Checking out sync branch %s", r.SyncBranch)
 	var out string
 	out, err = gitClient.CheckoutOrOrphan(r.SyncBranch, false)
@@ -117,7 +124,7 @@ func (s *Service) handleCommitRequest(logCtx *log.Entry, r *apiclient.CommitHydr
 	}
 
 	logCtx.Debug("Writing manifests")
-	err = WriteForPaths(dirPath, r.Repo.Repo, r.DrySha, r.Paths)
+	err = WriteForPaths(root, r.Repo.Repo, r.DrySha, r.Paths)
 	if err != nil {
 		return "", "", fmt.Errorf("failed to write manifests: %w", err)
 	}

commitserver/commit/hydratorhelper.go

@@ -2,9 +2,12 @@ package commit
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
-	"path"
+	"path/filepath"
+	"strings"
 	"text/template"
 
 	log "github.com/sirupsen/logrus"
@@ -12,14 +15,14 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	"github.com/argoproj/argo-cd/v3/commitserver/apiclient"
-	"github.com/argoproj/argo-cd/v3/util/io/files"
+	"github.com/argoproj/argo-cd/v3/util/io"
 )
 
 // WriteForPaths writes the manifests, hydrator.metadata, and README.md files for each path in the provided paths. It
 // also writes a root-level hydrator.metadata file containing the repo URL and dry SHA.
-func WriteForPaths(rootPath string, repoUrl string, drySha string, paths []*apiclient.PathDetails) error { //nolint:revive //FIXME(var-naming)
+func WriteForPaths(root *os.Root, repoUrl string, drySha string, paths []*apiclient.PathDetails) error { //nolint:revive //FIXME(var-naming)
 	// Write the top-level readme.
-	err := writeMetadata(rootPath, hydratorMetadataFile{DrySHA: drySha, RepoURL: repoUrl})
+	err := writeMetadata(root, "", hydratorMetadataFile{DrySHA: drySha, RepoURL: repoUrl})
 	if err != nil {
 		return fmt.Errorf("failed to write top-level hydrator metadata: %w", err)
 	}
@@ -30,14 +33,13 @@ func WriteForPaths(rootPath string, repoUrl string, drySha string, paths []*apic
 			hydratePath = ""
 		}
 
-		var fullHydratePath string
-		fullHydratePath, err = files.SecureMkdirAll(rootPath, hydratePath, os.ModePerm)
+		err = mkdirAll(root, hydratePath)
 		if err != nil {
 			return fmt.Errorf("failed to create path: %w", err)
 		}
 
 		// Write the manifests
-		err = writeManifests(fullHydratePath, p.Manifests)
+		err = writeManifests(root, hydratePath, p.Manifests)
 		if err != nil {
 			return fmt.Errorf("failed to write manifests: %w", err)
 		}
@@ -48,13 +50,13 @@ func WriteForPaths(rootPath string, repoUrl string, drySha string, paths []*apic
 			DrySHA:   drySha,
 			RepoURL:  repoUrl,
 		}
-		err = writeMetadata(fullHydratePath, hydratorMetadata)
+		err = writeMetadata(root, hydratePath, hydratorMetadata)
 		if err != nil {
 			return fmt.Errorf("failed to write hydrator metadata: %w", err)
 		}
 
 		// Write README
-		err = writeReadme(fullHydratePath, hydratorMetadata)
+		err = writeReadme(root, hydratePath, hydratorMetadata)
 		if err != nil {
 			return fmt.Errorf("failed to write readme: %w", err)
 		}
@@ -63,31 +65,35 @@ func WriteForPaths(rootPath string, repoUrl string, drySha string, paths []*apic
 }
 
 // writeMetadata writes the metadata to the hydrator.metadata file.
-func writeMetadata(dirPath string, metadata hydratorMetadataFile) error {
+func writeMetadata(root *os.Root, dirPath string, metadata hydratorMetadataFile) error {
 	hydratorMetadataJSON, err := json.MarshalIndent(metadata, "", "  ")
 	if err != nil {
 		return fmt.Errorf("failed to marshal hydrator metadata: %w", err)
 	}
 	// No need to use SecureJoin here, as the path is already sanitized.
-	hydratorMetadataPath := path.Join(dirPath, "hydrator.metadata")
-	err = os.WriteFile(hydratorMetadataPath, hydratorMetadataJSON, os.ModePerm)
+	hydratorMetadataPath := filepath.Join(dirPath, "hydrator.metadata")
+	f, err := root.Create(hydratorMetadataPath)
 	if err != nil {
-		return fmt.Errorf("failed to write hydrator metadata: %w", err)
+		return fmt.Errorf("failed to create hydrator metadata file: %w", err)
+	}
+	defer io.Close(f)
+	_, err = f.Write(hydratorMetadataJSON)
+	if err != nil {
+		return fmt.Errorf("failed to write hydrator metadata file: %w", err)
 	}
 	return nil
 }
 
 // writeReadme writes the readme to the README.md file.
-func writeReadme(dirPath string, metadata hydratorMetadataFile) error {
-	readmeTemplate := template.New("readme")
-	readmeTemplate, err := readmeTemplate.Parse(manifestHydrationReadmeTemplate)
+func writeReadme(root *os.Root, dirPath string, metadata hydratorMetadataFile) error {
+	readmeTemplate, err := template.New("readme").Parse(manifestHydrationReadmeTemplate)
 	if err != nil {
 		return fmt.Errorf("failed to parse readme template: %w", err)
 	}
 	// Create writer to template into
 	// No need to use SecureJoin here, as the path is already sanitized.
-	readmePath := path.Join(dirPath, "README.md")
-	readmeFile, err := os.Create(readmePath)
+	readmePath := filepath.Join(dirPath, "README.md")
+	readmeFile, err := root.Create(readmePath)
 	if err != nil && !os.IsExist(err) {
 		return fmt.Errorf("failed to create README file: %w", err)
 	}
@@ -104,12 +110,12 @@ func writeReadme(dirPath string, metadata hydratorMetadataFile) error {
 
 // writeManifests writes the manifests to the manifest.yaml file, truncating the file if it exists and appending the
 // manifests in the order they are provided.
-func writeManifests(dirPath string, manifests []*apiclient.HydratedManifestDetails) error {
+func writeManifests(root *os.Root, dirPath string, manifests []*apiclient.HydratedManifestDetails) error {
 	// If the file exists, truncate it.
 	// No need to use SecureJoin here, as the path is already sanitized.
-	manifestPath := path.Join(dirPath, "manifest.yaml")
+	manifestPath := filepath.Join(dirPath, "manifest.yaml")
 
-	file, err := os.OpenFile(manifestPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, os.ModePerm)
+	file, err := root.OpenFile(manifestPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, os.ModePerm)
 	if err != nil {
 		return fmt.Errorf("failed to open manifest file: %w", err)
 	}
@@ -143,3 +149,25 @@ func writeManifests(dirPath string, manifests []*apiclient.HydratedManifestDetai
 
 	return nil
 }
+
+// mkdirAll creates the directory and all its parents if they do not exist. It returns an error if the directory
+// cannot be.
+func mkdirAll(root *os.Root, dirPath string) error {
+	parts := strings.Split(dirPath, string(os.PathSeparator))
+	builtPath := ""
+	for _, part := range parts {
+		if part == "" {
+			continue
+		}
+		builtPath = filepath.Join(builtPath, part)
+		err := root.Mkdir(builtPath, os.ModePerm)
+		if err != nil {
+			if errors.Is(err, fs.ErrExist) {
+				log.WithError(err).Warnf("path %s already exists, skipping", dirPath)
+				continue
+			}
+			return fmt.Errorf("failed to create path: %w", err)
+		}
+	}
+	return nil
+}

commitserver/commit/hydratorhelper_test.go

@@ -4,17 +4,38 @@ import (
 	"encoding/json"
 	"os"
 	"path"
+	"path/filepath"
 	"testing"
 
-	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/argoproj/argo-cd/v3/commitserver/apiclient"
 )
 
+// tempRoot creates a temporary directory and returns an os.Root object for it.
+// We use this instead of t.TempDir() because OSX does weird things with temp directories, and it triggers
+// the os.Root protections.
+func tempRoot(t *testing.T) *os.Root {
+	t.Helper()
+
+	dir, err := os.MkdirTemp(".", "")
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		err := os.RemoveAll(dir)
+		require.NoError(t, err)
+	})
+	root, err := os.OpenRoot(dir)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		err := root.Close()
+		require.NoError(t, err)
+	})
+	return root
+}
+
 func TestWriteForPaths(t *testing.T) {
-	dir := t.TempDir()
+	root := tempRoot(t)
 
 	repoURL := "https://github.com/example/repo"
 	drySha := "abc123"
@@ -33,13 +54,20 @@ func TestWriteForPaths(t *testing.T) {
 			},
 			Commands: []string{"command3"},
 		},
+		{
+			Path: "path3/nested",
+			Manifests: []*apiclient.HydratedManifestDetails{
+				{ManifestJSON: `{"kind":"Deployment","apiVersion":"apps/v1"}`},
+			},
+			Commands: []string{"command4"},
+		},
 	}
 
-	err := WriteForPaths(dir, repoURL, drySha, paths)
+	err := WriteForPaths(root, repoURL, drySha, paths)
 	require.NoError(t, err)
 
 	// Check if the top-level hydrator.metadata exists and contains the repo URL and dry SHA
-	topMetadataPath := path.Join(dir, "hydrator.metadata")
+	topMetadataPath := filepath.Join(root.Name(), "hydrator.metadata")
 	topMetadataBytes, err := os.ReadFile(topMetadataPath)
 	require.NoError(t, err)
 
@@ -50,8 +78,7 @@ func TestWriteForPaths(t *testing.T) {
 	assert.Equal(t, drySha, topMetadata.DrySHA)
 
 	for _, p := range paths {
-		fullHydratePath, err := securejoin.SecureJoin(dir, p.Path)
-		require.NoError(t, err)
+		fullHydratePath := filepath.Join(root.Name(), p.Path)
 
 		// Check if each path directory exists
 		assert.DirExists(t, fullHydratePath)
@@ -81,17 +108,17 @@ func TestWriteForPaths(t *testing.T) {
 }
 
 func TestWriteMetadata(t *testing.T) {
-	dir := t.TempDir()
+	root := tempRoot(t)
 
 	metadata := hydratorMetadataFile{
 		RepoURL: "https://github.com/example/repo",
 		DrySHA:  "abc123",
 	}
 
-	err := writeMetadata(dir, metadata)
+	err := writeMetadata(root, "", metadata)
 	require.NoError(t, err)
 
-	metadataPath := path.Join(dir, "hydrator.metadata")
+	metadataPath := filepath.Join(root.Name(), "hydrator.metadata")
 	metadataBytes, err := os.ReadFile(metadataPath)
 	require.NoError(t, err)
 
@@ -102,33 +129,33 @@ func TestWriteMetadata(t *testing.T) {
 }
 
 func TestWriteReadme(t *testing.T) {
-	dir := t.TempDir()
+	root := tempRoot(t)
 
 	metadata := hydratorMetadataFile{
 		RepoURL: "https://github.com/example/repo",
 		DrySHA:  "abc123",
 	}
 
-	err := writeReadme(dir, metadata)
+	err := writeReadme(root, "", metadata)
 	require.NoError(t, err)
 
-	readmePath := path.Join(dir, "README.md")
+	readmePath := filepath.Join(root.Name(), "README.md")
 	readmeBytes, err := os.ReadFile(readmePath)
 	require.NoError(t, err)
 	assert.Contains(t, string(readmeBytes), metadata.RepoURL)
 }
 
 func TestWriteManifests(t *testing.T) {
-	dir := t.TempDir()
+	root := tempRoot(t)
 
 	manifests := []*apiclient.HydratedManifestDetails{
 		{ManifestJSON: `{"kind":"Pod","apiVersion":"v1"}`},
 	}
 
-	err := writeManifests(dir, manifests)
+	err := writeManifests(root, "", manifests)
 	require.NoError(t, err)
 
-	manifestPath := path.Join(dir, "manifest.yaml")
+	manifestPath := path.Join(root.Name(), "manifest.yaml")
 	manifestBytes, err := os.ReadFile(manifestPath)
 	require.NoError(t, err)
 	assert.Contains(t, string(manifestBytes), "kind")