diff --git a/pkg/jobrunner/journal.go b/pkg/jobrunner/journal.go index b5ee9f5b..c09ffcf6 100644 --- a/pkg/jobrunner/journal.go +++ b/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() diff --git a/pkg/jobrunner/journal_test.go b/pkg/jobrunner/journal_test.go index dac14a38..a17a88b4 100644 --- a/pkg/jobrunner/journal_test.go +++ b/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()