Skip to content

Commit c202520

Browse files
committed
fix(node): bind merge worktree to the exact local target branch
validate_git_ref proves branch-name grammar only; git's revision DWIM can resolve the same short name in another namespace (refs/tags/{name}), so a PR targeting a tag-shadowed name checked out a detached HEAD, merged onto it, discarded the commit at cleanup, and the unchecked plain rev-parse echoed refs/heads/{target} back as a bogus merge SHA — marking the PR merged and firing pull_request.merged with no branch advanced. Close the resolution class instead of denylisting names: (1) require the exact refs/heads/{target} via show-ref --verify before creating the worktree, (2) verify the worktree's symbolic HEAD is attached to exactly that ref before merging, (3) accept the merge only if rev-parse --verify {target}^{commit} exits zero. Add end-to-end regressions for a same-named tag with no local branch (merge fails, PR stays open, no merged event) and for a tag/branch coexistence (the local branch advances, the tag stays).
1 parent 7001cd9 commit c202520

2 files changed

Lines changed: 399 additions & 11 deletions

File tree

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

Lines changed: 65 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -935,19 +935,49 @@ pub fn merge_branch(
935935
) -> Result<String> {
936936
guard_refs(target_branch, source_branch)?;
937937
let source_ref = local_branch_ref(source_branch);
938+
let target_ref = local_branch_ref(target_branch);
938939
let worktree_path = repo_path.join("_merge_worktree");
939940

940-
// Clean up any leftover worktree
941-
if worktree_path.exists() {
941+
let remove_worktree = || {
942942
let _ = Command::new("git")
943943
.args(["worktree", "remove", "--force", "_merge_worktree"])
944944
.current_dir(repo_path)
945945
.output();
946946
let _ = std::fs::remove_dir_all(&worktree_path);
947+
};
948+
949+
// Clean up any leftover worktree
950+
if worktree_path.exists() {
951+
remove_worktree();
952+
}
953+
954+
// The merge must land on the LOCAL target branch, so the worktree has to end
955+
// up attached to exactly refs/heads/{target}. validate_git_ref proves
956+
// branch-name grammar only: it does not prove refs/heads/{target} exists,
957+
// and it cannot stop git's revision DWIM rules from resolving the same
958+
// short name in another namespace (refs/tags/{name}, refs/remotes/...). If
959+
// DWIM won, `worktree add` would check out a detached HEAD at the foreign
960+
// ref, the merge would succeed on that disposable HEAD, and cleanup would
961+
// discard the commit without ever advancing refs/heads/{target}.
962+
//
963+
// (1) Require the exact local ref before creating the worktree. show-ref
964+
// --verify matches the full refname literally — no DWIM, no abbreviation —
965+
// so a same-named tag with no local branch fails here instead of being
966+
// silently resolved.
967+
let target_exists = Command::new("git")
968+
.args(["show-ref", "--verify", "--quiet", &target_ref])
969+
.current_dir(repo_path)
970+
.output()
971+
.context("failed to run git show-ref")?;
972+
if !target_exists.status.success() {
973+
bail!("target branch {target_ref} does not exist as a local branch");
947974
}
948975

949-
// Check out the local target branch in the worktree. Passing refs/heads/{name}
950-
// would detach HEAD, so a successful merge would not advance refs/heads/{target}.
976+
// (2) Check out the local target branch in the worktree. The bare short
977+
// name is what makes git attach HEAD to the branch (passing
978+
// refs/heads/{name} would detach, so a successful merge would not advance
979+
// refs/heads/{target}); step (1) guarantees the branch exists, so an
980+
// attached checkout is the only acceptable outcome — verified right below.
951981
let wt = Command::new("git")
952982
.args(["worktree", "add", "_merge_worktree", target_branch])
953983
.current_dir(repo_path)
@@ -960,6 +990,22 @@ pub fn merge_branch(
960990
);
961991
}
962992

993+
// The worktree's HEAD must be a symbolic ref to exactly refs/heads/{target}.
994+
// A detached or differently-attached HEAD means git resolved the name as
995+
// something other than the local target branch; a merge committed on it
996+
// would be thrown away by cleanup, so refuse before merging rather than
997+
// relying on any denylist of symbolic shapes.
998+
let head_ref = Command::new("git")
999+
.args(["symbolic-ref", "--quiet", "HEAD"])
1000+
.current_dir(&worktree_path)
1001+
.output()
1002+
.context("failed to run git symbolic-ref")?;
1003+
let head_ref_name = String::from_utf8_lossy(&head_ref.stdout).trim().to_string();
1004+
if !head_ref.status.success() || head_ref_name != target_ref {
1005+
remove_worktree();
1006+
bail!("merge worktree is not attached to {target_ref} (HEAD is {head_ref_name:?})");
1007+
}
1008+
9631009
// Run merge in worktree
9641010
let merge = Command::new("git")
9651011
.args([
@@ -983,11 +1029,7 @@ pub fn merge_branch(
9831029
let success = merge.status.success();
9841030

9851031
// Always remove worktree
986-
let _ = Command::new("git")
987-
.args(["worktree", "remove", "--force", "_merge_worktree"])
988-
.current_dir(repo_path)
989-
.output();
990-
let _ = std::fs::remove_dir_all(&worktree_path);
1032+
remove_worktree();
9911033

9921034
if !success {
9931035
bail!(
@@ -996,12 +1038,24 @@ pub fn merge_branch(
9961038
);
9971039
}
9981040

999-
// Get new HEAD of target branch
1041+
// (3) The merge only counts if the exact target ref now points at a commit.
1042+
// Plain `rev-parse` echoes an unresolvable token back on stdout with a
1043+
// nonzero exit status, so an unchecked call here would hand the literal
1044+
// string "refs/heads/{target}" upward as an apparently valid merge SHA and
1045+
// the caller would mark the PR merged and fire the webhook. --verify plus
1046+
// ^{commit} plus an explicit status check turn "target ref missing or not a
1047+
// commit" into a hard failure before any of that happens.
10001048
let head = Command::new("git")
1001-
.args(["rev-parse", &format!("refs/heads/{target_branch}")])
1049+
.args(["rev-parse", "--verify", &format!("{target_ref}^{{commit}}")])
10021050
.current_dir(repo_path)
10031051
.output()
10041052
.context("failed to get merge commit")?;
1053+
if !head.status.success() {
1054+
bail!(
1055+
"merge completed but {target_ref} does not resolve to a commit: {}",
1056+
String::from_utf8_lossy(&head.stderr)
1057+
);
1058+
}
10051059

10061060
Ok(String::from_utf8_lossy(&head.stdout).trim().to_string())
10071061
}

0 commit comments

Comments
 (0)