fix(node): validate git branch refs to close option injection

PR branch refs and a repo's default_branch were stored from request bodies
with no ref validation, then interpolated into single git argv elements:
git diff {target}...{source} (branch_diff / branch_diff_names), and
git worktree add ... {target} / git merge {source} (merge_branch). A value
beginning with '-', e.g. --output=/tmp/x, is parsed by git as an option
rather than a revision, so it becomes an arbitrary file write. get_pr_diff
takes an optional identity, so on a public repo the trigger is unauthenticated;
planting the PR needs only read access, and the write happens at the withhold
check before the visibility gate.

Defense is applied at two layers:
- Storage boundaries: create_pr validates source_branch and the resolved
  target_branch; create_repo validates default_branch (which becomes a PR's
  target when the PR omits one). These fail fast with 400 and keep junk out
  of the DB.
- The sink: branch_diff, branch_diff_names, and merge_branch reject an
  option-shaped ref before building the git argv, so the property holds for
  every caller and every row, including legacy rows and any future writer
  that skips the boundary check.

The shared validator is crate::git::store::validate_git_ref (git
check-ref-format rules, leading-dash rejection as the core), re-exported as
crate::api::validate_git_ref for the boundary handlers. No -- delimiter is
used: the arguments are revisions, and -- there reinterprets them as
pathspecs.

Both boundary guards and the sink guard are mutation-proven load-bearing.
resolve_head is unaffected (it prefixes refs/heads/); fork_repo takes no
branch from the request.

Author: beardthelion

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

@@ -92,6 +92,12 @@ pub(crate) fn require_repo_owner(record: &RepoRecord, caller: &str) -> Result<()
     }
 }
 
+/// Re-export of the sink-level git ref validator (canonical home:
+/// `crate::git::store::validate_git_ref`). Storage boundaries call it here to
+/// fail fast with a 400; the sink guards enforce the same property for every
+/// caller. Its unit tests live beside the definition in `git/store.rs`.
+pub(crate) use crate::git::store::validate_git_ref;
+
 #[cfg(test)]
 mod did_tests {
     use super::did_matches;

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

@@ -50,6 +50,15 @@ pub async fn create_pr(
     let target_branch = req
         .target_branch
         .unwrap_or_else(|| record.default_branch.clone());
+
+    // Validate both refs before they are stored, since both are later
+    // interpolated into git argv (git diff / worktree add / merge). Validate the
+    // RESOLVED target, not just a caller-supplied one: create_repo also gates
+    // default_branch, but validating here as well means a PR can never feed the
+    // git sink an unchecked ref even if a default was poisoned by an older row
+    // or a future path that skips create_repo's gate.
+    crate::api::validate_git_ref(&req.source_branch).map_err(AppError::BadRequest)?;
+    crate::api::validate_git_ref(&target_branch).map_err(AppError::BadRequest)?;
     let number = state.db.next_pr_number(&record.id).await?;
     let now = Utc::now().to_rfc3339();
 

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

@@ -242,6 +242,11 @@ pub async fn create_repo(
         ));
     }
 
+    // default_branch is caller-supplied and becomes a PR's target_branch when the
+    // PR omits one, which is interpolated into a git revision argument. Validate
+    // it as a ref so it cannot begin with '-' and inject a git option downstream.
+    crate::api::validate_git_ref(&req.default_branch).map_err(AppError::BadRequest)?;
+
     // Owner is the authenticated agent's DID
     let owner_did = auth.0;
 

crates/gitlawb-node/src/git/store.rs

@@ -687,8 +687,69 @@ pub fn read_object(repo_path: &Path, sha256_hex: &str) -> Result<Option<(String,
     Ok(Some((obj_type, content)))
 }
 
+/// Validate a git branch ref before it is interpolated into a git argv element.
+/// Rules mirror `git check-ref-format`. The security-critical rule is the
+/// leading-`-` rejection: without it a name like `--output=/tmp/x` is parsed by
+/// git as an option rather than a revision, turning `branch_diff` / `merge_branch`
+/// into an arbitrary file write. A `--` delimiter is not the fix: these arguments
+/// are revisions, and `--` there reinterprets them as pathspecs.
+///
+/// This is the sink-level guard. Storage boundaries (`create_pr`, `create_repo`)
+/// call it too via the `crate::api` re-export to fail fast with a 400, but the
+/// guard here is what makes the property hold for every caller and every row,
+/// including legacy rows and any writer that skipped the boundary check.
+pub fn validate_git_ref(name: &str) -> std::result::Result<(), String> {
+    if name.is_empty() {
+        return Err("branch ref must not be empty".into());
+    }
+    // Option-injection core: a leading '-' makes git read the value as a flag.
+    if name.starts_with('-') {
+        return Err("branch ref must not begin with '-'".into());
+    }
+    if name.len() > 255 {
+        return Err("branch ref must be at most 255 bytes".into());
+    }
+    if name.chars().any(|c| c.is_ascii_control() || c == ' ') {
+        return Err("branch ref must not contain control characters or spaces".into());
+    }
+    if name.contains(['~', '^', ':', '?', '*', '[', '\\']) {
+        return Err("branch ref must not contain any of ~ ^ : ? * [ \\".into());
+    }
+    if name.contains("..") || name.contains("@{") {
+        return Err("branch ref must not contain '..' or '@{'".into());
+    }
+    if name == "@" {
+        return Err("branch ref must not be '@'".into());
+    }
+    if name.starts_with('/') || name.ends_with('/') || name.contains("//") {
+        return Err("branch ref must not have empty path components".into());
+    }
+    if name.ends_with(".lock") {
+        return Err("branch ref must not end with '.lock'".into());
+    }
+    for component in name.split('/') {
+        if component.starts_with('.') || component.ends_with(".lock") {
+            return Err(
+                "no branch ref path component may start with '.' or end with '.lock'".into(),
+            );
+        }
+    }
+    Ok(())
+}
+
+/// Reject both refs at the sink, so an option-shaped ref can never reach a git
+/// argv element regardless of how it was stored.
+fn guard_refs(target_branch: &str, source_branch: &str) -> Result<()> {
+    validate_git_ref(target_branch)
+        .map_err(|e| anyhow::anyhow!("invalid target branch ref: {e}"))?;
+    validate_git_ref(source_branch)
+        .map_err(|e| anyhow::anyhow!("invalid source branch ref: {e}"))?;
+    Ok(())
+}
+
 /// Get the diff between two branches: changes on source_branch not in target_branch.
 pub fn branch_diff(repo_path: &Path, target_branch: &str, source_branch: &str) -> Result<String> {
+    guard_refs(target_branch, source_branch)?;
     let output = Command::new("git")
         .args(["diff", &format!("{target_branch}...{source_branch}")])
         .current_dir(repo_path)
@@ -706,6 +767,7 @@ pub fn branch_diff_names(
     target_branch: &str,
     source_branch: &str,
 ) -> Result<Vec<String>> {
+    guard_refs(target_branch, source_branch)?;
     let output = Command::new("git")
         .args([
             "diff",
@@ -740,6 +802,7 @@ pub fn merge_branch(
     author_did: &str,
     pr_title: &str,
 ) -> Result<String> {
+    guard_refs(target_branch, source_branch)?;
     let worktree_path = repo_path.join("_merge_worktree");
 
     // Clean up any leftover worktree
@@ -819,6 +882,45 @@ pub fn repo_disk_path(repos_dir: &Path, owner_did: &str, repo_name: &str) -> Pat
 
 #[cfg(test)]
 mod tests {
+    use super::validate_git_ref;
+
+    #[test]
+    fn validate_git_ref_accepts_normal_branch_names() {
+        for good in [
+            "main",
+            "feature/foo",
+            "release-1.2",
+            "v1.0.0",
+            "user/fix-bug",
+        ] {
+            assert!(
+                validate_git_ref(good).is_ok(),
+                "{good:?} should be accepted"
+            );
+        }
+    }
+
+    #[test]
+    fn validate_git_ref_rejects_option_injection_and_malformed_refs() {
+        let long = "a".repeat(256);
+        for bad in [
+            "",
+            "--output=/tmp/x",
+            "-rf",
+            "a b",
+            "a..b",
+            "a~b",
+            "refs/heads/@{x}",
+            "foo.lock",
+            "/leading",
+            "trailing/",
+            "a//b",
+            long.as_str(),
+        ] {
+            assert!(validate_git_ref(bad).is_err(), "{bad:?} should be rejected");
+        }
+    }
+
     use super::branch_diff_names;
     use std::path::Path;
     use std::process::Command;