diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a0f6dc43a1f..779b53b3f2b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -169,6 +169,36 @@ The agent places validation logic appropriately: See [Validation Architecture](scratchpad/validation-architecture.md) for the complete decision tree. +#### File Path Security + +All file operations must validate paths to prevent path traversal attacks: + +**Use `fileutil.ValidateAbsolutePath` before file operations:** + +```go +import "github.com/github/gh-aw/pkg/fileutil" + +// Validate path before reading/writing files +cleanPath, err := fileutil.ValidateAbsolutePath(userInputPath) +if err != nil { + return fmt.Errorf("invalid path: %w", err) +} +content, err := os.ReadFile(cleanPath) +``` + +**Security checks performed:** +- Normalizes path using `filepath.Clean` (removes `.` and `..` components) +- Verifies path is absolute (blocks relative path traversal) +- Returns descriptive errors for invalid paths + +**When to use:** +- Before `os.ReadFile`, `os.WriteFile`, `os.Stat`, `os.Open` +- Before `os.MkdirAll` or other directory operations +- After constructing paths with `filepath.Join` +- When processing user-provided file paths + +This provides defense-in-depth against path traversal vulnerabilities (e.g., `../../../etc/passwd`). + #### CLI Breaking Changes The agent evaluates whether changes are breaking: diff --git a/pkg/cli/commands.go b/pkg/cli/commands.go index 4e50140615c..b1b8d0c7f1c 100644 --- a/pkg/cli/commands.go +++ b/pkg/cli/commands.go @@ -13,6 +13,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) @@ -222,6 +223,13 @@ func NewWorkflow(workflowName string, verbose bool, force bool) error { githubWorkflowsDir := filepath.Join(workingDir, constants.GetWorkflowDir()) commandsLog.Printf("Creating workflows directory: %s", githubWorkflowsDir) + // Validate the directory path + githubWorkflowsDir, err = fileutil.ValidateAbsolutePath(githubWorkflowsDir) + if err != nil { + commandsLog.Printf("Invalid workflows directory path: %v", err) + return fmt.Errorf("invalid workflows directory path: %w", err) + } + if err := os.MkdirAll(githubWorkflowsDir, 0755); err != nil { commandsLog.Printf("Failed to create workflows directory: %v", err) return fmt.Errorf("failed to create .github/workflows directory: %w", err) @@ -231,6 +239,13 @@ func NewWorkflow(workflowName string, verbose bool, force bool) error { destFile := filepath.Join(githubWorkflowsDir, workflowName+".md") commandsLog.Printf("Destination file: %s", destFile) + // Validate the destination file path + destFile, err = fileutil.ValidateAbsolutePath(destFile) + if err != nil { + commandsLog.Printf("Invalid destination file path: %v", err) + return fmt.Errorf("invalid destination file path: %w", err) + } + // Check if destination file already exists if _, err := os.Stat(destFile); err == nil && !force { commandsLog.Printf("Workflow file already exists and force=false: %s", destFile) diff --git a/pkg/cli/completions.go b/pkg/cli/completions.go index 2af12eba9e0..9d2d8269f6b 100644 --- a/pkg/cli/completions.go +++ b/pkg/cli/completions.go @@ -5,6 +5,7 @@ import ( "path/filepath" "strings" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/workflow" @@ -16,12 +17,10 @@ var completionsLog = logger.New("cli:completions") // getWorkflowDescription extracts the description field from a workflow's frontmatter // Returns empty string if the description is not found or if there's an error reading the file func getWorkflowDescription(filePath string) string { - // Sanitize the filepath to prevent path traversal attacks - cleanPath := filepath.Clean(filePath) - - // Verify the path is absolute to prevent relative path traversal - if !filepath.IsAbs(cleanPath) { - completionsLog.Printf("Invalid workflow file path (not absolute): %s", filePath) + // Validate the path for security + cleanPath, err := fileutil.ValidateAbsolutePath(filePath) + if err != nil { + completionsLog.Printf("Invalid workflow file path: %v", err) return "" } diff --git a/pkg/cli/git.go b/pkg/cli/git.go index 5487fa19b5e..8a0e1f9a8f6 100644 --- a/pkg/cli/git.go +++ b/pkg/cli/git.go @@ -9,6 +9,7 @@ import ( "github.com/charmbracelet/huh" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" ) @@ -43,6 +44,12 @@ func findGitRootForPath(path string) (string, error) { return "", fmt.Errorf("failed to get absolute path: %w", err) } + // Validate the absolute path + absPath, err = fileutil.ValidateAbsolutePath(absPath) + if err != nil { + return "", fmt.Errorf("invalid path: %w", err) + } + // Use the directory containing the file dir := filepath.Dir(absPath) @@ -117,6 +124,13 @@ func getRepositorySlugFromRemoteForPath(path string) string { return "" } + // Validate the absolute path + absPath, err = fileutil.ValidateAbsolutePath(absPath) + if err != nil { + gitLog.Printf("Invalid path: %v", err) + return "" + } + // Use the directory containing the file dir := filepath.Dir(absPath) @@ -379,8 +393,12 @@ func ToGitRootRelativePath(path string) string { gitLog.Printf("Converted to absolute path: %s", absPath) } - // Clean the absolute path - absPath = filepath.Clean(absPath) + // Validate and clean the absolute path + absPath, err := fileutil.ValidateAbsolutePath(absPath) + if err != nil { + gitLog.Printf("Invalid path: %v", err) + return path + } // Find the git root by looking for .github directory // Walk up the directory tree to find it diff --git a/pkg/cli/run_push.go b/pkg/cli/run_push.go index e964586cd4c..671b69bc0ce 100644 --- a/pkg/cli/run_push.go +++ b/pkg/cli/run_push.go @@ -12,6 +12,7 @@ import ( "github.com/github/gh-aw/pkg/stringutil" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" ) @@ -36,6 +37,13 @@ func collectWorkflowFiles(ctx context.Context, workflowPath string, verbose bool } runPushLog.Printf("Resolved absolute workflow path: %s", absWorkflowPath) + // Validate the absolute path + absWorkflowPath, err = fileutil.ValidateAbsolutePath(absWorkflowPath) + if err != nil { + runPushLog.Printf("Invalid workflow path: %v", err) + return nil, fmt.Errorf("invalid workflow path: %w", err) + } + // Add the workflow .md file files[absWorkflowPath] = true runPushLog.Printf("Added workflow file: %s", absWorkflowPath) @@ -173,6 +181,13 @@ func checkLockFileStatus(workflowPath string) (*LockFileStatus, error) { } runPushLog.Printf("Resolved absolute path: %s", absWorkflowPath) + // Validate the absolute path + absWorkflowPath, err = fileutil.ValidateAbsolutePath(absWorkflowPath) + if err != nil { + runPushLog.Printf("Invalid workflow path: %v", err) + return nil, fmt.Errorf("invalid workflow path: %w", err) + } + lockFilePath := stringutil.MarkdownToLockFile(absWorkflowPath) runPushLog.Printf("Expected lock file path: %s", lockFilePath) status := &LockFileStatus{ @@ -481,8 +496,14 @@ func pushWorkflowFiles(workflowName string, files []string, refOverride string, // Normalize the path absPath, err := filepath.Abs(file) if err == nil { - ourFiles[absPath] = true - runPushLog.Printf("Added to our files map: %s (absolute: %s)", file, absPath) + // Validate the absolute path + validPath, validErr := fileutil.ValidateAbsolutePath(absPath) + if validErr == nil { + ourFiles[validPath] = true + runPushLog.Printf("Added to our files map: %s (absolute: %s)", file, validPath) + } else { + runPushLog.Printf("Failed to validate path for %s: %v", absPath, validErr) + } } else { runPushLog.Printf("Failed to get absolute path for %s: %v", file, err) } @@ -497,9 +518,13 @@ func pushWorkflowFiles(workflowName string, files []string, refOverride string, runPushLog.Printf("Checking staged file: %s", stagedFile) // Try both absolute and relative paths absStagedPath, err := filepath.Abs(stagedFile) - if err == nil && ourFiles[absStagedPath] { - runPushLog.Printf("Staged file %s matches our file %s (absolute)", stagedFile, absStagedPath) - continue + if err == nil { + // Validate the staged path + validPath, validErr := fileutil.ValidateAbsolutePath(absStagedPath) + if validErr == nil && ourFiles[validPath] { + runPushLog.Printf("Staged file %s matches our file %s (absolute)", stagedFile, validPath) + continue + } } if ourFiles[stagedFile] { runPushLog.Printf("Staged file %s matches our file (relative)", stagedFile) diff --git a/pkg/cli/trial_repository.go b/pkg/cli/trial_repository.go index a9345d4e6fa..8f0008b8055 100644 --- a/pkg/cli/trial_repository.go +++ b/pkg/cli/trial_repository.go @@ -12,6 +12,7 @@ import ( "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) @@ -174,6 +175,12 @@ func cloneTrialHostRepository(repoSlug string, verbose bool) (string, error) { // Create temporary directory tempDir := filepath.Join(os.TempDir(), fmt.Sprintf("gh-aw-trial-%x", time.Now().UnixNano())) + // Validate the temporary directory path + tempDir, err := fileutil.ValidateAbsolutePath(tempDir) + if err != nil { + return "", fmt.Errorf("invalid temporary directory path: %w", err) + } + if verbose { fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Cloning host repository to: %s", tempDir))) } @@ -287,6 +294,12 @@ func installLocalWorkflowInTrialMode(originalDir, tempDir string, parsedSpec *Wo // Construct the source path (relative to original directory) sourcePath := filepath.Join(originalDir, parsedSpec.WorkflowPath) + // Validate source path + sourcePath, err := fileutil.ValidateAbsolutePath(sourcePath) + if err != nil { + return fmt.Errorf("invalid source path: %w", err) + } + // Check if the source file exists if _, err := os.Stat(sourcePath); os.IsNotExist(err) { return fmt.Errorf("local workflow file does not exist: %s", sourcePath) @@ -294,6 +307,13 @@ func installLocalWorkflowInTrialMode(originalDir, tempDir string, parsedSpec *Wo // Create the workflows directory in the temp directory workflowsDir := filepath.Join(tempDir, constants.GetWorkflowDir()) + + // Validate workflows directory path + workflowsDir, err = fileutil.ValidateAbsolutePath(workflowsDir) + if err != nil { + return fmt.Errorf("invalid workflows directory path: %w", err) + } + if err := os.MkdirAll(workflowsDir, 0755); err != nil { return fmt.Errorf("failed to create workflows directory: %w", err) } @@ -301,6 +321,12 @@ func installLocalWorkflowInTrialMode(originalDir, tempDir string, parsedSpec *Wo // Construct the destination path destPath := filepath.Join(workflowsDir, parsedSpec.WorkflowName+".md") + // Validate destination path + destPath, err = fileutil.ValidateAbsolutePath(destPath) + if err != nil { + return fmt.Errorf("invalid destination path: %w", err) + } + // Read the source file content, err := os.ReadFile(sourcePath) if err != nil { @@ -339,6 +365,12 @@ func modifyWorkflowForTrialMode(tempDir, workflowName, logicalRepoSlug string, v // Find the workflow markdown file workflowPath := filepath.Join(tempDir, constants.GetWorkflowDir(), fmt.Sprintf("%s.md", workflowName)) + // Validate workflow path + workflowPath, err := fileutil.ValidateAbsolutePath(workflowPath) + if err != nil { + return fmt.Errorf("invalid workflow path: %w", err) + } + content, err := os.ReadFile(workflowPath) if err != nil { return fmt.Errorf("failed to read workflow file: %w", err) diff --git a/pkg/fileutil/fileutil.go b/pkg/fileutil/fileutil.go new file mode 100644 index 00000000000..80e34cb47cb --- /dev/null +++ b/pkg/fileutil/fileutil.go @@ -0,0 +1,45 @@ +// Package fileutil provides utility functions for working with file paths and file operations. +package fileutil + +import ( + "fmt" + "path/filepath" +) + +// ValidateAbsolutePath validates that a file path is absolute and safe to use. +// It performs the following security checks: +// - Cleans the path using filepath.Clean to normalize . and .. components +// - Verifies the path is absolute to prevent relative path traversal attacks +// +// Returns the cleaned absolute path if validation succeeds, or an error if: +// - The path is empty +// - The path is relative (not absolute) +// +// This function should be used before any file operations (read, write, stat, etc.) +// to ensure defense-in-depth security against path traversal vulnerabilities. +// +// Example: +// +// cleanPath, err := fileutil.ValidateAbsolutePath(userInputPath) +// +// if err != nil { +// return fmt.Errorf("invalid path: %w", err) +// } +// +// content, err := os.ReadFile(cleanPath) +func ValidateAbsolutePath(path string) (string, error) { + // Check for empty path + if path == "" { + return "", fmt.Errorf("path cannot be empty") + } + + // Sanitize the filepath to prevent path traversal attacks + cleanPath := filepath.Clean(path) + + // Verify the path is absolute to prevent relative path traversal + if !filepath.IsAbs(cleanPath) { + return "", fmt.Errorf("path must be absolute, got: %s", path) + } + + return cleanPath, nil +} diff --git a/pkg/fileutil/fileutil_test.go b/pkg/fileutil/fileutil_test.go new file mode 100644 index 00000000000..2019f4596ca --- /dev/null +++ b/pkg/fileutil/fileutil_test.go @@ -0,0 +1,178 @@ +//go:build !integration + +package fileutil + +import ( + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateAbsolutePath(t *testing.T) { + tests := []struct { + name string + path string + shouldError bool + errorMsg string + }{ + { + name: "valid absolute Unix path", + path: "/home/user/file.txt", + shouldError: false, + }, + { + name: "valid absolute path with cleaned components", + path: "/home/user/../user/file.txt", + shouldError: false, + }, + { + name: "empty path", + path: "", + shouldError: true, + errorMsg: "path cannot be empty", + }, + { + name: "relative path", + path: "relative/path.txt", + shouldError: true, + errorMsg: "path must be absolute", + }, + { + name: "relative path with dot", + path: "./file.txt", + shouldError: true, + errorMsg: "path must be absolute", + }, + { + name: "relative path with double dot", + path: "../file.txt", + shouldError: true, + errorMsg: "path must be absolute", + }, + { + name: "path traversal attempt", + path: "../../../etc/passwd", + shouldError: true, + errorMsg: "path must be absolute", + }, + { + name: "single dot", + path: ".", + shouldError: true, + errorMsg: "path must be absolute", + }, + { + name: "double dot", + path: "..", + shouldError: true, + errorMsg: "path must be absolute", + }, + } + + // Add Windows-specific tests only on Windows + if runtime.GOOS == "windows" { + tests = append(tests, []struct { + name string + path string + shouldError bool + errorMsg string + }{ + { + name: "valid absolute Windows path", + path: "C:\\Users\\user\\file.txt", + shouldError: false, + }, + { + name: "valid absolute Windows UNC path", + path: "\\\\server\\share\\file.txt", + shouldError: false, + }, + }...) + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := ValidateAbsolutePath(tt.path) + + if tt.shouldError { + require.Error(t, err, "Expected error for path: %s", tt.path) + if tt.errorMsg != "" { + assert.Contains(t, err.Error(), tt.errorMsg, "Error message should contain expected text") + } + assert.Empty(t, result, "Result should be empty on error") + } else { + require.NoError(t, err, "Should not error for valid absolute path: %s", tt.path) + assert.NotEmpty(t, result, "Result should not be empty") + assert.True(t, filepath.IsAbs(result), "Result should be an absolute path: %s", result) + // Verify path is cleaned (no .. components) + assert.NotContains(t, result, "..", "Cleaned path should not contain .. components") + } + }) + } +} + +func TestValidateAbsolutePath_Cleaning(t *testing.T) { + // Test that paths are properly cleaned + tests := []struct { + name string + path string + expected string + }{ + { + name: "path with redundant separators", + path: "/home//user///file.txt", + expected: "/home/user/file.txt", + }, + { + name: "path with trailing separator", + path: "/home/user/", + expected: "/home/user", + }, + { + name: "path with . components", + path: "/home/./user/./file.txt", + expected: "/home/user/file.txt", + }, + { + name: "path with .. components", + path: "/home/user/../user/file.txt", + expected: "/home/user/file.txt", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Only run on Unix systems for consistent path separators + if runtime.GOOS != "windows" { + result, err := ValidateAbsolutePath(tt.path) + require.NoError(t, err, "Should not error for valid absolute path") + assert.Equal(t, tt.expected, result, "Path should be cleaned correctly") + } + }) + } +} + +func TestValidateAbsolutePath_SecurityScenarios(t *testing.T) { + // Test common path traversal attack patterns + traversalPatterns := []string{ + "../../etc/passwd", + "../../../etc/passwd", + "../../../../etc/passwd", + "..\\..\\windows\\system32\\config\\sam", + "./../../../etc/passwd", + "./../../etc/passwd", + } + + for _, pattern := range traversalPatterns { + t.Run("blocks_"+strings.ReplaceAll(pattern, "/", "_"), func(t *testing.T) { + result, err := ValidateAbsolutePath(pattern) + require.Error(t, err, "Should reject path traversal pattern: %s", pattern) + assert.Contains(t, err.Error(), "path must be absolute", "Error should mention absolute path requirement") + assert.Empty(t, result, "Result should be empty for invalid path") + }) + } +} diff --git a/pkg/workflow/max_turns_test.go b/pkg/workflow/max_turns_test.go index b1dc696d774..4b313ab6d9a 100644 --- a/pkg/workflow/max_turns_test.go +++ b/pkg/workflow/max_turns_test.go @@ -76,7 +76,7 @@ permissions: engine: id: claude max-turns: 10 -timeout_minutes: 15 +timeout-minutes: 15 strict: false tools: github: