fix: harden add tracking and config validation (#77)

Co-authored-by: Cursor Agent <cursoragent@cursor.com>

Author: captainsafia

src/commands/add.rs

@@ -3,7 +3,8 @@ use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
 
 use crate::git::{
-    add_worktree, branch_exists, discover_repo, project_root, tracked_branch_name, RepoContext,
+    add_worktree, branch_exists, discover_repo, normalize_tracking_reference_input, project_root,
+    tracked_branch_name, RepoContext,
 };
 use crate::utils::{
     default_worktree_name_seed, generate_default_worktree_name, read_repo_config,
@@ -36,8 +37,8 @@ pub fn run(name: Option<&str>, track: Option<&str>) {
     let repo_config = match read_repo_config(project_root) {
         Ok(config) => config,
         Err(e) => {
-            eprintln!("{} {}", "Warning:".yellow(), e);
-            RepoConfig::default()
+            eprintln!("{} {}", "Error:".red(), e);
+            std::process::exit(1);
         }
     };
     let worktree = match resolve_worktree_spec(name, &repo, project_root, &repo_config) {
@@ -215,12 +216,17 @@ fn apply_branch_prefix(
 
 fn resolve_target_branch(name: &str, track: Option<&str>) -> Result<String, String> {
     match track {
-        Some(track_ref) => tracked_branch_name(track_ref).ok_or_else(|| {
-            format!(
-                "Invalid tracking branch '{}'. Use '<remote>/<branch>' or 'refs/remotes/<remote>/<branch>'.",
-                track_ref
-            )
-        }).map(str::to_string),
+        Some(track_ref) => {
+            let normalized = normalize_tracking_reference_input(track_ref)?;
+            tracked_branch_name(&normalized)
+                .ok_or_else(|| {
+                    format!(
+                        "Invalid tracking branch '{}'. Use '<remote>/<branch>' or 'refs/remotes/<remote>/<branch>'.",
+                        track_ref
+                    )
+                })
+                .map(str::to_string)
+        }
         None => Ok(name.to_string()),
     }
 }
@@ -537,12 +543,24 @@ mod tests {
         assert_eq!(result, "feature/new-ui");
     }
 
+    #[test]
+    fn resolve_target_branch_trims_tracking_ref_trailing_slash() {
+        let result = resolve_target_branch("foo", Some("origin/feature/new-ui/")).unwrap();
+        assert_eq!(result, "feature/new-ui");
+    }
+
     #[test]
     fn resolve_target_branch_rejects_non_remote_ref() {
         let result = resolve_target_branch("foo", Some("refs/heads/feature/new-ui"));
         assert!(result.is_err());
     }
 
+    #[test]
+    fn resolve_target_branch_rejects_malformed_tracking_ref() {
+        let result = resolve_target_branch("foo", Some("origin/feature//new-ui"));
+        assert!(result.is_err());
+    }
+
     #[test]
     fn apply_branch_prefix_adds_separator_when_configured() {
         let prefixed = apply_branch_prefix(Some("safia"), "quiet-meadow").unwrap();

src/commands/go.rs

@@ -16,9 +16,8 @@ pub fn run(name: Option<&str>, path_only: bool) {
         && !atty::is(atty::Stream::Stdin)
     {
         eprintln!(
-            "{} {}",
-            "Error:".red(),
-            "Interactive selection requires a TTY. Provide a branch name when using --path-only."
+            "{} Interactive selection requires a TTY. Provide a branch name when using --path-only.",
+            "Error:".red()
         );
         std::process::exit(1);
     }

src/commands/init.rs

@@ -68,5 +68,5 @@ pub fn run(git_url: &str) {
     println!();
     println!("{}", "Next steps:".bold());
     println!("  {} {}", "cd".dimmed(), bare_repo_dir);
-    println!("  {} {}", "grove add".dimmed(), "<branch-name>");
+    println!("  {} <branch-name>", "grove add".dimmed());
 }

src/commands/prune.rs

@@ -105,13 +105,13 @@ pub fn run(dry_run: bool, force: bool, base: Option<&str>, older_than: Option<&s
         return;
     }
 
-    if older_than.is_some() {
+    if let Some(duration) = older_than {
         println!(
             "{}",
             format!(
                 "Found {} worktree(s) older than {}:",
                 candidates.len(),
-                older_than.unwrap()
+                duration
             )
             .green()
         );

src/git/mod.rs

@@ -2,6 +2,7 @@ pub mod worktree_manager;
 
 pub use worktree_manager::{
     add_worktree, branch_exists, clone_bare_repository, discover_repo, find_worktree_by_name,
-    get_default_branch, is_branch_merged, list_worktrees, project_root, remove_worktree,
-    remove_worktrees, repo_path, sync_branch, tracked_branch_name, RepoContext, DETACHED_HEAD,
+    get_default_branch, is_branch_merged, list_worktrees, normalize_tracking_reference_input,
+    project_root, remove_worktree, remove_worktrees, repo_path, sync_branch, tracked_branch_name,
+    RepoContext, DETACHED_HEAD,
 };

src/git/worktree_manager.rs

@@ -160,7 +160,12 @@ pub fn add_worktree(
     create_branch: bool,
     track: Option<&str>,
 ) -> Result<(), String> {
-    if let Some(track_branch) = track {
+    let normalized_track = match track {
+        Some(track_branch) => Some(normalize_tracking_reference_input(track_branch)?),
+        None => None,
+    };
+
+    if let Some(track_branch) = normalized_track.as_deref() {
         ensure_tracking_reference(context, track_branch)?;
     }
 
@@ -169,11 +174,11 @@ pub fn add_worktree(
         normalized_worktree_path.as_str(),
         branch_name,
         create_branch,
-        track,
+        normalized_track.as_deref(),
     );
 
     git_raw(context, &args).map_err(|e| format!("Failed to add worktree: {}", e))?;
-    if let Some(track_branch) = track {
+    if let Some(track_branch) = normalized_track.as_deref() {
         set_branch_upstream(context, branch_name, track_branch)?;
     }
     Ok(())
@@ -240,6 +245,18 @@ fn reference_exists(context: &RepoContext, reference: &str) -> bool {
     git_raw(context, &["rev-parse", "--verify", reference]).is_ok()
 }
 
+pub fn normalize_tracking_reference_input(reference: &str) -> Result<String, String> {
+    let normalized = trim_trailing_branch_slashes(reference);
+    let (remote, branch) = parse_remote_tracking_reference(normalized)
+        .ok_or_else(|| invalid_tracking_reference(reference))?;
+
+    if !is_valid_ref_component(remote) || !is_valid_ref_path(branch) {
+        return Err(invalid_tracking_reference(reference));
+    }
+
+    Ok(format!("{}/{}", remote, branch))
+}
+
 fn parse_remote_tracking_reference(reference: &str) -> Option<(&str, &str)> {
     let normalized = if let Some(rest) = reference.strip_prefix("refs/remotes/") {
         rest
@@ -261,6 +278,34 @@ pub fn tracked_branch_name(reference: &str) -> Option<&str> {
     parse_remote_tracking_reference(reference).map(|(_, branch)| branch)
 }
 
+fn invalid_tracking_reference(reference: &str) -> String {
+    format!(
+        "Invalid tracking branch '{}'. Use '<remote>/<branch>' or 'refs/remotes/<remote>/<branch>'.",
+        reference
+    )
+}
+
+fn is_valid_ref_path(path: &str) -> bool {
+    !path.is_empty()
+        && !path.contains("@{")
+        && !path.ends_with('.')
+        && path.split('/').all(is_valid_ref_component)
+}
+
+fn is_valid_ref_component(component: &str) -> bool {
+    !component.is_empty()
+        && component != "."
+        && component != ".."
+        && !component.starts_with('.')
+        && !component.ends_with(".lock")
+        && !component.contains("..")
+        && !component.chars().any(contains_invalid_ref_char)
+}
+
+fn contains_invalid_ref_char(ch: char) -> bool {
+    ch.is_ascii_control() || matches!(ch, ' ' | '~' | '^' | ':' | '?' | '*' | '[' | '\\')
+}
+
 fn set_branch_upstream(
     context: &RepoContext,
     branch_name: &str,
@@ -407,22 +452,22 @@ fn parse_worktree_lines(output: &str) -> Vec<PartialWorktree> {
     };
 
     for line in output.trim().lines() {
-        if line.starts_with("worktree ") {
+        if let Some(path) = line.strip_prefix("worktree ") {
             if current.path.is_some() && !current.is_bare {
                 worktrees.push(current);
             }
             current = PartialWorktree {
-                path: Some(line[9..].to_string()),
+                path: Some(path.to_string()),
                 head: None,
                 branch: None,
                 is_locked: false,
                 is_prunable: false,
                 is_bare: false,
             };
-        } else if line.starts_with("HEAD ") {
-            current.head = Some(line[5..].to_string());
-        } else if line.starts_with("branch ") {
-            current.branch = Some(line[7..].replace("refs/heads/", ""));
+        } else if let Some(head) = line.strip_prefix("HEAD ") {
+            current.head = Some(head.to_string());
+        } else if let Some(branch) = line.strip_prefix("branch ") {
+            current.branch = Some(branch.replace("refs/heads/", ""));
         } else if line == "detached" {
             current.branch = Some(DETACHED_HEAD.to_string());
         } else if line == "locked" {
@@ -720,6 +765,32 @@ mod tests {
         assert_eq!(parse_remote_tracking_reference("origin"), None);
     }
 
+    #[test]
+    fn normalize_tracking_reference_input_trims_trailing_slash() {
+        assert_eq!(
+            normalize_tracking_reference_input("origin/feature/test/").unwrap(),
+            "origin/feature/test"
+        );
+    }
+
+    #[test]
+    fn normalize_tracking_reference_input_normalizes_full_ref() {
+        assert_eq!(
+            normalize_tracking_reference_input("refs/remotes/origin/feature/test/").unwrap(),
+            "origin/feature/test"
+        );
+    }
+
+    #[test]
+    fn normalize_tracking_reference_input_rejects_empty_branch() {
+        assert!(normalize_tracking_reference_input("origin/").is_err());
+    }
+
+    #[test]
+    fn normalize_tracking_reference_input_rejects_empty_path_segment() {
+        assert!(normalize_tracking_reference_input("origin/feature//test").is_err());
+    }
+
     #[test]
     fn tracked_branch_name_returns_remote_branch_part() {
         assert_eq!(

src/main.rs

@@ -8,6 +8,7 @@ mod git;
 mod models;
 mod utils;
 
+use crate::git::normalize_tracking_reference_input;
 use crate::utils::{is_valid_git_url, parse_duration, trim_trailing_branch_slashes};
 
 const VERSION: &str = env!("CARGO_PKG_VERSION");
@@ -62,6 +63,10 @@ fn validate_duration(value: &str) -> Result<String, String> {
     parse_duration(value).map(|_| value.to_string())
 }
 
+fn validate_tracking_reference(value: &str) -> Result<String, String> {
+    normalize_tracking_reference_input(value)
+}
+
 #[derive(Parser)]
 #[command(name = "grove", about = "Grove is a Git worktree management tool", version = VERSION)]
 struct Cli {
@@ -77,7 +82,7 @@ enum Commands {
         #[arg(value_parser = validate_branch_name)]
         name: Option<String>,
         /// Set up tracking for the specified remote branch
-        #[arg(short = 't', long = "track")]
+        #[arg(short = 't', long = "track", value_parser = validate_tracking_reference)]
         track: Option<String>,
     },
     /// Navigate to a worktree by branch name
@@ -243,7 +248,7 @@ fn main() {
 
 #[cfg(test)]
 mod tests {
-    use super::{validate_branch_name, Cli, Commands};
+    use super::{validate_branch_name, validate_tracking_reference, Cli, Commands};
     use clap::Parser;
 
     #[test]
@@ -259,6 +264,19 @@ mod tests {
         assert!(validate_branch_name("///").is_err());
     }
 
+    #[test]
+    fn validate_tracking_reference_trims_trailing_slashes() {
+        assert_eq!(
+            validate_tracking_reference("origin/feature/my-branch///").unwrap(),
+            "origin/feature/my-branch"
+        );
+    }
+
+    #[test]
+    fn validate_tracking_reference_rejects_empty_path_segments() {
+        assert!(validate_tracking_reference("origin/feature//my-branch").is_err());
+    }
+
     #[test]
     fn add_command_allows_omitted_name() {
         let cli = Cli::try_parse_from(["grove", "add"]).unwrap();
@@ -289,4 +307,22 @@ mod tests {
             _ => panic!("expected add command"),
         }
     }
+
+    #[test]
+    fn add_command_normalizes_track_input() {
+        let cli = Cli::try_parse_from([
+            "grove",
+            "add",
+            "feature/new-worktree",
+            "--track",
+            "origin/main/",
+        ])
+        .unwrap();
+        match cli.command {
+            Some(Commands::Add { track, .. }) => {
+                assert_eq!(track.as_deref(), Some("origin/main"));
+            }
+            _ => panic!("expected add command"),
+        }
+    }
 }