Skip to content

Commit 3f8bf0b

Browse files
committed
fix(node): skip withheld-walk when no path-scoped rule can withhold (#50)
The serve path (git_upload_pack) ran the full per-ref withheld_blob_oids walk on every clone/fetch before falling back to a plain upload_pack, even for repos with no path-scoped rules where the walk can never withhold anything. Cost grew with #42 (per-ref git ls-tree -r). Guard both call sites on has_path_scoped_rule: the serve path skips the spawn_blocking walk entirely and serves upload_pack directly; the replication path generalizes its is_empty() short-circuit to also cover root-only rule sets. Safe because the whole-repo "/" gate runs before both sites, so a denying root rule 404s/withholds upstream and never reaches the predicate. No change to served-pack contents on the success path.
1 parent 6dfb0de commit 3f8bf0b

1 file changed

Lines changed: 39 additions & 26 deletions

File tree

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

Lines changed: 39 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,13 @@ async fn replication_withheld_set(
5252
return (false, None);
5353
}
5454
let withheld = match rules {
55-
Some(rules) if rules.is_empty() => Some(std::collections::HashSet::new()),
55+
// No path-scoped rule can withhold anything (covers the empty-rules and
56+
// root-only-rules cases), so skip the full withheld_blob_oids walk and
57+
// withhold nothing. The predicate's safety-invariant test guards that
58+
// this short-circuit matches what the walk would have returned.
59+
Some(rules) if !crate::git::visibility_pack::has_path_scoped_rule(&rules) => {
60+
Some(std::collections::HashSet::new())
61+
}
5662
// withheld_blob_oids walks every ref with blocking `git ls-tree`; keep
5763
// that off the async worker thread.
5864
Some(rules) => {
@@ -499,33 +505,40 @@ pub async fn git_upload_pack(
499505
.map_err(|e| AppError::Git(e.to_string()))?;
500506
let body_len = body.len();
501507

502-
// withheld_blob_oids walks every ref with blocking `git ls-tree`; keep that
503-
// off the async worker thread.
504-
let withheld = {
505-
let path = disk_path.clone();
506-
let rules = rules.clone();
507-
let owner_did = record.owner_did.clone();
508-
let caller_owned = caller.map(str::to_string);
509-
let is_public = record.is_public;
510-
tokio::task::spawn_blocking(move || {
511-
visibility_pack::withheld_blob_oids(
512-
&path,
513-
&rules,
514-
is_public,
515-
&owner_did,
516-
caller_owned.as_deref(),
517-
)
518-
})
519-
.await
520-
.map_err(|e| AppError::Git(e.to_string()))?
521-
.map_err(|e| AppError::Git(e.to_string()))?
522-
};
523-
524-
let resp = if withheld.is_empty() {
508+
// No path-scoped rule can withhold an individual blob, and the whole-repo
509+
// "/" gate above already enforced repo-level access. Skip the per-blob
510+
// withheld walk and serve the pack directly.
511+
let resp = if !visibility_pack::has_path_scoped_rule(&rules) {
525512
smart_http::upload_pack(&disk_path, body).await
526513
} else {
527-
tracing::info!(repo = %name, caller = ?caller, withheld = withheld.len(), "serving filtered pack");
528-
smart_http::upload_pack_excluding(&disk_path, body, &withheld).await
514+
// withheld_blob_oids walks every ref with blocking `git ls-tree`; keep
515+
// that off the async worker thread.
516+
let withheld = {
517+
let path = disk_path.clone();
518+
let rules = rules.clone();
519+
let owner_did = record.owner_did.clone();
520+
let caller_owned = caller.map(str::to_string);
521+
let is_public = record.is_public;
522+
tokio::task::spawn_blocking(move || {
523+
visibility_pack::withheld_blob_oids(
524+
&path,
525+
&rules,
526+
is_public,
527+
&owner_did,
528+
caller_owned.as_deref(),
529+
)
530+
})
531+
.await
532+
.map_err(|e| AppError::Git(e.to_string()))?
533+
.map_err(|e| AppError::Git(e.to_string()))?
534+
};
535+
536+
if withheld.is_empty() {
537+
smart_http::upload_pack(&disk_path, body).await
538+
} else {
539+
tracing::info!(repo = %name, caller = ?caller, withheld = withheld.len(), "serving filtered pack");
540+
smart_http::upload_pack_excluding(&disk_path, body, &withheld).await
541+
}
529542
}
530543
.map_err(|e| {
531544
let msg = e.to_string();

0 commit comments

Comments
 (0)