Merge pull request 'fix(security): path traversal in journal logging (#46)' (#156) from fix/issue-46-path-traversal into new

pkg/jobrunner/journal.go

@@ -5,9 +5,14 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"regexp"
+	"strings"
 	"sync"
 )
 
+// validPathComponent matches safe repo owner/name characters (alphanumeric, hyphen, underscore, dot).
+var validPathComponent = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`)
+
 // JournalEntry is a single line in the JSONL audit log.
 type JournalEntry struct {
 	Timestamp string         `json:"ts"`
@@ -52,6 +57,36 @@ func NewJournal(baseDir string) (*Journal, error) {
 	return &Journal{baseDir: baseDir}, nil
 }
 
+// sanitizePathComponent validates a single path component (owner or repo name)
+// to prevent path traversal attacks. It rejects "..", empty strings, paths
+// containing separators, and any value outside the safe character set.
+func sanitizePathComponent(name string) (string, error) {
+	// Reject empty or whitespace-only values.
+	if name == "" || strings.TrimSpace(name) == "" {
+		return "", fmt.Errorf("invalid path component: %q", name)
+	}
+
+	// Reject inputs containing path separators (directory traversal attempt).
+	if strings.ContainsAny(name, `/\`) {
+		return "", fmt.Errorf("path component contains directory separator: %q", name)
+	}
+
+	// Use filepath.Clean to normalize (e.g., collapse redundant dots).
+	clean := filepath.Clean(name)
+
+	// Reject traversal components.
+	if clean == "." || clean == ".." {
+		return "", fmt.Errorf("invalid path component: %q", name)
+	}
+
+	// Validate against the safe character set.
+	if !validPathComponent.MatchString(clean) {
+		return "", fmt.Errorf("path component contains invalid characters: %q", name)
+	}
+
+	return clean, nil
+}
+
 // Append writes a journal entry for the given signal and result.
 func (j *Journal) Append(signal *PipelineSignal, result *ActionResult) error {
 	if signal == nil {
@@ -90,8 +125,31 @@ func (j *Journal) Append(signal *PipelineSignal, result *ActionResult) error {
 	}
 	data = append(data, '\n')
 
+	// Sanitize path components to prevent path traversal (CVE: issue #46).
+	owner, err := sanitizePathComponent(signal.RepoOwner)
+	if err != nil {
+		return fmt.Errorf("invalid repo owner: %w", err)
+	}
+	repo, err := sanitizePathComponent(signal.RepoName)
+	if err != nil {
+		return fmt.Errorf("invalid repo name: %w", err)
+	}
+
 	date := result.Timestamp.UTC().Format("2006-01-02")
-	dir := filepath.Join(j.baseDir, signal.RepoOwner, signal.RepoName)
+	dir := filepath.Join(j.baseDir, owner, repo)
+
+	// Resolve to absolute path and verify it stays within baseDir.
+	absBase, err := filepath.Abs(j.baseDir)
+	if err != nil {
+		return fmt.Errorf("resolve base directory: %w", err)
+	}
+	absDir, err := filepath.Abs(dir)
+	if err != nil {
+		return fmt.Errorf("resolve journal directory: %w", err)
+	}
+	if !strings.HasPrefix(absDir, absBase+string(filepath.Separator)) {
+		return fmt.Errorf("journal path %q escapes base directory %q", absDir, absBase)
+	}
 
 	j.mu.Lock()
 	defer j.mu.Unlock()

pkg/jobrunner/journal_test.go

@@ -113,6 +113,123 @@ func TestJournal_Append_Good(t *testing.T) {
 	assert.Equal(t, 2, lines, "expected two JSONL lines after two appends")
 }
 
+func TestJournal_Append_Bad_PathTraversal(t *testing.T) {
+	dir := t.TempDir()
+
+	j, err := NewJournal(dir)
+	require.NoError(t, err)
+
+	ts := time.Now()
+
+	tests := []struct {
+		name      string
+		repoOwner string
+		repoName  string
+		wantErr   string
+	}{
+		{
+			name:      "dotdot owner",
+			repoOwner: "..",
+			repoName:  "core",
+			wantErr:   "invalid repo owner",
+		},
+		{
+			name:      "dotdot repo",
+			repoOwner: "host-uk",
+			repoName:  "../../etc/cron.d",
+			wantErr:   "invalid repo name",
+		},
+		{
+			name:      "slash in owner",
+			repoOwner: "../etc",
+			repoName:  "core",
+			wantErr:   "invalid repo owner",
+		},
+		{
+			name:      "absolute path in repo",
+			repoOwner: "host-uk",
+			repoName:  "/etc/passwd",
+			wantErr:   "invalid repo name",
+		},
+		{
+			name:      "empty owner",
+			repoOwner: "",
+			repoName:  "core",
+			wantErr:   "invalid repo owner",
+		},
+		{
+			name:      "empty repo",
+			repoOwner: "host-uk",
+			repoName:  "",
+			wantErr:   "invalid repo name",
+		},
+		{
+			name:      "dot only owner",
+			repoOwner: ".",
+			repoName:  "core",
+			wantErr:   "invalid repo owner",
+		},
+		{
+			name:      "spaces only owner",
+			repoOwner: "   ",
+			repoName:  "core",
+			wantErr:   "invalid repo owner",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			signal := &PipelineSignal{
+				RepoOwner: tc.repoOwner,
+				RepoName:  tc.repoName,
+			}
+			result := &ActionResult{
+				Action:    "merge",
+				Timestamp: ts,
+			}
+
+			err := j.Append(signal, result)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), tc.wantErr)
+		})
+	}
+}
+
+func TestJournal_Append_Good_ValidNames(t *testing.T) {
+	dir := t.TempDir()
+
+	j, err := NewJournal(dir)
+	require.NoError(t, err)
+
+	ts := time.Date(2026, 2, 5, 14, 30, 0, 0, time.UTC)
+
+	// Verify valid names with dots, hyphens, underscores all work.
+	validNames := []struct {
+		owner string
+		repo  string
+	}{
+		{"host-uk", "core"},
+		{"my_org", "my_repo"},
+		{"org.name", "repo.v2"},
+		{"a", "b"},
+		{"Org-123", "Repo_456.go"},
+	}
+
+	for _, vn := range validNames {
+		signal := &PipelineSignal{
+			RepoOwner: vn.owner,
+			RepoName:  vn.repo,
+		}
+		result := &ActionResult{
+			Action:    "test",
+			Timestamp: ts,
+		}
+
+		err := j.Append(signal, result)
+		assert.NoError(t, err, "expected valid name pair %s/%s to succeed", vn.owner, vn.repo)
+	}
+}
+
 func TestJournal_Append_Bad_NilSignal(t *testing.T) {
 	dir := t.TempDir()