From ae302cb346ea679f31a345163cac89568c924bee Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 18 Mar 2026 18:09:08 -0400 Subject: [PATCH] Simplify recent CLI additions: dedup, fix TOCTOU, remove wrappers - Extract git_repo_root() helper in init.rs (was duplicated between run_init and run_deinit) - Fix TOCTOU in run_deinit: remove .exists() check, handle NotFound from remove_file directly - Change dotenv::remove_env_key() to return Option so callers don't need to separately parse the file to check key existence - Remove merge_env wrapper in install.rs, call shared function directly - Remove duplicate merge_env tests from install.rs (already in dotenv.rs) Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/commands/secret.rs | 16 ++++---- lib/crates/fabro-cli/src/init.rs | 39 +++++++------------ lib/crates/fabro-cli/src/install.rs | 43 +-------------------- lib/crates/fabro-config/src/dotenv.rs | 24 +++++++----- 4 files changed, 38 insertions(+), 84 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/secret.rs b/lib/crates/fabro-cli/src/commands/secret.rs index f74920ae1..b35419b34 100644 --- a/lib/crates/fabro-cli/src/commands/secret.rs +++ b/lib/crates/fabro-cli/src/commands/secret.rs @@ -68,15 +68,15 @@ pub fn rm_command(args: &SecretRmArgs) -> Result<()> { } Err(e) => bail!("failed to read {}: {e}", path.display()), }; - // Check the key actually exists before removing - let pairs = dotenv::parse_env(&contents); - if !pairs.iter().any(|(k, _)| k == &args.key) { - bail!("secret not found: {}", args.key); - } let updated = dotenv::remove_env_key(&contents, &args.key); - dotenv::write_env_file(&path, &updated)?; - eprintln!("Removed {}", args.key); - Ok(()) + match updated { + Some(new_contents) => { + dotenv::write_env_file(&path, &new_contents)?; + eprintln!("Removed {}", args.key); + Ok(()) + } + None => bail!("secret not found: {}", args.key), + } } pub fn set_command(args: &SecretSetArgs) -> Result<()> { diff --git a/lib/crates/fabro-cli/src/init.rs b/lib/crates/fabro-cli/src/init.rs index 00fd50b3f..8b8fab529 100644 --- a/lib/crates/fabro-cli/src/init.rs +++ b/lib/crates/fabro-cli/src/init.rs @@ -1,21 +1,23 @@ use anyhow::{bail, Context, Result}; use std::path::PathBuf; -pub async fn run_init() -> Result<()> { +fn git_repo_root() -> Result { let output = std::process::Command::new("git") .args(["rev-parse", "--show-toplevel"]) .output() .context("failed to run git")?; - if !output.status.success() { bail!("not a git repository — run `git init` first"); } - - let repo_root = PathBuf::from( + Ok(PathBuf::from( String::from_utf8(output.stdout) .context("git output was not valid UTF-8")? .trim(), - ); + )) +} + +pub async fn run_init() -> Result<()> { + let repo_root = git_repo_root()?; let fabro_toml = repo_root.join("fabro.toml"); if fabro_toml.exists() { @@ -111,31 +113,20 @@ draft = true } pub fn run_deinit() -> Result<()> { - let output = std::process::Command::new("git") - .args(["rev-parse", "--show-toplevel"]) - .output() - .context("failed to run git")?; - - if !output.status.success() { - bail!("not a git repository"); - } - - let repo_root = PathBuf::from( - String::from_utf8(output.stdout) - .context("git output was not valid UTF-8")? - .trim(), - ); + let repo_root = git_repo_root()?; let fabro_toml = repo_root.join("fabro.toml"); - if !fabro_toml.exists() { - bail!("not initialized — fabro.toml not found"); - } let green = console::Style::new().green(); let dim = console::Style::new().dim(); - std::fs::remove_file(&fabro_toml) - .with_context(|| format!("failed to remove {}", fabro_toml.display()))?; + match std::fs::remove_file(&fabro_toml) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + bail!("not initialized — fabro.toml not found"); + } + Err(e) => bail!("failed to remove {}: {e}", fabro_toml.display()), + } eprintln!( " {} {}", green.apply_to("✔"), diff --git a/lib/crates/fabro-cli/src/install.rs b/lib/crates/fabro-cli/src/install.rs index 294206016..56204a387 100644 --- a/lib/crates/fabro-cli/src/install.rs +++ b/lib/crates/fabro-cli/src/install.rs @@ -199,14 +199,6 @@ ca = "~/.fabro/certs/ca.crt" ) } -// --------------------------------------------------------------------------- -// .env merge (delegates to fabro_config::dotenv) -// --------------------------------------------------------------------------- - -fn merge_env(existing: &str, new_vars: &[(&str, &str)]) -> String { - fabro_config::dotenv::merge_env(existing, new_vars) -} - // --------------------------------------------------------------------------- // Provider key URLs // --------------------------------------------------------------------------- @@ -549,7 +541,7 @@ fn write_env_file(arc_dir: &Path, env_pairs: &[(String, String)], s: &Styles) -> .iter() .map(|(k, v)| (k.as_str(), v.as_str())) .collect(); - let merged = merge_env(&existing, &refs); + let merged = fabro_config::dotenv::merge_env(&existing, &refs); fabro_config::dotenv::write_env_file(&env_path, &merged)?; eprintln!( " {}", @@ -1160,39 +1152,6 @@ mod tests { assert_eq!(tls.ca, PathBuf::from("~/.fabro/certs/ca.crt")); } - // -- .env merge -- - - #[test] - fn merge_env_replaces_existing() { - let result = merge_env("FOO=old\nBAR=keep\n", &[("FOO", "new"), ("BAZ", "added")]); - assert!(result.contains("FOO=new")); - assert!(result.contains("BAR=keep")); - assert!(result.contains("BAZ=added")); - } - - #[test] - fn merge_env_empty_existing() { - let result = merge_env("", &[("FOO", "bar"), ("BAZ", "qux")]); - assert!(result.contains("FOO=bar")); - assert!(result.contains("BAZ=qux")); - } - - #[test] - fn merge_env_preserves_comments_and_blanks() { - let existing = "# A comment\n\nFOO=old\n# Another\nBAR=keep\n"; - let result = merge_env(existing, &[("FOO", "new")]); - assert!(result.contains("# A comment")); - assert!(result.contains("# Another")); - assert!(result.contains("FOO=new")); - assert!(result.contains("BAR=keep")); - } - - #[test] - fn merge_env_full_scenario() { - let result = merge_env("FOO=old\nBAR=keep", &[("FOO", "new"), ("BAZ", "added")]); - assert_eq!(result, "FOO=new\nBAR=keep\nBAZ=added\n"); - } - // -- Provider key URLs -- #[test] diff --git a/lib/crates/fabro-config/src/dotenv.rs b/lib/crates/fabro-config/src/dotenv.rs index da97ae6f9..e51e73b2c 100644 --- a/lib/crates/fabro-config/src/dotenv.rs +++ b/lib/crates/fabro-config/src/dotenv.rs @@ -65,23 +65,29 @@ pub fn merge_env(existing: &str, new_vars: &[(&str, &str)]) -> String { } /// Remove a key from an env file's raw contents. Comments and blank lines are -/// preserved. Returns the new file contents. -pub fn remove_env_key(contents: &str, key_to_remove: &str) -> String { +/// preserved. Returns `Some(new_contents)` if the key was found and removed, +/// or `None` if the key was not present. +pub fn remove_env_key(contents: &str, key_to_remove: &str) -> Option { let mut result_lines: Vec = Vec::new(); + let mut found = false; for line in contents.lines() { if let Some(eq_pos) = line.find('=') { let key = line[..eq_pos].trim(); if !key.is_empty() && !key.starts_with('#') && key == key_to_remove { + found = true; continue; } } result_lines.push(line.to_string()); } + if !found { + return None; + } let mut result = result_lines.join("\n"); if !result.ends_with('\n') { result.push('\n'); } - result + Some(result) } /// Write content to the env file, creating parent directories if needed. @@ -180,29 +186,27 @@ mod tests { #[test] fn remove_env_key_removes_target() { - let result = remove_env_key("FOO=bar\nBAZ=qux\n", "FOO"); + let result = remove_env_key("FOO=bar\nBAZ=qux\n", "FOO").unwrap(); assert!(!result.contains("FOO")); assert!(result.contains("BAZ=qux")); } #[test] fn remove_env_key_preserves_others() { - let result = remove_env_key("A=1\nB=2\nC=3\n", "B"); + let result = remove_env_key("A=1\nB=2\nC=3\n", "B").unwrap(); assert!(result.contains("A=1")); assert!(!result.contains("B=2")); assert!(result.contains("C=3")); } #[test] - fn remove_env_key_handles_missing_key() { - let original = "FOO=bar\n"; - let result = remove_env_key(original, "MISSING"); - assert_eq!(result, original); + fn remove_env_key_returns_none_for_missing_key() { + assert!(remove_env_key("FOO=bar\n", "MISSING").is_none()); } #[test] fn remove_env_key_preserves_comments() { - let result = remove_env_key("# keep me\nFOO=bar\nBAZ=qux\n", "FOO"); + let result = remove_env_key("# keep me\nFOO=bar\nBAZ=qux\n", "FOO").unwrap(); assert!(result.contains("# keep me")); assert!(result.contains("BAZ=qux")); assert!(!result.contains("FOO"));