From cb6ccef7c2e1fe227adc410721af5c4186222a7a Mon Sep 17 00:00:00 2001 From: Joel Klabo Date: Fri, 28 Nov 2025 18:58:04 -0800 Subject: [PATCH] fix: harden JSONL path handling - bound fresh-clone JSONL discovery to the .beads dir (abs path, traversal guard) before reading counts - add safeWorkspacePath/isWithinWorkspace helpers and use in doctor fixes (database_config, untracked) to reject absolute/traversal inputs and confine .gitattributes edits - normalize git status paths and path-guard tests for cross-OS (Windows) compatibility - add regression tests for the new guards --- cmd/bd/doctor/fix/common.go | 41 +++++++++++++++++++++ cmd/bd/doctor/fix/common_test.go | 55 ++++++++++++++++++++++++++++ cmd/bd/doctor/fix/database_config.go | 29 +++++++++++++-- cmd/bd/doctor/fix/untracked.go | 33 +++++++++++++---- cmd/bd/main.go | 46 +++++++++++++++++++---- cmd/bd/main_test.go | 48 +++++++++++++++++++++++- 6 files changed, 233 insertions(+), 19 deletions(-) create mode 100644 cmd/bd/doctor/fix/common_test.go diff --git a/cmd/bd/doctor/fix/common.go b/cmd/bd/doctor/fix/common.go index d3a6247a..edc665ec 100644 --- a/cmd/bd/doctor/fix/common.go +++ b/cmd/bd/doctor/fix/common.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" ) // getBdBinary returns the path to the bd binary to use for fix operations. @@ -47,3 +48,43 @@ func validateBeadsWorkspace(path string) error { return nil } + +// safeWorkspacePath resolves relPath within the workspace root and ensures it +// cannot escape the workspace via path traversal. +func safeWorkspacePath(root, relPath string) (string, error) { + absRoot, err := filepath.Abs(root) + if err != nil { + return "", fmt.Errorf("invalid workspace path: %w", err) + } + + cleanRel := filepath.Clean(relPath) + if filepath.IsAbs(cleanRel) { + return "", fmt.Errorf("expected relative path, got absolute: %s", relPath) + } + + joined := filepath.Join(absRoot, cleanRel) + rel, err := filepath.Rel(absRoot, joined) + if err != nil { + return "", fmt.Errorf("failed to resolve path: %w", err) + } + + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "", fmt.Errorf("path escapes workspace: %s", relPath) + } + + return joined, nil +} + +// isWithinWorkspace reports whether candidate resides within the workspace root. +func isWithinWorkspace(root, candidate string) bool { + cleanRoot, err := filepath.Abs(root) + if err != nil { + return false + } + cleanCandidate := filepath.Clean(candidate) + rel, err := filepath.Rel(cleanRoot, cleanCandidate) + if err != nil { + return false + } + return rel == "." || !(rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator))) +} diff --git a/cmd/bd/doctor/fix/common_test.go b/cmd/bd/doctor/fix/common_test.go new file mode 100644 index 00000000..d9c3d4ac --- /dev/null +++ b/cmd/bd/doctor/fix/common_test.go @@ -0,0 +1,55 @@ +package fix + +import ( + "path/filepath" + "testing" +) + +func TestSafeWorkspacePath(t *testing.T) { + root := t.TempDir() + absEscape, _ := filepath.Abs(filepath.Join(root, "..", "escape")) + + tests := []struct { + name string + relPath string + wantErr bool + }{ + { + name: "normal relative path", + relPath: ".beads/issues.jsonl", + wantErr: false, + }, + { + name: "nested relative path", + relPath: filepath.Join(".beads", "nested", "file.txt"), + wantErr: false, + }, + { + name: "absolute path rejected", + relPath: absEscape, + wantErr: true, + }, + { + name: "path traversal rejected", + relPath: filepath.Join("..", "escape"), + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := safeWorkspacePath(root, tt.relPath) + if (err != nil) != tt.wantErr { + t.Fatalf("safeWorkspacePath() error = %v, wantErr %v", err, tt.wantErr) + } + if err == nil { + if !isWithinWorkspace(root, got) { + t.Fatalf("resolved path %q not within workspace %q", got, root) + } + if !filepath.IsAbs(got) { + t.Fatalf("resolved path is not absolute: %q", got) + } + } + }) + } +} diff --git a/cmd/bd/doctor/fix/database_config.go b/cmd/bd/doctor/fix/database_config.go index 83e1a70d..2e938cc6 100644 --- a/cmd/bd/doctor/fix/database_config.go +++ b/cmd/bd/doctor/fix/database_config.go @@ -18,7 +18,16 @@ func DatabaseConfig(path string) error { return err } - beadsDir := filepath.Join(path, ".beads") + absPath, err := filepath.Abs(path) + if err != nil { + return fmt.Errorf("invalid workspace path: %w", err) + } + path = absPath + + beadsDir, err := safeWorkspacePath(path, ".beads") + if err != nil { + return err + } // Load existing config cfg, err := configfile.Load(beadsDir) @@ -129,7 +138,16 @@ func LegacyJSONLConfig(path string) error { return err } - beadsDir := filepath.Join(path, ".beads") + absPath, err := filepath.Abs(path) + if err != nil { + return fmt.Errorf("invalid workspace path: %w", err) + } + path = absPath + + beadsDir, err := safeWorkspacePath(path, ".beads") + if err != nil { + return err + } // Load existing config cfg, err := configfile.Load(beadsDir) @@ -162,8 +180,11 @@ func LegacyJSONLConfig(path string) error { cfg.JSONLExport = "issues.jsonl" // Update .gitattributes if it references beads.jsonl - gitattrsPath := filepath.Join(path, ".gitattributes") - if content, err := os.ReadFile(gitattrsPath); err == nil { + gitattrsPath, err := safeWorkspacePath(path, ".gitattributes") + if err != nil { + fmt.Printf(" Skipping .gitattributes update: %v\n", err) + // #nosec G304 -- gitattrsPath constrained to workspace root + } else if content, err := os.ReadFile(gitattrsPath); err == nil { if strings.Contains(string(content), ".beads/beads.jsonl") { newContent := strings.ReplaceAll(string(content), ".beads/beads.jsonl", ".beads/issues.jsonl") // #nosec G306 -- .gitattributes should be world-readable diff --git a/cmd/bd/doctor/fix/untracked.go b/cmd/bd/doctor/fix/untracked.go index a4e95a4b..1e0f1446 100644 --- a/cmd/bd/doctor/fix/untracked.go +++ b/cmd/bd/doctor/fix/untracked.go @@ -16,7 +16,16 @@ func UntrackedJSONL(path string) error { return err } - beadsDir := filepath.Join(path, ".beads") + absPath, err := filepath.Abs(path) + if err != nil { + return fmt.Errorf("invalid workspace path: %w", err) + } + path = absPath + + beadsDir, err := safeWorkspacePath(path, ".beads") + if err != nil { + return err + } // Find untracked JSONL files cmd := exec.Command("git", "status", "--porcelain", ".beads/") @@ -49,21 +58,31 @@ func UntrackedJSONL(path string) error { // Stage the untracked files for _, file := range untrackedFiles { - fullPath := filepath.Join(path, file) - // Verify file exists in .beads directory (security check) - if !strings.HasPrefix(fullPath, beadsDir) { + cleanFile := filepath.Clean(file) + if filepath.IsAbs(cleanFile) || cleanFile == ".." || strings.HasPrefix(cleanFile, ".."+string(os.PathSeparator)) { + continue + } + + // Only allow files inside .beads/ + slashFile := filepath.ToSlash(cleanFile) + if !strings.HasPrefix(slashFile, ".beads/") { + continue + } + + fullPath, err := safeWorkspacePath(path, cleanFile) + if err != nil || !isWithinWorkspace(beadsDir, fullPath) { continue } if _, err := os.Stat(fullPath); os.IsNotExist(err) { continue } - addCmd := exec.Command("git", "add", file) + addCmd := exec.Command("git", "add", cleanFile) // #nosec G204 -- cleanFile constrained to .beads/*.jsonl within the validated workspace addCmd.Dir = path if err := addCmd.Run(); err != nil { - return fmt.Errorf("failed to stage %s: %w", file, err) + return fmt.Errorf("failed to stage %s: %w", cleanFile, err) } - fmt.Printf(" Staged %s\n", filepath.Base(file)) + fmt.Printf(" Staged %s\n", filepath.Base(cleanFile)) } // Commit the staged files diff --git a/cmd/bd/main.go b/cmd/bd/main.go index fcbadd14..129356e0 100644 --- a/cmd/bd/main.go +++ b/cmd/bd/main.go @@ -92,11 +92,11 @@ var ( ) var ( - noAutoFlush bool - noAutoImport bool - sandboxMode bool - allowStale bool // Use --allow-stale: skip staleness check (emergency escape hatch) - noDb bool // Use --no-db mode: load from JSONL, write back after each command + noAutoFlush bool + noAutoImport bool + sandboxMode bool + allowStale bool // Use --allow-stale: skip staleness check (emergency escape hatch) + noDb bool // Use --no-db mode: load from JSONL, write back after each command profileEnabled bool profileFile *os.File traceFile *os.File @@ -590,8 +590,14 @@ var rootCmd = &cobra.Command{ if store != nil { _ = store.Close() } - if profileFile != nil { pprof.StopCPUProfile(); _ = profileFile.Close() } - if traceFile != nil { trace.Stop(); _ = traceFile.Close() } + if profileFile != nil { + pprof.StopCPUProfile() + _ = profileFile.Close() + } + if traceFile != nil { + trace.Stop() + _ = traceFile.Close() + } // Cancel the signal context to clean up resources if rootCancel != nil { @@ -623,6 +629,24 @@ func isFreshCloneError(err error) bool { strings.Contains(errStr, "required config key missing: issue_prefix") } +// isPathWithinDir reports whether candidate resides within baseDir (or is the same path). +// Paths are cleaned before comparison to defend against directory traversal. +func isPathWithinDir(baseDir, candidate string) bool { + cleanBase := filepath.Clean(baseDir) + cleanCandidate := filepath.Clean(candidate) + + rel, err := filepath.Rel(cleanBase, cleanCandidate) + if err != nil { + return false + } + + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return false + } + + return true +} + // handleFreshCloneError displays a helpful message when a fresh clone is detected // and returns true if the error was handled (so caller should exit). // If not a fresh clone error, returns false and does nothing. @@ -636,12 +660,20 @@ func handleFreshCloneError(err error, beadsDir string) bool { issueCount := 0 if beadsDir != "" { + if absBeadsDir, err := filepath.Abs(beadsDir); err == nil { + beadsDir = absBeadsDir + } + // Check for issues.jsonl (canonical) first, then beads.jsonl (legacy) for _, name := range []string{"issues.jsonl", "beads.jsonl"} { candidate := filepath.Join(beadsDir, name) + if !isPathWithinDir(beadsDir, candidate) { + continue + } if info, statErr := os.Stat(candidate); statErr == nil && !info.IsDir() { jsonlPath = candidate // Count lines (approximately = issue count) + // #nosec G304 -- candidate limited to known JSONL files inside .beads if data, readErr := os.ReadFile(candidate); readErr == nil { for _, line := range strings.Split(string(data), "\n") { if strings.TrimSpace(line) != "" { diff --git a/cmd/bd/main_test.go b/cmd/bd/main_test.go index ae0703be..c83a7207 100644 --- a/cmd/bd/main_test.go +++ b/cmd/bd/main_test.go @@ -152,6 +152,52 @@ func TestAutoFlushOnExit(t *testing.T) { } } +func TestIsPathWithinDir(t *testing.T) { + root := t.TempDir() + nested := filepath.Join(root, ".beads", "issues.jsonl") + sibling := filepath.Join(filepath.Dir(root), "other", "issues.jsonl") + traversal := filepath.Join(root, "..", "etc", "passwd") + tests := []struct { + name string + base string + candidate string + want bool + }{ + { + name: "same path", + base: root, + candidate: root, + want: true, + }, + { + name: "nested path", + base: root, + candidate: nested, + want: true, + }, + { + name: "sibling path", + base: root, + candidate: sibling, + want: false, + }, + { + name: "traversal outside base", + base: root, + candidate: traversal, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isPathWithinDir(tt.base, tt.candidate); got != tt.want { + t.Fatalf("isPathWithinDir(%q, %q) = %v, want %v", tt.base, tt.candidate, got, tt.want) + } + }) + } +} + // TestAutoFlushConcurrency tests that concurrent operations don't cause races // TestAutoFlushStoreInactive tests that flush doesn't run when store is inactive // TestAutoFlushJSONLContent tests that flushed JSONL has correct content @@ -339,7 +385,7 @@ func TestAutoImportIfNewer(t *testing.T) { } // Wait a moment to ensure different timestamps - time.Sleep(10 * time.Millisecond) // 10x faster + time.Sleep(10 * time.Millisecond) // 10x faster // Create a JSONL file with different content (simulating a git pull) jsonlIssue := &types.Issue{