security: harden filesystem path operations against path traversal

pkg/cli/download_workflow.go

@@ -138,6 +138,9 @@ func downloadWorkflowContentViaGitClone(repo, path, ref string, verbose bool) ([
 
 	// Read the file
 	filePath := filepath.Join(tmpDir, path)
+	if err := fileutil.MustBeWithin(tmpDir, filePath); err != nil {
+		return nil, fmt.Errorf("refusing to read file outside clone directory: %w", err)
+	}
 	content, err := os.ReadFile(filePath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read file from cloned repository: %w", err)

pkg/fileutil/fileutil.go

@@ -51,6 +51,38 @@ func ValidateAbsolutePath(path string) (string, error) {
 	return cleanPath, nil
 }
 
+// MustBeWithin checks that candidate is located within the base directory tree.
+// Both paths are resolved via filepath.EvalSymlinks (with filepath.Abs as
+// fallback when a path does not yet exist) before comparison, so neither ".."
+// components nor symlinks pointing outside base can be used to escape.
+//
+// Returns an error when:
+//   - Either path cannot be resolved to an absolute form.
+//   - The resolved candidate path starts outside the resolved base directory.
+func MustBeWithin(base, candidate string) error {
+	// EvalSymlinks resolves both symlinks and ".." components.
+	// Fall back to Abs when a path does not exist on disk yet.
+	absBase, err := filepath.EvalSymlinks(base)
+	if err != nil {
+		absBase, err = filepath.Abs(base)
+		if err != nil {
+			return fmt.Errorf("failed to resolve base path %q: %w", base, err)
+		}
+	}
+	absCand, err := filepath.EvalSymlinks(candidate)
+	if err != nil {
+		absCand, err = filepath.Abs(candidate)
+		if err != nil {
+			return fmt.Errorf("failed to resolve candidate path %q: %w", candidate, err)
+		}
+	}
+	rel, err := filepath.Rel(absBase, absCand)
+	if err != nil || !filepath.IsLocal(rel) {
+		return fmt.Errorf("path %q escapes base directory %q", candidate, base)
+	}
+	return nil
+}
+
 // FileExists checks if a file exists and is not a directory.
 func FileExists(path string) bool {
 	info, err := os.Stat(path)

pkg/fileutil/fileutil_test.go

@@ -316,6 +316,147 @@ func TestCopyFile(t *testing.T) {
 	})
 }
 
+func TestMustBeWithin(t *testing.T) {
+	base := t.TempDir()
+
+	tests := []struct {
+		name      string
+		candidate string
+		shouldErr bool
+	}{
+		{
+			name:      "file directly inside base",
+			candidate: filepath.Join(base, "file.txt"),
+			shouldErr: false,
+		},
+		{
+			name:      "file in subdirectory",
+			candidate: filepath.Join(base, "sub", "file.txt"),
+			shouldErr: false,
+		},
+		{
+			name:      "base directory itself",
+			candidate: base,
+			shouldErr: false,
+		},
+		{
+			name:      "path traversal with ..",
+			candidate: filepath.Join(base, "..", "escape.txt"),
+			shouldErr: true,
+		},
+		{
+			name:      "deeply nested traversal",
+			candidate: filepath.Join(base, "a", "b", "..", "..", "..", "escape.txt"),
+			shouldErr: true,
+		},
+		{
+			name:      "absolute path outside base",
+			candidate: "/etc/passwd",
+			shouldErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := MustBeWithin(base, tt.candidate)
+			if tt.shouldErr {
+				require.Error(t, err, "MustBeWithin should reject path %q relative to %q", tt.candidate, base)
+				assert.Contains(t, err.Error(), "escapes base directory", "Error should describe the escape")
+			} else {
+				require.NoError(t, err, "MustBeWithin should accept path %q within %q", tt.candidate, base)
+			}
+		})
+	}
+
+	t.Run("symlink escape", func(t *testing.T) {
+		// Create a real file outside the base directory.
+		outsideFile, err := os.CreateTemp("", "mustbewithin-outside-*")
+		require.NoError(t, err, "failed to create outside file")
+		t.Cleanup(func() { _ = os.Remove(outsideFile.Name()) })
+		outsidePath := outsideFile.Name()
+		require.NoError(t, outsideFile.Close())
+
+		// Create a symlink inside base that points to the outside file.
+		linkPath := filepath.Join(base, "link-to-outside")
+		if err := os.Symlink(outsidePath, linkPath); err != nil {
+			t.Skipf("symlinks not supported: %v", err)
+		}
+		t.Cleanup(func() { _ = os.Remove(linkPath) })
+
+		err = MustBeWithin(base, linkPath)
+		require.Error(t, err, "MustBeWithin should reject symlink that points outside base")
+		assert.Contains(t, err.Error(), "escapes base directory", "Error should describe the symlink escape")
+	})
+}
+
+func TestExtractFileFromTar_UnsafePaths(t *testing.T) {
+	buildTar := func(files map[string][]byte) []byte {
+		var buf bytes.Buffer
+		tw := tar.NewWriter(&buf)
+		for name, content := range files {
+			hdr := &tar.Header{
+				Name: name,
+				Mode: 0600,
+				Size: int64(len(content)),
+			}
+			if err := tw.WriteHeader(hdr); err != nil {
+				t.Fatalf("buildTar: WriteHeader: %v", err)
+			}
+			if _, err := tw.Write(content); err != nil {
+				t.Fatalf("buildTar: Write: %v", err)
+			}
+		}
+		if err := tw.Close(); err != nil {
+			t.Fatalf("buildTar: Close: %v", err)
+		}
+		return buf.Bytes()
+	}
+
+	t.Run("rejects absolute path as search target", func(t *testing.T) {
+		archive := buildTar(map[string][]byte{"file.txt": []byte("data")})
+		got, err := ExtractFileFromTar(archive, "/etc/passwd")
+		require.Error(t, err, "Should reject absolute path as search target")
+		assert.Contains(t, err.Error(), "unsafe path", "Error should mention unsafe path")
+		assert.Nil(t, got, "Result should be nil for unsafe path")
+	})
+
+	t.Run("rejects dotdot in search target", func(t *testing.T) {
+		archive := buildTar(map[string][]byte{"file.txt": []byte("data")})
+		got, err := ExtractFileFromTar(archive, "../escape.txt")
+		require.Error(t, err, "Should reject .. in search target")
+		assert.Contains(t, err.Error(), "unsafe path", "Error should mention unsafe path")
+		assert.Nil(t, got, "Result should be nil for unsafe path")
+	})
+
+	t.Run("allows filename containing dotdot as substring", func(t *testing.T) {
+		want := []byte("not a traversal")
+		archive := buildTar(map[string][]byte{"file..backup.txt": want})
+		got, err := ExtractFileFromTar(archive, "file..backup.txt")
+		require.NoError(t, err, "Should allow filename with dotdot as substring, not path component")
+		assert.Equal(t, want, got, "Should return correct content for file..backup.txt")
+	})
+
+	t.Run("skips tar entry with absolute name, does not match", func(t *testing.T) {
+		// Build archive with an absolute-named entry; it should be skipped even
+		// if the caller searches for the same name.
+		archive := buildTar(map[string][]byte{"/etc/passwd": []byte("root")})
+		// Searching for the same absolute path must be rejected by the safe-target check.
+		got, err := ExtractFileFromTar(archive, "/etc/passwd")
+		require.Error(t, err, "Should reject absolute search target")
+		assert.Nil(t, got)
+	})
+
+	t.Run("skips tar entry with dotdot name and returns not found", func(t *testing.T) {
+		archive := buildTar(map[string][]byte{"../escape.txt": []byte("bad")})
+		// Searching for the relative form that doesn't start with .. is fine as
+		// a target; the archive entry is just silently skipped.
+		got, err := ExtractFileFromTar(archive, "escape.txt")
+		require.Error(t, err, "File should not be found because dotdot entry was skipped")
+		assert.Contains(t, err.Error(), "not found", "Error should indicate file not found")
+		assert.Nil(t, got)
+	})
+}
+
 func TestExtractFileFromTar(t *testing.T) {
 	// Helper to build an in-memory tar archive
 	buildTar := func(files map[string][]byte) []byte {

pkg/fileutil/tar.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"path/filepath"
 
 	"github.com/github/gh-aw/pkg/logger"
 )
@@ -15,7 +16,16 @@ var tarLog = logger.New("fileutil:tar")
 // ExtractFileFromTar extracts a single file from a tar archive.
 // Uses Go's standard archive/tar for cross-platform compatibility instead of
 // spawning an external tar process which may not be available on all platforms.
+//
+// path must be a local, relative path (no absolute paths or ".." components).
+// filepath.IsLocal is used to enforce this for both the search target and each
+// tar entry name, guarding against path-traversal payloads embedded in archives.
 func ExtractFileFromTar(data []byte, path string) ([]byte, error) {
+	// Reject unsafe search targets before opening the archive.
+	if !filepath.IsLocal(path) {
+		return nil, fmt.Errorf("unsafe path requested from tar archive: %q", path)
+	}
+
 	tarLog.Printf("Extracting file from tar archive: target=%s, archive_size=%d bytes", path, len(data))
 	tr := tar.NewReader(bytes.NewReader(data))
 	entriesScanned := 0
@@ -29,6 +39,11 @@ func ExtractFileFromTar(data []byte, path string) ([]byte, error) {
 			return nil, fmt.Errorf("failed to read tar archive: %w", err)
 		}
 		entriesScanned++
+		// Reject tar entries that could escape a destination directory.
+		if !filepath.IsLocal(header.Name) {
+			tarLog.Printf("Skipping unsafe tar entry: %s", header.Name)
+			continue
+		}
 		if header.Name == path {
 			tarLog.Printf("Found file in tar archive after scanning %d entries: %s", entriesScanned, path)
 			return io.ReadAll(tr)

pkg/parser/remote_fetch.go

@@ -545,6 +545,9 @@ func downloadFileViaGitClone(owner, repo, path, ref, host string) ([]byte, error
 
 	// Read the file from the cloned repository
 	filePath := filepath.Join(tmpDir, path)
+	if err := fileutil.MustBeWithin(tmpDir, filePath); err != nil {
+		return nil, fmt.Errorf("refusing to read file outside clone directory: %w", err)
+	}
 	content, err := os.ReadFile(filePath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read file from cloned repository: %w", err)