fix(api,sync): Delimit positional arguments in git clone mirror invocations

crates/gitlawb-node/src/api/repos.rs

@@ -3000,6 +3000,17 @@ pub struct ForkRepoRequest {
 }
 
 /// POST /api/v1/repos/:owner/:repo/fork
+/// Argv for the fork's mirror clone, with `--` before the first positional (#374).
+///
+/// Both positionals here are server-derived paths rather than caller-supplied
+/// URLs, so this is defence in depth rather than a reachable injection. It is
+/// still the same argv contract as the other clone sites, and it is extracted for
+/// the same reason: so the delimiter's placement is pinned by a test instead of
+/// resting on review.
+fn fork_clone_args<'a>(source: &'a str, dest: &'a str) -> [&'a str; 5] {
+    ["clone", "--mirror", "--", source, dest]
+}
+
 pub async fn fork_repo(
     State(state): State<AppState>,
     Extension(auth): Extension<AuthenticatedDid>,
@@ -3068,12 +3079,10 @@ pub async fn fork_repo(
 
     // Clone the source repo as a mirror
     let output = std::process::Command::new("git")
-        .args([
-            "clone",
-            "--mirror",
+        .args(fork_clone_args(
             source_path.to_str().unwrap_or(""),
             disk_path.to_str().unwrap_or(""),
-        ])
+        ))
         .output()
         .map_err(|e| AppError::Git(format!("git clone --mirror failed: {e}")))?;
 
@@ -3335,6 +3344,29 @@ fn dedupe_canonical_repos(rows: Vec<(RepoRecord, i64)>) -> Vec<(RepoRecord, i64)
 #[cfg(test)]
 mod tests {
     use super::*;
+
+    /// #374: the fork path builds its own `git clone --mirror` argv. Driven
+    /// through real git so a missing or misplaced `--` fails, matching the
+    /// coverage on the `gl clone`, `gl mirror`, and sync sinks.
+    #[test]
+    fn fork_clone_never_parses_the_source_as_a_git_option() {
+        let td = tempfile::TempDir::new().unwrap();
+        let dest = td.path().join("fork.git");
+        let injected = "--upload-pack=false";
+
+        let out = std::process::Command::new("git")
+            .args(fork_clone_args(injected, dest.to_str().unwrap()))
+            .output()
+            .unwrap();
+        let stderr = String::from_utf8_lossy(&out.stderr);
+
+        assert!(!out.status.success(), "an option-shaped source must fail");
+        assert!(
+            stderr.contains(&format!("repository '{injected}'")),
+            "git must name the source as the repository, not consume it as an option: {stderr}"
+        );
+    }
+
     use crate::auth::caller_authorized_to_push;
     use crate::error::AppError;
     use gitlawb_core::identity::Keypair;

crates/gitlawb-node/src/sync.rs

@@ -701,6 +701,7 @@ async fn clone_repo(remote_url: &str, local_path: &Path, mode: MirrorMode) -> an
     if mode == MirrorMode::Promisor {
         args.push("--filter=blob:limit=10g");
     }
+    args.push("--");
     args.push(remote_url);
     args.push(local_str);
 
@@ -729,7 +730,10 @@ async fn clone_repo(remote_url: &str, local_path: &Path, mode: MirrorMode) -> an
 async fn fetch_repo(local_path: &Path, remote_url: &str, mode: MirrorMode) -> anyhow::Result<()> {
     let local_str = local_path.to_str().unwrap_or(".");
 
-    git_run(&["-C", local_str, "remote", "set-url", "origin", remote_url]).await?;
+    git_run(&[
+        "-C", local_str, "remote", "set-url", "origin", "--", remote_url,
+    ])
+    .await?;
 
     match mode {
         MirrorMode::Promisor => {
@@ -1006,6 +1010,82 @@ mod tests {
         assert_eq!(probe, PromisorProbe::NotPromisor);
     }
 
+    /// #374: `clone_repo` interpolates the peer's origin URL into a `git
+    /// clone --mirror` argument list. Without a `--` delimiter git parses a
+    /// remote beginning with `-` as an option instead of a repository.
+    ///
+    /// The delimiter is what forces the same string to be a repository, and
+    /// git says so in its own words: it reports the whole argument as a
+    /// repository it cannot find. Undelimited, git consumes it as an option
+    /// and fails against the *destination* instead (`fatal: repository '<dest>' does not exist`),
+    /// never naming the injected string — which is why this assertion is RED
+    /// before the fix and GREEN after.
+    ///
+    /// The payload is deliberately free of `:` and spaces: a colon would make
+    /// git read the argument as an ssh-style `host:path` URL and report a
+    /// hostname rather than a repository, which is a different code path.
+    #[tokio::test]
+    async fn clone_never_parses_a_remote_as_a_git_option() {
+        let td = TempDir::new().unwrap();
+        let dest = td.path().join("mirror.git");
+        let injected = "--upload-pack=false";
+
+        let err = clone_repo(injected, &dest, MirrorMode::Plain)
+            .await
+            .expect_err("a remote that is not a repository must fail the clone")
+            .to_string();
+
+        assert!(
+            err.contains(injected),
+            "git must report the argument as a repository, not consume it as an option: {err}"
+        );
+    }
+
+    /// #374: `fetch_repo` interpolates the peer's origin URL into
+    /// `git remote set-url`, a second argv construction with the same contract as
+    /// the clone. It needs its own proof: the clone test cannot reach this path.
+    ///
+    /// The assertion is on stored state rather than on the error string, and that
+    /// is deliberate. `git_run` formats its failure as `git {args:?} failed:
+    /// {stderr}`, so the injected value is in the message via the debug-printed
+    /// argv whether or not the delimiter is present — asserting on containment
+    /// would pass with `--` deleted. Delimited, `set-url` succeeds and stores the
+    /// value verbatim; undelimited, git exits 129 with `unknown option` and leaves
+    /// the previous URL in place, so this goes red without the fix.
+    ///
+    /// The fetch that follows the rewrite fails, because the stored URL is not a
+    /// repository. That is expected and ignored: the property under test is what
+    /// `set-url` wrote, not whether the subsequent fetch could succeed.
+    #[tokio::test]
+    async fn fetch_never_parses_a_remote_as_a_git_option() {
+        let td = TempDir::new().unwrap();
+        let local = td.path().join("mirror.git");
+        let local_str = local.to_str().unwrap();
+        let placeholder = "http://example.invalid/x.git";
+
+        assert!(Command::new("git")
+            .args(["init", "-q", "--bare", local_str])
+            .status()
+            .unwrap()
+            .success());
+        assert!(Command::new("git")
+            .args(["-C", local_str, "remote", "add", "origin", placeholder])
+            .status()
+            .unwrap()
+            .success());
+
+        let injected = "--upload-pack=false";
+        let _ = fetch_repo(&local, injected, MirrorMode::Plain).await;
+
+        assert_eq!(
+            git_config_get(local_str, "remote.origin.url")
+                .await
+                .as_deref(),
+            Some(injected),
+            "set-url must store an option-shaped remote verbatim, not reject it as an option"
+        );
+    }
+
     #[tokio::test]
     async fn probe_reports_unknown_on_git_error() {
         // A path git cannot resolve as a repo at all (exit 128) is an indeterminate

crates/gl/src/clone.rs

@@ -159,8 +159,8 @@ pub fn setup_partial_clone(
 
     if withheld_globs.is_empty() {
         match branch {
-            Some(b) => git_global(&["clone", "-q", "--branch", b, remote_url, dest_str])?,
-            None => git_global(&["clone", "-q", remote_url, dest_str])?,
+            Some(b) => git_global(&["clone", "-q", "--branch", b, "--", remote_url, dest_str])?,
+            None => git_global(&["clone", "-q", "--", remote_url, dest_str])?,
         }
         return Ok(());
     }
@@ -170,6 +170,7 @@ pub fn setup_partial_clone(
         "-q",
         "--filter=blob:none",
         "--no-checkout",
+        "--",
         remote_url,
         dest_str,
     ])?;
@@ -384,7 +385,7 @@ async fn recover_encrypted_blobs(
         let plaintext = match open_blob(&envelope, keypair) {
             Ok(p) => p,
             Err(e) => {
-                eprintln!("warning: could not decrypt {oid}: {e}");
+                emit_warning(&format!("warning: could not decrypt {oid}: {e}"));
                 continue;
             }
         };
@@ -402,7 +403,9 @@ async fn recover_encrypted_blobs(
                 recovered.push(p.clone());
             }
         } else {
-            eprintln!("warning: recovered blob {oid} hashed to {written}; discarding");
+            emit_warning(&format!(
+                "warning: recovered blob {oid} hashed to {written}; discarding"
+            ));
         }
     }
     Ok(recovered)
@@ -800,7 +803,9 @@ async fn recover_from_arweave(
                 recovered.push(p.clone());
             }
         } else {
-            eprintln!("warning: recovered blob {oid} hashed to {written}; discarding");
+            emit_warning(&format!(
+                "warning: recovered blob {oid} hashed to {written}; discarding"
+            ));
         }
     }
     Ok(recovered)
@@ -1086,6 +1091,85 @@ mod tests {
         );
     }
 
+    #[test]
+    fn setup_partial_clone_plain_without_withheld_paths() {
+        let (td, url) = bare_remote(&[("file.txt", b"hello\n")]);
+        let dest = td.path().join("dest");
+        setup_partial_clone(&dest, &url, &[], &[], None).unwrap();
+        assert!(dest.join("file.txt").exists());
+    }
+
+    #[test]
+    fn setup_partial_clone_with_branch_without_withheld_paths() {
+        let (td, url) = bare_remote(&[("file.txt", b"hello\n")]);
+        let branch_out = Command::new("git")
+            .args([
+                "-C",
+                td.path().join("origin").to_str().unwrap(),
+                "branch",
+                "--show-current",
+            ])
+            .output()
+            .unwrap();
+        let branch = String::from_utf8(branch_out.stdout)
+            .unwrap()
+            .trim()
+            .to_string();
+        let dest = td.path().join("dest");
+        setup_partial_clone(&dest, &url, &[], &[], Some(&branch)).unwrap();
+        assert!(dest.join("file.txt").exists());
+    }
+
+    /// #374: every `setup_partial_clone` shape interpolates `remote_url` into a
+    /// `git clone` argv. The `--` delimiter is what forces an option-shaped
+    /// remote to be read as a repository rather than parsed as an option.
+    ///
+    /// The assertion is on git's own words — `repository '<value>'`, single-quoted
+    /// — and not merely on the value appearing somewhere in the error. `git_global`
+    /// formats its failure as `git {args:?} failed: {stderr}`, so the injected
+    /// string is already present in the debug-printed argv whether or not the
+    /// delimiter is there; asserting on containment alone would pass with `--`
+    /// deleted. Undelimited, git consumes the value as an option, leaving the
+    /// destination as the only positional, and names *that* instead
+    /// (`repository '<dest>' does not exist`) — so this goes red without the fix.
+    ///
+    /// The payload carries no `:` or space: a colon would make git read the
+    /// argument as an ssh-style `host:path` URL and report a hostname, which is a
+    /// different parse.
+    fn assert_option_shaped_remote_is_a_repository(withheld: &[String], branch: Option<&str>) {
+        let td = TempDir::new().unwrap();
+        let dest = td.path().join("dest");
+        let injected = "--upload-pack=false";
+
+        let err = setup_partial_clone(&dest, injected, withheld, &[], branch)
+            .expect_err("an option-shaped remote is not a repository and must fail")
+            .to_string();
+
+        assert!(
+            err.contains(&format!("repository '{injected}'")),
+            "git must name the argument as the repository, not consume it as an option: {err}"
+        );
+    }
+
+    #[test]
+    fn plain_clone_never_parses_a_remote_as_a_git_option() {
+        assert_option_shaped_remote_is_a_repository(&[], None);
+    }
+
+    #[test]
+    fn branch_clone_never_parses_a_remote_as_a_git_option() {
+        // Pins that `--branch <b>` stays ahead of the delimiter: if `--` were moved
+        // before `--branch`, git would read the branch name as a positional instead.
+        assert_option_shaped_remote_is_a_repository(&[], Some("main"));
+    }
+
+    #[test]
+    fn sparse_clone_never_parses_a_remote_as_a_git_option() {
+        // The withheld-globs arm is a separate argv construction
+        // (`--filter=blob:none --no-checkout`) and needs its own proof.
+        assert_option_shaped_remote_is_a_repository(&["/secret/**".to_string()], None);
+    }
+
     #[test]
     fn sparse_patterns_subtree_and_exact() {
         assert_eq!(sparse_patterns("/secret/**"), vec!["/secret/".to_string()]);

crates/gl/src/mirror.rs

@@ -37,6 +37,17 @@ pub struct MirrorArgs {
     pub dir: Option<PathBuf>,
 }
 
+/// Argv for the mirror clone, with `--` immediately before the first positional
+/// so an option-shaped `source` cannot be parsed as a git option (#374).
+///
+/// This is a separate function purely so the delimiter's placement can be driven
+/// through real git by a test. The production call keeps `.status()` rather than
+/// `.output()`: a mirror clone of a large repo streams git's progress to the
+/// user's terminal, and capturing it to assert on stderr here would swallow that.
+fn mirror_clone_args<'a>(source: &'a str, dest: &'a str) -> [&'a str; 5] {
+    ["clone", "--mirror", "--", source, dest]
+}
+
 pub async fn run(args: MirrorArgs) -> Result<()> {
     let source = args.source.trim_end_matches('/').to_string();
 
@@ -81,7 +92,7 @@ pub async fn run(args: MirrorArgs) -> Result<()> {
 
     println!("Cloning source (this may take a while for large repos)...");
     let clone_status = Command::new("git")
-        .args(["clone", "--mirror", &source, mirror_path.to_str().unwrap()])
+        .args(mirror_clone_args(&source, mirror_path.to_str().unwrap()))
         .status()
         .context("failed to run git clone — is git installed?")?;
 
@@ -170,6 +181,41 @@ pub fn extract_repo_name(url: &str) -> Option<String> {
 mod tests {
     use super::*;
 
+    /// #374: `gl mirror` interpolates the user-supplied source into a
+    /// `git clone --mirror` argv. `--` must sit immediately before the first
+    /// positional so an option-shaped source is read as a repository.
+    ///
+    /// This drives the production argv through real git rather than asserting on
+    /// the array's contents, so it fails on a delimiter that is missing *or*
+    /// misplaced. It cannot go through `run()`: that loads an identity keypair and
+    /// contacts a node before reaching the clone, and its bail message interpolates
+    /// `{source}` unconditionally, so an assertion there would pass with `--` gone.
+    ///
+    /// Asserting on git's single-quoted `repository '<value>'` is what makes this
+    /// red without the delimiter — undelimited, git consumes the value as an option
+    /// and names the destination as the repository instead.
+    #[test]
+    fn mirror_clone_never_parses_the_source_as_a_git_option() {
+        let td = tempfile::TempDir::new().unwrap();
+        let dest = td.path().join("mirror.git");
+        let injected = "--upload-pack=false";
+
+        let out = Command::new("git")
+            .args(mirror_clone_args(injected, dest.to_str().unwrap()))
+            .output()
+            .unwrap();
+        let stderr = String::from_utf8_lossy(&out.stderr);
+
+        assert!(
+            !out.status.success(),
+            "an option-shaped source is not a repository and must fail the clone"
+        );
+        assert!(
+            stderr.contains(&format!("repository '{injected}'")),
+            "git must name the source as the repository, not consume it as an option: {stderr}"
+        );
+    }
+
     #[test]
     fn test_extract_github_url() {
         assert_eq!(
@@ -240,7 +286,8 @@ mod tests {
             .create_async()
             .await;
 
-        // We can't easily test the git subprocess calls, but we can test the
+        // The git subprocess argv is pinned separately by
+        // `mirror_clone_never_parses_the_source_as_a_git_option`; this covers the
         // API error path by calling the create step directly via NodeClient.
         let kp2 = gitlawb_core::identity::Keypair::generate();
         let client = NodeClient::new(server.url(), Some(kp2));