From a7257dce2088773ba3d3ffa1006cd83f312f1678 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 7 Sep 2026 20:19:17 -0500 Subject: [PATCH 1/2] fix(node): hold fork to the same repo-name rules as every other creation route fork_repo built its clone destination with store::repo_disk_path, a raw join with no validation, while create_repo and the sync mirror path both go through validated_repo_disk_path. Its own name check is `.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_')`, which is Unicode-aware and has no length or leading-character rule, so it admits four shapes validate_repo_name refuses: the empty string (all() is vacuously true on it), a non-ASCII alphanumeric such as "cafe" with an acute e, a leading '-', and a name over the 100-char bound. The empty name was the reachable defect. Driven through the production router with a real Ed25519 signature, a `{"name":""}` fork returned 201 Created with a repo row whose name is "" and a clone_url ending in `/.git`, materialized at `//.git`. It now returns 400 and creates neither the row nor the directory. This is a deliberate narrowing, not only an empty-name fix: the other three shapes above are now 400 on fork where they previously reached the raw join. None of them could ever have been created through create_repo, which fails at repo_store.init on the same validator, and no existing row can carry such a name (create_repo validates before its DB insert, and mirror rows go through validate_repo_slug), so no repo becomes unforkable. A fork that defaults to source.name is unaffected. The traversal shape of the same join is NOT reachable and is not what this changes: forker_did comes from AuthenticatedDid, which auth/mod.rs inserts only after Did::to_verifying_key resolves a multibase-decodable did:key, so it can carry neither ".." nor "/". repo_disk_path is now cfg(test). It has no production caller left, and the attribute turns the next one into a compile error rather than a silent bypass. Tests keep it because a fixture that must escape repos_dir cannot be built with the validated form. Two tests. A unit test on the mounted handler, and an end-to-end test through server::build_router with real signatures that covers all four refused shapes plus a positive control asserting an ordinary name still forks to disk and to a row. Both are mutation-verified in both directions: reverting to the raw join reddens the refusals, and forcing fork to always refuse reddens the control. Closed #272 fixed this class on the sync route and pointed at repo_disk_path as the sanitizing convention; it is in fact the unvalidated join, and the fork route was never scoped. --- crates/gitlawb-node/src/api/repos.rs | 12 +- crates/gitlawb-node/src/git/store.rs | 16 +- crates/gitlawb-node/src/test_support.rs | 226 ++++++++++++++++++++++++ 3 files changed, 251 insertions(+), 3 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 4e327c42..86130104 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -3064,7 +3064,17 @@ pub async fn fork_repo( .await .map_err(|e| AppError::Git(e.to_string()))?; - let disk_path = store::repo_disk_path(&state.config.repos_dir, &forker_did, &fork_name); + // Same barrier every other repo-creation route goes through. The character + // allowlist above is vacuously true on an empty name, and `repo_disk_path` + // is a raw join, so without this a `{"name":""}` fork lands a row with an + // empty name at `//.git`. Closed #272 fixed this + // class on the sync route and never scoped fork. + let disk_path = crate::git::repo_store::validated_repo_disk_path( + &state.config.repos_dir, + &forker_did, + &fork_name, + ) + .map_err(|e| AppError::BadRequest(e.to_string()))?; // Clone the source repo as a mirror let output = std::process::Command::new("git") diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 5617b419..65f02a8d 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -1,5 +1,5 @@ use anyhow::{bail, Context, Result}; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::process::Command; /// Initialize a new bare git repository with SHA-1 object format (default). @@ -870,7 +870,19 @@ pub fn merge_branch( } /// Resolve a repo disk path: {repos_dir}/{owner_slug}/{repo_name}.git -pub fn repo_disk_path(repos_dir: &Path, owner_did: &str, repo_name: &str) -> PathBuf { +/// +/// UNVALIDATED, and test-only for that reason. It performs the bare join with no +/// allowlist, no containment check and no component walk, so it accepts an empty +/// or traversal-bearing name. Production code must use +/// `git::repo_store::validated_repo_disk_path`; the last production caller was +/// `fork_repo`, which used this and turned a `{"name":""}` request into a repo +/// row at `//.git`. The `cfg(test)` is the guard: a new +/// production caller fails to compile rather than silently skipping the barrier. +/// +/// Tests keep it because a fixture that must escape `repos_dir` cannot be built +/// with the validated form. +#[cfg(test)] +pub fn repo_disk_path(repos_dir: &Path, owner_did: &str, repo_name: &str) -> std::path::PathBuf { // Sanitize the DID for use as a directory name let owner_slug = owner_did.replace([':', '/'], "_"); repos_dir.join(owner_slug).join(format!("{repo_name}.git")) diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 430c0600..8b760fc7 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -557,6 +557,232 @@ mod tests { ); } + /// A fork name that is empty passes `fork_repo`'s character allowlist + /// (`.chars().all(..)` is vacuously true on ""), and the handler then builds + /// its clone destination with the RAW `store::repo_disk_path`. Every other + /// repo-creation route goes through `validated_repo_disk_path`, whose + /// `validate_repo_name` rejects an empty name, so fork is the one entrypoint + /// that skips that barrier and lands a repo at `//.git` + /// instead of `//.git`. + /// + /// Closed #272 fixed this class on the sync route and named + /// `repo_disk_path` as the sanitizing convention; it is in fact the + /// unvalidated join, and the fork route was never scoped. + #[sqlx::test] + async fn fork_rejects_an_empty_name(pool: PgPool) { + // `repo_store::for_testing` pins its on-disk root to /tmp, so the config + // the handler reads at the raw join must name the same root or the two + // halves of this test look at different directories. + let owner = "did:key:zFORKEMPTYNAMEAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let owner_slug = owner.replace([':', '/'], "_"); + let repos_dir = std::path::PathBuf::from("/tmp"); + let owner_dir = repos_dir.join(&owner_slug); + let _cleanup = OwnerDirGuard(owner_dir.clone()); + let _ = std::fs::remove_dir_all(&owner_dir); + + let state = test_state_with(pool, |cfg| cfg.repos_dir = repos_dir.clone()).await; + let repo = seed_repo(owner, "source-repo"); + state.db.create_repo(&repo).await.expect("seed repo"); + + // Put the source where `repo_store.acquire` reads, so the handler gets + // past acquire and the empty name is what this test measures. + let source_path = crate::git::store::repo_disk_path(&repos_dir, owner, "source-repo"); + std::fs::create_dir_all(&owner_dir).expect("owner dir"); + crate::git::store::init_bare(&source_path).expect("init source repo"); + + 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}/source-repo/fork"); + let resp = router + .oneshot(signed_request_as( + owner, + Method::POST, + &uri, + Body::from(r#"{"name":""}"#), + )) + .await + .unwrap(); + + let status = resp.status(); + let body = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("read fork response body"); + let body = String::from_utf8_lossy(&body); + assert_eq!( + status, + StatusCode::BAD_REQUEST, + "an empty fork name must be refused, not turned into a '.git' path; body={body}" + ); + + // The barrier's real job: nothing may be created at /.git. + assert!( + !owner_dir.join(".git").exists(), + "an empty fork name must not materialize //.git" + ); + let owner_short = owner.split(':').next_back().unwrap(); + assert!( + state + .db + .get_repo(owner_short, "") + .await + .expect("get_repo") + .is_none(), + "no repo row may be created for a refused fork" + ); + } + + /// End-to-end companion to `fork_rejects_an_empty_name`, through the + /// PRODUCTION router with REAL Ed25519 signatures rather than an injected + /// `AuthenticatedDid`. `signed_request_as` only sets the extension, so the + /// unit test above proves the handler but says nothing about the request + /// actually clearing `require_signature` / `require_ucan_chain` and the + /// creation-route rate limiter that `build_router` puts in front of fork. + /// + /// It also pins the FULL set the barrier refuses, not just the empty name. + /// The handler's own allowlist is `char::is_alphanumeric`, which is + /// Unicode-aware and has no length or leading-character rule, so it admits + /// "", "cafe\u{301}"-style names, a leading '-', and a 101-char name. + /// `validate_repo_name` is ASCII-only and bounded, so routing fork through + /// the shared barrier narrows all five at once. The positive control is what + /// keeps that from being a blanket refusal: an ordinary name must still fork. + #[sqlx::test] + async fn fork_name_rules_match_the_shared_barrier_e2e(pool: PgPool) { + use gitlawb_core::http_sig::sign_request; + use gitlawb_core::identity::Keypair; + + let kp = Keypair::generate(); + let owner_did = kp.did().to_string(); + let short = owner_did.split(':').next_back().unwrap().to_string(); + + // repo_store::for_testing pins its root to /tmp; the handler reads + // config.repos_dir at the join, so both must name the same root. + let repos_dir = std::path::PathBuf::from("/tmp"); + let owner_slug = owner_did.replace([':', '/'], "_"); + let owner_dir = repos_dir.join(&owner_slug); + let _cleanup = OwnerDirGuard(owner_dir.clone()); + + let state = test_state_with(pool, |cfg| cfg.repos_dir = repos_dir.clone()).await; + let source = seed_repo(&owner_did, "source-repo"); + state + .db + .create_repo(&source) + .await + .expect("seed source repo"); + + let source_path = crate::git::store::repo_disk_path(&repos_dir, &owner_did, "source-repo"); + std::fs::create_dir_all(&owner_dir).expect("owner dir"); + crate::git::store::init_bare(&source_path).expect("init source repo"); + + let path = format!("/api/v1/repos/{short}/source-repo/fork"); + let long_name = "x".repeat(101); + + // Every name the handler's own allowlist admits but the shared barrier + // refuses. Each must now be a 400 through the real stack. + let refused: [(&str, &str); 4] = [ + ("", "empty"), + ("caf\u{e9}", "non-ascii alphanumeric"), + ("-lead", "leading hyphen"), + (long_name.as_str(), "over the 100-char bound"), + ]; + for (name, why) in refused { + let body = serde_json::to_vec(&serde_json::json!({ "name": name })).unwrap(); + let signed = sign_request(&kp, "POST", &path, &body); + let req = Request::builder() + .method(Method::POST) + .uri(&path) + .header("content-type", "application/json") + .header("content-digest", signed.content_digest) + .header("signature-input", signed.signature_input) + .header("signature", signed.signature) + .body(Body::from(body)) + .unwrap(); + let resp = crate::server::build_router(state.clone()) + .oneshot(req) + .await + .unwrap(); + let status = resp.status(); + let rb = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let rb = String::from_utf8_lossy(&rb); + assert_eq!( + status, + StatusCode::BAD_REQUEST, + "a fork name that is {why} must be refused through the real router; body={rb}" + ); + } + + // The specific artifact the empty name used to leave behind. + assert!( + !owner_dir.join(".git").exists(), + "no fork may materialize //.git" + ); + assert!( + state + .db + .get_repo(&short, "") + .await + .expect("get_repo") + .is_none(), + "no repo row may be created for a refused fork" + ); + + // Positive control: an ordinary name still forks, all the way to disk + // and a row. Without this the four assertions above would also pass if + // the barrier rejected everything. + let body = serde_json::to_vec(&serde_json::json!({ "name": "forked-ok" })).unwrap(); + let signed = sign_request(&kp, "POST", &path, &body); + let req = Request::builder() + .method(Method::POST) + .uri(&path) + .header("content-type", "application/json") + .header("content-digest", signed.content_digest) + .header("signature-input", signed.signature_input) + .header("signature", signed.signature) + .body(Body::from(body)) + .unwrap(); + let resp = crate::server::build_router(state.clone()) + .oneshot(req) + .await + .unwrap(); + let status = resp.status(); + let rb = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let rb = String::from_utf8_lossy(&rb); + assert_eq!( + status, + StatusCode::CREATED, + "an ordinary fork name must still succeed through the real router; body={rb}" + ); + assert!( + owner_dir.join("forked-ok.git").is_dir(), + "the accepted fork must exist on disk at //forked-ok.git" + ); + assert!( + state + .db + .get_repo(&short, "forked-ok") + .await + .expect("get_repo") + .is_some(), + "the accepted fork must have a repo row" + ); + } + + /// Fixed-path temp cleanup that survives a panic (the suite's convention; + /// `TempDir` cannot own a path this test must compute up front). + struct OwnerDirGuard(std::path::PathBuf); + impl Drop for OwnerDirGuard { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + /// 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 02fba70bc6a1067f5c3f0936e689dee2c3548078 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:54:52 -0500 Subject: [PATCH 2/2] fix(node): validate the fork name before spending the proof The barrier landed after `proof.consume` and after `repo_store.acquire`, so a name the character allowlist admits but `validate_repo_name` refuses burned a valid iCaptcha proof and paid a source-repo download before its 400. The handler's own header comment promises the opposite: a fork rejected for a bad name never burns a proof. Reported by CodeRabbit on the review of a7257dce. It now runs with the other admissibility checks, above both. The regression test pins the ORDER without iCaptcha plumbing. Seed a repo row whose bytes are not on disk: late validation lets the acquire fail first and the caller sees a 500 from git, so only early validation can produce the 400 the test asserts. Verified load-bearing by degrading the early call to `unwrap_or_else`, which reddens it. Both name rules are kept. CodeRabbit also suggested deleting the character allowlist as a strict subset of `validate_repo_name`, and that is not the relation between them: the allowlist rejects a dot, so `v1.2.3` and `my.repo` are refused today while `validate_repo_name` accepts both, and the allowlist accepts a non-ASCII alphanumeric that `validate_repo_name` rejects. Removing it would newly admit dotted fork names, which is a behaviour change and not a cleanup. A comment now records why the two coexist. The empty-name fixture also generates its owner DID per run. It writes to a fixed `/tmp/`, so a shared constant let two concurrent `cargo test` processes delete each other's repo mid-test (reported by Greptile). --- crates/gitlawb-node/src/api/repos.rs | 31 ++++++++----- crates/gitlawb-node/src/test_support.rs | 62 ++++++++++++++++++++++++- 2 files changed, 80 insertions(+), 13 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 86130104..bb4cfa18 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -3046,6 +3046,25 @@ pub async fn fork_repo( )); } + // Same barrier every other repo-creation route goes through. The character + // allowlist above is vacuously true on an empty name, and `repo_disk_path` + // is a raw join, so without this a `{"name":""}` fork lands a row with an + // empty name at `//.git`. Closed #272 fixed this + // class on the sync route and never scoped fork. + // + // It runs with the other admissibility checks, above the proof spend and the + // source acquire, because the header comment promises that a fork rejected + // for a bad name never burns a valid proof. The two name rules are kept + // separate on purpose: neither is a subset of the other, since the allowlist + // above rejects a dot (`v1.2.3`) that `validate_repo_name` accepts, and + // accepts a non-ASCII alphanumeric that it rejects. + let disk_path = crate::git::repo_store::validated_repo_disk_path( + &state.config.repos_dir, + &forker_did, + &fork_name, + ) + .map_err(|e| AppError::BadRequest(e.to_string()))?; + // Check no name conflict under the forker's ownership let forker_short = crate::db::normalize_owner_key(&forker_did); if state.db.get_repo(forker_short, &fork_name).await?.is_some() { @@ -3064,18 +3083,6 @@ pub async fn fork_repo( .await .map_err(|e| AppError::Git(e.to_string()))?; - // Same barrier every other repo-creation route goes through. The character - // allowlist above is vacuously true on an empty name, and `repo_disk_path` - // is a raw join, so without this a `{"name":""}` fork lands a row with an - // empty name at `//.git`. Closed #272 fixed this - // class on the sync route and never scoped fork. - let disk_path = crate::git::repo_store::validated_repo_disk_path( - &state.config.repos_dir, - &forker_did, - &fork_name, - ) - .map_err(|e| AppError::BadRequest(e.to_string()))?; - // Clone the source repo as a mirror let output = std::process::Command::new("git") .args([ diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 8b760fc7..fc6031e6 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -573,7 +573,17 @@ mod tests { // `repo_store::for_testing` pins its on-disk root to /tmp, so the config // the handler reads at the raw join must name the same root or the two // halves of this test look at different directories. - let owner = "did:key:zFORKEMPTYNAMEAAAAAAAAAAAAAAAAAAAAAAAAAA"; + // A per-run owner: the fixture path is a fixed /tmp/, so a shared + // constant lets two concurrent `cargo test` PROCESSES delete each + // other's repo mid-test. + let owner = format!( + "did:key:zFORKEMPTY{}", + gitlawb_core::identity::Keypair::generate() + .did() + .to_string() + .replace("did:key:", "") + ); + let owner = owner.as_str(); let owner_slug = owner.replace([':', '/'], "_"); let repos_dir = std::path::PathBuf::from("/tmp"); let owner_dir = repos_dir.join(&owner_slug); @@ -774,6 +784,56 @@ mod tests { ); } + /// The name barrier must run BEFORE the proof spend and the source acquire. + /// `fork_repo`'s header comment promises that a fork rejected for a bad name + /// never burns a valid proof, and the first version of this fix validated + /// after both, so an empty name paid a Tigris download and spent the proof + /// before its 400. + /// + /// The seam that pins the order without iCaptcha plumbing: seed a repo ROW + /// whose bytes are not on disk. If validation still ran after the acquire, + /// the acquire fails first and the caller sees a 500 from git. A 400 proves + /// the refusal came first. + #[sqlx::test] + async fn fork_refuses_a_bad_name_before_acquiring_the_source(pool: PgPool) { + let owner = "did:key:zFORKORDERAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let repos_dir = std::path::PathBuf::from("/tmp"); + let state = test_state_with(pool, |cfg| cfg.repos_dir = repos_dir.clone()).await; + + // Row only. Nothing is written to disk, so `repo_store.acquire` cannot + // succeed and any 500 here means validation ran too late. + let repo = seed_repo(owner, "absent-source"); + state.db.create_repo(&repo).await.expect("seed source row"); + + 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}/absent-source/fork"); + let resp = router + .oneshot(signed_request_as( + owner, + Method::POST, + &uri, + Body::from(r#"{"name":""}"#), + )) + .await + .unwrap(); + + let status = resp.status(); + let body = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("read body"); + let body = String::from_utf8_lossy(&body); + assert_eq!( + status, + StatusCode::BAD_REQUEST, + "a bad fork name must be refused before the source acquire, not after; body={body}" + ); + } + /// Fixed-path temp cleanup that survives a panic (the suite's convention; /// `TempDir` cannot own a path this test must compute up front). struct OwnerDirGuard(std::path::PathBuf);