fix: security hardening from codebase audit (password scoping, lock file, terminal escapes)

cmd/shuttle/main.go

@@ -375,7 +375,7 @@ func executeRun(ctx context.Context, cli cliFlags) error {
 		logger.Info(fmt.Sprintf("pruned %d old log file(s)", pruneDeleted))
 	}
 
-	promptForPassword(logger)
+	rclonePass := resolveRclonePassword(logger)
 
 	// In quiet mode, the per-job spinner and status lines are suppressed so
 	// the terminal stays silent unless something fails.
@@ -387,7 +387,15 @@ func executeRun(ctx context.Context, cli cliFlags) error {
 	}
 
 	pw := engine.NewProgressWriter(progressOut, progressInteractive, useColor)
-	runner := engine.NewRunner(cfg, configPath, logger, pw, opts.DryRun, logPath)
+	runner := engine.NewRunner(engine.RunnerConfig{
+		Cfg:            cfg,
+		ConfigPath:     configPath,
+		Logger:         logger,
+		Progress:       pw,
+		DryRun:         opts.DryRun,
+		LogFile:        logPath,
+		RclonePassword: rclonePass,
+	})
 	summary, err := runner.Run(ctx, opts)
 	if err != nil {
 		return err
@@ -430,24 +438,27 @@ func validateRemoteNames(selected, configured []string) error {
 	return nil
 }
 
-// promptForPassword checks for RCLONE_CONFIG_PASS and prompts interactively
-// when it is absent and stdin is a TTY. The password is set in the environment
-// for rclone to pick up automatically.
-func promptForPassword(logger *log.Logger) {
+// resolveRclonePassword returns the rclone config password to inject into
+// rclone child processes, or "" when none should be injected. It returns ""
+// when RCLONE_CONFIG_PASS is already in the environment (rclone inherits it),
+// when stdin is not a TTY, or when the prompt is empty/aborted. Unlike the
+// previous implementation it does not mutate the process environment, so the
+// prompted secret never reaches sibling children such as rsync.
+func resolveRclonePassword(logger *log.Logger) string {
 	if os.Getenv("RCLONE_CONFIG_PASS") != "" {
-		return
+		return ""
 	}
 	if !term.IsTerminal(int(os.Stdin.Fd())) {
 		logger.Warn("RCLONE_CONFIG_PASS not set and stdin is not a terminal.")
-		return
+		return ""
 	}
 	fmt.Print("Enter rclone config password (or press Enter if none): ")
 	pass, err := term.ReadPassword(int(os.Stdin.Fd()))
 	fmt.Println()
 	if err != nil || len(pass) == 0 {
-		return
+		return ""
 	}
-	os.Setenv("RCLONE_CONFIG_PASS", string(pass)) //nolint:errcheck // setting env cannot fail in practice
+	return string(pass)
 }
 
 // logDirectory returns the path for log files, respecting XDG_STATE_HOME.

cmd/shuttle/main_test.go

@@ -861,3 +861,19 @@ optional = true
 		t.Errorf("stdout should not mention 'failed' when only optional-missing:\n%s", res.stdout)
 	}
 }
+
+func TestResolveRclonePassword_EnvPreset_ReturnsEmpty(t *testing.T) {
+	t.Setenv("RCLONE_CONFIG_PASS", "already-set")
+	logger, err := log.NewWithWriter(&bytes.Buffer{}, filepath.Join(t.TempDir(), "t.log"), false, log.VerbosityNormal)
+	if err != nil {
+		t.Fatalf("logger: %v", err)
+	}
+	defer logger.Close()
+
+	if got := resolveRclonePassword(logger); got != "" {
+		t.Errorf("resolveRclonePassword() = %q, want \"\" when env is preset", got)
+	}
+	if v := os.Getenv("RCLONE_CONFIG_PASS"); v != "already-set" {
+		t.Errorf("RCLONE_CONFIG_PASS = %q, want unchanged 'already-set'", v)
+	}
+}

internal/engine/progress.go

@@ -4,14 +4,29 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"strings"
 	"sync"
 	"time"
+	"unicode"
 )
 
 const spinnerInterval = 80 * time.Millisecond
 
 var spinnerFrames = []string{"⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"}
 
+// sanitizeProgress removes control characters (C0, C1, DEL) from progress text
+// so attacker-influenceable bytes from tool output (e.g. a filename with
+// embedded ANSI/OSC escapes) cannot drive the terminal. Shuttle's own color
+// codes are added after sanitization in renderSpinner, so they are unaffected.
+func sanitizeProgress(s string) string {
+	return strings.Map(func(r rune) rune {
+		if unicode.IsControl(r) {
+			return -1
+		}
+		return r
+	}, s)
+}
+
 // ProgressWriter manages the live terminal display during job execution.
 // In interactive mode, it shows a spinner on the active job line and replaces
 // it with a compact status line when the job finishes. In non-interactive mode
@@ -81,13 +96,15 @@ func (pw *ProgressWriter) StartJob(ctx context.Context, label string) {
 
 // UpdateProgress sets the progress text displayed beside the spinner.
 // Replaces the elapsed-time default when non-empty.
+// Control characters (C0, C1, DEL) are stripped before storage so that
+// attacker-influenceable filenames in tool output cannot inject terminal escapes.
 // In non-interactive mode, this is a no-op.
 func (pw *ProgressWriter) UpdateProgress(text string) {
 	if !pw.interactive {
 		return
 	}
 	pw.mu.Lock()
-	pw.currentProgress = text
+	pw.currentProgress = sanitizeProgress(text)
 	pw.mu.Unlock()
 }
 

internal/engine/progress_test.go

@@ -175,3 +175,36 @@ func TestProgressWriter_Interactive_MultipleJobs(t *testing.T) {
 		t.Error("missing failure status")
 	}
 }
+
+func TestSanitizeProgress(t *testing.T) {
+	tests := []struct {
+		name string
+		in   string
+		want string
+	}{
+		{"plain stats unchanged", "604 KiB / 1 MiB, 59%, 206 KiB/s, ETA 2s", "604 KiB / 1 MiB, 59%, 206 KiB/s, ETA 2s"},
+		{"bare ESC stripped", "evil\x1bxstats", "evilxstats"},
+		{"CSI clear-screen stripped", "a\x1b[2Jb", "a[2Jb"},
+		{"OSC title stripped", "x\x1b]0;pwned\x07y", "x]0;pwnedy"},
+		{"DEL stripped", "a\x7fb", "ab"},
+		{"newline and tab stripped", "a\nb\tc", "abc"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := sanitizeProgress(tt.in); got != tt.want {
+				t.Errorf("sanitizeProgress(%q) = %q, want %q", tt.in, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestUpdateProgress_StoresSanitizedText(t *testing.T) {
+	pw := NewProgressWriter(&bytes.Buffer{}, true, false) // interactive so UpdateProgress stores
+	pw.UpdateProgress("danger\x1b]0;x\x07 100%")
+	pw.mu.Lock()
+	got := pw.currentProgress
+	pw.mu.Unlock()
+	if strings.ContainsAny(got, "\x1b\x07") {
+		t.Errorf("currentProgress retains control bytes: %q", got)
+	}
+}

internal/engine/rclone.go

@@ -20,14 +20,16 @@ import (
 // argument lists from the runner (built by BuildRcloneArgs) and handles command
 // execution and stats parsing from the shared log file.
 type RcloneExecutor struct {
-	logger  *log.Logger
-	logFile string
+	logger     *log.Logger
+	logFile    string
+	configPass string // rclone config password; injected per-command, empty means none
 }
 
-// NewRcloneExecutor returns a configured RcloneExecutor.
-// logFile is the path to the shared log file used for stats parsing.
-func NewRcloneExecutor(logger *log.Logger, logFile string) *RcloneExecutor {
-	return &RcloneExecutor{logger: logger, logFile: logFile}
+// NewRcloneExecutor returns a configured RcloneExecutor. logFile is the shared
+// log file used for stats parsing; configPass, when non-empty, is injected as
+// RCLONE_CONFIG_PASS into each rclone command's environment (and nowhere else).
+func NewRcloneExecutor(logger *log.Logger, logFile, configPass string) *RcloneExecutor {
+	return &RcloneExecutor{logger: logger, logFile: logFile, configPass: configPass}
 }
 
 // rcloneProgressTracker extracts transfer progress from rclone -P stdout.
@@ -107,6 +109,19 @@ func splitOnCROrLF(data []byte, atEOF bool) (advance int, token []byte, err erro
 	return 0, nil, nil
 }
 
+// rcloneCommand builds an rclone *exec.Cmd, injecting the config password into
+// this command's environment only when one was supplied. When configPass is
+// empty the command inherits the ambient environment unchanged (covering both
+// the no-password case and a user-exported RCLONE_CONFIG_PASS). Callers choose
+// their own execution method on the returned command.
+func (e *RcloneExecutor) rcloneCommand(ctx context.Context, args ...string) *exec.Cmd {
+	cmd := exec.CommandContext(ctx, "rclone", args...)
+	if e.configPass != "" {
+		cmd.Env = append(os.Environ(), "RCLONE_CONFIG_PASS="+e.configPass)
+	}
+	return cmd
+}
+
 // Exec runs rclone with the given pre-assembled argument list.
 // Stdout is piped to a goroutine that parses -P progress output. Stats are
 // parsed from the log file section written during this call.
@@ -125,7 +140,7 @@ func (e *RcloneExecutor) Exec(ctx context.Context, args []string, onProgress fun
 
 	start := time.Now()
 
-	cmd := exec.CommandContext(ctx, "rclone", args...)
+	cmd := e.rcloneCommand(ctx, args...)
 	var stderrBuf bytes.Buffer
 	cmd.Stderr = &stderrBuf
 
@@ -235,7 +250,7 @@ func (e *RcloneExecutor) CleanupArchives(ctx context.Context, remoteName, backup
 	if e.logFile != "" {
 		lsdArgs = append(lsdArgs, "--log-file", e.logFile, "--log-level", "INFO")
 	}
-	output, err := exec.CommandContext(ctx, "rclone", lsdArgs...).Output()
+	output, err := e.rcloneCommand(ctx, lsdArgs...).Output()
 	if err != nil {
 		e.logger.Info(fmt.Sprintf("no archive directory on %s (nothing to clean)", remoteName))
 		return nil
@@ -266,7 +281,7 @@ func (e *RcloneExecutor) CleanupArchives(ctx context.Context, remoteName, backup
 			if e.logFile != "" {
 				purgeArgs = append(purgeArgs, "--log-file", e.logFile, "--log-level", "INFO")
 			}
-			if purgeErr := exec.CommandContext(ctx, "rclone", purgeArgs...).Run(); purgeErr != nil {
+			if purgeErr := e.rcloneCommand(ctx, purgeArgs...).Run(); purgeErr != nil {
 				e.logger.Warn(fmt.Sprintf("failed to purge %s: %v", target, purgeErr))
 			} else {
 				purged++

internal/engine/rclone_test.go

@@ -267,7 +267,7 @@ func newRcloneTestExecutor(t *testing.T) (*RcloneExecutor, string) {
 	t.Helper()
 	logPath := filepath.Join(t.TempDir(), "rclone-test.log")
 	logger := newTestLogger(t)
-	return NewRcloneExecutor(logger, logPath), logPath
+	return NewRcloneExecutor(logger, logPath, ""), logPath
 }
 
 func TestRcloneExec_CopyFile_Succeeds(t *testing.T) {
@@ -509,3 +509,29 @@ func TestRcloneExec_ExpiredContext_ReturnsTimedOut(t *testing.T) {
 		t.Errorf("Status = %q, want %q", result.Status, StatusTimedOut)
 	}
 }
+
+func TestRcloneCommand_InjectsPasswordOnlyWhenSet(t *testing.T) {
+	logger := newTestLogger(t)
+
+	withPass := NewRcloneExecutor(logger, "", "s3cr3t")
+	cmd := withPass.rcloneCommand(context.Background(), "version")
+	found := false
+	for _, e := range cmd.Env {
+		if e == "RCLONE_CONFIG_PASS=s3cr3t" {
+			found = true
+		}
+	}
+	if !found {
+		t.Error("expected RCLONE_CONFIG_PASS in cmd.Env when configPass set")
+	}
+	for _, a := range cmd.Args {
+		if strings.Contains(a, "s3cr3t") {
+			t.Errorf("password must never appear in argv, found in %q", a)
+		}
+	}
+
+	noPass := NewRcloneExecutor(logger, "", "")
+	if cmd2 := noPass.rcloneCommand(context.Background(), "version"); cmd2.Env != nil {
+		t.Errorf("cmd.Env = %v, want nil (inherit) when configPass empty", cmd2.Env)
+	}
+}