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:
parent
7900b8c4da
commit
5de7ee4fb8
2 changed files with 176 additions and 1 deletions
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue