diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 4e327c42..5d103e4c 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/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, Extension(auth): Extension, @@ -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; diff --git a/crates/gitlawb-node/src/sync.rs b/crates/gitlawb-node/src/sync.rs index 0ed4a9f9..bc7e943b 100644 --- a/crates/gitlawb-node/src/sync.rs +++ b/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 '' 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 diff --git a/crates/gl/src/clone.rs b/crates/gl/src/clone.rs index 926a2d57..44cc3cdf 100644 --- a/crates/gl/src/clone.rs +++ b/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 ''`, 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 '' 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 ` 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()]); diff --git a/crates/gl/src/mirror.rs b/crates/gl/src/mirror.rs index 400d3d45..0dce9834 100644 --- a/crates/gl/src/mirror.rs +++ b/crates/gl/src/mirror.rs @@ -37,6 +37,17 @@ pub struct MirrorArgs { pub dir: Option, } +/// 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 { 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 ''` 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));