Merge pull request #412 from joelklabo/fix/secure-jsonl-paths

Security fix for JSONL path handling - adds path traversal protection

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)))
+}

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)
+				}
+			}
+		})
+	}
+}

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

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

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
@@ -628,8 +628,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 {
@@ -661,6 +667,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.
@@ -674,12 +698,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) != "" {

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{