diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 4e327c42..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,8 +3083,6 @@ 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); - // Clone the source repo as a mirror let output = std::process::Command::new("git") .args([ 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..fc6031e6 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -557,6 +557,292 @@ 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. + // 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); + 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" + ); + } + + /// 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); + 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]