fix(security): sanitize path components in journal logging (#46)

Prevent path traversal in Journal.Append() by validating RepoOwner and
RepoName before using them in file paths. Malicious values like
"../../etc/cron.d" could previously write outside the journal baseDir.

Defence layers:
- Reject inputs containing path separators (/ or \)
- Reject ".." and "." traversal components
- Validate against safe character regex ^[a-zA-Z0-9][a-zA-Z0-9._-]*$
- Verify resolved absolute path stays within baseDir

Closes #46
CVSS 6.3 — OWASP A01:2021-Broken Access Control

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Claude 2026-02-12 20:30:47 +00:00
parent 45d8b5b7d4
commit b0ef3fb215
No known key found for this signature in database
GPG key ID: AF404715446AEB41
2 changed files with 176 additions and 1 deletions

View file

@ -5,9 +5,14 @@ import (
"fmt" "fmt"
"os" "os"
"path/filepath" "path/filepath"
"regexp"
"strings"
"sync" "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. // JournalEntry is a single line in the JSONL audit log.
type JournalEntry struct { type JournalEntry struct {
Timestamp string `json:"ts"` Timestamp string `json:"ts"`
@ -52,6 +57,36 @@ func NewJournal(baseDir string) (*Journal, error) {
return &Journal{baseDir: baseDir}, nil 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. // Append writes a journal entry for the given signal and result.
func (j *Journal) Append(signal *PipelineSignal, result *ActionResult) error { func (j *Journal) Append(signal *PipelineSignal, result *ActionResult) error {
if signal == nil { if signal == nil {
@ -90,8 +125,31 @@ func (j *Journal) Append(signal *PipelineSignal, result *ActionResult) error {
} }
data = append(data, '\n') 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") 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() j.mu.Lock()
defer j.mu.Unlock() defer j.mu.Unlock()

View file

@ -113,6 +113,123 @@ func TestJournal_Append_Good(t *testing.T) {
assert.Equal(t, 2, lines, "expected two JSONL lines after two appends") 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) { func TestJournal_Append_Bad_NilSignal(t *testing.T) {
dir := t.TempDir() dir := t.TempDir()