Merge pull request #485 from bborn/task/1944-non-git-followup

Harden non-git project support: security, DRY, tests

Author: bborn

internal/config/config_test.go

@@ -0,0 +1,55 @@
+package config
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/bborn/workflow/internal/db"
+)
+
+func TestProjectUsesWorktrees(t *testing.T) {
+	tmpDir := t.TempDir()
+	dbPath := filepath.Join(tmpDir, "test.db")
+
+	database, err := db.Open(dbPath)
+	if err != nil {
+		t.Fatalf("failed to open database: %v", err)
+	}
+	defer database.Close()
+
+	cfg := New(database)
+
+	t.Run("empty project name defaults to true", func(t *testing.T) {
+		if !cfg.ProjectUsesWorktrees("") {
+			t.Error("empty project name should default to true")
+		}
+	})
+
+	t.Run("unknown project defaults to true", func(t *testing.T) {
+		if !cfg.ProjectUsesWorktrees("nonexistent-project") {
+			t.Error("unknown project should default to true")
+		}
+	})
+
+	t.Run("worktree-enabled project returns true", func(t *testing.T) {
+		if err := database.CreateProject(&db.Project{
+			Name: "git-proj", Path: filepath.Join(tmpDir, "git"), UseWorktrees: true,
+		}); err != nil {
+			t.Fatalf("failed to create project: %v", err)
+		}
+		if !cfg.ProjectUsesWorktrees("git-proj") {
+			t.Error("worktree-enabled project should return true")
+		}
+	})
+
+	t.Run("worktree-disabled project returns false", func(t *testing.T) {
+		if err := database.CreateProject(&db.Project{
+			Name: "no-git-proj", Path: filepath.Join(tmpDir, "nogit"), UseWorktrees: false,
+		}); err != nil {
+			t.Fatalf("failed to create project: %v", err)
+		}
+		if cfg.ProjectUsesWorktrees("no-git-proj") {
+			t.Error("worktree-disabled project should return false")
+		}
+	})
+}

internal/db/sqlite_test.go

@@ -272,4 +272,20 @@ func TestProjectUseWorktrees(t *testing.T) {
 	if !p.UsesWorktrees() {
 		t.Error("no-git-project should use worktrees after update")
 	}
+
+	// Test alias-based lookup preserves UseWorktrees
+	aliasProject := &Project{Name: "alias-proj", Path: filepath.Join(tmpDir, "alias"), Aliases: "ap,aliased", UseWorktrees: false}
+	if err := db.CreateProject(aliasProject); err != nil {
+		t.Fatalf("failed to create alias project: %v", err)
+	}
+	p, err = db.GetProjectByName("ap")
+	if err != nil {
+		t.Fatalf("failed to get project by alias: %v", err)
+	}
+	if p == nil {
+		t.Fatal("expected project from alias lookup, got nil")
+	}
+	if p.UsesWorktrees() {
+		t.Error("alias-proj looked up by alias should NOT use worktrees")
+	}
 }

internal/db/tasks.go

@@ -1096,17 +1096,21 @@ func (p *Project) GetAction(trigger string) *ProjectAction {
 	return nil
 }
 
+// boolToInt converts a bool to an int for SQLite storage (1=true, 0=false).
+func boolToInt(b bool) int {
+	if b {
+		return 1
+	}
+	return 0
+}
+
 // CreateProject creates a new project.
 func (db *DB) CreateProject(p *Project) error {
 	actionsJSON, _ := json.Marshal(p.Actions)
-	useWorktrees := 1
-	if !p.UseWorktrees {
-		useWorktrees = 0
-	}
 	result, err := db.Exec(`
 		INSERT INTO projects (name, path, aliases, instructions, actions, color, claude_config_dir, use_worktrees)
 		VALUES (?, ?, ?, ?, ?, ?, ?, ?)
-	`, p.Name, p.Path, p.Aliases, p.Instructions, string(actionsJSON), p.Color, p.ClaudeConfigDir, useWorktrees)
+	`, p.Name, p.Path, p.Aliases, p.Instructions, string(actionsJSON), p.Color, p.ClaudeConfigDir, boolToInt(p.UseWorktrees))
 	if err != nil {
 		return fmt.Errorf("insert project: %w", err)
 	}
@@ -1118,14 +1122,10 @@ func (db *DB) CreateProject(p *Project) error {
 // UpdateProject updates a project.
 func (db *DB) UpdateProject(p *Project) error {
 	actionsJSON, _ := json.Marshal(p.Actions)
-	useWorktrees := 1
-	if !p.UseWorktrees {
-		useWorktrees = 0
-	}
 	_, err := db.Exec(`
 		UPDATE projects SET name = ?, path = ?, aliases = ?, instructions = ?, actions = ?, color = ?, claude_config_dir = ?, use_worktrees = ?
 		WHERE id = ?
-	`, p.Name, p.Path, p.Aliases, p.Instructions, string(actionsJSON), p.Color, p.ClaudeConfigDir, useWorktrees, p.ID)
+	`, p.Name, p.Path, p.Aliases, p.Instructions, string(actionsJSON), p.Color, p.ClaudeConfigDir, boolToInt(p.UseWorktrees), p.ID)
 	if err != nil {
 		return fmt.Errorf("update project: %w", err)
 	}

internal/executor/codex_executor.go

@@ -160,7 +160,7 @@ func (c *CodexExecutor) runCodex(ctx context.Context, task *db.Task, workDir, pr
 		task.ID, sessionID, task.Port, task.WorktreePath, envPrefix, dangerousFlag, resumeFlag, promptFile.Name())
 
 	// Create new window in task-daemon session
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, c.executor.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		c.logger.Error("tmux new-window failed", "error", tmuxErr, "session", daemonSession)
 		c.executor.logLine(task.ID, "error", fmt.Sprintf("Failed to create tmux window: %s", tmuxErr.Error()))

internal/executor/executor.go

@@ -1933,12 +1933,10 @@ func ensureTmuxDaemon() (string, error) {
 
 // createTmuxWindow creates a new tmux window in the daemon session with retry logic.
 // If the session doesn't exist, it will re-create it and retry once.
-// SECURITY: workDir must be within a .task-worktrees directory to prevent Claude from
-// accidentally writing to the main project directory.
-func createTmuxWindow(daemonSession, windowName, workDir, script string) (string, error) {
-	// SECURITY: Validate that workDir is within a .task-worktrees directory,
-	// OR is a valid project directory (for non-worktree projects).
-	if !isValidWorkDir(workDir) {
+// SECURITY: workDir must be within a .task-worktrees directory, or match allowedProjectDir
+// for non-worktree projects. Pass empty allowedProjectDir to require worktree paths only.
+func createTmuxWindow(daemonSession, windowName, workDir, script, allowedProjectDir string) (string, error) {
+	if !isValidWorkDir(workDir, allowedProjectDir) {
 		return "", fmt.Errorf("security: refusing to create tmux window with invalid workDir: %s", workDir)
 	}
 
@@ -2224,7 +2222,7 @@ func (e *Executor) runClaude(ctx context.Context, task *db.Task, workDir, prompt
 	}
 
 	// Create new window in task-daemon session (with retry logic for race conditions)
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, e.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		e.logger.Error("tmux new-window failed", "error", tmuxErr, "session", daemonSession)
 		e.logLine(task.ID, "error", fmt.Sprintf("Failed to create tmux window: %s", tmuxErr.Error()))
@@ -2380,7 +2378,7 @@ func (e *Executor) runClaudeResume(ctx context.Context, task *db.Task, workDir,
 		task.ID, taskSessionID, task.Port, task.WorktreePath, envPrefix, dangerousFlag, systemPromptFlag, claudeSessionID, feedbackFile.Name())
 
 	// Create new window in task-daemon session (with retry logic for race conditions)
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, e.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		e.logger.Error("tmux new-window failed", "error", tmuxErr, "session", daemonSession)
 		e.logLine(task.ID, "error", fmt.Sprintf("Failed to create tmux window: %s", tmuxErr.Error()))
@@ -2537,7 +2535,7 @@ func (e *Executor) resumeClaudeDangerous(task *db.Task, workDir string) bool {
 		taskID, taskSessionID, task.Port, task.WorktreePath, envPrefix, claudeSessionID)
 
 	// Create new window in task-daemon session (with retry logic for race conditions)
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, e.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		e.logger.Warn("tmux failed to create window", "error", tmuxErr, "session", daemonSession)
 		if cleanupHooks != nil {
@@ -2702,7 +2700,7 @@ func (e *Executor) resumeClaudeSafe(task *db.Task, workDir string) bool {
 		taskID, taskSessionID, task.Port, task.WorktreePath, envPrefix, claudeSessionID)
 
 	// Create new window in task-daemon session (with retry logic for race conditions)
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, e.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		e.logger.Warn("tmux failed to create window", "error", tmuxErr, "session", daemonSession)
 		if cleanupHooks != nil {
@@ -2819,7 +2817,7 @@ func (e *Executor) resumeCodexWithMode(task *db.Task, workDir string, dangerousM
 	script := fmt.Sprintf(`WORKTREE_TASK_ID=%d WORKTREE_SESSION_ID=%s WORKTREE_PORT=%d WORKTREE_PATH=%q %scodex %s--resume %s`,
 		taskID, taskSessionID, task.Port, task.WorktreePath, envPrefix, dangerousFlag, sessionID)
 
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, e.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		e.logger.Warn("tmux failed to create window", "error", tmuxErr, "session", daemonSession)
 		return false
@@ -2927,7 +2925,7 @@ func (e *Executor) resumeGeminiWithMode(task *db.Task, workDir string, dangerous
 	script := fmt.Sprintf(`WORKTREE_TASK_ID=%d WORKTREE_SESSION_ID=%s WORKTREE_PORT=%d WORKTREE_PATH=%q %sgemini %s--resume %s`,
 		taskID, taskSessionID, task.Port, task.WorktreePath, envPrefix, dangerousFlag, sessionID)
 
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, e.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		e.logger.Warn("tmux failed to create window", "error", tmuxErr, "session", daemonSession)
 		return false
@@ -4882,7 +4880,7 @@ func (e *Executor) runPi(ctx context.Context, task *db.Task, workDir, prompt str
 	}
 
 	// Create new window in task-daemon session (with retry logic for race conditions)
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, e.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		e.logger.Error("tmux new-window failed", "error", tmuxErr, "session", daemonSession)
 		e.logLine(task.ID, "error", fmt.Sprintf("Failed to create tmux window: %s", tmuxErr.Error()))
@@ -5004,7 +5002,7 @@ func (e *Executor) runPiResume(ctx context.Context, task *db.Task, workDir, prom
 		task.ID, taskSessionID, task.Port, task.WorktreePath, sessionPath, systemPromptFlag, feedbackFile.Name())
 
 	// Create new window in task-daemon session (with retry logic for race conditions)
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, e.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		e.logger.Error("tmux new-window failed", "error", tmuxErr, "session", daemonSession)
 		e.logLine(task.ID, "error", fmt.Sprintf("Failed to create tmux window: %s", tmuxErr.Error()))
@@ -5147,45 +5145,53 @@ func (e *Executor) KillPiProcess(taskID int64) bool {
 // isValidWorktreePath validates that a working directory is within a .task-worktrees directory.
 // This prevents Claude from accidentally writing to the main project directory.
 // Returns true if the path is valid for task execution.
-// isValidWorktreePath validates that a working directory is within a .task-worktrees directory.
 func isValidWorktreePath(workDir string) bool {
-	// Empty path is never valid
 	if workDir == "" {
 		return false
 	}
 
-	// Resolve symlinks and clean the path
 	absPath, err := filepath.Abs(workDir)
 	if err != nil {
 		return false
 	}
 
-	// Evaluate any symlinks in the path
 	resolvedPath, err := filepath.EvalSymlinks(absPath)
 	if err != nil {
-		// Path might not exist yet, use the absolute path
 		resolvedPath = absPath
 	}
 
-	// Check that the path contains .task-worktrees
 	// Valid paths look like: /path/to/project/.task-worktrees/123-task-slug
 	return strings.Contains(resolvedPath, string(filepath.Separator)+".task-worktrees"+string(filepath.Separator))
 }
 
 // isValidWorkDir validates that a working directory is either within a .task-worktrees directory
-// (for git worktree projects) or is an existing directory (for non-worktree projects).
-func isValidWorkDir(workDir string) bool {
-	// First check the traditional worktree path
+// (for git worktree projects) or matches a specific allowed project directory (for non-worktree
+// projects). The allowedProjectDir parameter restricts which non-worktree paths are accepted,
+// preventing arbitrary directory access.
+func isValidWorkDir(workDir string, allowedProjectDir string) bool {
 	if isValidWorktreePath(workDir) {
 		return true
 	}
 
-	// For non-worktree projects, the workDir is the project directory itself.
-	// Validate it exists and is a directory.
-	if workDir == "" {
+	// For non-worktree projects, only accept the exact configured project directory.
+	if workDir == "" || allowedProjectDir == "" {
+		return false
+	}
+
+	absWork, err := filepath.Abs(workDir)
+	if err != nil {
+		return false
+	}
+	absAllowed, err := filepath.Abs(allowedProjectDir)
+	if err != nil {
+		return false
+	}
+
+	// Must match the allowed path AND exist as a directory
+	if absWork != absAllowed {
 		return false
 	}
-	info, err := os.Stat(workDir)
+	info, err := os.Stat(absWork)
 	return err == nil && info.IsDir()
 }
 

internal/executor/executor_test.go

@@ -912,27 +912,42 @@ func TestIsValidWorkDir(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	t.Run("worktree path is valid", func(t *testing.T) {
-		if !isValidWorkDir(taskDir) {
-			t.Errorf("isValidWorkDir(%q) should return true for worktree directory", taskDir)
+	t.Run("worktree path is valid regardless of allowedProjectDir", func(t *testing.T) {
+		if !isValidWorkDir(taskDir, "") {
+			t.Errorf("isValidWorkDir(%q, \"\") should return true for worktree directory", taskDir)
+		}
+		if !isValidWorkDir(taskDir, "/some/other/path") {
+			t.Errorf("isValidWorkDir(%q, other) should return true for worktree directory", taskDir)
+		}
+	})
+
+	t.Run("project directory is valid when it matches allowedProjectDir", func(t *testing.T) {
+		if !isValidWorkDir(tmpDir, tmpDir) {
+			t.Errorf("isValidWorkDir(%q, %q) should return true when paths match", tmpDir, tmpDir)
+		}
+	})
+
+	t.Run("project directory is rejected when allowedProjectDir differs", func(t *testing.T) {
+		if isValidWorkDir(tmpDir, "/some/other/path") {
+			t.Errorf("isValidWorkDir(%q, other) should return false when paths don't match", tmpDir)
 		}
 	})
 
-	t.Run("project directory is valid for non-worktree projects", func(t *testing.T) {
-		// For non-worktree projects, the project directory itself is the work dir
-		if !isValidWorkDir(tmpDir) {
-			t.Errorf("isValidWorkDir(%q) should return true for existing project directory", tmpDir)
+	t.Run("arbitrary directory is rejected without allowedProjectDir", func(t *testing.T) {
+		if isValidWorkDir(tmpDir, "") {
+			t.Errorf("isValidWorkDir(%q, \"\") should return false for non-worktree path with empty allowed", tmpDir)
 		}
 	})
 
 	t.Run("empty path is not valid", func(t *testing.T) {
-		if isValidWorkDir("") {
-			t.Error("isValidWorkDir('') should return false")
+		if isValidWorkDir("", tmpDir) {
+			t.Error("isValidWorkDir('', ...) should return false")
 		}
 	})
 
-	t.Run("non-existent path is not valid", func(t *testing.T) {
-		if isValidWorkDir("/nonexistent/path/xyz") {
+	t.Run("non-existent path is not valid even if allowed", func(t *testing.T) {
+		nonExistent := "/nonexistent/path/xyz"
+		if isValidWorkDir(nonExistent, nonExistent) {
 			t.Error("isValidWorkDir should return false for non-existent path")
 		}
 	})

internal/executor/gemini_executor.go

@@ -125,7 +125,7 @@ func (g *GeminiExecutor) runGemini(ctx context.Context, task *db.Task, workDir,
 	script := fmt.Sprintf(`WORKTREE_TASK_ID=%d WORKTREE_SESSION_ID=%s WORKTREE_PORT=%d WORKTREE_PATH=%q %sgemini %s%s-i "$(cat %q)"`,
 		task.ID, sessionID, task.Port, task.WorktreePath, envPrefix, dangerousFlag, resumeFlag, promptFile.Name())
 
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, g.executor.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		g.logger.Error("tmux new-window failed", "error", tmuxErr, "session", daemonSession)
 		g.executor.logLine(task.ID, "error", fmt.Sprintf("Failed to create tmux window: %s", tmuxErr.Error()))

internal/executor/openclaw_executor.go

@@ -140,7 +140,7 @@ func (o *OpenClawExecutor) runOpenClaw(ctx context.Context, task *db.Task, workD
 	script := fmt.Sprintf(`WORKTREE_TASK_ID=%d WORKTREE_SESSION_ID=%s WORKTREE_PORT=%d WORKTREE_PATH=%q %sopenclaw tui --session %s %s--message "$(cat %q)"`,
 		task.ID, worktreeSessionID, task.Port, task.WorktreePath, envPrefix, sessionKey, thinkingFlag, promptFile.Name())
 
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, o.executor.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		o.logger.Error("tmux new-window failed", "error", tmuxErr, "session", daemonSession)
 		o.executor.logLine(task.ID, "error", fmt.Sprintf("Failed to create tmux window: %s", tmuxErr.Error()))

internal/executor/opencode_executor.go

@@ -133,7 +133,7 @@ func (o *OpenCodeExecutor) runOpenCode(ctx context.Context, task *db.Task, workD
 			workDir, task.ID, worktreeSessionID, task.Port, task.WorktreePath, envPrefix, promptFile.Name(), promptFile.Name())
 	}
 
-	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script)
+	actualSession, tmuxErr := createTmuxWindow(daemonSession, windowName, workDir, script, o.executor.getProjectDir(task.Project))
 	if tmuxErr != nil {
 		o.logger.Error("tmux new-window failed", "error", tmuxErr, "session", daemonSession)
 		o.executor.logLine(task.ID, "error", fmt.Sprintf("Failed to create tmux window: %s", tmuxErr.Error()))