Skip to content

Commit b98eb2c

Browse files
committed
fix(node): close review round on namespace gate, intent lifecycle, and promotion atomicity
Namespace gate hardening: - Refuse non-UTF-8 ref-update lines (git accepts bytes ≥0x80; gate never sees them) - Catch bare 'refs/gitlawb' name (starts_with missed it) - Add deny test for both holes plus refusal wiring verification Intent lifecycle guards: - Add IntentDropGuard: terminalize stranded intents on dropped futures - Bound refuse_receive_pack_before_git to 10s (no statement_timeout on pool) - Guard mirror fetch: hideRefs blocks non-exempt refs/gitlawb/* from +refs/*:refs/* Promotion merge atomicity: - Take row under SELECT FOR UPDATE so merge is atomic with state read - Prevents racing due worker from stranding newly applied children outside report
1 parent 10c185a commit b98eb2c

4 files changed

Lines changed: 270 additions & 43 deletions

File tree

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

Lines changed: 88 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2068,7 +2068,7 @@ pub async fn git_receive_pack(
20682068
}
20692069

20702070
// Parse ref updates from pkt-line body before handing to git
2071-
let ref_updates = parse_ref_updates(&body);
2071+
let ref_updates = parse_ref_updates(&body)?;
20722072
tracing::debug!(
20732073
ref_count = ref_updates.len(),
20742074
"parsed ref updates from pack"
@@ -2130,10 +2130,14 @@ pub async fn git_receive_pack(
21302130
// or tree is skipped by `rev-list --all` yet still served, bypassing
21312131
// the fail-closed under-withhold check. Refuse before intent is
21322132
// written so a denied push leaves no rows behind.
2133+
//
2134+
// Also match the bare `refs/gitlawb` name (no trailing slash): a push
2135+
// of that exact ref would not be caught by `starts_with("refs/gitlawb/")`
2136+
// and could land on a repo with no internal refs yet, bypassing the gate.
21332137
if let Some(bad) = ref_updates
21342138
.iter()
21352139
.map(|u| u.ref_name.as_str())
2136-
.find(|r| r.starts_with("refs/gitlawb/"))
2140+
.find(|r| *r == "refs/gitlawb" || r.starts_with("refs/gitlawb/"))
21372141
{
21382142
tracing::warn!(
21392143
repo = %name,
@@ -2296,6 +2300,12 @@ pub async fn git_receive_pack(
22962300
// `received` + `prepared` aggregate strands (never purged, never
22972301
// promotable without git evidence, and poisoning its tuple via
22982302
// `has_competing_claimant`).
2303+
//
2304+
// Drop-guard: if the handler exits (early return, cancellation, panic)
2305+
// between here and the `receive_pack_raw_with_reflog` call below, the
2306+
// guard terminalizes the intent on drop so it never strands in
2307+
// `received` state.
2308+
let _intent_guard = IntentDropGuard::new(state.db.clone(), request_id.clone());
22992309
let lease = match state
23002310
.repo_write_leases
23012311
.acquire(&repo_key, lease_steal_after)
@@ -2528,6 +2538,10 @@ pub async fn git_receive_pack(
25282538
// still relies on marker + tuple/timestamp + history guards. Marker
25292539
// namespace hiding was verified above by `verify_recovery_prereqs`.
25302540
let reflog_action = format!("gitlawb-request:{request_id}");
2541+
// Disarm the drop-guard: git is about to start, so a dropped future
2542+
// from here forward is a mid-git disconnect (not a pre-git shed), and
2543+
// the mark_uncertain path below handles that case.
2544+
_intent_guard.disarm();
25312545
let (receive_raw, exit_ok) = match smart_http::receive_pack_raw_with_reflog(
25322546
&state.git_bin,
25332547
&disk_path,
@@ -3818,6 +3832,60 @@ pub async fn get_icaptcha_proof(
38183832
})))
38193833
}
38203834

3835+
// ── Drop guard for stranded intents ──────────────────────────────────────
3836+
3837+
/// Drop-guard that terminalizes a durable intent if the handler exits (via
3838+
/// early return, cancellation, or panic) after the intent insert but before
3839+
/// `receive_pack` runs. A dropped future between the insert and git leaves a
3840+
/// `received` parent + `prepared` children that nothing retires: purge
3841+
/// excludes `received` even at 8 days, the drain skips it, and reconcile
3842+
/// cannot prove a landing git never ran. The stranded children also poison
3843+
/// `has_competing_claimant` checks. Disarm this guard once git starts.
3844+
struct IntentDropGuard {
3845+
db: std::sync::Arc<crate::db::Db>,
3846+
request_id: String,
3847+
armed: std::sync::Arc<std::sync::atomic::AtomicBool>,
3848+
}
3849+
3850+
impl IntentDropGuard {
3851+
fn new(db: std::sync::Arc<crate::db::Db>, request_id: String) -> Self {
3852+
Self {
3853+
db,
3854+
request_id,
3855+
armed: std::sync::Arc::new(std::sync::atomic::AtomicBool::new(true)),
3856+
}
3857+
}
3858+
3859+
fn disarm(&self) {
3860+
self.armed
3861+
.store(false, std::sync::atomic::Ordering::Release);
3862+
}
3863+
}
3864+
3865+
impl Drop for IntentDropGuard {
3866+
fn drop(&mut self) {
3867+
if self.armed.load(std::sync::atomic::Ordering::Acquire) {
3868+
let db = self.db.clone();
3869+
let request_id = self.request_id.clone();
3870+
tokio::spawn(async move {
3871+
if let Err(e) = db
3872+
.refuse_receive_pack_before_git(
3873+
&request_id,
3874+
"handler dropped before git started",
3875+
)
3876+
.await
3877+
{
3878+
tracing::warn!(
3879+
err = %e,
3880+
request_id = %request_id,
3881+
"drop-guard cleanup failed"
3882+
);
3883+
}
3884+
});
3885+
}
3886+
}
3887+
}
3888+
38213889
// ── Pkt-line parsing ──────────────────────────────────────────────────────
38223890

38233891
/// `Clone` so `git_receive_pack` can hand the parsed updates to the detached
@@ -3832,7 +3900,13 @@ pub(crate) struct RefUpdate {
38323900

38333901
/// Parse git receive-pack pkt-line ref updates from the request body.
38343902
/// Format per line: `<40-hex-old> <40-hex-new> <refname>[NUL capabilities]\n`
3835-
fn parse_ref_updates(body: &[u8]) -> Vec<RefUpdate> {
3903+
///
3904+
/// Returns `Err` if any command line in the ref-update section is not valid
3905+
/// UTF-8. Silently skipping non-UTF-8 lines would allow a pusher to plant
3906+
/// refs/gitlawb/* refs (bytes ≥ 0x80 are accepted by git but invisible to
3907+
/// the namespace gate and visibility_pack exemption), so the whole push must
3908+
/// be refused instead.
3909+
fn parse_ref_updates(body: &[u8]) -> Result<Vec<RefUpdate>> {
38363910
let mut updates = Vec::new();
38373911
let mut pos = 0;
38383912

@@ -3858,9 +3932,18 @@ fn parse_ref_updates(body: &[u8]) -> Vec<RefUpdate> {
38583932
let data = &body[pos + 4..pos + len];
38593933
pos += len;
38603934

3935+
// Non-UTF-8 ref-update lines must be refused, not skipped. Git accepts
3936+
// refnames with bytes ≥ 0x80 (e.g. \xff-prefixed), so a skipped line
3937+
// bypasses the refs/gitlawb namespace gate below and lands a blob ref
3938+
// that is invisible to parse_ref_updates yet still served by
3939+
// visibility_pack. Reject the entire push so no such ref reaches git.
38613940
let line = match std::str::from_utf8(data) {
38623941
Ok(s) => s,
3863-
Err(_) => continue,
3942+
Err(_) => {
3943+
return Err(AppError::BadRequest(
3944+
"ref-update line is not valid UTF-8; push refused".into(),
3945+
))
3946+
}
38643947
};
38653948

38663949
// Strip capabilities (after NUL) and trailing newline
@@ -3880,7 +3963,7 @@ fn parse_ref_updates(body: &[u8]) -> Vec<RefUpdate> {
38803963
}
38813964
}
38823965

3883-
updates
3966+
Ok(updates)
38843967
}
38853968

38863969
// ── Helpers ───────────────────────────────────────────────────────────────

‎crates/gitlawb-node/src/db/mod.rs‎

Lines changed: 62 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -3417,46 +3417,61 @@ impl Db {
34173417
/// return between intent insert and `receive_pack` must call this so a
34183418
/// shed push never strands `received` + `prepared` rows the purge path
34193419
/// never touches and reconcile can never promote (no git evidence).
3420+
///
3421+
/// Bounded to 10s: the transaction is `pool.begin()` + three UPDATEs +
3422+
/// commit with no statement_timeout configured on the pool, so a
3423+
/// lock-blocked cleanup can pin a global write slot (and on four of the
3424+
/// five call sites, the lease and per-source permit too) indefinitely.
3425+
/// The 10s convention matches the other bounded git calls on the
3426+
/// receive-pack path.
34203427
pub async fn refuse_receive_pack_before_git(
34213428
&self,
34223429
request_id: &str,
34233430
reason: &str,
34243431
) -> Result<()> {
3425-
let mut tx = self.pool.begin().await?;
3426-
let now = Utc::now().to_rfc3339();
3427-
sqlx::query(
3428-
r#"UPDATE receive_pack_requests
3429-
SET state = $2, git_exit_ok = FALSE, last_error = $3,
3430-
completed_at = $4
3431-
WHERE id = $1 AND state = $5"#,
3432-
)
3433-
.bind(request_id)
3434-
.bind(request_state::REJECTED_AT_GIT)
3435-
.bind(reason)
3436-
.bind(&now)
3437-
.bind(request_state::RECEIVED)
3438-
.execute(&mut *tx)
3439-
.await?;
3440-
sqlx::query(
3441-
r#"UPDATE pending_ref_transitions
3442-
SET state = $2, cancelled_at = $3
3443-
WHERE request_id = $1 AND state = $4"#,
3444-
)
3445-
.bind(request_id)
3446-
.bind(pending_state::CANCELLED)
3447-
.bind(&now)
3448-
.bind(pending_state::PREPARED)
3449-
.execute(&mut *tx)
3450-
.await?;
3451-
sqlx::query(
3452-
r#"UPDATE request_proofs SET acked_at = $2 WHERE request_id = $1 AND acked_at IS NULL"#,
3453-
)
3454-
.bind(request_id)
3455-
.bind(&now)
3456-
.execute(&mut *tx)
3457-
.await?;
3458-
tx.commit().await?;
3459-
Ok(())
3432+
let cleanup = async {
3433+
let mut tx = self.pool.begin().await?;
3434+
let now = Utc::now().to_rfc3339();
3435+
sqlx::query(
3436+
r#"UPDATE receive_pack_requests
3437+
SET state = $2, git_exit_ok = FALSE, last_error = $3,
3438+
completed_at = $4
3439+
WHERE id = $1 AND state = $5"#,
3440+
)
3441+
.bind(request_id)
3442+
.bind(request_state::REJECTED_AT_GIT)
3443+
.bind(reason)
3444+
.bind(&now)
3445+
.bind(request_state::RECEIVED)
3446+
.execute(&mut *tx)
3447+
.await?;
3448+
sqlx::query(
3449+
r#"UPDATE pending_ref_transitions
3450+
SET state = $2, cancelled_at = $3
3451+
WHERE request_id = $1 AND state = $4"#,
3452+
)
3453+
.bind(request_id)
3454+
.bind(pending_state::CANCELLED)
3455+
.bind(&now)
3456+
.bind(pending_state::PREPARED)
3457+
.execute(&mut *tx)
3458+
.await?;
3459+
sqlx::query(
3460+
r#"UPDATE request_proofs SET acked_at = $2 WHERE request_id = $1 AND acked_at IS NULL"#,
3461+
)
3462+
.bind(request_id)
3463+
.bind(&now)
3464+
.execute(&mut *tx)
3465+
.await?;
3466+
tx.commit().await?;
3467+
Ok(())
3468+
};
3469+
match tokio::time::timeout(std::time::Duration::from_secs(10), cleanup).await {
3470+
Ok(result) => result,
3471+
Err(_) => Err(anyhow::anyhow!(
3472+
"refuse_receive_pack_before_git timed out after 10s"
3473+
)),
3474+
}
34603475
}
34613476

34623477
/// #26 Split PR 1 step 5 — flip any non-terminal state to
@@ -3840,14 +3855,22 @@ impl Db {
38403855
// as `applied` outside it. State is never moved here — only the
38413856
// report widens.
38423857
if res.rows_affected() == 0 {
3858+
// Take the row under FOR UPDATE in a transaction so the merge
3859+
// is atomic with the state it read: if the due worker flips
3860+
// `outcomes_committed` → `effects_pending` between the SELECT and
3861+
// the UPDATE, the UPDATE would match zero rows, `Ok(0)` would
3862+
// return, and the newly applied child would be left out of the
3863+
// report with no later scan revisiting it.
3864+
let mut tx = self.pool.begin().await?;
38433865
let existing: Option<(serde_json::Value, Option<i32>, String)> = sqlx::query_as(
38443866
r#"SELECT parsed_report, accepted_ordinal, state FROM receive_pack_requests
3845-
WHERE id = $1 AND state IN ($2, $3)"#,
3867+
WHERE id = $1 AND state IN ($2, $3)
3868+
FOR UPDATE"#,
38463869
)
38473870
.bind(request_id)
38483871
.bind(request_state::OUTCOMES_COMMITTED)
38493872
.bind(request_state::EFFECTS_PENDING)
3850-
.fetch_optional(&self.pool)
3873+
.fetch_optional(&mut *tx)
38513874
.await?;
38523875
let Some((stored_report, stored_ordinal, stored_state)) = existing else {
38533876
return Ok(0);
@@ -3865,8 +3888,9 @@ impl Db {
38653888
.bind(request_id)
38663889
.bind(&stored_state)
38673890
.bind(&merged)
3868-
.execute(&self.pool)
3891+
.execute(&mut *tx)
38693892
.await?;
3893+
tx.commit().await?;
38703894
// accepted_ordinal intentionally untouched: the stored value
38713895
// (captured above as stored_ordinal) remains the event key.
38723896
let _ = stored_ordinal;

‎crates/gitlawb-node/src/sync.rs‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -731,6 +731,48 @@ async fn fetch_repo(local_path: &Path, remote_url: &str, mode: MirrorMode) -> an
731731

732732
git_run(&["-C", local_str, "remote", "set-url", "origin", remote_url]).await?;
733733

734+
// Guard against importing non-exempt refs/gitlawb/* refs from the origin.
735+
// The push gate (repos.rs) prevents creating them locally, but mirrors
736+
// `clone --mirror` then `fetch +refs/*:refs/*` via the stored refspec.
737+
// Pre-gate nodes or the non-UTF-8 hole (fixed this PR) could have planted
738+
// a `refs/gitlawb/<other>` ref pointing at a blob. That ref was exempt
739+
// before the narrowing (visibility_pack.rs) but now fails
740+
// `assert_all_refs_are_commits` closed. Prevent the fetch from importing
741+
// these refs so one origin ref doesn't wedge serving on every mirror.
742+
//
743+
// The two exempt subtrees (requests/, issues/) are excluded from hideRefs
744+
// so they still import; any other `refs/gitlawb/*` ref is hidden and won't
745+
// be fetched. This is cumulative config (git merges multiple hideRefs
746+
// entries), and `--prune` still applies to visible refs, so the mirror
747+
// prunes deleted exempt refs but never imports non-exempt ones.
748+
git_run(&[
749+
"-C",
750+
local_str,
751+
"config",
752+
"--add",
753+
"transfer.hideRefs",
754+
"refs/gitlawb/",
755+
])
756+
.await?;
757+
git_run(&[
758+
"-C",
759+
local_str,
760+
"config",
761+
"--add",
762+
"transfer.hideRefs",
763+
"!refs/gitlawb/requests/",
764+
])
765+
.await?;
766+
git_run(&[
767+
"-C",
768+
local_str,
769+
"config",
770+
"--add",
771+
"transfer.hideRefs",
772+
"!refs/gitlawb/issues/",
773+
])
774+
.await?;
775+
734776
match mode {
735777
MirrorMode::Promisor => {
736778
git_run(&["-C", local_str, "config", "remote.origin.promisor", "true"]).await?;

0 commit comments

Comments
 (0)