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

[beardthelion]

Closes #378.

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 downstream: git diff {target}...{source} in branch_diff/branch_diff_names, and git worktree add ... {target} / git merge {source} in merge_branch. A value starting with - is read by git as an option, so a stored --output=<path> turns the PR diff endpoint into an arbitrary file write. get_pr_diff takes an optional identity, so the trigger is unauthenticated on a public repo; planting the PR needs only read access, and the write happens at the withhold check before the visibility gate.

Two caller-supplied entry points feed those sinks:

• create_pr stores source_branch and target_branch from the body.
• create_repo stores default_branch from the body, which becomes a PR's target_branch when the PR omits one.

This adds a shared validate_git_ref (canonical in git/store.rs, re-exported as crate::api::validate_git_ref), following git check-ref-format rules with a leading-dash rejection as the core, and calls it at both boundaries. create_pr validates source_branch and the resolved target_branch, so a poisoned or legacy default cannot reach the sink even if it bypassed create_repo's gate. create_repo validates default_branch. Both run after the existing auth/name checks and before the row is written. No -- delimiter is added at the git call sites: the argument is a revision, and -- there would reinterpret it as a pathspec.

Not affected: resolve_head prefixes refs/heads/ before passing the branch to git, so a leading dash cannot lead there; fork_repo takes no branch from the request.

The guard runs at two layers. The storage boundaries above fail fast with a 400 and keep option-shaped refs out of the DB. The git sink functions (branch_diff, branch_diff_names, merge_branch) also reject them before building the argv, so the property holds for every caller and every row, including a row written before this change or by any writer that skips the boundary check.

Tests

• validate_git_ref unit tests: accepts main, feature/foo, release-1.2, v1.0.0, user/fix-bug; rejects empty, --output=/tmp/x, -rf, a space, .., ~, @{, .lock, leading/trailing/// slashes, and an over-255-byte name.
• End-to-end: create_pr as the owner of a public repo with an --output= target returns 400 with no PR row (fails before the fix, which returns 201); create_repo with an --output= default_branch returns 400 with no repo row.
• A poisoned PR row inserted directly (bypassing the handler) does not write a file when get_pr_diff runs, proving the sink guard, not just the boundary; it is RED without the sink guard.
• create_pr also rejects an option-shaped source_branch (the git merge arm).
• Full gitlawb-node suite: 829 passed, 0 failed.

[kevincodex1]

@coderabbitai please review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

git diff --unified=80 HEAD^ HEAD --
printf '\n--- changed files ---\n'
git diff --name-only HEAD^ HEAD
printf '\n--- Rust symbol outline ---\n'
fd -e rs . | xargs -r ast-grep outline --items all --type function --match 'validate_git_ref|create_pr|create_repo|branch_diff|branch_diff_names|merge_branch|get_pr_diff' 2>/dev/null || true

Length of output: 57930

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- pull handlers and merge path ---'
ast-grep outline crates/gitlawb-node/src/api/pulls.rs --items all --type function --match 'get_pr_diff|merge_pr|create_pr' --view expanded
sed -n '130,380p' crates/gitlawb-node/src/api/pulls.rs

printf '%s\n' '--- git sink call sites and branch-field writers ---'
rg -n -C 5 -P '\b(branch_diff|branch_diff_names|merge_branch)\s*\(|\b(source_branch|target_branch|default_branch)\s*:' crates/gitlawb-node/src --glob '*.rs'

printf '%s\n' '--- Git reference-format behavior for trailing dots ---'
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
git -C "$tmp" init -q
if git -C "$tmp" check-ref-format --branch 'feature.'; then
  echo 'unexpected: feature. accepted'
else
  echo 'expected: feature. rejected'
fi

Length of output: 46104

---

@kevincodex1 I found one issue.

• P2 — Reject refs that end with .. validate_git_ref does not reject feature.. Git rejects this value: git check-ref-format --branch feature. fails. The API can therefore store a PR branch or repository default_branch that later makes the diff and merge operations fail. Add a name.ends_with('.') check and include a trailing-dot case in validate_git_ref_rejects_option_injection_and_malformed_refs.

The option-injection protection is otherwise applied at both request boundaries and all current Git sinks.

_{You are interacting with an AI system.}

[jatmn]

I found issues that need to be addressed before this is ready.

Merge readiness

• [P1] Rebase onto current main and re-review the resolved security diff
  crates/gitlawb-node/src/git/store.rs:701
  The PR merge base (e4c7458) is behind the live target (73fd747), which has changed the same node API and Git-store security surfaces. Rebase before merge so the option-injection fix is verified against the current authorization and subprocess behavior.

Findings

• [P2] Validate branch names rather than accepting revision expressions
  crates/gitlawb-node/src/git/store.rs:701
validate_git_ref is introduced as the shared gate for persisted PR source_branch/target_branch values and repository default_branch values, but it implements only part of Git’s branch-name grammar. In particular, it accepts HEAD and components ending in ., while git check-ref-format --branch rejects HEAD, feature., and feature/x..

  This is not just a validation-message mismatch. create_pr accepts these values, writes the row, and emits pull_request.opened. A trailing-dot name then makes the later branch_diff_names, branch_diff, or merge_branch invocation fail. More importantly, HEAD is a revision expression, not a branch: merge_branch checks out the target branch and then runs git merge --no-ff HEAD, which Git treats as the already checked-out target commit and exits successfully without merging a submitted source branch. The handler then resolves the target ref and records the PR as merged, emitting pull_request.merged despite no source-branch merge having occurred.

  Please address the root cause by making this one shared validator enforce the actual branch-name contract—not merely a subset sufficient to block leading options—and retain its use at both request storage boundaries and all legacy-row subprocess sinks. A Git-equivalent branch-format check (or a complete equivalent implementation) should reject symbolic revision names and every forbidden component form, while preserving ordinary branch names and the existing leading-dash protection. Add load-bearing boundary and sink tests for at least HEAD, feature., and feature/x.: rejected API requests must create no row or webhook, and direct legacy rows must be rejected before any Git command or merge-side effect.