From 80675e06ac0f27821ab86c2c6e33e0119cd219da Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sat, 27 Jun 2026 10:53:46 -0500 Subject: [PATCH 1/2] fix(node): gate fork_repo on per-caller path-scoped visibility (#98) fork_repo authorized the caller only at the repo root "/" and then git clone --mirror'd the full source, discarding the visibility rules. A non-owner with root read but denied a path-scoped subtree could fork the full mirror and obtain blobs the filtered git_upload_pack path withholds. Refuse the fork when the caller has any withheld glob (per-caller withheld_globs non-empty), the same decision the read path makes; owners and authorized subtree readers are unaffected. Fail closed with a not-found response, before repo_store.acquire, so no withheld object is materialized. Add unit coverage for the policy and a direct-mount integration test pinning the gate wiring. --- crates/gitlawb-node/src/api/repos.rs | 121 +++++++++++++++++++++++- crates/gitlawb-node/src/test_support.rs | 61 ++++++++++++ 2 files changed, 181 insertions(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 6afe68f7..cd6e9778 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -584,6 +584,30 @@ fn owner_push_rejection( } } +/// Decide whether the fork gate refuses a `fork_repo` request (#98). +/// +/// Returns `true` when the fork must be refused: the source carries at least one +/// path-scoped subtree that `caller` may not read, so a full `git clone --mirror` +/// would copy out content the filtered read path (`git_upload_pack`) withholds. +/// Pure function so the policy is unit-testable without a database or git backend. +/// +/// Delegates the per-caller decision to [`withheld_globs`](crate::visibility::withheld_globs) +/// / `visibility_check`, so the owner bypass (full and short DID) and `reader_dids` +/// grants are inherited from the read path and the two cannot drift on who may read +/// what. The predicate is a conservative (fail-closed) over-approximation of the +/// read path's object-level withholding: never weaker (so the fork cannot leak +/// content the read path withholds), and stricter only in the narrow +/// duplicate/co-located-blob case. Only called after `authorize_repo_read("/")` +/// has already granted the caller root read. +fn fork_withheld_blocks( + rules: &[crate::db::VisibilityRule], + is_public: bool, + owner_did: &str, + caller: &str, +) -> bool { + !crate::visibility::withheld_globs(rules, is_public, owner_did, Some(caller)).is_empty() +} + /// Path of the peer sync-notify endpoint. Used both to build the target URL /// and as the signing path, so they can never drift apart. const SYNC_NOTIFY_PATH: &str = "/api/v1/sync/notify"; @@ -1291,9 +1315,23 @@ pub async fn fork_repo( ) -> Result<(StatusCode, Json)> { // Enforce read visibility on the source before cloning: an unauthorized // caller must not be able to fork (full mirror) a repo they cannot read. - let (source, _rules) = + let (source, rules) = crate::api::authorize_repo_read(&state, &owner, &name, Some(auth.0.as_str()), "/").await?; + // #98: the "/" check above only proves root read. A full `git clone --mirror` + // would still copy out any path-scoped subtree withheld from this caller, so + // refuse the fork when the caller has any withheld glob. Fail closed with a + // not-found response (mirrors authorize_repo_read's Deny) so the existence of + // a subtree the caller cannot see is not leaked. Runs before repo_store.acquire + // so no withheld object is ever materialized on disk. + if fork_withheld_blocks(&rules, source.is_public, &source.owner_did, auth.0.as_str()) { + tracing::warn!( + owner = %owner, repo = %name, forker = %auth.0, + "fork rejected — source has a path-scoped subtree withheld from the caller" + ); + return Err(AppError::RepoNotFound(format!("{owner}/{name}"))); + } + let fork_name = req.name.unwrap_or_else(|| source.name.clone()); let forker_did = auth.0; @@ -1633,6 +1671,87 @@ mod tests { assert!(!caller_authorized_to_push(&repo, STRANGER_DID)); } + // ── fork_withheld_blocks (#98 path-scoped fork gate) ── + // A path-scoped visibility rule is an allow-list keyed by `reader_dids`, so + // the fork gate must ask the per-caller question "is anything withheld from + // this caller?" (`withheld_globs` non-empty), not the structural "does any + // non-`/` rule exist?". `READER_DID` is a non-owner who is granted a subtree. + const READER_DID: &str = "did:key:z6Mkreader000000000000000000000000000000000000000"; + + fn vis_rule(path_glob: &str, readers: &[&str]) -> crate::db::VisibilityRule { + crate::db::VisibilityRule { + id: "rule-id".into(), + repo_id: "repo-id".into(), + path_glob: path_glob.into(), + mode: crate::db::VisibilityMode::B, + reader_dids: readers.iter().map(|s| s.to_string()).collect(), + created_by: OWNER_DID.into(), + created_at: chrono::Utc::now(), + } + } + + #[test] + fn fork_owner_full_did_with_path_rule_allowed() { + // Owner reads everything (implicit reader), so nothing is withheld. + let rules = [vis_rule("/secret/**", &[])]; + assert!(!fork_withheld_blocks(&rules, true, OWNER_DID, OWNER_DID)); + } + + #[test] + fn fork_owner_short_did_with_path_rule_allowed() { + // Owner recognized in bare short-form via visibility_check's is_owner. + let rules = [vis_rule("/secret/**", &[])]; + assert!(!fork_withheld_blocks(&rules, true, OWNER_DID, OWNER_SHORT)); + } + + #[test] + fn fork_non_owner_denied_subtree_refused() { + // Core #98 regression: caller is not a reader of /secret, so it is + // withheld and the full-mirror fork must be refused. + let rules = [vis_rule("/secret/**", &[])]; + assert!(fork_withheld_blocks(&rules, true, OWNER_DID, STRANGER_DID)); + } + + #[test] + fn fork_non_owner_granted_subtree_allowed() { + // The case the structural predicate got wrong: a listed reader of + // /secret can read it on the read path, so the fork must be allowed. + let rules = [vis_rule("/secret/**", &[READER_DID])]; + assert!(!fork_withheld_blocks(&rules, true, OWNER_DID, READER_DID)); + } + + #[test] + fn fork_non_owner_root_rule_only_allowed() { + // Whole-repo "/" rules are excluded by withheld_globs; nothing withheld. + // is_public=true models the caller having passed authorize_repo_read("/"). + let rules = [vis_rule("/", &[])]; + assert!(!fork_withheld_blocks(&rules, true, OWNER_DID, STRANGER_DID)); + } + + #[test] + fn fork_non_owner_no_rules_public_allowed() { + assert!(!fork_withheld_blocks(&[], true, OWNER_DID, STRANGER_DID)); + } + + #[test] + fn fork_non_owner_mixed_root_and_denied_subtree_refused() { + // A permissive root rule does not rescue a denied path-scoped subtree. + let rules = [vis_rule("/", &[]), vis_rule("/secret/**", &[])]; + assert!(fork_withheld_blocks(&rules, true, OWNER_DID, STRANGER_DID)); + } + + #[test] + fn fork_partial_reader_still_refused() { + // Caller granted /secret/public but denied the rest of /secret still + // cannot read all of /secret, so the full mirror is refused (a filtered + // fork is Option 2 / deferred). + let rules = [ + vis_rule("/secret/**", &[]), + vis_rule("/secret/public/**", &[READER_DID]), + ]; + assert!(fork_withheld_blocks(&rules, true, OWNER_DID, READER_DID)); + } + fn ts(s: &str) -> DateTime { DateTime::parse_from_rfc3339(s).unwrap().with_timezone(&Utc) } diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 154db142..e4cb7ba6 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -218,6 +218,67 @@ mod tests { ); } + /// #98: forking a repo with a path-scoped subtree the caller cannot read is + /// refused with 404, before any clone. A public repo with a `/secret/**` rule + /// that excludes the stranger lets the stranger pass the `/` read gate but not + /// fork the full mirror. Pins the wiring (rules bound, gate before the clone); + /// a regression to `_rules` or moving the gate past `repo_store.acquire` fails + /// here. No on-disk source repo is needed — the refusal precedes acquire. + #[sqlx::test] + async fn fork_rejects_non_owner_with_withheld_subtree(pool: PgPool) { + let owner = "did:key:zFORKOWNERAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let stranger = "did:key:zFORKSTRANGERBBBBBBBBBBBBBBBBBBBBBBBBBB"; + let state = test_state(pool).await; + let repo = seed_repo(owner, "fork-repo"); + let repo_id = repo.id.clone(); + state.db.create_repo(&repo).await.expect("seed repo"); + state + .db + .set_visibility_rule( + &repo_id, + "/secret/**", + crate::db::VisibilityMode::B, + &[], + owner, + ) + .await + .expect("seed visibility rule"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/fork", + axum::routing::post(crate::api::repos::fork_repo), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner}/fork-repo/fork"); + let resp = router + .oneshot(signed_request_as( + stranger, + Method::POST, + &uri, + Body::from("{}"), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::NOT_FOUND, + "fork of a repo with a withheld subtree must be refused with 404" + ); + + // The fork must not have been created under the stranger's ownership. + let stranger_short = stranger.split(':').next_back().unwrap(); + assert!( + state + .db + .get_repo(stranger_short, "fork-repo") + .await + .expect("get_repo") + .is_none(), + "no fork row may be created for a refused fork" + ); + } + /// N13: the task handlers bind the acting DID to the signer. A caller signed /// as B claiming delegator_did A is rejected before any DB write (DB-free). #[sqlx::test] From b2c83ea172e75d7d9e701c9d75c0fc1c1f2ef727 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sat, 27 Jun 2026 11:04:32 -0500 Subject: [PATCH 2/2] fix(review): note fork-gate glob-grammar dependency, import withheld_globs Address code-review findings on the #98 fork gate: - Document that fork_withheld_blocks's prefix-probe equivalence with the serve path depends on validate_path_glob keeping "/" the only whole-repo scope, mirroring has_path_scoped_rule's caveat (latent-coupling guard). - Import withheld_globs and call it unqualified, consistent with the sibling visibility_check import in the same module. --- crates/gitlawb-node/src/api/repos.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index cd6e9778..2065438f 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -15,7 +15,7 @@ use crate::cert; use crate::error::{AppError, Result}; use crate::git::{smart_http, store, visibility_pack}; use crate::state::AppState; -use crate::visibility::{visibility_check, Decision}; +use crate::visibility::{visibility_check, withheld_globs, Decision}; use crate::webhooks; /// The git all-zeros object id — the create/delete sentinel in a ref update. @@ -599,13 +599,19 @@ fn owner_push_rejection( /// content the read path withholds), and stricter only in the narrow /// duplicate/co-located-blob case. Only called after `authorize_repo_read("/")` /// has already granted the caller root read. +/// +/// The gate evaluates rules at each glob's representative prefix while the serve +/// path withholds per blob path; their "is anything withheld" results agree only +/// because `validate_path_glob` keeps `/` the lone whole-repo scope (no glob can +/// collapse a non-`/` rule's prefix to `/`). If the glob grammar is ever extended, +/// revisit this equivalence — same caveat as `visibility_pack::has_path_scoped_rule`. fn fork_withheld_blocks( rules: &[crate::db::VisibilityRule], is_public: bool, owner_did: &str, caller: &str, ) -> bool { - !crate::visibility::withheld_globs(rules, is_public, owner_did, Some(caller)).is_empty() + !withheld_globs(rules, is_public, owner_did, Some(caller)).is_empty() } /// Path of the peer sync-notify endpoint. Used both to build the target URL