From 9182376b090ccc440ed225dba5da1500873f8d00 Mon Sep 17 00:00:00 2001
From: "google-labs-jules[bot]"
 <161369871+google-labs-jules[bot]@users.noreply.github.com>
Date: Tue, 11 Aug 2026 18:03:28 +0000
Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20[security]=20Propagate=20file=20?=
 =?UTF-8?q?cleanup=20errors=20during=20git=20fallback?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

🎯 What: Fixes a vulnerability where `std::fs::remove_dir_all` and `std::fs::write` results were ignored during `.or_else()` git fallback in `src/git_ops/mod.rs`.
⚠️ Risk: Ignoring these results could lead to path traversal, symlink escapes, or configuration reuse vulnerabilities if a directory or file wasn't successfully removed but the git CLI fallback command proceeds anyway.
🛡️ Solution: We now handle the `Result` from file system operations using `anyhow::Context` and the `?` operator. This propagates any cleanup errors upwards rather than silently allowing the unsafe CLI fallback state. No additional permission tests were added to automated suites as enforcing permission errors in test harnesses is brittle and depends on OS behavior. Instead, we rely on the static compiler checks enforcing Result handling.
---
 src/git_ops/mod.rs | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/git_ops/mod.rs b/src/git_ops/mod.rs
index 28f09b5..1c33bda 100644
--- a/src/git_ops/mod.rs
+++ b/src/git_ops/mod.rs
@@ -389,7 +389,8 @@ impl GitOperations for GitOpsManager {
             // Clean up potentially partially initialized submodule path before fallback
             let sub_path = workdir.join(&opts.path);
             if sub_path.exists() {
-                let _ = std::fs::remove_dir_all(&sub_path);
+                std::fs::remove_dir_all(&sub_path)
+                    .context("Failed to clean up partially initialized submodule path before CLI fallback")?;
             }
 
             // git2 also adds the submodule to .gitmodules, which will cause CLI to fail
@@ -415,7 +416,8 @@ impl GitOperations for GitOpsManager {
                             new_content.push('\n');
                         }
                     }
-                    let _ = std::fs::write(&gitmodules_path, new_content);
+                    std::fs::write(&gitmodules_path, new_content)
+                        .context("Failed to rewrite .gitmodules during fallback cleanup")?;
                 }
             }
 
@@ -448,7 +450,8 @@ impl GitOperations for GitOpsManager {
             // Also git2 might have created the internal git directory
             let internal_git_dir = workdir.join(".git").join("modules").join(&opts.name);
             if internal_git_dir.exists() {
-                let _ = std::fs::remove_dir_all(&internal_git_dir);
+                std::fs::remove_dir_all(&internal_git_dir)
+                    .context("Failed to clean up partially initialized internal git directory before CLI fallback")?;
             }
 
             // git2's repo.submodule() uses the *path* (not the name) as the key for the
@@ -456,7 +459,8 @@ impl GitOperations for GitOpsManager {
             // ".git/modules/<name>" has already been cleaned up.  Remove both.
             let path_internal_git_dir = workdir.join(".git").join("modules").join(&opts.path);
             if path_internal_git_dir.exists() {
-                let _ = std::fs::remove_dir_all(&path_internal_git_dir);
+                std::fs::remove_dir_all(&path_internal_git_dir)
+                    .context("Failed to clean up partially initialized internal git directory (by path) before CLI fallback")?;
             }
 
             // And removed from index