From 45064806741001bf120a2e414adcdb9a5250f3a8 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Tue, 30 Jun 2026 19:52:16 +0600 Subject: [PATCH 01/14] fix(node): gate GET /ipfs/{cid} on reachable allowed-set, not deny-set (#126) The IPFS visibility gate used withheld_blob_oids (a deny-set enumerating only reachable blobs), so a dangling/unreachable blob was absent from the set and served in cleartext to anonymous callers. Flip to an allowed-set (allowed_blob_set_for_caller) that enumerates reachable blobs the caller may read: a dangling blob has no path, is never in the set, and 404s. --- crates/gitlawb-node/src/api/ipfs.rs | 115 ++++++++++-------- .../gitlawb-node/src/git/visibility_pack.rs | 96 ++++++++++++++- crates/gitlawb-node/src/test_support.rs | 112 +++++++++++++++++ 3 files changed, 273 insertions(+), 50 deletions(-) diff --git a/crates/gitlawb-node/src/api/ipfs.rs b/crates/gitlawb-node/src/api/ipfs.rs index 6a43cb58..405eaedf 100644 --- a/crates/gitlawb-node/src/api/ipfs.rs +++ b/crates/gitlawb-node/src/api/ipfs.rs @@ -27,7 +27,7 @@ use std::str::FromStr; use crate::auth::AuthenticatedDid; use crate::error::{AppError, Result}; use crate::git::store; -use crate::git::visibility_pack::{has_path_scoped_rule, withheld_blob_oids}; +use crate::git::visibility_pack::{allowed_blob_set_for_caller, has_path_scoped_rule}; use crate::state::AppState; use crate::visibility::{visibility_check, Decision}; @@ -36,16 +36,22 @@ use crate::visibility::{visibility_check, Decision}; /// Search all repos on the node for a git object whose SHA-256 hash matches /// the given CIDv1, returning its raw content if the caller may read it. /// -/// Visibility (#110): the object is served only from a repo row the caller -/// passes. For each iterated row we gate against that row's OWN rules +/// Visibility (#110, #126): the object is served only from a repo row the +/// caller passes. For each iterated row we gate against that row's OWN rules /// (`visibility_check` at `"/"`), never re-resolving via `authorize_repo_read` /// — `get_repo`'s fuzzy match could otherwise authorize a different physical -/// row than the one read (KTD2a). When the row carries path-scoped rules, a -/// blob withheld from the caller (`withheld_blob_oids`) is skipped. Denial and -/// genuine not-found both fall through to an opaque 404. +/// row than the one read (KTD2a). When the row carries path-scoped rules +/// (KTD4) the served object must be either a non-blob (trees/commits are +/// structural; KTD3) OR a blob in the caller's *reachable* allowed-set +/// (`allowed_blob_set_for_caller`). The reachable allowed-set excludes +/// dangling blobs — a blob written via `git hash-object -w` and never +/// committed has no path to gate, so it is fail-closed 404'd under +/// path-scoped rules (#126). Denial and genuine not-found both fall through +/// to an opaque 404. /// -/// Scope: this closes the direct unauthenticated scan. A stale-public mirror -/// row still serves withheld content (tracked separately, #124). +/// Scope: this closes the direct unauthenticated scan, including the dangling +/// case. A stale-public mirror row still serves withheld content (tracked +/// separately, #124). pub async fn get_by_cid( Path(cid_str): Path, State(state): State, @@ -85,11 +91,16 @@ pub async fn get_by_cid( .await .map_err(AppError::Internal)?; - // Request-scoped memo of the per-repo withheld set (KTD1). The caller is - // constant for one request, so `repo.id` alone is a safe, sufficient key — - // never a coarse caller "class", which `visibility_check`'s exact full-DID - // reader match would make unsafe. - let mut withheld_memo: HashMap> = HashMap::new(); + // Request-scoped memo of the per-repo allowed-blob set (KTD1, #126). The + // caller is constant for one request, so `repo.id` alone is a safe, + // sufficient key — never a coarse caller "class", which + // `visibility_check`'s exact full-DID reader match would make unsafe. + // + // We flipped from a deny-set (`withheld_blob_oids`) to an allowed-set + // (`allowed_blob_set_for_caller`) so dangling blobs — never enumerated by + // the reachable walk — fail closed instead of slipping through an empty + // deny entry (#126). + let mut allowed_memo: HashMap> = HashMap::new(); for repo in &repos { // Repo-level read gate against THIS row's own rules (KTD2a). @@ -106,45 +117,51 @@ pub async fn get_by_cid( Err(_) => continue, }; - // Per-blob withholding only applies when a path-scoped rule exists (KTD4). - if has_path_scoped_rule(rules) { - if !withheld_memo.contains_key(&repo.id) { - let rp = repo_path.clone(); - let r = rules.to_vec(); - let is_public = repo.is_public; - let owner = repo.owner_did.clone(); - let caller_for_walk = caller_owned.clone(); - // Full-history walk shells out to git — keep it off the async runtime. - let walk = tokio::task::spawn_blocking(move || { - withheld_blob_oids(&rp, &r, is_public, &owner, caller_for_walk.as_deref()) - }) - .await; - // Fail closed on EITHER a task panic (JoinError) or a walk error: - // we cannot prove the caller may read here, so skip this repo and - // let a public copy (if any) serve. Never serve on an unproven gate. - let set = match walk { - Ok(Ok(set)) => set, - Ok(Err(e)) => { - tracing::warn!(repo = %repo.name, err = %e, "withheld walk failed; skipping repo"); - continue; - } - Err(e) => { - tracing::warn!(repo = %repo.name, err = %e, "withheld walk task panicked; skipping repo"); - continue; - } - }; - withheld_memo.insert(repo.id.clone(), set); - } - if withheld_memo - .get(&repo.id) - .is_some_and(|set| set.contains(&sha256_hex)) - { - continue; - } + // Per-blob gating only applies when a path-scoped rule exists (KTD4). + // Without any path-scoped rule, the "/" gate above is the whole story. + let path_scoped = has_path_scoped_rule(rules); + if path_scoped && !allowed_memo.contains_key(&repo.id) { + let rp = repo_path.clone(); + let r = rules.to_vec(); + let is_public = repo.is_public; + let owner = repo.owner_did.clone(); + let caller_for_walk = caller_owned.clone(); + // Full-history walk shells out to git — keep it off the async runtime. + let walk = tokio::task::spawn_blocking(move || { + allowed_blob_set_for_caller(&rp, &r, is_public, &owner, caller_for_walk.as_deref()) + }) + .await; + // Fail closed on EITHER a task panic (JoinError) or a walk error: + // we cannot prove the caller may read here, so skip this repo and + // let a public copy (if any) serve. Never serve on an unproven gate. + let set = match walk { + Ok(Ok(set)) => set, + Ok(Err(e)) => { + tracing::warn!(repo = %repo.name, err = %e, "allowed-blob walk failed; skipping repo"); + continue; + } + Err(e) => { + tracing::warn!(repo = %repo.name, err = %e, "allowed-blob walk task panicked; skipping repo"); + continue; + } + }; + allowed_memo.insert(repo.id.clone(), set); } match store::read_object(&repo_path, &sha256_hex) { - Ok(Some((_obj_type, content))) => { + Ok(Some((obj_type, content))) => { + // Path-scoped rules: serve trees/commits unconditionally + // (structural; KTD3); a blob must be in the reachable + // allowed-set, which excludes dangling blobs (#126). + if path_scoped && obj_type == "blob" { + let in_allowed = allowed_memo + .get(&repo.id) + .is_some_and(|set| set.contains(&sha256_hex)); + if !in_allowed { + continue; + } + } + // 3. Return the content with IPFS-style headers let mut headers = HeaderMap::new(); headers.insert( diff --git a/crates/gitlawb-node/src/git/visibility_pack.rs b/crates/gitlawb-node/src/git/visibility_pack.rs index 578ee40b..cb70e39c 100644 --- a/crates/gitlawb-node/src/git/visibility_pack.rs +++ b/crates/gitlawb-node/src/git/visibility_pack.rs @@ -309,11 +309,33 @@ pub fn replicable_blob_set( rules: &[VisibilityRule], is_public: bool, owner_did: &str, +) -> Result> { + allowed_blob_set_for_caller(repo_path, rules, is_public, owner_did, None) +} + +/// Reachable blob OIDs that visibility ALLOWS `caller` at some path. The +/// caller-aware generalization of `replicable_blob_set` (which is the anonymous +/// `caller = None` case). Used by `GET /ipfs/{cid}` to gate fail-closed against +/// dangling/unreachable blobs (#126): a blob written via `git hash-object -w` +/// but unreferenced is absent from the reachable walk, so it is never in this +/// set and the IPFS serve path drops it — even from the owner, who has no path +/// to authorize the blob at. +/// +/// A blob reachable at an allowed path is included even when also denied +/// elsewhere (its content is readable to this caller elsewhere). Trees and +/// commits are NOT included here; the caller decides per object type whether +/// the allow-set applies (it does not for trees/commits — KTD3). +pub fn allowed_blob_set_for_caller( + repo_path: &Path, + rules: &[VisibilityRule], + is_public: bool, + owner_did: &str, + caller: Option<&str>, ) -> Result> { let pairs = blob_paths(repo_path)?; let mut allowed = HashSet::new(); for (oid, path) in &pairs { - if visibility_check(rules, is_public, owner_did, None, path) == Decision::Allow { + if visibility_check(rules, is_public, owner_did, caller, path) == Decision::Allow { allowed.insert(oid.clone()); } } @@ -743,6 +765,78 @@ mod tests { ); } + #[test] + fn allowed_set_excludes_dangling_blob_for_every_caller() { + // #126: a blob written via `git hash-object -w` but never referenced has + // no path to gate on, so it is absent from the reachable allowed-set — + // for anonymous callers, listed readers, AND the owner. The IPFS serve + // path relies on this fail-closed property to drop dangling withheld + // blobs that the deny-set model leaked. + let td = TempDir::new().unwrap(); + let work = td.path().join("work"); + std::fs::create_dir_all(work.join("public")).unwrap(); + std::fs::write(work.join("public/a.txt"), b"public bytes\n").unwrap(); + let run = |args: &[&str]| { + assert!( + Command::new("git") + .args(args) + .current_dir(&work) + .status() + .unwrap() + .success(), + "git {args:?} failed" + ); + }; + run(&["init", "-q"]); + run(&["config", "user.email", "t@t"]); + run(&["config", "user.name", "t"]); + run(&["add", "."]); + run(&["commit", "-qm", "init"]); + let oid_of = |rev: &str| { + let out = Command::new("git") + .args(["rev-parse", rev]) + .current_dir(&work) + .output() + .unwrap(); + String::from_utf8_lossy(&out.stdout).trim().to_string() + }; + let public_oid = oid_of("HEAD:public/a.txt"); + + std::fs::write(work.join("orphan.bin"), b"DANGLING SECRET\n").unwrap(); + let dangling_oid = { + let out = Command::new("git") + .args(["hash-object", "-w", "orphan.bin"]) + .current_dir(&work) + .output() + .unwrap(); + String::from_utf8_lossy(&out.stdout).trim().to_string() + }; + assert!( + matches!(dangling_oid.len(), 40 | 64), + "precondition: hash-object stored the dangling blob" + ); + + // Path-scoped rule: /secret/** denied to anon, allowed to a listed reader. + let reader = "did:key:zReader"; + let rules = [rule("/secret/**", &[reader])]; + + // Every gate-relevant caller: anonymous, listed reader, owner. None of + // them can put the dangling blob in the allowed set — it has no path. + for caller in [None, Some(reader), Some(OWNER)] { + let allowed = allowed_blob_set_for_caller(&work, &rules, true, OWNER, caller).unwrap(); + assert!( + !allowed.contains(&dangling_oid), + "dangling blob must be absent from allowed-set (caller={caller:?})" + ); + // Sanity: the reachable public blob is still in the set for every + // caller (the rule does not deny /public/**). + assert!( + allowed.contains(&public_oid), + "reachable public blob must be in allowed-set (caller={caller:?})" + ); + } + } + #[test] fn recipients_are_owner_plus_allowed_readers_only() { let (_td, repo, secret_oid, public_oid) = fixture(); diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 98fccc56..6de9b0f0 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -1842,4 +1842,116 @@ mod tests { "walk error fails closed: repo skipped, even the public blob is not served" ); } + + /// #126: a dangling blob (written via `git hash-object -w`, never referenced + /// by any commit/tree) must 404 through `GET /ipfs/{cid}` under path-scoped + /// rules — for anon AND the owner. The pre-#126 deny-set was fail-open by + /// construction: dangling oids were absent from the reachable enumeration + /// and thus absent from the deny-set, so the handler served 200. The + /// allowed-set is fail-closed: dangling oids are absent from the reachable + /// allowed-set, so the handler 404s (per team memory: the owner shift to + /// 404 is the accepted fail-closed default — owners can still + /// `git cat-file` directly). + #[sqlx::test] + async fn ipfs_cid_dangling_blob_fails_closed_under_path_rules(pool: PgPool) { + use crate::db::VisibilityMode; + use gitlawb_core::identity::Keypair; + + let owner = Keypair::generate(); + let owner_did = owner.did().to_string(); + let slug = owner_did.replace([':', '/'], "_"); + let short = owner_did.split(':').next_back().unwrap().to_string(); + let state = test_state(pool).await; + + // Seed a normal repo with `secret/b.txt` reachable from HEAD, so the + // path-scoped rule has something to match — without this the rule has + // no anchor and we'd be testing nothing. + let _fx = seed_cid_repos(&slug, &short, &["dangling"]); + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join("dangling.git"); + + // Write a dangling blob: `git hash-object -w --stdin` adds it to the + // object DB but nothing references it, so the reachable walk never + // enumerates it. + let mut cmd = std::process::Command::new("git"); + cmd.args(["hash-object", "-w", "--stdin"]) + .current_dir(&bare) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()); + let mut child = cmd.spawn().expect("spawn git hash-object"); + { + use std::io::Write; + let stdin = child.stdin.as_mut().expect("stdin"); + stdin.write_all(b"DANGLING SECRET\n").expect("write stdin"); + } + let out = child.wait_with_output().expect("hash-object output"); + assert!( + out.status.success(), + "git hash-object: {}", + String::from_utf8_lossy(&out.stderr) + ); + let dangling_oid = String::from_utf8_lossy(&out.stdout).trim().to_string(); + // Sanity: must be a 64-hex sha256 oid, since the repo is sha256-format. + assert_eq!( + dangling_oid.len(), + 64, + "expected sha256 oid: {dangling_oid}" + ); + let dangling_cid = cid_for_oid(&dangling_oid); + + state + .db + .create_repo(&seed_repo(&owner_did, "dangling")) + .await + .expect("seed repo"); + let rec = state + .db + .get_repo(&owner_did, "dangling") + .await + .unwrap() + .unwrap(); + // Path-scoped rule triggers the per-blob allowed-set gate (KTD4). + state + .db + .set_visibility_rule(&rec.id, "/secret/**", VisibilityMode::B, &[], &owner_did) + .await + .expect("deny rule"); + + // anon: the dangling blob is absent from the reachable allowed-set → + // 404, no leak. Pre-#126 (deny-set) would serve 200. + let (st, body) = cid_parts( + cid_router(&state) + .oneshot(cid_anon(&dangling_cid)) + .await + .unwrap(), + ) + .await; + assert_eq!( + st, + StatusCode::NOT_FOUND, + "dangling blob must 404 under path-scoped rules" + ); + assert!( + !body.contains("DANGLING SECRET"), + "404 body must not leak the dangling content" + ); + + // owner (signed): same 404. The dangling blob has no path, so it's + // never visibility-checked → never in the allowed set, even for the + // owner. This is the accepted fail-closed shift documented in the PR. + let (st, body) = cid_parts( + cid_router(&state) + .oneshot(cid_signed(&owner, &dangling_cid)) + .await + .unwrap(), + ) + .await; + assert_eq!( + st, + StatusCode::NOT_FOUND, + "owner also 404s on dangling blobs under path-scoped rules (fail-closed default)" + ); + assert!(!body.contains("DANGLING SECRET")); + } } From 3aa7bf06f8cd9acfc26f6b7cea99441760a4716b Mon Sep 17 00:00:00 2001 From: Gravirei Date: Tue, 30 Jun 2026 20:07:58 +0600 Subject: [PATCH 02/14] perf(ipfs): check object existence before allowed-blob walk Move store::read_object before the allowed_blob_set_for_caller spawn_blocking call so random-CID spray against repos with path-scoped rules cannot trigger full-history git walks on repos that don't carry the object. --- crates/gitlawb-node/src/api/ipfs.rs | 141 ++++++++++++++-------------- 1 file changed, 72 insertions(+), 69 deletions(-) diff --git a/crates/gitlawb-node/src/api/ipfs.rs b/crates/gitlawb-node/src/api/ipfs.rs index 405eaedf..2177350a 100644 --- a/crates/gitlawb-node/src/api/ipfs.rs +++ b/crates/gitlawb-node/src/api/ipfs.rs @@ -40,14 +40,16 @@ use crate::visibility::{visibility_check, Decision}; /// caller passes. For each iterated row we gate against that row's OWN rules /// (`visibility_check` at `"/"`), never re-resolving via `authorize_repo_read` /// — `get_repo`'s fuzzy match could otherwise authorize a different physical -/// row than the one read (KTD2a). When the row carries path-scoped rules -/// (KTD4) the served object must be either a non-blob (trees/commits are -/// structural; KTD3) OR a blob in the caller's *reachable* allowed-set -/// (`allowed_blob_set_for_caller`). The reachable allowed-set excludes -/// dangling blobs — a blob written via `git hash-object -w` and never -/// committed has no path to gate, so it is fail-closed 404'd under -/// path-scoped rules (#126). Denial and genuine not-found both fall through -/// to an opaque 404. +/// row than the one read (KTD2a). We check object existence via +/// `store::read_object` *before* the expensive reachability walk so random-CID +/// spray cannot trigger full-history git walks on repos that don't carry the +/// object. When the row carries path-scoped rules (KTD4) the served object +/// must be either a non-blob (trees/commits are structural; KTD3) OR a blob +/// in the caller's *reachable* allowed-set (`allowed_blob_set_for_caller`). +/// The reachable allowed-set excludes dangling blobs — a blob written via +/// `git hash-object -w` and never committed has no path to gate, so it is +/// fail-closed 404'd under path-scoped rules (#126). Denial and genuine +/// not-found both fall through to an opaque 404. /// /// Scope: this closes the direct unauthenticated scan, including the dangling /// case. A stale-public mirror row still serves withheld content (tracked @@ -117,76 +119,77 @@ pub async fn get_by_cid( Err(_) => continue, }; + // Check whether the object exists in this repo before any expensive + // reachability walk. This prevents random-CID spray from triggering + // full-history git walks on repos that don't carry the object. + let object = store::read_object(&repo_path, &sha256_hex); + let (obj_type, content) = match object { + Ok(Some(t)) => t, + Ok(None) => continue, + Err(e) => { + tracing::warn!(repo = %repo.name, err = %e, "error reading git object"); + continue; + } + }; + // Per-blob gating only applies when a path-scoped rule exists (KTD4). // Without any path-scoped rule, the "/" gate above is the whole story. + // Trees/commits are always served under path-scoped rules (KTD3). let path_scoped = has_path_scoped_rule(rules); - if path_scoped && !allowed_memo.contains_key(&repo.id) { - let rp = repo_path.clone(); - let r = rules.to_vec(); - let is_public = repo.is_public; - let owner = repo.owner_did.clone(); - let caller_for_walk = caller_owned.clone(); - // Full-history walk shells out to git — keep it off the async runtime. - let walk = tokio::task::spawn_blocking(move || { - allowed_blob_set_for_caller(&rp, &r, is_public, &owner, caller_for_walk.as_deref()) - }) - .await; - // Fail closed on EITHER a task panic (JoinError) or a walk error: - // we cannot prove the caller may read here, so skip this repo and - // let a public copy (if any) serve. Never serve on an unproven gate. - let set = match walk { - Ok(Ok(set)) => set, - Ok(Err(e)) => { - tracing::warn!(repo = %repo.name, err = %e, "allowed-blob walk failed; skipping repo"); - continue; - } - Err(e) => { - tracing::warn!(repo = %repo.name, err = %e, "allowed-blob walk task panicked; skipping repo"); - continue; - } - }; - allowed_memo.insert(repo.id.clone(), set); - } - - match store::read_object(&repo_path, &sha256_hex) { - Ok(Some((obj_type, content))) => { - // Path-scoped rules: serve trees/commits unconditionally - // (structural; KTD3); a blob must be in the reachable - // allowed-set, which excludes dangling blobs (#126). - if path_scoped && obj_type == "blob" { - let in_allowed = allowed_memo - .get(&repo.id) - .is_some_and(|set| set.contains(&sha256_hex)); - if !in_allowed { + if path_scoped && obj_type == "blob" { + if !allowed_memo.contains_key(&repo.id) { + let rp = repo_path.clone(); + let r = rules.to_vec(); + let is_public = repo.is_public; + let owner = repo.owner_did.clone(); + let caller_for_walk = caller_owned.clone(); + // Full-history walk shells out to git — keep it off the async runtime. + let walk = tokio::task::spawn_blocking(move || { + allowed_blob_set_for_caller(&rp, &r, is_public, &owner, caller_for_walk.as_deref()) + }) + .await; + // Fail closed on EITHER a task panic (JoinError) or a walk error: + // we cannot prove the caller may read here, so skip this repo and + // let a public copy (if any) serve. Never serve on an unproven gate. + let set = match walk { + Ok(Ok(set)) => set, + Ok(Err(e)) => { + tracing::warn!(repo = %repo.name, err = %e, "allowed-blob walk failed; skipping repo"); + continue; + } + Err(e) => { + tracing::warn!(repo = %repo.name, err = %e, "allowed-blob walk task panicked; skipping repo"); continue; } - } - - // 3. Return the content with IPFS-style headers - let mut headers = HeaderMap::new(); - headers.insert( - HeaderName::from_static("content-type"), - HeaderValue::from_static("application/octet-stream"), - ); - headers.insert( - HeaderName::from_static("x-content-cid"), - HeaderValue::from_str(&cid_str) - .unwrap_or_else(|_| HeaderValue::from_static("invalid")), - ); - headers.insert( - HeaderName::from_static("x-git-hash"), - HeaderValue::from_str(&sha256_hex) - .unwrap_or_else(|_| HeaderValue::from_static("invalid")), - ); - - return Ok((StatusCode::OK, headers, content).into_response()); + }; + allowed_memo.insert(repo.id.clone(), set); } - Ok(None) => continue, - Err(e) => { - tracing::warn!(repo = %repo.name, err = %e, "error reading git object"); + let in_allowed = allowed_memo + .get(&repo.id) + .is_some_and(|set| set.contains(&sha256_hex)); + if !in_allowed { continue; } } + + // 3. Return the content with IPFS-style headers + let mut headers = HeaderMap::new(); + headers.insert( + HeaderName::from_static("content-type"), + HeaderValue::from_static("application/octet-stream"), + ); + headers.insert( + HeaderName::from_static("x-content-cid"), + HeaderValue::from_str(&cid_str) + .unwrap_or_else(|_| HeaderValue::from_static("invalid")), + ); + headers.insert( + HeaderName::from_static("x-git-hash"), + HeaderValue::from_str(&sha256_hex) + .unwrap_or_else(|_| HeaderValue::from_static("invalid")), + ); + + return Ok((StatusCode::OK, headers, content).into_response()); } // Not found in any repo From 63580c730da890851324a45d872ede1c3c7d9a62 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Tue, 30 Jun 2026 23:10:27 +0600 Subject: [PATCH 03/14] refactor(ipfs): improve formatting and readability in get_by_cid function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ✓ P3 blocker fixed: cargo fmt applied — the format gate will pass. ✓ P3 cleanup resolved: withheld_blob_oids is still used by replication code in repos.rs, so it stays. • P2 follow-up: Tree/commit disclosure tracked in #135 — out of scope here. --- crates/gitlawb-node/src/api/ipfs.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/crates/gitlawb-node/src/api/ipfs.rs b/crates/gitlawb-node/src/api/ipfs.rs index 2177350a..d6405fa0 100644 --- a/crates/gitlawb-node/src/api/ipfs.rs +++ b/crates/gitlawb-node/src/api/ipfs.rs @@ -145,7 +145,13 @@ pub async fn get_by_cid( let caller_for_walk = caller_owned.clone(); // Full-history walk shells out to git — keep it off the async runtime. let walk = tokio::task::spawn_blocking(move || { - allowed_blob_set_for_caller(&rp, &r, is_public, &owner, caller_for_walk.as_deref()) + allowed_blob_set_for_caller( + &rp, + &r, + is_public, + &owner, + caller_for_walk.as_deref(), + ) }) .await; // Fail closed on EITHER a task panic (JoinError) or a walk error: @@ -180,8 +186,7 @@ pub async fn get_by_cid( ); headers.insert( HeaderName::from_static("x-content-cid"), - HeaderValue::from_str(&cid_str) - .unwrap_or_else(|_| HeaderValue::from_static("invalid")), + HeaderValue::from_str(&cid_str).unwrap_or_else(|_| HeaderValue::from_static("invalid")), ); headers.insert( HeaderName::from_static("x-git-hash"), From 002f35405874898dc538773f0dcda13bafb56f49 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Wed, 1 Jul 2026 02:25:29 +0600 Subject: [PATCH 04/14] refactor(ipfs): streamline object retrieval by separating type and content reading --- crates/gitlawb-node/src/api/ipfs.rs | 14 +++++++++--- crates/gitlawb-node/src/git/store.rs | 34 ++++++++++++++++++++-------- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/crates/gitlawb-node/src/api/ipfs.rs b/crates/gitlawb-node/src/api/ipfs.rs index d6405fa0..46cbf739 100644 --- a/crates/gitlawb-node/src/api/ipfs.rs +++ b/crates/gitlawb-node/src/api/ipfs.rs @@ -122,12 +122,11 @@ pub async fn get_by_cid( // Check whether the object exists in this repo before any expensive // reachability walk. This prevents random-CID spray from triggering // full-history git walks on repos that don't carry the object. - let object = store::read_object(&repo_path, &sha256_hex); - let (obj_type, content) = match object { + let obj_type = match store::object_type(&repo_path, &sha256_hex) { Ok(Some(t)) => t, Ok(None) => continue, Err(e) => { - tracing::warn!(repo = %repo.name, err = %e, "error reading git object"); + tracing::warn!(repo = %repo.name, err = %e, "error checking git object type"); continue; } }; @@ -178,6 +177,15 @@ pub async fn get_by_cid( } } + // Now that we've passed the gate, read the content. + let content = match store::read_object_content(&repo_path, &sha256_hex, &obj_type) { + Ok(c) => c, + Err(e) => { + tracing::warn!(repo = %repo.name, err = %e, "error reading git object content"); + continue; + } + }; + // 3. Return the content with IPFS-style headers let mut headers = HeaderMap::new(); headers.insert( diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index b9759147..290da6cc 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -271,9 +271,8 @@ pub struct TreeEntry { /// `/ipfs/` is computed from these same content bytes via /// `gitlawb_core::cid::Cid::from_git_object_bytes`. /// -/// Returns `None` if the object does not exist in this repo. -pub fn read_object(repo_path: &Path, sha256_hex: &str) -> Result)>> { - // First check if the object exists and get its type +/// Get just the object type. Returns `None` if the object doesn't exist. +pub fn object_type(repo_path: &Path, sha256_hex: &str) -> Result> { let type_output = Command::new("git") .args(["cat-file", "-t", sha256_hex]) .current_dir(repo_path) @@ -284,13 +283,13 @@ pub fn read_object(repo_path: &Path, sha256_hex: &str) -> Result Result> { let content_output = Command::new("git") - .args(["cat-file", &obj_type, sha256_hex]) + .args(["cat-file", obj_type, sha256_hex]) .current_dir(repo_path) .output() .context("failed to run git cat-file ")?; @@ -300,7 +299,24 @@ pub fn read_object(repo_path: &Path, sha256_hex: &str) -> Result` is computed from these same content bytes via +/// `gitlawb_core::cid::Cid::from_git_object_bytes`. +/// +/// Returns `None` if the object does not exist in this repo. +pub fn read_object(repo_path: &Path, sha256_hex: &str) -> Result)>> { + let obj_type = match object_type(repo_path, sha256_hex)? { + Some(t) => t, + None => return Ok(None), + }; + let content = read_object_content(repo_path, sha256_hex, &obj_type)?; + Ok(Some((obj_type, content))) } /// Get the diff between two branches: changes on source_branch not in target_branch. From f2c91a868afb57b7e2a2b949536015490492cef7 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Wed, 1 Jul 2026 07:22:24 +0600 Subject: [PATCH 05/14] docs(ipfs): update get_by_cid comment to reflect split object retrieval --- crates/gitlawb-node/src/api/ipfs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/api/ipfs.rs b/crates/gitlawb-node/src/api/ipfs.rs index 46cbf739..f3de7570 100644 --- a/crates/gitlawb-node/src/api/ipfs.rs +++ b/crates/gitlawb-node/src/api/ipfs.rs @@ -41,7 +41,7 @@ use crate::visibility::{visibility_check, Decision}; /// (`visibility_check` at `"/"`), never re-resolving via `authorize_repo_read` /// — `get_repo`'s fuzzy match could otherwise authorize a different physical /// row than the one read (KTD2a). We check object existence via -/// `store::read_object` *before* the expensive reachability walk so random-CID +/// `store::object_type` *before* the expensive reachability walk so random-CID /// spray cannot trigger full-history git walks on repos that don't carry the /// object. When the row carries path-scoped rules (KTD4) the served object /// must be either a non-blob (trees/commits are structural; KTD3) OR a blob From 03ba7149fc248798df15f9fb26bf897fd2901b4d Mon Sep 17 00:00:00 2001 From: Gravirei Date: Wed, 1 Jul 2026 23:40:07 +0600 Subject: [PATCH 06/14] Run cargo fmt on store.rs --- crates/gitlawb-node/src/git/store.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 290da6cc..229ee695 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -283,7 +283,11 @@ pub fn object_type(repo_path: &Path, sha256_hex: &str) -> Result> return Ok(None); } - Ok(Some(String::from_utf8_lossy(&type_output.stdout).trim().to_string())) + Ok(Some( + String::from_utf8_lossy(&type_output.stdout) + .trim() + .to_string(), + )) } /// Read an object's content if its type is already known. From 8d0f41c71f889e0985db83838a81fa5a951bd619 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Thu, 9 Jul 2026 20:08:55 +0600 Subject: [PATCH 07/14] test(gl): add tests for iCaptcha transparent retry flow (#167) --- crates/gl/src/http.rs | 338 ++++++++++++++++++++++++++++++ crates/icaptcha-client/src/lib.rs | 21 +- 2 files changed, 357 insertions(+), 2 deletions(-) diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index 8cd7c97e..ee041f9c 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -185,3 +185,341 @@ async fn obtain_proof(cfg: IcaptchaCfg) -> Result { .await .context("iCaptcha solver task panicked")? } + +#[cfg(test)] +mod tests { + use super::*; + use gitlawb_core::identity::Keypair; + use mockito::Server; + + fn test_keypair() -> Keypair { + Keypair::generate() + } + + fn headers_from_pairs(pairs: &[(&str, &str)]) -> reqwest::header::HeaderMap { + let mut h = reqwest::header::HeaderMap::new(); + for (k, v) in pairs { + h.insert( + k.parse::().unwrap(), + v.parse::().unwrap(), + ); + } + h + } + + // ── icaptcha_cfg ──────────────────────────────────────────────────── + + #[test] + fn icaptcha_cfg_returns_some_when_both_headers_present() { + let kp = test_keypair(); + let client = NodeClient::new("http://localhost", Some(kp.clone())); + let headers = headers_from_pairs(&[ + ("x-icaptcha-url", "https://icaptcha.gitlawb.com"), + ("x-icaptcha-level", "3"), + ]); + let cfg = client.icaptcha_cfg(&headers).unwrap().unwrap(); + assert_eq!(cfg.did, kp.did().to_string()); + assert_eq!(cfg.level, 3); + } + + #[test] + fn icaptcha_cfg_defaults_level_when_only_url_present() { + let kp = test_keypair(); + let client = NodeClient::new("http://localhost", Some(kp)); + let headers = headers_from_pairs(&[("x-icaptcha-url", "https://icaptcha.gitlawb.com")]); + let cfg = client.icaptcha_cfg(&headers).unwrap().unwrap(); + assert_eq!(cfg.level, icaptcha_client::DEFAULT_LEVEL); + } + + #[test] + fn icaptcha_cfg_defaults_url_when_only_level_present() { + let kp = test_keypair(); + let client = NodeClient::new("http://localhost", Some(kp)); + let headers = headers_from_pairs(&[("x-icaptcha-level", "5")]); + let cfg = client.icaptcha_cfg(&headers).unwrap().unwrap(); + assert_eq!(cfg.level, 5); + } + + #[test] + fn icaptcha_cfg_returns_none_without_icaptcha_headers() { + let client = NodeClient::new("http://localhost", Some(test_keypair())); + let headers = reqwest::header::HeaderMap::new(); + assert!(client.icaptcha_cfg(&headers).unwrap().is_none()); + } + + #[test] + fn icaptcha_cfg_returns_none_with_unrelated_headers() { + let client = NodeClient::new("http://localhost", Some(test_keypair())); + let headers = headers_from_pairs(&[("content-type", "application/json")]); + assert!(client.icaptcha_cfg(&headers).unwrap().is_none()); + } + + #[test] + fn icaptcha_cfg_errors_when_no_keypair() { + let client = NodeClient::new("http://localhost", None); + let headers = headers_from_pairs(&[("x-icaptcha-level", "3")]); + let err = client.icaptcha_cfg(&headers).unwrap_err(); + assert!(err.to_string().contains("identity keypair")); + } + + #[test] + fn icaptcha_cfg_ignores_unparseable_level() { + let client = NodeClient::new("http://localhost", Some(test_keypair())); + let headers = headers_from_pairs(&[ + ("x-icaptcha-url", "https://icaptcha.gitlawb.com"), + ("x-icaptcha-level", "not-a-number"), + ]); + let cfg = client.icaptcha_cfg(&headers).unwrap().unwrap(); + assert_eq!(cfg.level, icaptcha_client::DEFAULT_LEVEL); + } + + // ── send_once ─────────────────────────────────────────────────────── + + #[tokio::test] + async fn send_once_attaches_proof_header_when_provided() { + let mut server = Server::new_async().await; + let m = server + .mock("POST", "/api/test") + .match_header("x-icaptcha-proof", "test.proof.token") + .with_status(200) + .with_body("ok") + .create_async() + .await; + let client = NodeClient::new(server.url(), None); + let resp = client + .send_once("POST", "/api/test", b"{}", Some("test.proof.token")) + .await + .unwrap(); + assert_eq!(resp.status(), 200); + m.assert(); + } + + #[tokio::test] + async fn send_once_omits_proof_header_when_not_provided() { + let mut server = Server::new_async().await; + let m = server + .mock("POST", "/api/test") + .with_status(200) + .with_body("ok") + .create_async() + .await; + let client = NodeClient::new(server.url(), None); + let resp = client + .send_once("POST", "/api/test", b"{}", None) + .await + .unwrap(); + assert_eq!(resp.status(), 200); + m.assert(); + } + + #[tokio::test] + async fn send_once_signs_request_when_keypair_present() { + let mut server = Server::new_async().await; + let m = server + .mock("POST", "/api/test") + .match_header("Signature", mockito::Matcher::Any) + .match_header("Signature-Input", mockito::Matcher::Any) + .match_header("Content-Digest", mockito::Matcher::Any) + .with_status(200) + .with_body("ok") + .create_async() + .await; + let client = NodeClient::new(server.url(), Some(test_keypair())); + let resp = client + .send_once("POST", "/api/test", b"{}", None) + .await + .unwrap(); + assert_eq!(resp.status(), 200); + m.assert(); + } + + #[tokio::test] + async fn send_once_does_not_sign_when_no_keypair() { + let mut server = Server::new_async().await; + let m = server + .mock("POST", "/api/test") + .with_status(200) + .with_body("ok") + .create_async() + .await; + let client = NodeClient::new(server.url(), None); + let resp = client + .send_once("POST", "/api/test", b"{}", None) + .await + .unwrap(); + assert_eq!(resp.status(), 200); + m.assert(); + } + + // ── send_signed ───────────────────────────────────────────────────── + + #[tokio::test] + async fn send_signed_returns_non_icaptcha_403_without_retry() { + let mut server = Server::new_async().await; + let m = server + .mock("POST", "/api/register") + .with_status(403) + .with_header("content-type", "application/json") + .with_body(r#"{"error":"forbidden"}"#) + .create_async() + .await; + let client = NodeClient::new(server.url(), Some(test_keypair())); + let resp = client + .send_signed("POST", "/api/register", b"{}") + .await + .unwrap(); + assert_eq!(resp.status(), 403); + m.assert(); + } + + #[tokio::test] + async fn send_signed_returns_first_response_on_success() { + let mut server = Server::new_async().await; + let m = server + .mock("POST", "/api/register") + .with_status(201) + .with_header("content-type", "application/json") + .with_body(r#"{"status":"created"}"#) + .create_async() + .await; + let client = NodeClient::new(server.url(), Some(test_keypair())); + let resp = client + .send_signed("POST", "/api/register", b"{}") + .await + .unwrap(); + assert_eq!(resp.status(), 201); + m.assert(); + } + + #[tokio::test] + async fn send_signed_handles_405_not_icaptcha() { + let mut server = Server::new_async().await; + let m = server + .mock("POST", "/api/register") + .with_status(405) + .with_body(r#"{"error":"method not allowed"}"#) + .create_async() + .await; + let client = NodeClient::new(server.url(), Some(test_keypair())); + let resp = client + .send_signed("POST", "/api/register", b"{}") + .await + .unwrap(); + assert_eq!(resp.status(), 405); + m.assert(); + } + + // ── send_signed iCaptcha retry (full integration) ──────────────────── + + /// Helper: set GITLAWB_ICAPTCHA_URL so the iCaptcha client trusts the mock + /// server URL and clean it up on drop. + struct IcaptchaEnv; + + impl IcaptchaEnv { + fn new(url: &str) -> Self { + std::env::set_var("GITLAWB_ICAPTCHA_URL", url); + IcaptchaEnv + } + } + + impl Drop for IcaptchaEnv { + fn drop(&mut self) { + std::env::remove_var("GITLAWB_ICAPTCHA_URL"); + } + } + + /// Helper: set up a mock iCaptcha server that responds to challenge + answer. + struct MockIcaptcha { + _c: mockito::Mock, + _a: mockito::Mock, + _guard: IcaptchaEnv, + url: String, + } + + impl MockIcaptcha { + async fn new(server: &mut mockito::ServerGuard) -> Self { + let url = server.url(); + let guard = IcaptchaEnv::new(&url); + let _c = server + .mock("POST", "/v1/challenge") + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"challengeId":"c1","type":"arithmetic","difficulty":1,"prompt":"What is 1 + 1?","token":"tk1"}"#, + ) + .create_async() + .await; + let _a = server + .mock("POST", "/v1/answer") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"status":"passed","proof":"mock.proof"}"#) + .create_async() + .await; + Self { + _c, + _a, + _guard: guard, + url, + } + } + } + + #[tokio::test] + async fn send_signed_solves_icaptcha_and_retries_to_success() { + let mut node = Server::new_async().await; + let mut icaptcha = Server::new_async().await; + let _ic = MockIcaptcha::new(&mut icaptcha).await; + + let _n1 = node + .mock("POST", "/api/register") + .with_status(403) + .with_header("content-type", "application/json") + .with_header("x-icaptcha-url", &_ic.url) + .with_header("x-icaptcha-level", "3") + .with_body(r#"{"error":"icaptcha_proof_required"}"#) + .create_async() + .await; + let _n2 = node + .mock("POST", "/api/register") + .match_header("x-icaptcha-proof", mockito::Matcher::Any) + .with_status(201) + .with_header("content-type", "application/json") + .with_body(r#"{"status":"created"}"#) + .create_async() + .await; + + let client = NodeClient::new(node.url(), Some(test_keypair())); + let resp = client + .send_signed("POST", "/api/register", b"{}") + .await + .unwrap(); + assert_eq!(resp.status(), 201); + } + + #[tokio::test] + async fn send_signed_returns_403_after_icaptcha_retries_exhausted() { + let mut node = Server::new_async().await; + let mut icaptcha = Server::new_async().await; + let _ic = MockIcaptcha::new(&mut icaptcha).await; + + // Every call to the node returns 403 with iCaptcha headers, exhausting + // MAX_ICAPTCHA_RETRIES (2). The original + 2 retries = 3 node calls. + let _n = node + .mock("POST", "/api/register") + .with_status(403) + .with_header("content-type", "application/json") + .with_header("x-icaptcha-url", &_ic.url) + .with_header("x-icaptcha-level", "3") + .with_body(r#"{"error":"icaptcha_proof_required"}"#) + .create_async() + .await; + + let client = NodeClient::new(node.url(), Some(test_keypair())); + let resp = client + .send_signed("POST", "/api/register", b"{}") + .await + .unwrap(); + assert_eq!(resp.status(), 403); + } +} diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index c41e368e..78999a29 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -81,11 +81,28 @@ fn sanitize_excerpt(s: &str) -> String { out } -/// Whether `u` parses as an `https` URL. +/// Whether `u` parses as a trusted URL (https in production; also allows +/// localhost/127.0.0.1 in tests so the full iCaptcha retry path can be +/// exercised against a mockito server). fn is_https(u: &str) -> bool { - reqwest::Url::parse(u) + let parsed = reqwest::Url::parse(u); + if parsed + .as_ref() .map(|p| p.scheme() == "https") .unwrap_or(false) + { + return true; + } + // Allow local test servers in test builds (mockito runs on 127.0.0.1:PORT). + #[cfg(test)] + if parsed + .as_ref() + .map(|p| p.host_str() == Some("127.0.0.1") || p.host_str() == Some("localhost")) + .unwrap_or(false) + { + return true; + } + false } /// Lowercased host of a URL, if it parses and has one. From cb8761da1e9f5192576578aa16df08d60267bb1e Mon Sep 17 00:00:00 2001 From: Gravirei Date: Thu, 9 Jul 2026 20:12:54 +0600 Subject: [PATCH 08/14] fix(clippy): remove redundant borrow in format! arg --- crates/gitlawb-node/src/api/profiles.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/api/profiles.rs b/crates/gitlawb-node/src/api/profiles.rs index 6f3a12a9..4e42c6e4 100644 --- a/crates/gitlawb-node/src/api/profiles.rs +++ b/crates/gitlawb-node/src/api/profiles.rs @@ -98,7 +98,7 @@ pub async fn set_profile( &state.http_client, &state.config.pinata_upload_url, &state.config.pinata_jwt, - &format!("profile-{}", &did), + &format!("profile-{}", did), &profile_json, ) .await From d98e20247d1df4c58fbcc671f04fd60d3883d2a4 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Thu, 9 Jul 2026 22:52:19 +0600 Subject: [PATCH 09/14] fix(review): use runtime GITLAWB_ICAPTCHA_INSECURE flag instead of cfg(test), add mock assertions --- crates/gl/src/http.rs | 49 ++++++++++++++++++++----------- crates/icaptcha-client/src/lib.rs | 16 +++++----- 2 files changed, 40 insertions(+), 25 deletions(-) diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index ee041f9c..bf8dca7f 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -411,13 +411,14 @@ mod tests { // ── send_signed iCaptcha retry (full integration) ──────────────────── - /// Helper: set GITLAWB_ICAPTCHA_URL so the iCaptcha client trusts the mock - /// server URL and clean it up on drop. + /// Set GITLAWB_ICAPTCHA_URL and GITLAWB_ICAPTCHA_INSECURE so the iCaptcha + /// client trusts a local mockito HTTP server, cleaning them up on drop. struct IcaptchaEnv; impl IcaptchaEnv { fn new(url: &str) -> Self { std::env::set_var("GITLAWB_ICAPTCHA_URL", url); + std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", "1"); IcaptchaEnv } } @@ -425,13 +426,16 @@ mod tests { impl Drop for IcaptchaEnv { fn drop(&mut self) { std::env::remove_var("GITLAWB_ICAPTCHA_URL"); + std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"); } } - /// Helper: set up a mock iCaptcha server that responds to challenge + answer. + /// Set up a mock iCaptcha server that responds to challenge + answer. + /// The caller MUST call `.assert()` on the returned mocks after the test + /// action to verify they were actually hit by the solve loop. struct MockIcaptcha { - _c: mockito::Mock, - _a: mockito::Mock, + challenge: mockito::Mock, + answer: mockito::Mock, _guard: IcaptchaEnv, url: String, } @@ -440,7 +444,7 @@ mod tests { async fn new(server: &mut mockito::ServerGuard) -> Self { let url = server.url(); let guard = IcaptchaEnv::new(&url); - let _c = server + let challenge = server .mock("POST", "/v1/challenge") .with_status(200) .with_header("content-type", "application/json") @@ -449,7 +453,7 @@ mod tests { ) .create_async() .await; - let _a = server + let answer = server .mock("POST", "/v1/answer") .with_status(200) .with_header("content-type", "application/json") @@ -457,8 +461,8 @@ mod tests { .create_async() .await; Self { - _c, - _a, + challenge, + answer, _guard: guard, url, } @@ -469,23 +473,25 @@ mod tests { async fn send_signed_solves_icaptcha_and_retries_to_success() { let mut node = Server::new_async().await; let mut icaptcha = Server::new_async().await; - let _ic = MockIcaptcha::new(&mut icaptcha).await; + let ic = MockIcaptcha::new(&mut icaptcha).await; - let _n1 = node + let n1 = node .mock("POST", "/api/register") .with_status(403) .with_header("content-type", "application/json") - .with_header("x-icaptcha-url", &_ic.url) + .with_header("x-icaptcha-url", &ic.url) .with_header("x-icaptcha-level", "3") .with_body(r#"{"error":"icaptcha_proof_required"}"#) + .expect(1) .create_async() .await; - let _n2 = node + let n2 = node .mock("POST", "/api/register") .match_header("x-icaptcha-proof", mockito::Matcher::Any) .with_status(201) .with_header("content-type", "application/json") .with_body(r#"{"status":"created"}"#) + .expect(1) .create_async() .await; @@ -495,23 +501,29 @@ mod tests { .await .unwrap(); assert_eq!(resp.status(), 201); + n1.assert(); + n2.assert(); + ic.challenge.assert(); + ic.answer.assert(); } #[tokio::test] async fn send_signed_returns_403_after_icaptcha_retries_exhausted() { let mut node = Server::new_async().await; let mut icaptcha = Server::new_async().await; - let _ic = MockIcaptcha::new(&mut icaptcha).await; + let ic = MockIcaptcha::new(&mut icaptcha).await; // Every call to the node returns 403 with iCaptcha headers, exhausting - // MAX_ICAPTCHA_RETRIES (2). The original + 2 retries = 3 node calls. - let _n = node + // MAX_ICAPTCHA_RETRIES (2). The original + 2 retries → 3 node calls, + // each triggering a fresh challenge/answer cycle (2 cycles = 2 each). + let n = node .mock("POST", "/api/register") .with_status(403) .with_header("content-type", "application/json") - .with_header("x-icaptcha-url", &_ic.url) + .with_header("x-icaptcha-url", &ic.url) .with_header("x-icaptcha-level", "3") .with_body(r#"{"error":"icaptcha_proof_required"}"#) + .expect(3) .create_async() .await; @@ -521,5 +533,8 @@ mod tests { .await .unwrap(); assert_eq!(resp.status(), 403); + n.assert(); + ic.challenge.expect(2).assert(); + ic.answer.expect(2).assert(); } } diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index 78999a29..78174580 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -82,8 +82,9 @@ fn sanitize_excerpt(s: &str) -> String { } /// Whether `u` parses as a trusted URL (https in production; also allows -/// localhost/127.0.0.1 in tests so the full iCaptcha retry path can be -/// exercised against a mockito server). +/// http://127.0.0.1 and http://localhost when `GITLAWB_ICAPTCHA_INSECURE` is +/// set, so the full iCaptcha retry path can be exercised against a mockito +/// server in integration tests without shipping the backdoor to production). fn is_https(u: &str) -> bool { let parsed = reqwest::Url::parse(u); if parsed @@ -93,12 +94,11 @@ fn is_https(u: &str) -> bool { { return true; } - // Allow local test servers in test builds (mockito runs on 127.0.0.1:PORT). - #[cfg(test)] - if parsed - .as_ref() - .map(|p| p.host_str() == Some("127.0.0.1") || p.host_str() == Some("localhost")) - .unwrap_or(false) + if std::env::var_os("GITLAWB_ICAPTCHA_INSECURE").is_some() + && parsed + .as_ref() + .map(|p| p.host_str() == Some("127.0.0.1") || p.host_str() == Some("localhost")) + .unwrap_or(false) { return true; } From 41d277e019f1af2db73dd3ef65b4e864431958cb Mon Sep 17 00:00:00 2001 From: Gravirei Date: Thu, 9 Jul 2026 23:13:11 +0600 Subject: [PATCH 10/14] fix(review): set mock expectations at creation time, tighten insecure scheme check --- crates/gl/src/http.rs | 22 ++++++++++++---------- crates/icaptcha-client/src/lib.rs | 26 ++++++++++++-------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index bf8dca7f..d166b6b6 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -431,8 +431,8 @@ mod tests { } /// Set up a mock iCaptcha server that responds to challenge + answer. - /// The caller MUST call `.assert()` on the returned mocks after the test - /// action to verify they were actually hit by the solve loop. + /// `hits` sets the expected call count for both endpoints so the test can + /// verify the solve loop was entered the correct number of times. struct MockIcaptcha { challenge: mockito::Mock, answer: mockito::Mock, @@ -441,7 +441,7 @@ mod tests { } impl MockIcaptcha { - async fn new(server: &mut mockito::ServerGuard) -> Self { + async fn new(server: &mut mockito::ServerGuard, hits: usize) -> Self { let url = server.url(); let guard = IcaptchaEnv::new(&url); let challenge = server @@ -451,6 +451,7 @@ mod tests { .with_body( r#"{"challengeId":"c1","type":"arithmetic","difficulty":1,"prompt":"What is 1 + 1?","token":"tk1"}"#, ) + .expect(hits) .create_async() .await; let answer = server @@ -458,6 +459,7 @@ mod tests { .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"status":"passed","proof":"mock.proof"}"#) + .expect(hits) .create_async() .await; Self { @@ -473,7 +475,7 @@ mod tests { async fn send_signed_solves_icaptcha_and_retries_to_success() { let mut node = Server::new_async().await; let mut icaptcha = Server::new_async().await; - let ic = MockIcaptcha::new(&mut icaptcha).await; + let ic = MockIcaptcha::new(&mut icaptcha, 1).await; let n1 = node .mock("POST", "/api/register") @@ -511,11 +513,11 @@ mod tests { async fn send_signed_returns_403_after_icaptcha_retries_exhausted() { let mut node = Server::new_async().await; let mut icaptcha = Server::new_async().await; - let ic = MockIcaptcha::new(&mut icaptcha).await; + // MAX_ICAPTCHA_RETRIES = 2, so with every call returning 403 with + // iCaptcha headers the solve loop runs twice (2 challenge + 2 answer). + let ic = MockIcaptcha::new(&mut icaptcha, 2).await; - // Every call to the node returns 403 with iCaptcha headers, exhausting - // MAX_ICAPTCHA_RETRIES (2). The original + 2 retries → 3 node calls, - // each triggering a fresh challenge/answer cycle (2 cycles = 2 each). + // The original + 2 retries = 3 node calls before the loop gives up. let n = node .mock("POST", "/api/register") .with_status(403) @@ -534,7 +536,7 @@ mod tests { .unwrap(); assert_eq!(resp.status(), 403); n.assert(); - ic.challenge.expect(2).assert(); - ic.answer.expect(2).assert(); + ic.challenge.assert(); + ic.answer.assert(); } } diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index 78174580..2468b10e 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -81,24 +81,22 @@ fn sanitize_excerpt(s: &str) -> String { out } -/// Whether `u` parses as a trusted URL (https in production; also allows -/// http://127.0.0.1 and http://localhost when `GITLAWB_ICAPTCHA_INSECURE` is -/// set, so the full iCaptcha retry path can be exercised against a mockito -/// server in integration tests without shipping the backdoor to production). +/// Whether `u` parses as a trusted URL. +/// +/// Production trusts only `https`. When `GITLAWB_ICAPTCHA_INSECURE` is set (a +/// runtime escape hatch for integration tests) the function also trusts +/// `http://127.0.0.1` and `http://localhost` so the full iCaptcha retry path +/// can be exercised against a local mockito server. fn is_https(u: &str) -> bool { - let parsed = reqwest::Url::parse(u); - if parsed - .as_ref() - .map(|p| p.scheme() == "https") - .unwrap_or(false) - { + let Ok(parsed) = reqwest::Url::parse(u) else { + return false; + }; + if parsed.scheme() == "https" { return true; } if std::env::var_os("GITLAWB_ICAPTCHA_INSECURE").is_some() - && parsed - .as_ref() - .map(|p| p.host_str() == Some("127.0.0.1") || p.host_str() == Some("localhost")) - .unwrap_or(false) + && parsed.scheme() == "http" + && matches!(parsed.host_str(), Some("127.0.0.1" | "localhost")) { return true; } From 5698c2e4778a964a5f259f95342a9ec7d54cf0b6 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Thu, 9 Jul 2026 23:32:33 +0600 Subject: [PATCH 11/14] fix(test): serialize icaptcha integration tests with global Mutex to prevent env-var race --- crates/gl/src/http.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index d166b6b6..9bab79d0 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -191,6 +191,12 @@ mod tests { use super::*; use gitlawb_core::identity::Keypair; use mockito::Server; + use std::sync::{Mutex, MutexGuard}; + + /// Serializes the two integration tests that touch the process-global + /// `GITLAWB_ICAPTCHA_URL` / `GITLAWB_ICAPTCHA_INSECURE` env vars so they + /// never race. + static ICAPTCHA_ENV_LOCK: Mutex<()> = Mutex::new(()); fn test_keypair() -> Keypair { Keypair::generate() @@ -413,13 +419,18 @@ mod tests { /// Set GITLAWB_ICAPTCHA_URL and GITLAWB_ICAPTCHA_INSECURE so the iCaptcha /// client trusts a local mockito HTTP server, cleaning them up on drop. - struct IcaptchaEnv; + /// Holds [`ICAPTCHA_ENV_LOCK`] for its lifetime so concurrent tests don't + /// race on the process-global env vars. + struct IcaptchaEnv { + _lock: MutexGuard<'static, ()>, + } impl IcaptchaEnv { fn new(url: &str) -> Self { + let lock = ICAPTCHA_ENV_LOCK.lock().unwrap(); std::env::set_var("GITLAWB_ICAPTCHA_URL", url); std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", "1"); - IcaptchaEnv + IcaptchaEnv { _lock: lock } } } From edaf2385a5935ba51952bca43fc98fc2b9ee0e7c Mon Sep 17 00:00:00 2001 From: Gravirei Date: Fri, 10 Jul 2026 11:37:56 +0600 Subject: [PATCH 12/14] fix(icaptcha): compare origins (scheme+host+port) in resolve_solver_url; tighten proof assertion in test --- crates/gl/src/http.rs | 2 +- crates/icaptcha-client/src/lib.rs | 57 ++++++++++++++++++++++++------- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index 9bab79d0..b3820181 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -500,7 +500,7 @@ mod tests { .await; let n2 = node .mock("POST", "/api/register") - .match_header("x-icaptcha-proof", mockito::Matcher::Any) + .match_header("x-icaptcha-proof", "mock.proof") .with_status(201) .with_header("content-type", "application/json") .with_body(r#"{"status":"created"}"#) diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index 2468b10e..63f61fd3 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -103,11 +103,20 @@ fn is_https(u: &str) -> bool { false } -/// Lowercased host of a URL, if it parses and has one. -fn host_of(u: &str) -> Option { - reqwest::Url::parse(u) - .ok() - .and_then(|p| p.host_str().map(|h| h.to_ascii_lowercase())) +/// Lowercased origin string (`scheme://host[:port]`) of a URL. +/// +/// Includes the effective port so that `http://localhost:3000` and +/// `http://localhost:9000` are treated as distinct origins — a hostile node +/// cannot redirect the iCaptcha solve to a different loopback port. +fn origin_key(u: &str) -> Option { + let parsed = reqwest::Url::parse(u).ok()?; + let scheme = parsed.scheme(); + let host = parsed.host_str()?.to_ascii_lowercase(); + let port = parsed + .port_or_known_default() + .map(|p| format!(":{p}")) + .unwrap_or_default(); + Some(format!("{scheme}://{host}{port}")) } /// Decide which iCaptcha origin to actually talk to, and whether it is trusted @@ -124,12 +133,13 @@ fn host_of(u: &str) -> Option { /// exfiltrated to an origin the operator did not choose. fn resolve_solver_url(advertised: Option<&str>, operator: Option<&str>) -> (String, bool) { let operator = operator.filter(|u| is_https(u)); - let operator_host = operator.and_then(host_of); - let default_host = host_of(DEFAULT_URL); + let operator_origin = operator.as_ref().and_then(|u| origin_key(u)); + let default_origin = origin_key(DEFAULT_URL); - let allowed = |host: &Option| -> bool { - host.as_ref() - .map(|h| Some(h) == default_host.as_ref() || Some(h) == operator_host.as_ref()) + let allowed = |origin: &Option| -> bool { + origin + .as_ref() + .map(|o| Some(o) == default_origin.as_ref() || Some(o) == operator_origin.as_ref()) .unwrap_or(false) }; @@ -140,7 +150,7 @@ fn resolve_solver_url(advertised: Option<&str>, operator: Option<&str>) -> (Stri }; let chosen = match advertised { - Some(a) if is_https(a) && allowed(&host_of(a)) => a.to_string(), + Some(a) if is_https(a) && allowed(&origin_key(a)) => a.to_string(), Some(a) => { tracing::warn!( advertised = %a, @@ -152,7 +162,7 @@ fn resolve_solver_url(advertised: Option<&str>, operator: Option<&str>) -> (Stri }; // Key goes only to the operator's own configured origin. - let key_trusted = operator_host.is_some() && host_of(&chosen) == operator_host; + let key_trusted = operator_origin.is_some() && origin_key(&chosen) == operator_origin; (chosen, key_trusted) } @@ -418,4 +428,27 @@ mod tests { assert_eq!(url, DEFAULT_URL); assert!(!key_trusted); } + + #[test] + fn rejects_insecure_advertised_url_on_different_port() { + // With GITLAWB_ICAPTCHA_INSECURE=1, two loopback URLs on different + // ports must be treated as distinct origins so a hostile node cannot + // redirect the bearer key to a different listener on localhost. + struct _EnvGuard; + impl Drop for _EnvGuard { + fn drop(&mut self) { + std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"); + } + } + std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", "1"); + let _guard = _EnvGuard; + + let op = "http://localhost:3000"; + let (url, key_trusted) = resolve_solver_url(Some("http://localhost:9000"), Some(op)); + assert_eq!( + url, op, + "must fall back to operator origin, not use different port" + ); + assert!(key_trusted, "key stays trusted with operator origin"); + } } From 2f25fb3ae69ac562f1892530ee1562b04411d35f Mon Sep 17 00:00:00 2001 From: Gravirei Date: Fri, 10 Jul 2026 12:04:37 +0600 Subject: [PATCH 13/14] fix(test): save/restore inherited iCaptcha env vars instead of unconditional remove; share lock in icaptcha-client tests --- crates/gl/src/http.rs | 24 +++++++++++++++---- crates/icaptcha-client/src/lib.rs | 40 ++++++++++++++++++++++++------- 2 files changed, 52 insertions(+), 12 deletions(-) diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index b3820181..4b03602e 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -191,6 +191,7 @@ mod tests { use super::*; use gitlawb_core::identity::Keypair; use mockito::Server; + use std::ffi::OsString; use std::sync::{Mutex, MutexGuard}; /// Serializes the two integration tests that touch the process-global @@ -418,26 +419,41 @@ mod tests { // ── send_signed iCaptcha retry (full integration) ──────────────────── /// Set GITLAWB_ICAPTCHA_URL and GITLAWB_ICAPTCHA_INSECURE so the iCaptcha - /// client trusts a local mockito HTTP server, cleaning them up on drop. + /// client trusts a local mockito HTTP server, restoring any prior values on + /// drop so a test run launched with those variables keeps working. /// Holds [`ICAPTCHA_ENV_LOCK`] for its lifetime so concurrent tests don't /// race on the process-global env vars. struct IcaptchaEnv { _lock: MutexGuard<'static, ()>, + prev_url: Option, + prev_insecure: Option, } impl IcaptchaEnv { fn new(url: &str) -> Self { let lock = ICAPTCHA_ENV_LOCK.lock().unwrap(); + let prev_url = std::env::var_os("GITLAWB_ICAPTCHA_URL"); + let prev_insecure = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE"); std::env::set_var("GITLAWB_ICAPTCHA_URL", url); std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", "1"); - IcaptchaEnv { _lock: lock } + IcaptchaEnv { + _lock: lock, + prev_url, + prev_insecure, + } } } impl Drop for IcaptchaEnv { fn drop(&mut self) { - std::env::remove_var("GITLAWB_ICAPTCHA_URL"); - std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"); + match self.prev_url.take() { + Some(v) => std::env::set_var("GITLAWB_ICAPTCHA_URL", v), + None => std::env::remove_var("GITLAWB_ICAPTCHA_URL"), + } + match self.prev_insecure.take() { + Some(v) => std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", v), + None => std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"), + } } } diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index 63f61fd3..788c0e3d 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -345,6 +345,37 @@ fn interactive_prompt(challenge: &Challenge) -> Option { #[cfg(test)] mod tests { use super::*; + use std::ffi::OsString; + use std::sync::{Mutex, MutexGuard}; + + /// Serializes tests that touch the process-global + /// `GITLAWB_ICAPTCHA_INSECURE` env var so they never race. + static ICAPTCHA_ENV_LOCK: Mutex<()> = Mutex::new(()); + + /// Set `GITLAWB_ICAPTCHA_INSECURE` for the test lifetime, restoring any + /// prior value on drop. + struct InsecureEnv { + _lock: MutexGuard<'static, ()>, + prev: Option, + } + + impl InsecureEnv { + fn new() -> Self { + let lock = ICAPTCHA_ENV_LOCK.lock().unwrap(); + let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE"); + std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", "1"); + InsecureEnv { _lock: lock, prev } + } + } + + impl Drop for InsecureEnv { + fn drop(&mut self) { + match self.prev.take() { + Some(v) => std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", v), + None => std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"), + } + } + } // ── P1: hostile error bodies must not reach the terminal raw ────────── @@ -434,14 +465,7 @@ mod tests { // With GITLAWB_ICAPTCHA_INSECURE=1, two loopback URLs on different // ports must be treated as distinct origins so a hostile node cannot // redirect the bearer key to a different listener on localhost. - struct _EnvGuard; - impl Drop for _EnvGuard { - fn drop(&mut self) { - std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"); - } - } - std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", "1"); - let _guard = _EnvGuard; + let _env = InsecureEnv::new(); let op = "http://localhost:3000"; let (url, key_trusted) = resolve_solver_url(Some("http://localhost:9000"), Some(op)); From 58769e7dd475c50140b2de85b2d47041bbb92a30 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Fri, 10 Jul 2026 13:46:51 +0600 Subject: [PATCH 14/14] fix(test): constrain negative send_once mocks with Matcher::Missing so they actually protect the contract --- crates/gl/src/http.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/gl/src/http.rs b/crates/gl/src/http.rs index 4b03602e..dc9f45da 100644 --- a/crates/gl/src/http.rs +++ b/crates/gl/src/http.rs @@ -306,6 +306,7 @@ mod tests { let mut server = Server::new_async().await; let m = server .mock("POST", "/api/test") + .match_header("x-icaptcha-proof", mockito::Matcher::Missing) .with_status(200) .with_body("ok") .create_async() @@ -345,6 +346,9 @@ mod tests { let mut server = Server::new_async().await; let m = server .mock("POST", "/api/test") + .match_header("Signature", mockito::Matcher::Missing) + .match_header("Signature-Input", mockito::Matcher::Missing) + .match_header("Content-Digest", mockito::Matcher::Missing) .with_status(200) .with_body("ok") .create_async()