Standardize path validation across file operations to prevent path traversal

[Copilot]

Problem

Path validation was inconsistent across the codebase - completions.go demonstrated proper absolute path validation with security logging, but this pattern wasn't applied to ~30 other file operations using filepath.Join.

Changes

New validation helper (pkg/fileutil)

• ValidateAbsolutePath(path string) (string, error) - performs filepath.Clean and absolute path check
• Returns cleaned path or descriptive error
• Comprehensive tests including path traversal attack patterns

Applied to CLI file operations (5 files, 13 locations)

• completions.go - Refactored existing validation to use helper
• commands.go - Workflow directory and destination file creation
• trial_repository.go - Temp directories, workflow source/dest paths (5 locations)
• git.go - Git root lookups and path conversions (3 locations)
• run_push.go - Workflow and staged file path handling (3 locations)

Documentation (CONTRIBUTING.md)

• Added "File Path Security" section with usage pattern and examples

Usage

import "github.com/github/gh-aw/pkg/fileutil"

// Before file operations
cleanPath, err := fileutil.ValidateAbsolutePath(userInputPath)
if err != nil {
    return fmt.Errorf("invalid path: %w", err)
}
content, err := os.ReadFile(cleanPath)

Blocks relative path traversal patterns: ../../../etc/passwd, ., .., etc.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[plan] Standardize path validation patterns across file operations</issue_title>
<issue_description>## Objective

Extract the excellent path validation pattern from pkg/cli/completions.go:20 into a reusable helper function and apply it consistently across ~30 files using filepath.Join and file operations to strengthen defense-in-depth security posture.

Context

Issue #14844 identified inconsistent path validation: while completions.go demonstrates excellent path traversal protection with absolute path validation and security logging, this pattern is not consistently applied across the codebase. This creates maintenance challenges and makes it harder to audit for path traversal vulnerabilities.

Good Example to Extract

// pkg/cli/completions.go:20 - GOOD PATTERN
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)
        return ""
    }
    
    content, err := os.ReadFile(cleanPath)
    // ...
}

Approach

1. Create Reusable Helper: Extract validation into pkg/fileutil/fileutil.go

   • Function: ValidateAbsolutePath(path string) (string, error)
   • Performs filepath.Clean and absolute path check
   • Returns cleaned path or descriptive error

2. Refactor Existing Code: Update files to use the helper

   • pkg/cli/completions.go - Refactor to use new helper
   • pkg/cli/commands.go - Add validation to file operations
   • pkg/cli/trial_repository.go - Add validation (5+ locations)
   • pkg/cli/git.go - Standardize validation logic
   • pkg/cli/run_push.go - Replace ad-hoc validation

3. Add Tests: Comprehensive path validation tests

   • Valid absolute paths pass
   • Relative paths rejected
   • Path traversal attempts (../../../etc/passwd) blocked
   • Edge cases (empty path, ., .., symlinks)

4. Document Pattern: Add security guidelines to CONTRIBUTING.md

Files to Modify

• Create: pkg/fileutil/fileutil.go - New validation helper
• Create: pkg/fileutil/fileutil_test.go - Validation tests
• Update: pkg/cli/completions.go - Use new helper (lines 20-30)
• Update: pkg/cli/commands.go - Add validation (lines 222, 231)
• Update: pkg/cli/trial_repository.go - Add validation (lines 175, 288, 296, 302, 340)
• Update: pkg/cli/git.go - Standardize (lines 41, 114, 383)
• Update: pkg/cli/run_push.go - Standardize (lines 32, 169, 482, 499)
• Update: CONTRIBUTING.md - Document file operation security pattern

Acceptance Criteria

• pkg/fileutil package created with ValidateAbsolutePath helper
• Helper function validates absolute paths and returns cleaned path
• Clear error messages for validation failures
• All identified files refactored to use helper
• Tests cover valid paths, relative paths, traversal attempts, edge cases
• No breaking changes to existing functionality
• Security pattern documented in CONTRIBUTING.md
• CI passes with no new security warnings
  Related to [sergo] Sergo Report: Documentation-Security-Naming - 2026-02-10 #14844

AI generated by Plan Command for #14844

• expires on Feb 13, 2026, 12:43 AM UTC

Comments on the Issue (you are @copilot in this section)

• Fixes [plan] Standardize path validation patterns across file operations #14858

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[Copilot]

Pull request overview

This PR standardizes path validation for CLI file operations by extracting an absolute-path validation helper into pkg/fileutil and refactoring several CLI paths to use it, strengthening defense-in-depth against relative path traversal.

Changes:

• Added fileutil.ValidateAbsolutePath() helper and unit tests.
• Refactored multiple CLI code paths to validate/clean absolute paths before file operations.
• Documented the required validation pattern in CONTRIBUTING.md.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
pkg/fileutil/fileutil.go	Introduces ValidateAbsolutePath helper used to clean and enforce absolute paths.
pkg/fileutil/fileutil_test.go	Adds unit tests covering valid/invalid paths and common traversal patterns.
pkg/cli/completions.go	Replaces inline absolute-path checks with the shared helper before reading workflow files.
pkg/cli/commands.go	Validates workflow directory and destination file paths before creating/writing.
pkg/cli/trial_repository.go	Validates temp/workflow source and destination paths before stat/read/write/mkdir.
pkg/cli/git.go	Standardizes validation/cleaning when converting and using paths in git helpers.
pkg/cli/run_push.go	Validates resolved workflow paths and normalizes comparisons in push staging logic.
CONTRIBUTING.md	Adds “File Path Security” guidance pointing contributors to the helper.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

pkg/fileutil/fileutil.go is guarded by //go:build !integration, but this helper is imported by non-test CLI code. Building/running with -tags integration will exclude the only source file in the fileutil package and break compilation. Remove the build tag from the non-test source file (it’s fine to keep build tags on test files if needed).

Suggested change

//go:build !integration

[Copilot]

The error text includes the unquoted raw path value. Using %q (or otherwise sanitizing) makes logs/error strings unambiguous and avoids confusing output if the path contains spaces/newlines.

Suggested change

	return "", fmt.Errorf("path must be absolute, got: %s", path)
	return "", fmt.Errorf("path must be absolute, got: %q", path)

[Copilot]

assert.NotContains(t, result, "..") is too broad: a valid absolute path can legally contain .. as part of a directory/file name (e.g. /var/data/..snapshot/file). If the intent is to ensure filepath.Clean removed parent-directory elements, assert that no path element equals ".." rather than doing a substring match.

[Copilot]

In TestValidateAbsolutePath_Cleaning, the Windows branch silently does nothing, so the subtests still run but don’t assert anything. Consider calling t.Skip(...) when GOOS == "windows" (or splitting into OS-specific tests) so it’s clear these cases are intentionally not executed on Windows.

Suggested change

	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")
	}
	if runtime.GOOS == "windows" {
	t.Skip("TestValidateAbsolutePath_Cleaning only runs on Unix-like systems")
	}
	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")