diff --git a/Cargo.lock b/Cargo.lock index 5b23fef..9c00a72 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -289,6 +289,15 @@ dependencies = [ "crossbeam-utils", ] +[[package]] +name = "crossbeam-epoch" +version = "0.9.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +dependencies = [ + "crossbeam-utils", +] + [[package]] name = "crossbeam-utils" version = "0.8.21" @@ -659,11 +668,11 @@ name = "gitks" version = "1.0.0" dependencies = [ "async-trait", - "clru", "dotenvy", "duct", "gix", "gix-archive", + "moka", "prost", "prost-types", "ractor", @@ -1956,6 +1965,23 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "moka" +version = "0.12.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "957228ad12042ee839f93c8f257b62b4c0ab5eaae1d4fa60de53b27c9d7c5046" +dependencies = [ + "crossbeam-channel", + "crossbeam-epoch", + "crossbeam-utils", + "equivalent", + "parking_lot", + "portable-atomic", + "smallvec", + "tagptr", + "uuid", +] + [[package]] name = "multimap" version = "0.10.1" @@ -2727,6 +2753,12 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0bf256ce5efdfa370213c1dabab5935a12e49f2c58d15e9eac2870d3b4f27263" +[[package]] +name = "tagptr" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7b2093cf4c8eb1e67749a6762251bc9cd836b6fc171623bd0a9d324d37af2417" + [[package]] name = "tar" version = "0.4.46" @@ -3103,6 +3135,17 @@ version = "0.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8ecb6da28b8a351d773b68d5825ac39017e680750f980f3a1a85cd8dd28a47c1" +[[package]] +name = "uuid" +version = "1.23.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d258b83ceec21034727ecee8c382cfa6c3e133699b0742c64571814fb420c9f7" +dependencies = [ + "getrandom 0.4.2", + "js-sys", + "wasm-bindgen", +] + [[package]] name = "valuable" version = "0.1.1" diff --git a/Cargo.toml b/Cargo.toml index 2c34607..d981218 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,7 +16,7 @@ documentation = "" path = "lib.rs" name = "gitks" [dependencies] -clru = "0.6" +moka = { version = "0.12", default-features = false, features = ["sync"] } serde = { version = "1", features = ["derive"] } gix = { version = "0.84", default-features = false, features = ["serde", "blame", "sha256", "sha1", "tracing", "merge", "max-performance-safe", "revision"] } gix-archive = { version = "0.33", features = ["sha256","sha1","document-features"] } diff --git a/actor/handler.rs b/actor/handler.rs index f338a05..38715de 100644 --- a/actor/handler.rs +++ b/actor/handler.rs @@ -1,9 +1,11 @@ -use std::collections::HashMap; +use crate::actor::message::{ + GitNodeMessage, NodeHealth, ROLE_PRIMARY, ROLE_REPLICA, RefUpdateEvent, RouteDecision, +}; +use crate::server::GitksService; use async_trait::async_trait; use ractor::pg; use ractor::{Actor, ActorProcessingErr, ActorRef, SupervisionEvent}; -use crate::actor::message::{GitNodeMessage, NodeHealth, RefUpdateEvent, RouteDecision, ROLE_PRIMARY, ROLE_REPLICA}; -use crate::server::GitksService; +use std::collections::HashMap; #[derive(Clone)] pub struct GitNodeActor { @@ -50,7 +52,11 @@ impl Actor for GitNodeActor { ) -> Result { let actor_name = format!("git_node_{}", args.storage_name); pg::join("gitks_nodes".to_string(), vec![myself.get_cell()]); - pg::join_scoped(args.storage_name.clone(), "node".to_string(), vec![myself.get_cell()]); + pg::join_scoped( + args.storage_name.clone(), + "node".to_string(), + vec![myself.get_cell()], + ); tracing::info!(storage_name = %args.storage_name, actor_name = %actor_name, grpc_addr = %args.grpc_addr, "GitNodeActor started"); Ok(GitNodeState { storage_name: args.storage_name, @@ -90,43 +96,60 @@ impl Actor for GitNodeActor { } GitNodeMessage::RefUpdated(event) => { - if let Some(entry) = state.repos.get(&event.relative_path) { - if entry.role == ROLE_REPLICA { - let local_path = self.service.repo_prefix.join(&event.relative_path); - crate::actor::sync::sync_from_primary(event, local_path).await; - } + if let Some(entry) = state.repos.get(&event.relative_path) + && entry.role == ROLE_REPLICA + { + let local_path = self.service.repo_prefix.join(&event.relative_path); + crate::actor::sync::sync_from_primary(event, local_path).await; } } GitNodeMessage::FindPrimary(header, reply) => { let entry = state.repos.get(&header.relative_path); let is_primary = entry.is_some_and(|e| e.role == ROLE_PRIMARY); - reply.send(build_decision(state, &header, is_primary, entry.map(|e| e.role.as_str()))).ok(); + reply + .send(build_decision( + state, + &header, + is_primary, + entry.map(|e| e.role.as_str()), + )) + .ok(); } GitNodeMessage::FindReplica(header, reply) => { let entry = state.repos.get(&header.relative_path); let has = entry.is_some(); - reply.send(build_decision(state, &header, has, entry.map(|e| e.role.as_str()))).ok(); + reply + .send(build_decision( + state, + &header, + has, + entry.map(|e| e.role.as_str()), + )) + .ok(); } - GitNodeMessage::ListRepositoryPaths(reply) => { let paths: Vec = state.repos.keys().cloned().collect(); reply.send(paths.join("\n")).ok(); } GitNodeMessage::RepositoryExists(header, reply) => { - reply.send(state.repos.contains_key(&header.relative_path)).ok(); + reply + .send(state.repos.contains_key(&header.relative_path)) + .ok(); } GitNodeMessage::GetNodeHealth(reply) => { - reply.send(NodeHealth { - storage_name: state.storage_name.clone(), - repo_count: state.repos.len() as u64, - healthy: true, - version: self.version.clone(), - }).ok(); + reply + .send(NodeHealth { + storage_name: state.storage_name.clone(), + repo_count: state.repos.len() as u64, + healthy: true, + version: self.version.clone(), + }) + .ok(); } } Ok(()) @@ -139,14 +162,18 @@ impl Actor for GitNodeActor { _state: &mut Self::State, ) -> Result<(), ActorProcessingErr> { match evt { - SupervisionEvent::ActorStarted(who) => tracing::debug!(actor = ?who.get_id(), "child started"), + SupervisionEvent::ActorStarted(who) => { + tracing::debug!(actor = ?who.get_id(), "child started") + } SupervisionEvent::ActorTerminated(who, _, reason) => { tracing::warn!(actor = ?who.get_id(), reason = ?reason, "child terminated") } SupervisionEvent::ActorFailed(who, panic_msg) => { tracing::error!(actor = ?who.get_id(), msg = %panic_msg, "child panicked") } - SupervisionEvent::ProcessGroupChanged(group) => tracing::info!(group = ?group, "PG membership changed"), + SupervisionEvent::ProcessGroupChanged(group) => { + tracing::info!(group = ?group, "PG membership changed") + } _ => {} } Ok(()) @@ -162,48 +189,83 @@ impl Actor for GitNodeActor { } } -fn build_decision(state: &GitNodeState, header: &crate::pb::RepositoryHeader, found: bool, role: Option<&str>) -> RouteDecision { +fn build_decision( + state: &GitNodeState, + header: &crate::pb::RepositoryHeader, + found: bool, + role: Option<&str>, +) -> RouteDecision { RouteDecision { found, - storage_name: if found { state.storage_name.clone() } else { String::new() }, + storage_name: if found { + state.storage_name.clone() + } else { + String::new() + }, relative_path: header.relative_path.clone(), - actor_name: if found { state.actor_name.clone() } else { String::new() }, - grpc_addr: if found { state.grpc_addr.clone() } else { String::new() }, + actor_name: if found { + state.actor_name.clone() + } else { + String::new() + }, + grpc_addr: if found { + state.grpc_addr.clone() + } else { + String::new() + }, role: role.unwrap_or("").to_string(), } } -fn register_repo(myself: &ActorRef, state: &mut GitNodeState, relative_path: String) { +fn register_repo( + myself: &ActorRef, + state: &mut GitNodeState, + relative_path: String, +) { if state.repos.contains_key(&relative_path) { return; } - let role = if is_path_registered_elsewhere(&state.storage_name, &relative_path) { + // Determine role based on cluster state + // For simplicity and correctness, we use a conservative approach: + // If there are other nodes in the cluster, register as replica initially. + // The route_repository logic will determine the actual primary at query time. + let members = ractor::pg::get_members(&"gitks_nodes".to_string()); + let my_cell = myself.get_cell(); + let other_nodes_exist = members.iter().any(|m| m != &my_cell); + + let role = if other_nodes_exist { + // Conservative: assume another node might be primary + // The actual primary will be determined by route_repository query ROLE_REPLICA.to_string() } else { + // We're the only node, so we're primary ROLE_PRIMARY.to_string() }; let category = extract_category(&relative_path); - pg::join_scoped(state.storage_name.clone(), category.to_string(), vec![myself.get_cell()]); - state.repos.insert(relative_path.clone(), RepoEntry { - role: role.clone(), - last_commit: String::new(), - }); + pg::join_scoped( + state.storage_name.clone(), + category.to_string(), + vec![myself.get_cell()], + ); + state.repos.insert( + relative_path.clone(), + RepoEntry { + role: role.clone(), + last_commit: String::new(), + }, + ); tracing::info!( storage_name = %state.storage_name, category = %category, relative_path = %relative_path, actor_name = %state.actor_name, role = %role, - "repository route registered" + "repository route registered (role will be refined at query time)" ); } -fn is_path_registered_elsewhere(_storage_name: &str, _relative_path: &str) -> bool { - false -} - fn extract_category(relative_path: &str) -> &str { relative_path.split('/').next().unwrap_or("root") } @@ -217,8 +279,12 @@ pub async fn start_node_actor( let (actor_ref, handle) = Actor::spawn( Some(format!("git_node_{storage_name}")), actor, - GitNodeArgs { storage_name, grpc_addr }, - ).await?; + GitNodeArgs { + storage_name, + grpc_addr, + }, + ) + .await?; actor_ref.cast(GitNodeMessage::ScanAndRegister).ok(); Ok((actor_ref, handle)) } @@ -239,13 +305,12 @@ pub fn list_all_groups() -> Vec { pg::which_groups() } -pub fn broadcast_ref_update( - _node_actor: &ActorRef, - event: RefUpdateEvent, -) { +pub fn broadcast_ref_update(_node_actor: &ActorRef, event: RefUpdateEvent) { let members = ractor::pg::get_members(&"gitks_nodes".to_string()); for member in members { let actor_ref: ActorRef = member.into(); - actor_ref.cast(GitNodeMessage::RefUpdated(event.clone())).ok(); + actor_ref + .cast(GitNodeMessage::RefUpdated(event.clone())) + .ok(); } } diff --git a/actor/message.rs b/actor/message.rs index 2d8869d..e223437 100644 --- a/actor/message.rs +++ b/actor/message.rs @@ -1,7 +1,7 @@ +use crate::pb::RepositoryHeader; use ractor::RpcReplyPort; use ractor_cluster::BytesConvertable; use ractor_cluster::RactorClusterMessage; -use crate::pb::RepositoryHeader; impl BytesConvertable for RepositoryHeader { fn into_bytes(self) -> Vec { @@ -73,7 +73,10 @@ impl BytesConvertable for NodeHealth { let values = decode_strings(bytes); Self { storage_name: values.first().cloned().unwrap_or_default(), - repo_count: values.get(1).and_then(|v| v.parse().ok()).unwrap_or_default(), + repo_count: values + .get(1) + .and_then(|v| v.parse().ok()) + .unwrap_or_default(), healthy: values.get(2).is_some_and(|v| v == "1"), version: values.get(3).cloned().unwrap_or_default(), } @@ -156,17 +159,69 @@ fn encode_strings(values: &[String]) -> Vec { buf } +// Maximum allowed length for a single string in the message +const MAX_STRING_LEN: usize = 10 * 1024 * 1024; // 10MB +// Maximum total message size +const MAX_TOTAL_SIZE: usize = 50 * 1024 * 1024; // 50MB + fn decode_strings(bytes: Vec) -> Vec { let mut values = Vec::new(); let mut offset = 0; + + // Check total message size + if bytes.len() > MAX_TOTAL_SIZE { + tracing::warn!( + total = bytes.len(), + max = MAX_TOTAL_SIZE, + "message exceeds maximum size, truncating" + ); + return values; + } + while offset + 8 <= bytes.len() { - let len = u64::from_be_bytes(bytes[offset..offset + 8].try_into().unwrap()) as usize; - offset += 8; - if offset + len > bytes.len() { + let len_bytes: [u8; 8] = bytes[offset..offset + 8].try_into().unwrap_or([0u8; 8]); + let len_u64 = u64::from_be_bytes(len_bytes); + + // Prevent DoS via extremely large length values + if len_u64 > MAX_STRING_LEN as u64 { + tracing::warn!( + offset, + claimed_len = len_u64, + max = MAX_STRING_LEN, + "string length exceeds maximum, stopping decode" + ); break; } - values.push(String::from_utf8_lossy(&bytes[offset..offset + len]).into_owned()); - offset += len; + + let len = len_u64 as usize; + offset += 8; + + // Prevent integer overflow in offset calculation + let end_offset = match offset.checked_add(len) { + Some(end) => end, + None => { + tracing::warn!( + offset, + len, + "integer overflow in offset calculation, stopping decode" + ); + break; + } + }; + + if len == 0 || end_offset > bytes.len() { + // Invalid length — stop decoding, return what we have so far + tracing::warn!( + offset, + claimed_len = len, + total = bytes.len(), + "malformed bytes in decode_strings, stopping early" + ); + break; + } + + values.push(String::from_utf8_lossy(&bytes[offset..end_offset]).into_owned()); + offset = end_offset; } values } diff --git a/actor/mod.rs b/actor/mod.rs index b7c687e..8e9f52d 100644 --- a/actor/mod.rs +++ b/actor/mod.rs @@ -1,8 +1,14 @@ -pub mod message; pub mod handler; +pub mod message; pub mod server; pub mod sync; -pub use handler::{GitNodeActor, GitNodeArgs, RepoEntry, start_node_actor, get_cluster_nodes, get_category_members, route_group_for, list_all_groups, broadcast_ref_update}; +pub use handler::{ + GitNodeActor, GitNodeArgs, RepoEntry, broadcast_ref_update, get_category_members, + get_cluster_nodes, list_all_groups, route_group_for, start_node_actor, +}; +pub use message::{ + GitNodeMessage, NodeHealth, ROLE_PRIMARY, ROLE_REPLICA, RefUpdateEvent, RepoActorMessage, + RouteDecision, +}; pub use server::init_actor_cluster; -pub use message::{GitNodeMessage, NodeHealth, RefUpdateEvent, RepoActorMessage, RouteDecision, ROLE_PRIMARY, ROLE_REPLICA}; diff --git a/actor/server.rs b/actor/server.rs index 009f9c8..61776b9 100644 --- a/actor/server.rs +++ b/actor/server.rs @@ -1,7 +1,7 @@ -use ractor::ActorRef; use crate::actor::handler::start_node_actor; use crate::actor::message::GitNodeMessage; use crate::server::GitksService; +use ractor::ActorRef; pub async fn init_actor_cluster( service: GitksService, diff --git a/actor/sync.rs b/actor/sync.rs index d7ed47b..db5efc9 100644 --- a/actor/sync.rs +++ b/actor/sync.rs @@ -1,6 +1,6 @@ -use std::path::PathBuf; use crate::actor::message::RefUpdateEvent; use crate::pb::Oid; +use std::path::{Path, PathBuf}; pub struct BundleApplicator { pub repo_path: PathBuf, @@ -13,7 +13,13 @@ impl BundleApplicator { pub fn apply_bundle(&self, data: &[u8]) -> Result<(), String> { let mut child = std::process::Command::new("git") - .args(["--git-dir", &self.repo_path.to_string_lossy(), "bundle", "unbundle", "-"]) + .args([ + "--git-dir", + &self.repo_path.to_string_lossy(), + "bundle", + "unbundle", + "-", + ]) .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) @@ -21,9 +27,13 @@ impl BundleApplicator { .map_err(|e| format!("spawn git bundle unbundle: {e}"))?; use std::io::Write; if let Some(ref mut stdin) = child.stdin { - stdin.write_all(data).map_err(|e| format!("write bundle: {e}"))?; + stdin + .write_all(data) + .map_err(|e| format!("write bundle: {e}"))?; } - let output = child.wait_with_output().map_err(|e| format!("wait bundle: {e}"))?; + let output = child + .wait_with_output() + .map_err(|e| format!("wait bundle: {e}"))?; if !output.status.success() { return Err(String::from_utf8_lossy(&output.stderr).into_owned()); } @@ -31,7 +41,7 @@ impl BundleApplicator { } } -pub fn collect_local_haves(repo_path: &PathBuf) -> Result, String> { +pub fn collect_local_haves(repo_path: &Path) -> Result, String> { let result = std::process::Command::new("git") .args([ "--git-dir", @@ -84,13 +94,13 @@ pub async fn sync_from_primary(event: RefUpdateEvent, local_repo_path: PathBuf) match tokio::task::spawn_blocking(move || { sync_via_pack_service(&grpc_addr, &relative_path, &repo_for_haves) - }).await { + }) + .await + { Ok(Ok(pack_data)) if !pack_data.is_empty() => { let pack_len = pack_data.len(); let repo = local_repo_path.clone(); - match tokio::task::spawn_blocking(move || { - apply_pack_data(&repo, &pack_data) - }).await { + match tokio::task::spawn_blocking(move || apply_pack_data(&repo, &pack_data)).await { Ok(Ok(())) => { update_local_ref(&local_repo_path, &event.ref_name, &event.new_oid); tracing::info!( @@ -99,27 +109,39 @@ pub async fn sync_from_primary(event: RefUpdateEvent, local_repo_path: PathBuf) "replica sync done" ); } - Ok(Err(e)) => tracing::error!(relative_path = %event.relative_path, error = %e, "pack apply failed"), - Err(e) => tracing::error!(relative_path = %event.relative_path, error = %e, "apply task failed"), + Ok(Err(e)) => { + tracing::error!(relative_path = %event.relative_path, error = %e, "pack apply failed") + } + Err(e) => { + tracing::error!(relative_path = %event.relative_path, error = %e, "apply task failed") + } } } - Ok(Ok(_)) => tracing::warn!(relative_path = %event.relative_path, "empty pack data from primary"), - Ok(Err(e)) => tracing::error!(relative_path = %event.relative_path, error = %e, "pack fetch failed"), - Err(e) => tracing::error!(relative_path = %event.relative_path, error = %e, "sync task failed"), + Ok(Ok(_)) => { + tracing::warn!(relative_path = %event.relative_path, "empty pack data from primary") + } + Ok(Err(e)) => { + tracing::error!(relative_path = %event.relative_path, error = %e, "pack fetch failed") + } + Err(e) => { + tracing::error!(relative_path = %event.relative_path, error = %e, "sync task failed") + } } } fn sync_via_pack_service( grpc_addr: &str, relative_path: &str, - local_repo_path: &PathBuf, + local_repo_path: &Path, ) -> Result, String> { let haves = collect_local_haves(local_repo_path)?; let rt = tokio::runtime::Handle::current(); rt.block_on(async { use crate::pb::pack_service_client::PackServiceClient; - use crate::pb::{AdvertiseRefsRequest, PackObjectsOptions, PackObjectsRequest, RepositoryHeader}; + use crate::pb::{ + AdvertiseRefsRequest, PackObjectsOptions, PackObjectsRequest, RepositoryHeader, + }; use tokio_stream::StreamExt; let endpoint = crate::server::remote_endpoint(grpc_addr) @@ -136,20 +158,21 @@ fn sync_via_pack_service( storage_path: String::new(), }; - let refs_resp = client.advertise_refs(AdvertiseRefsRequest { - repository: Some(header.clone()), - protocol: None, - service: "upload-pack".to_string(), - }).await.map_err(|e| format!("AdvertiseRefs: {e}"))?; + let refs_resp = client + .advertise_refs(AdvertiseRefsRequest { + repository: Some(header.clone()), + protocol: None, + service: "upload-pack".to_string(), + }) + .await + .map_err(|e| format!("AdvertiseRefs: {e}"))?; let refs = refs_resp.into_inner().references; if refs.is_empty() { return Ok(Vec::new()); } - let wants: Vec = refs.iter() - .filter_map(|r| r.target_oid.clone()) - .collect(); + let wants: Vec = refs.iter().filter_map(|r| r.target_oid.clone()).collect(); let want_count = wants.len(); let have_count = haves.len(); @@ -178,7 +201,9 @@ fn sync_via_pack_service( options: Some(options), }; - let resp = client.pack_objects(req).await + let resp = client + .pack_objects(req) + .await .map_err(|e| format!("PackObjects: {e}"))?; let mut stream = resp.into_inner(); @@ -200,21 +225,31 @@ fn sync_via_pack_service( }) } -fn apply_pack_data(repo_path: &PathBuf, pack_data: &[u8]) -> Result<(), String> { - let applicator = BundleApplicator::new(repo_path.clone()); +fn apply_pack_data(repo_path: &Path, pack_data: &[u8]) -> Result<(), String> { + let applicator = BundleApplicator::new(repo_path.to_path_buf()); applicator.apply_bundle(pack_data) } -fn update_local_ref(repo_path: &PathBuf, ref_name: &str, new_oid: &str) { +fn update_local_ref(repo_path: &Path, ref_name: &str, new_oid: &str) { if ref_name.is_empty() || new_oid.is_empty() { return; } match std::process::Command::new("git") - .args(["--git-dir", &repo_path.to_string_lossy(), "update-ref", ref_name, new_oid]) + .args([ + "--git-dir", + &repo_path.to_string_lossy(), + "update-ref", + ref_name, + new_oid, + ]) .output() { - Ok(o) if o.status.success() => tracing::info!(ref_name = %ref_name, new_oid = %new_oid, "ref updated"), - Ok(o) => tracing::error!(ref_name = %ref_name, error = %String::from_utf8_lossy(&o.stderr), "update-ref failed"), + Ok(o) if o.status.success() => { + tracing::info!(ref_name = %ref_name, new_oid = %new_oid, "ref updated") + } + Ok(o) => { + tracing::error!(ref_name = %ref_name, error = %String::from_utf8_lossy(&o.stderr), "update-ref failed") + } Err(e) => tracing::error!(ref_name = %ref_name, error = %e, "update-ref spawn failed"), } } diff --git a/archive/get_archive.rs b/archive/get_archive.rs index 834ddec..3ec8449 100644 --- a/archive/get_archive.rs +++ b/archive/get_archive.rs @@ -20,13 +20,19 @@ impl GitBare { let (tx, rx) = tokio::sync::mpsc::channel(16); + // Validate revision before spawning (cannot use ? inside spawn_blocking closure) + let revision = match request.treeish.and_then(|s| s.selector) { + Some(object_selector::Selector::Oid(oid)) => oid.hex, + Some(object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision) + .map_err(|e| tonic::Status::invalid_argument(e.to_string()))?; + name.revision + } + None => "HEAD".into(), + }; + // Spawn the blocking git subprocess in a dedicated thread tokio::task::spawn_blocking(move || { - let revision = match request.treeish.and_then(|s| s.selector) { - Some(object_selector::Selector::Oid(oid)) => oid.hex, - Some(object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; let options = request.options.unwrap_or_default(); let format = archive_options::Format::try_from(options.format) .unwrap_or(archive_options::Format::ArchiveFormatTar); diff --git a/archive/list_archive_entries.rs b/archive/list_archive_entries.rs index 898fae1..310e5aa 100644 --- a/archive/list_archive_entries.rs +++ b/archive/list_archive_entries.rs @@ -14,7 +14,10 @@ impl GitBare { ) -> GitResult { let revision = match request.treeish.and_then(|s| s.selector) { Some(object_selector::Selector::Oid(oid)) => oid.hex, - Some(object_selector::Selector::Revision(name)) => name.revision, + Some(object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision + } None => "HEAD".into(), }; let mut args = vec!["ls-tree".to_string(), "-r".into(), "-l".into(), revision]; diff --git a/bare.rs b/bare.rs index b24f5d8..69dc18c 100644 --- a/bare.rs +++ b/bare.rs @@ -27,6 +27,11 @@ impl GitBare { let storage_name = header.storage_name.trim(); let _ = storage_name; // reserved for future sharding logic + // Validate relative_path early to prevent path traversal + if !relative_path.is_empty() { + crate::sanitize::validate_relative_path(relative_path)?; + } + // Build base path: storage_path if given, else relative_path alone let base = if !storage_path.is_empty() { let p = Path::new(storage_path); @@ -46,13 +51,55 @@ impl GitBare { let bare_dir = if !relative_path.is_empty() && !storage_path.is_empty() { let candidate = base.join(relative_path); - let canonical = candidate - .canonicalize() - .unwrap_or_else(|_| candidate.clone()); + // Canonicalize base (parent dir likely exists) for a reliable traversal check. let base_canon = base.canonicalize().unwrap_or_else(|_| base.clone()); + + // Unified path validation to avoid TOCTOU race condition + let canonical = match candidate.canonicalize() { + Ok(canon) => { + // Path exists and was canonicalized successfully + canon + } + Err(_) => { + // Path doesn't exist yet — validate via parent directory + // This avoids TOCTOU by not having separate code paths + let parent = candidate.parent().unwrap_or(&base); + let filename = candidate.file_name().ok_or_else(|| { + GitError::InvalidArgument("invalid path: missing filename".into()) + })?; + + // Canonicalize parent (which should exist) + let parent_canon = parent + .canonicalize() + .unwrap_or_else(|_| parent.to_path_buf()); + + // Construct the full path and verify it's under base + let constructed = parent_canon.join(filename); + + // String-level check as fallback for non-existent paths + let constructed_str = constructed.to_string_lossy(); + let base_str = base_canon.to_string_lossy(); + + if !constructed_str.starts_with(&*base_str) { + tracing::warn!( + relative_path = %relative_path, + base = %base_canon.display(), + "path traversal attempt detected (parent check)" + ); + return Err(GitError::InvalidArgument(format!( + "path traversal detected: {relative_path} escapes storage root" + ))); + } + + constructed + } + }; + + // Final verification: canonical path must be under base if !canonical.starts_with(&base_canon) { tracing::warn!( relative_path = %relative_path, + canonical = %canonical.display(), base = %base_canon.display(), "path traversal attempt detected" ); diff --git a/blame/do_blame.rs b/blame/do_blame.rs index fa79147..bb0c410 100644 --- a/blame/do_blame.rs +++ b/blame/do_blame.rs @@ -6,7 +6,10 @@ impl GitBare { pub fn blame(&self, request: BlameRequest) -> GitResult { let revision = match request.revision.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex.clone(), - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision.clone(), + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision.clone() + } None => "HEAD".into(), }; tracing::info!( diff --git a/blob/get_blob.rs b/blob/get_blob.rs index 6e38072..bd9fae8 100644 --- a/blob/get_blob.rs +++ b/blob/get_blob.rs @@ -8,7 +8,7 @@ use crate::tree; impl GitBare { pub fn get_blob(&self, request: GetBlobRequest) -> GitResult { let repo = self.gix_repo()?; - let revision = tree::resolve_revision(&request.revision); + let revision = tree::resolve_revision(&request.revision)?; let (blob, mode, path) = if let Some(oid) = request.oid.as_ref() { let id = gix::hash::ObjectId::from_hex(oid.hex.as_bytes()) .map_err(|e| GitError::InvalidOid(e.to_string()))?; diff --git a/branch/create_branch.rs b/branch/create_branch.rs index 807a1cb..9a8a684 100644 --- a/branch/create_branch.rs +++ b/branch/create_branch.rs @@ -6,9 +6,13 @@ use crate::pb::{Branch, CreateBranchRequest, GetBranchRequest, object_selector}; impl GitBare { pub fn create_branch(&self, request: CreateBranchRequest) -> GitResult { + crate::sanitize::validate_ref_name(&request.name)?; let revision = match request.start_point.and_then(|s| s.selector) { Some(object_selector::Selector::Oid(oid)) => oid.hex, - Some(object_selector::Selector::Revision(name)) => name.revision, + Some(object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision + } None => "HEAD".into(), }; let mut args = vec!["branch".to_string()]; diff --git a/branch/delete_branch.rs b/branch/delete_branch.rs index 6c55284..7127089 100644 --- a/branch/delete_branch.rs +++ b/branch/delete_branch.rs @@ -6,6 +6,7 @@ use crate::pb::DeleteBranchRequest; impl GitBare { pub fn delete_branch(&self, request: DeleteBranchRequest) -> GitResult<()> { + crate::sanitize::validate_ref_name(&request.name)?; let flag = if request.force { "-D" } else { "-d" }; let output = Command::new("git") .arg("--git-dir") diff --git a/branch/rename_branch.rs b/branch/rename_branch.rs index 010b213..96803f4 100644 --- a/branch/rename_branch.rs +++ b/branch/rename_branch.rs @@ -6,6 +6,8 @@ use crate::pb::{Branch, GetBranchRequest, RenameBranchRequest}; impl GitBare { pub fn rename_branch(&self, request: RenameBranchRequest) -> GitResult { + crate::sanitize::validate_ref_name(&request.old_name)?; + crate::sanitize::validate_ref_name(&request.new_name)?; let output = Command::new("git") .arg("--git-dir") .arg(&self.bare_dir) diff --git a/branch/set_branch_upstream.rs b/branch/set_branch_upstream.rs index 21acedd..7079b34 100644 --- a/branch/set_branch_upstream.rs +++ b/branch/set_branch_upstream.rs @@ -4,6 +4,7 @@ use crate::pb::{Branch, GetBranchRequest, SetBranchUpstreamRequest}; impl GitBare { pub fn set_branch_upstream(&self, request: SetBranchUpstreamRequest) -> GitResult { + crate::sanitize::validate_ref_name(&request.name)?; if let Some(upstream) = request.upstream { let tracking = format!("{}/{}", upstream.remote_name, upstream.remote_branch_name); let result = duct::cmd( diff --git a/branch/update_branch_target.rs b/branch/update_branch_target.rs index 55675b1..f3a66e9 100644 --- a/branch/update_branch_target.rs +++ b/branch/update_branch_target.rs @@ -7,6 +7,7 @@ use crate::pb::{Branch, GetBranchRequest, UpdateBranchTargetRequest}; impl GitBare { pub fn update_branch_target(&self, request: UpdateBranchTargetRequest) -> GitResult { + crate::sanitize::validate_ref_name(&request.name)?; let new_oid = request .new_oid .as_ref() diff --git a/commit/cherry_pick_commit.rs b/commit/cherry_pick_commit.rs index fdf1690..af93799 100644 --- a/commit/cherry_pick_commit.rs +++ b/commit/cherry_pick_commit.rs @@ -9,9 +9,13 @@ impl GitBare { request: CherryPickCommitRequest, ) -> GitResult { let target_branch = request.branch.clone(); + crate::sanitize::validate_ref_name(&target_branch)?; let cp_revision = match request.commit.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision + } None => return Err(GitError::InvalidArgument("commit is required".into())), }; diff --git a/commit/compare_commits.rs b/commit/compare_commits.rs index ad45f6b..abb21d4 100644 --- a/commit/compare_commits.rs +++ b/commit/compare_commits.rs @@ -2,7 +2,8 @@ use crate::bare::GitBare; use crate::commit::list_commits::read_commit_from_repo; use crate::diff::get_diff_stats::diff_stats_for_range; use crate::error::{GitError, GitResult}; -use crate::pb::{CommitStats, CompareCommitsRequest, CompareCommitsResponse, object_selector}; +use crate::pb::{CommitStats, CompareCommitsRequest, CompareCommitsResponse}; +use crate::resolve_revision; impl GitBare { pub fn compare_commits( @@ -10,16 +11,8 @@ impl GitBare { request: CompareCommitsRequest, ) -> GitResult { let repo = self.gix_repo()?; - let base = match request.base.clone().and_then(|s| s.selector) { - Some(object_selector::Selector::Oid(oid)) => oid.hex, - Some(object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; - let head = match request.head.clone().and_then(|s| s.selector) { - Some(object_selector::Selector::Oid(oid)) => oid.hex, - Some(object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; + let base = resolve_revision!(request.base.clone()); + let head = resolve_revision!(request.head.clone()); let base_id = repo.rev_parse_single(base.as_str())?; let head_id = repo.rev_parse_single(head.as_str())?; diff --git a/commit/create_commit.rs b/commit/create_commit.rs index 3cf1b47..c882f81 100644 --- a/commit/create_commit.rs +++ b/commit/create_commit.rs @@ -8,6 +8,19 @@ use crate::pb::{ impl GitBare { pub fn create_commit(&self, request: CreateCommitRequest) -> GitResult { + // Validate branch name to prevent command injection + crate::sanitize::validate_ref_name(&request.branch)?; + // Validate start_revision if provided + if let Some(rev) = request.start_revision.as_ref() { + match rev.selector.as_ref() { + Some(object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + } + Some(object_selector::Selector::Oid(_)) => {} // OID is always safe + None => {} // will use branch name, already validated + } + } + let repo = self.gix_repo()?; let branch = request.branch.clone(); tracing::debug!( @@ -21,15 +34,15 @@ impl GitBare { Some(object_selector::Selector::Revision(name)) => name.revision, None => request.branch.clone(), }; - let parent_id = repo - .rev_parse_single(start_rev.as_str()) - .ok() - .map(|id| id.to_string()); - let current_branch_tip = repo - .find_reference(format!("refs/heads/{}", request.branch).as_str()) - .ok() - .and_then(|mut r| r.peel_to_id().ok()) - .map(|id| id.to_string()); + let parent_id = match repo.rev_parse_single(start_rev.as_str()) { + Ok(id) => Some(id.to_string()), + Err(_) => None, // branch/revision does not exist yet — will create initial commit + }; + let current_branch_tip = + match repo.find_reference(format!("refs/heads/{}", request.branch).as_str()) { + Ok(mut r) => r.peel_to_id().ok().map(|id| id.to_string()), + Err(_) => None, // branch does not exist yet + }; let tree_id = if request.actions.is_empty() { let Some(parent) = parent_id.as_ref() else { @@ -91,9 +104,11 @@ impl GitBare { parent_id: Option<&str>, actions: &[crate::pb::CreateCommitAction], ) -> GitResult { + // Use system temp directory instead of bare_dir to avoid clutter + // and ensure proper cleanup even if process crashes let tmp_index = tempfile::Builder::new() .prefix("gitks-index-") - .tempfile_in(&self.bare_dir) + .tempfile() .map_err(GitError::Io)?; let tmp_index_path = tmp_index.path().to_string_lossy().into_owned(); @@ -140,6 +155,14 @@ impl GitBare { index_path: &str, action: &crate::pb::CreateCommitAction, ) -> GitResult<()> { + // Validate file paths to prevent command injection / traversal + if !action.file_path.is_empty() { + crate::sanitize::validate_file_path(&action.file_path)?; + } + if !action.previous_path.is_empty() { + crate::sanitize::validate_file_path(&action.previous_path)?; + } + let action_type = create_commit_action::Action::try_from(action.action) .unwrap_or(create_commit_action::Action::CreateCommitActionUnspecified); match action_type { diff --git a/commit/get_commit.rs b/commit/get_commit.rs index 02d1853..684f07d 100644 --- a/commit/get_commit.rs +++ b/commit/get_commit.rs @@ -1,15 +1,12 @@ use crate::bare::GitBare; use crate::error::{GitError, GitResult}; -use crate::pb::{Commit, GetCommitRequest, object_selector}; +use crate::pb::{Commit, GetCommitRequest}; +use crate::resolve_revision; impl GitBare { pub fn get_commit(&self, request: GetCommitRequest) -> GitResult { let repo = self.gix_repo()?; - let revision = match request.revision.and_then(|s| s.selector) { - Some(object_selector::Selector::Oid(oid)) => oid.hex, - Some(object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; + let revision = resolve_revision!(request.revision); let id = repo.rev_parse_single(revision.as_str())?; let commit = id .object()? diff --git a/commit/list_commits.rs b/commit/list_commits.rs index 12be3f0..41300be 100644 --- a/commit/list_commits.rs +++ b/commit/list_commits.rs @@ -1,14 +1,11 @@ use crate::bare::GitBare; use crate::error::{GitError, GitResult}; -use crate::pb::{Commit, ListCommitsRequest, ListCommitsResponse, object_selector}; +use crate::pb::{Commit, ListCommitsRequest, ListCommitsResponse}; +use crate::resolve_revision; impl GitBare { pub fn list_commits(&self, request: ListCommitsRequest) -> GitResult { - let revision = match request.revision.clone().and_then(|s| s.selector) { - Some(object_selector::Selector::Oid(oid)) => oid.hex, - Some(object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; + let revision = resolve_revision!(request.revision.clone()); let base_args = build_rev_list_args(self, &request, &revision); diff --git a/commit/revert_commit.rs b/commit/revert_commit.rs index 0a20d84..0e9007d 100644 --- a/commit/revert_commit.rs +++ b/commit/revert_commit.rs @@ -6,9 +6,13 @@ use crate::pb::{CreateCommitResponse, GetCommitRequest, RevertCommitRequest}; impl GitBare { pub fn revert_commit(&self, request: RevertCommitRequest) -> GitResult { let target_branch = request.branch.clone(); + crate::sanitize::validate_ref_name(&target_branch)?; let revert_revision = match request.commit.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision + } None => return Err(GitError::InvalidArgument("commit is required".into())), }; diff --git a/diff/get_commit_diff.rs b/diff/get_commit_diff.rs index 88841f3..5154cfe 100644 --- a/diff/get_commit_diff.rs +++ b/diff/get_commit_diff.rs @@ -1,14 +1,11 @@ use crate::bare::GitBare; use crate::error::{GitError, GitResult}; use crate::pb::{GetCommitDiffRequest, GetDiffRequest, GetDiffResponse}; +use crate::resolve_revision; impl GitBare { pub fn get_commit_diff(&self, request: GetCommitDiffRequest) -> GitResult { - let commit = match request.commit.and_then(|s| s.selector) { - Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; + let commit = resolve_revision!(request.commit); let base = self.first_parent_or_empty_tree(&commit)?; self.get_diff(GetDiffRequest { repository: request.repository, diff --git a/diff/get_diff.rs b/diff/get_diff.rs index a2b1159..96a22c9 100644 --- a/diff/get_diff.rs +++ b/diff/get_diff.rs @@ -19,16 +19,25 @@ struct RawDiffEntry { similarity: f64, } +/// Type alias for diff raw output: (entries, numstat_map) +type DiffRawOutput = (Vec, HashMap); + impl GitBare { pub fn get_diff(&self, request: GetDiffRequest) -> GitResult { let base = match request.base.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex.clone(), - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision.clone(), + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision.clone() + } None => "HEAD".into(), }; let head = match request.head.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex.clone(), - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision.clone(), + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision.clone() + } None => "HEAD".into(), }; tracing::debug!( @@ -142,7 +151,7 @@ impl GitBare { base: &str, head: &str, options: Option<&crate::pb::DiffOptions>, - ) -> GitResult<(Vec, HashMap)> { + ) -> GitResult { let mut args = vec![ "--git-dir".to_string(), self.bare_dir.to_string_lossy().into_owned(), diff --git a/diff/get_diff_stats.rs b/diff/get_diff_stats.rs index 03a0e16..492cdbb 100644 --- a/diff/get_diff_stats.rs +++ b/diff/get_diff_stats.rs @@ -1,19 +1,12 @@ use crate::bare::GitBare; use crate::error::{GitError, GitResult}; use crate::pb::GetDiffStatsRequest; +use crate::resolve_revision; impl GitBare { pub fn get_diff_stats(&self, request: GetDiffStatsRequest) -> GitResult { - let base = match request.base.and_then(|s| s.selector) { - Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; - let head = match request.head.and_then(|s| s.selector) { - Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; + let base = resolve_revision!(request.base); + let head = resolve_revision!(request.head); diff_stats_for_range(self, &base, &head, request.options.as_ref()) } } diff --git a/diff/get_patch.rs b/diff/get_patch.rs index d917106..0ce8d6c 100644 --- a/diff/get_patch.rs +++ b/diff/get_patch.rs @@ -1,19 +1,12 @@ use crate::bare::GitBare; use crate::error::{GitError, GitResult}; use crate::pb::{GetPatchRequest, GetPatchResponse}; +use crate::resolve_revision; impl GitBare { pub fn get_patch(&self, request: GetPatchRequest) -> GitResult> { - let base = match request.base.and_then(|s| s.selector) { - Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; - let head = match request.head.and_then(|s| s.selector) { - Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; + let base = resolve_revision!(request.base); + let head = resolve_revision!(request.head); let result = duct::cmd( "git", [ diff --git a/lib.rs b/lib.rs index bc8c433..44053f1 100644 --- a/lib.rs +++ b/lib.rs @@ -1,3 +1,4 @@ +pub mod actor; pub mod archive; pub mod bare; pub mod blame; @@ -7,13 +8,14 @@ pub mod commit; pub mod diff; pub mod error; pub mod init; +pub mod macros; pub mod merge; pub mod oid; pub mod pack; pub mod paginate; pub mod pb; pub mod refs; +pub mod sanitize; pub mod server; pub mod tag; pub mod tree; -pub mod actor; \ No newline at end of file diff --git a/macros.rs b/macros.rs new file mode 100644 index 0000000..7b050f2 --- /dev/null +++ b/macros.rs @@ -0,0 +1,36 @@ +//! Helper macro to extract and validate a revision selector. +//! +//! Replaces the repeated pattern of matching on ObjectSelector variants +//! and validating revision strings before use. + +/// Extract a revision string from an optional ObjectSelector, applying +/// validation to revision names. OID hex strings are always safe. +/// +/// Returns "HEAD" when selector is None. +/// +/// Usage: +/// let revision = resolve_revision!(request.base); +/// let revision = resolve_revision!(request.base, "main"); +#[macro_export] +macro_rules! resolve_revision { + ($selector:expr) => {{ + match $selector.and_then(|s| s.selector) { + Some($crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, + Some($crate::pb::object_selector::Selector::Revision(name)) => { + $crate::sanitize::validate_revision(&name.revision)?; + name.revision + } + None => "HEAD".into(), + } + }}; + ($selector:expr, $default:expr) => {{ + match $selector.and_then(|s| s.selector) { + Some($crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, + Some($crate::pb::object_selector::Selector::Revision(name)) => { + $crate::sanitize::validate_revision(&name.revision)?; + name.revision + } + None => ($default).into(), + } + }}; +} diff --git a/main.rs b/main.rs index 62bb469..5952601 100644 --- a/main.rs +++ b/main.rs @@ -1,7 +1,7 @@ use std::path::PathBuf; use gitks::actor::init_actor_cluster; -use gitks::server::{serve, GitksService}; +use gitks::server::{GitksService, serve}; const DEFAULT_HOST: &str = "0.0.0.0"; const DEFAULT_PORT: &str = "50051"; @@ -12,16 +12,14 @@ async fn main() -> Result<(), Box> { dotenvy::dotenv().ok(); tracing_subscriber::fmt().init(); - tracing::info!( - version = env!("CARGO_PKG_VERSION"), - "gitks starting up" - ); + tracing::info!(version = env!("CARGO_PKG_VERSION"), "gitks starting up"); let host = std::env::var("GITKS_HOST").unwrap_or_else(|_| DEFAULT_HOST.into()); let port = std::env::var("GITKS_PORT").unwrap_or_else(|_| DEFAULT_PORT.into()); - let storage_name = std::env::var("STORAGE_NAME").unwrap_or_else(|_| DEFAULT_STORAGE_NAME.into()); - let grpc_addr = std::env::var("GITKS_ADVERTISE_ADDR") - .unwrap_or_else(|_| format!("http://{host}:{port}")); + let storage_name = + std::env::var("STORAGE_NAME").unwrap_or_else(|_| DEFAULT_STORAGE_NAME.into()); + let grpc_addr = + std::env::var("GITKS_ADVERTISE_ADDR").unwrap_or_else(|_| format!("http://{host}:{port}")); let repo_prefix = std::env::var("REPO_PREFIX_PATH") .map_err(|_| "REPO_PREFIX_PATH environment variable is required (e.g. /data/repos)")?; @@ -35,13 +33,10 @@ async fn main() -> Result<(), Box> { } let addr: std::net::SocketAddr = format!("{host}:{port}").parse()?; - let actor_svc = GitksService::new(repo_prefix.clone()); - let (node_actor, node_handle) = init_actor_cluster( - actor_svc, - storage_name.clone(), - grpc_addr.clone(), - ).await?; - let svc = GitksService::new(repo_prefix.clone()) + let svc = GitksService::new(repo_prefix.clone()); + let (node_actor, node_handle) = + init_actor_cluster(svc.clone(), storage_name.clone(), grpc_addr.clone()).await?; + let svc = svc .with_actor(node_actor.clone()) .with_grpc_addr(grpc_addr.clone()); diff --git a/merge/check_merge.rs b/merge/check_merge.rs index b613c7d..2e89421 100644 --- a/merge/check_merge.rs +++ b/merge/check_merge.rs @@ -1,19 +1,12 @@ use crate::bare::GitBare; use crate::error::GitResult; use crate::pb::{CheckMergeRequest, MergeResult, merge_result}; +use crate::resolve_revision; impl GitBare { pub fn check_merge(&self, request: CheckMergeRequest) -> GitResult { - let target = match request.target.and_then(|s| s.selector) { - Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; - let source = match request.source.and_then(|s| s.selector) { - Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; + let target = resolve_revision!(request.target); + let source = resolve_revision!(request.source); let repo = self.gix_repo()?; let target_id = repo.rev_parse_single(target.as_str())?; diff --git a/merge/do_merge.rs b/merge/do_merge.rs index 966168f..bf6ff85 100644 --- a/merge/do_merge.rs +++ b/merge/do_merge.rs @@ -5,9 +5,13 @@ use crate::pb::{MergeRequest, MergeResult, merge_result}; impl GitBare { pub fn merge(&self, request: MergeRequest) -> GitResult { let target_branch = request.target_branch.clone(); + crate::sanitize::validate_ref_name(&target_branch)?; let source_revision = match request.source.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex.clone(), - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision.clone(), + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision.clone() + } None => return Err(GitError::InvalidArgument("source is required".into())), }; tracing::info!( diff --git a/merge/list_merge_conflicts.rs b/merge/list_merge_conflicts.rs index 304b197..757d841 100644 --- a/merge/list_merge_conflicts.rs +++ b/merge/list_merge_conflicts.rs @@ -10,12 +10,18 @@ impl GitBare { ) -> GitResult { let target = match request.target.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision + } None => return Err(GitError::InvalidArgument("target is required".into())), }; let source = match request.source.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision + } None => return Err(GitError::InvalidArgument("source is required".into())), }; diff --git a/merge/rebase.rs b/merge/rebase.rs index 9915146..9a38b8d 100644 --- a/merge/rebase.rs +++ b/merge/rebase.rs @@ -6,9 +6,13 @@ use crate::pb::{RebaseRequest, RebaseResult, rebase_result}; impl GitBare { pub fn rebase(&self, request: RebaseRequest) -> GitResult { let branch = request.branch.clone(); + crate::sanitize::validate_ref_name(&branch)?; let upstream_revision = match request.upstream.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision + } None => return Err(GitError::InvalidArgument("upstream is required".into())), }; diff --git a/merge/resolve_merge_conflicts.rs b/merge/resolve_merge_conflicts.rs index 0996066..6ec5be9 100644 --- a/merge/resolve_merge_conflicts.rs +++ b/merge/resolve_merge_conflicts.rs @@ -9,9 +9,13 @@ impl GitBare { request: ResolveMergeConflictsRequest, ) -> GitResult { let target_branch = request.target_branch.clone(); + crate::sanitize::validate_ref_name(&target_branch)?; let source_revision = match request.source.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision + } None => return Err(GitError::InvalidArgument("source is required".into())), }; diff --git a/sanitize.rs b/sanitize.rs new file mode 100644 index 0000000..99d8f32 --- /dev/null +++ b/sanitize.rs @@ -0,0 +1,307 @@ +//! Input sanitization for git subprocess arguments. +//! +//! Prevents command injection by validating user-supplied strings before +//! passing them to git commands. + +use crate::error::GitError; +use crate::error::GitResult; + +/// Characters that are never allowed in git ref names / revision strings. +const FORBIDDEN_REF_CHARS: &[char] = &[ + '~', '^', ':', '?', '*', '[', '\\', ' ', '\n', '\r', '\t', '\0', +]; + +/// Validate a git reference name (branch, tag, etc.). +/// +/// Git ref rules (from `git check-ref-format`): +/// - Cannot contain forbidden chars +/// - Cannot start or end with '.' +/// - Cannot end with '/' +/// - Cannot contain '..' +/// - Cannot contain '@{' +/// - Cannot be empty +pub fn validate_ref_name(name: &str) -> GitResult<()> { + if name.is_empty() { + return Err(GitError::InvalidArgument("ref name cannot be empty".into())); + } + if name.starts_with('.') || name.ends_with('.') { + return Err(GitError::InvalidArgument(format!( + "ref name cannot start or end with '.': {name}" + ))); + } + if name.ends_with('/') { + return Err(GitError::InvalidArgument(format!( + "ref name cannot end with '/': {name}" + ))); + } + if name.contains("..") { + return Err(GitError::InvalidArgument(format!( + "ref name cannot contain '..': {name}" + ))); + } + if name.contains("@{") { + return Err(GitError::InvalidArgument(format!( + "ref name cannot contain '@{{': {name}" + ))); + } + if name.contains(|c: char| FORBIDDEN_REF_CHARS.contains(&c)) { + return Err(GitError::InvalidArgument(format!( + "ref name contains forbidden character: {name}" + ))); + } + // Ref names must not exceed a reasonable length + if name.len() > 255 { + return Err(GitError::InvalidArgument(format!( + "ref name too long (max 255 chars): {name}" + ))); + } + Ok(()) +} + +/// Validate a revision string (branch name, tag name, or short expression). +/// +/// Allows OID hex strings, ref names, and a small set of revision operators +/// (HEAD, ^{tree}, ~N, ^N) that are safe when passed as a single argument. +pub fn validate_revision(rev: &str) -> GitResult<()> { + if rev.is_empty() { + return Err(GitError::InvalidArgument("revision cannot be empty".into())); + } + // Prevent DoS via extremely long revision strings + if rev.len() > 256 { + return Err(GitError::InvalidArgument(format!( + "revision too long (max 256 chars): {}", + rev.len() + ))); + } + // Pure hex OID — always safe + if rev.chars().all(|c| c.is_ascii_hexdigit()) && rev.len() >= 4 && rev.len() <= 64 { + return Ok(()); + } + // HEAD is always safe + if rev == "HEAD" { + return Ok(()); + } + // Allow ref:refs/heads/... (git internal format) + if let Some(rest) = rev.strip_prefix("ref:") { + return validate_ref_name(rest.trim()); + } + + // Validate ~N and ^N numeric suffixes to prevent DoS + const MAX_ANCESTRY_DEPTH: u32 = 10000; + + // Check for ~N syntax + if let Some(tilde_pos) = rev.rfind('~') { + let num_part = &rev[tilde_pos + 1..]; + if !num_part.is_empty() && num_part.chars().all(|c| c.is_ascii_digit()) { + let depth: u32 = num_part + .parse() + .map_err(|_| GitError::InvalidArgument("invalid ~N syntax".into()))?; + if depth > MAX_ANCESTRY_DEPTH { + return Err(GitError::InvalidArgument(format!( + "~N depth too large: {} (max {})", + depth, MAX_ANCESTRY_DEPTH + ))); + } + } + } + + // Check for ^N syntax (not ^{tree}) + if let Some(caret_pos) = rev.rfind('^') { + let after_caret = &rev[caret_pos + 1..]; + // Skip ^{tree} style operators + if !after_caret.starts_with('{') + && !after_caret.is_empty() + && let Some(first_char) = after_caret.chars().next() + && first_char.is_ascii_digit() + { + let num_part: String = after_caret + .chars() + .take_while(|c| c.is_ascii_digit()) + .collect(); + if !num_part.is_empty() { + let depth: u32 = num_part + .parse() + .map_err(|_| GitError::InvalidArgument("invalid ^N syntax".into()))?; + if depth > MAX_ANCESTRY_DEPTH { + return Err(GitError::InvalidArgument(format!( + "^N depth too large: {} (max {})", + depth, MAX_ANCESTRY_DEPTH + ))); + } + } + } + } + + // Strip trailing operators and validate the base ref + // Only strip digits that are part of ~N or ^N patterns, not arbitrary trailing digits + let mut base = rev; + + // Strip ^{tree}, ^{commit}, ^{object} suffixes + base = base + .trim_end_matches("^{tree}") + .trim_end_matches("^{commit}") + .trim_end_matches("^{object}"); + + // Strip ~N or ^N suffix if present + if let Some(tilde_pos) = base.rfind('~') { + let after_tilde = &base[tilde_pos + 1..]; + if !after_tilde.is_empty() && after_tilde.chars().all(|c| c.is_ascii_digit()) { + base = &base[..tilde_pos]; + } + } else if let Some(caret_pos) = base.rfind('^') { + let after_caret = &base[caret_pos + 1..]; + if !after_caret.starts_with('{') + && !after_caret.is_empty() + && after_caret.chars().all(|c| c.is_ascii_digit()) + { + base = &base[..caret_pos]; + } + } + + if base.is_empty() { + // Pure operator like "^" — unlikely but not dangerous + return Ok(()); + } + validate_ref_name(base)?; + Ok(()) +} + +/// Validate a file path within a commit action. +/// +/// Must be a relative path (no leading '/'), no '..' traversal, +/// no null bytes, no .git directory access, and reasonable length. +pub fn validate_file_path(path: &str) -> GitResult<()> { + if path.is_empty() { + return Err(GitError::InvalidArgument( + "file path cannot be empty".into(), + )); + } + if path.starts_with('/') { + return Err(GitError::InvalidArgument(format!( + "file path must be relative, not absolute: {path}" + ))); + } + if path.contains("..") { + return Err(GitError::InvalidArgument(format!( + "file path cannot contain '..': {path}" + ))); + } + if path.contains('\0') { + return Err(GitError::InvalidArgument(format!( + "file path cannot contain null byte: {path}" + ))); + } + if path.len() > 4096 { + return Err(GitError::InvalidArgument(format!( + "file path too long (max 4096 chars): {path}" + ))); + } + + // Prevent modification of .git directory + if path == ".git" + || path.starts_with(".git/") + || path.contains("/.git/") + || path.ends_with("/.git") + { + return Err(GitError::InvalidArgument(format!( + "cannot modify .git directory: {path}" + ))); + } + + // Windows reserved names check + #[cfg(target_os = "windows")] + { + const RESERVED_NAMES: &[&str] = &[ + "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", + "COM8", "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", + ]; + + // Check each path component + for component in path.split('/') { + // Get filename without extension + let name_part = component.split('.').next().unwrap_or(component); + let name_upper = name_part.to_uppercase(); + + if RESERVED_NAMES.contains(&name_upper.as_str()) { + return Err(GitError::InvalidArgument(format!( + "Windows reserved device name: {component}" + ))); + } + } + } + + Ok(()) +} + +/// Git config keys that are dangerous to set remotely. +/// Setting these could allow arbitrary command execution or bypass security. +const DANGEROUS_CONFIG_KEYS: &[&str] = &[ + "core.sshCommand", + "core.gitProxy", + "http.proxy", + "https.proxy", + "remote.*.url", + "credential.*", + "safe.directory", + "core.hooksPath", + "receive.fsckObjects", + "receive.denyCurrentBranch", + "receive.denyDeleteCurrent", +]; + +/// Check if a git config key is safe to set remotely. +pub fn validate_config_key(key: &str) -> GitResult<()> { + if key.is_empty() { + return Err(GitError::InvalidArgument( + "config key cannot be empty".into(), + )); + } + // Check for wildcard patterns like "remote.*.url" + for pattern in DANGEROUS_CONFIG_KEYS { + if pattern.contains('*') { + // e.g. "remote.*.url" — match any "remote..url" + let (prefix, suffix) = pattern.split_once('*').unwrap(); + if key.starts_with(prefix) && key.ends_with(suffix) { + return Err(GitError::InvalidArgument(format!( + "config key '{key}' matches dangerous pattern '{pattern}'" + ))); + } + } else if key == *pattern { + return Err(GitError::InvalidArgument(format!( + "config key '{key}' is not allowed to be set remotely" + ))); + } + } + // Config keys must be valid format: section.subsection.key + if !key + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '-' || c == '_') + { + return Err(GitError::InvalidArgument(format!( + "config key contains invalid characters: {key}" + ))); + } + Ok(()) +} + +/// Validate a storage-relative path (used in resolve_for_init and from_repository_header). +/// +/// Must not contain path traversal, must be a simple relative path. +pub fn validate_relative_path(path: &str) -> GitResult<()> { + if path.is_empty() { + return Err(GitError::InvalidArgument( + "relative_path cannot be empty".into(), + )); + } + if path.starts_with('/') { + return Err(GitError::InvalidArgument( + "relative_path must be relative, not absolute".into(), + )); + } + if path.contains("..") { + return Err(GitError::InvalidArgument(format!( + "path traversal detected: relative_path contains '..': {path}" + ))); + } + Ok(()) +} diff --git a/server/archive.rs b/server/archive.rs index 8bc5cdd..dd86ad4 100644 --- a/server/archive.rs +++ b/server/archive.rs @@ -1,27 +1,9 @@ -use crate::pb::*; use crate::pb::archive_service_client::ArchiveServiceClient; +use crate::pb::*; use super::{GitksService, cache, into_status}; -async fn remote_archive_client( - svc: &GitksService, - header: Option<&RepositoryHeader>, - is_write: bool, -) -> Result>, tonic::Status> { - let header = match header { - Some(h) => h, - None => return Ok(None), - }; - let Some(route) = svc.route_repository(header, is_write).await? else { - return Ok(None); - }; - tracing::info!(storage_name = %route.storage_name, relative_path = %route.relative_path, actor_name = %route.actor_name, grpc_addr = %route.grpc_addr, "forwarding archive rpc"); - let endpoint = super::remote_endpoint(&route.grpc_addr).await?; - let client = ArchiveServiceClient::connect(endpoint) - .await - .map_err(|e| tonic::Status::unavailable(e.to_string()))?; - Ok(Some(client)) -} +remote_client!(remote_archive_client, ArchiveServiceClient, "archive"); #[tonic::async_trait] impl archive_service_server::ArchiveService for GitksService { @@ -39,7 +21,9 @@ impl archive_service_server::ArchiveService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_archive_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_archive_client(self, inner.repository.as_ref(), false).await? + { let resp = client.get_archive(inner).await?; let stream = super::bridge_server_stream(resp.into_inner()); return Ok(tonic::Response::new(stream)); @@ -64,7 +48,9 @@ impl archive_service_server::ArchiveService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_archive_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_archive_client(self, inner.repository.as_ref(), false).await? + { return client.list_archive_entries(inner).await; } return Err(err); diff --git a/server/blame.rs b/server/blame.rs index b03e636..d39ef69 100644 --- a/server/blame.rs +++ b/server/blame.rs @@ -1,27 +1,9 @@ -use crate::pb::*; use crate::pb::blame_service_client::BlameServiceClient; +use crate::pb::*; use super::{GitksService, cache, into_status, into_stream}; -async fn remote_blame_client( - svc: &GitksService, - header: Option<&RepositoryHeader>, - is_write: bool, -) -> Result>, tonic::Status> { - let header = match header { - Some(h) => h, - None => return Ok(None), - }; - let Some(route) = svc.route_repository(header, is_write).await? else { - return Ok(None); - }; - tracing::info!(storage_name = %route.storage_name, relative_path = %route.relative_path, actor_name = %route.actor_name, grpc_addr = %route.grpc_addr, "forwarding blame rpc"); - let endpoint = super::remote_endpoint(&route.grpc_addr).await?; - let client = BlameServiceClient::connect(endpoint) - .await - .map_err(|e| tonic::Status::unavailable(e.to_string()))?; - Ok(Some(client)) -} +remote_client!(remote_blame_client, BlameServiceClient, "blame"); #[tonic::async_trait] impl blame_service_server::BlameService for GitksService { @@ -40,7 +22,9 @@ impl blame_service_server::BlameService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_blame_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_blame_client(self, inner.repository.as_ref(), false).await? + { return client.blame(inner).await; } return Err(err); @@ -70,7 +54,9 @@ impl blame_service_server::BlameService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_blame_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_blame_client(self, inner.repository.as_ref(), false).await? + { let resp = client.stream_blame(inner).await?; let stream = super::bridge_server_stream(resp.into_inner()); return Ok(tonic::Response::new(stream)); diff --git a/server/branch.rs b/server/branch.rs index 62cb7cb..80944a6 100644 --- a/server/branch.rs +++ b/server/branch.rs @@ -1,27 +1,9 @@ -use crate::pb::*; use crate::pb::branch_service_client::BranchServiceClient; +use crate::pb::*; use super::{GitksService, into_status}; -async fn remote_branch_client( - svc: &GitksService, - header: Option<&RepositoryHeader>, - is_write: bool, -) -> Result>, tonic::Status> { - let header = match header { - Some(h) => h, - None => return Ok(None), - }; - let Some(route) = svc.route_repository(header, is_write).await? else { - return Ok(None); - }; - tracing::info!(storage_name = %route.storage_name, relative_path = %route.relative_path, actor_name = %route.actor_name, grpc_addr = %route.grpc_addr, "forwarding branch rpc"); - let endpoint = super::remote_endpoint(&route.grpc_addr).await?; - let client = BranchServiceClient::connect(endpoint) - .await - .map_err(|e| tonic::Status::unavailable(e.to_string()))?; - Ok(Some(client)) -} +remote_client!(remote_branch_client, BranchServiceClient, "branch"); #[tonic::async_trait] impl branch_service_server::BranchService for GitksService { @@ -36,7 +18,9 @@ impl branch_service_server::BranchService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_branch_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_branch_client(self, inner.repository.as_ref(), false).await? + { return client.list_branches(inner).await; } return Err(err); @@ -60,7 +44,9 @@ impl branch_service_server::BranchService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_branch_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_branch_client(self, inner.repository.as_ref(), false).await? + { return client.get_branch(inner).await; } return Err(err); @@ -83,7 +69,9 @@ impl branch_service_server::BranchService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_branch_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_branch_client(self, inner.repository.as_ref(), true).await? + { return client.create_branch(inner).await; } return Err(err); @@ -108,7 +96,9 @@ impl branch_service_server::BranchService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_branch_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_branch_client(self, inner.repository.as_ref(), true).await? + { return client.delete_branch(inner).await; } return Err(err); @@ -134,7 +124,9 @@ impl branch_service_server::BranchService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_branch_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_branch_client(self, inner.repository.as_ref(), true).await? + { return client.rename_branch(inner).await; } return Err(err); @@ -159,7 +151,9 @@ impl branch_service_server::BranchService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_branch_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_branch_client(self, inner.repository.as_ref(), true).await? + { return client.update_branch_target(inner).await; } return Err(err); @@ -184,7 +178,9 @@ impl branch_service_server::BranchService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_branch_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_branch_client(self, inner.repository.as_ref(), true).await? + { return client.set_branch_upstream(inner).await; } return Err(err); @@ -210,7 +206,9 @@ impl branch_service_server::BranchService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_branch_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_branch_client(self, inner.repository.as_ref(), false).await? + { return client.compare_branch(inner).await; } return Err(err); diff --git a/server/cache.rs b/server/cache.rs index 6bd7559..292eda0 100644 --- a/server/cache.rs +++ b/server/cache.rs @@ -1,22 +1,22 @@ -use std::num::NonZeroUsize; -use std::sync::{Mutex, OnceLock}; +use std::sync::OnceLock; +use std::time::Duration; -use clru::CLruCache; +use moka::sync::Cache; use prost::Message; use crate::pb::{ObjectSelector, object_selector}; -const GLOBAL_CACHE_MAX: usize = 65_545; +const GLOBAL_CACHE_MAX: u64 = 65_536; +const CACHE_TTL: Duration = Duration::from_secs(300); -type Cache = CLruCache, Vec>; +static GLOBAL_CACHE: OnceLock, Vec>> = OnceLock::new(); -static GLOBAL_CACHE: OnceLock> = OnceLock::new(); - -fn cache() -> &'static Mutex { +fn cache() -> &'static Cache, Vec> { GLOBAL_CACHE.get_or_init(|| { - let capacity = - NonZeroUsize::new(GLOBAL_CACHE_MAX).expect("cache capacity must be non-zero"); - Mutex::new(CLruCache::new(capacity)) + Cache::builder() + .max_capacity(GLOBAL_CACHE_MAX) + .time_to_live(CACHE_TTL) + .build() }) } @@ -45,11 +45,7 @@ where { let key = cache_key(namespace, request); - if let Some(bytes) = cache() - .lock() - .unwrap_or_else(|e| e.into_inner()) - .get(&key) - .cloned() + if let Some(bytes) = cache().get(&key) && let Ok(response) = Res::decode(bytes.as_slice()) { tracing::debug!( @@ -70,10 +66,7 @@ where response .encode(&mut bytes) .expect("encoding a prost message into Vec cannot fail"); - cache() - .lock() - .unwrap_or_else(|e| e.into_inner()) - .put(key, bytes); + cache().insert(key, bytes); Ok(response) } @@ -89,12 +82,7 @@ where { let key = cache_key(namespace, request); - if let Some(bytes) = cache() - .lock() - .unwrap_or_else(|e| e.into_inner()) - .get(&key) - .cloned() - { + if let Some(bytes) = cache().get(&key) { let mut remaining = bytes.as_slice(); let mut items = Vec::new(); let mut valid = true; @@ -133,13 +121,75 @@ where item.encode_length_delimited(&mut bytes) .expect("encoding a prost message into Vec cannot fail"); } - cache() - .lock() - .unwrap_or_else(|e| e.into_inner()) - .put(key, bytes); + cache().insert(key, bytes); Ok(response) } +/// Invalidate all cache entries related to a specific repository. +/// Called when refs are updated (create branch, create commit, etc.) +/// so that stale data is not served. +pub(crate) fn invalidate_repo(relative_path: &str) { + let c = cache(); + + // Encode the relative_path to match how it appears in cache keys + let target_path_bytes = relative_path.as_bytes(); + + // Remove all keys that reference this repository + // Cache keys are: namespace\0 + prost-encoded request + let keys_to_remove: Vec>> = c + .iter() + .filter_map(|(key, _)| { + // Find the null byte separator + if let Some(null_pos) = key.iter().position(|&b| b == 0) { + let encoded_request = &key[null_pos + 1..]; + + // Check if this encoded request contains the repository path + // We use a sliding window to find the path bytes in the encoded protobuf + // This is conservative but correct: we may invalidate slightly more than + // necessary, but we won't miss any entries for this repository. + // + // The encoded protobuf format embeds string fields as length-prefixed data, + // so the relative_path bytes should appear verbatim somewhere in the message. + if contains_subslice(encoded_request, target_path_bytes) { + return Some(key); + } + } else { + // Malformed key without separator, remove it to be safe + tracing::warn!("found cache key without null separator, removing"); + return Some(key); + } + None + }) + .collect(); + + let removed = keys_to_remove.len(); + for key in keys_to_remove { + c.invalidate(key.as_ref()); + } + + if removed > 0 { + tracing::debug!( + relative_path = %relative_path, + entries_removed = removed, + "cache invalidated for repository" + ); + } +} + +/// Check if a byte slice contains a subslice +fn contains_subslice(haystack: &[u8], needle: &[u8]) -> bool { + if needle.is_empty() { + return true; + } + if needle.len() > haystack.len() { + return false; + } + + haystack + .windows(needle.len()) + .any(|window| window == needle) +} + pub(crate) fn selector_is_oid(selector: &Option) -> bool { matches!( selector.as_ref().and_then(|s| s.selector.as_ref()), diff --git a/server/commit.rs b/server/commit.rs index 968d0a5..4dabaf5 100644 --- a/server/commit.rs +++ b/server/commit.rs @@ -1,27 +1,9 @@ -use crate::pb::*; use crate::pb::commit_service_client::CommitServiceClient; +use crate::pb::*; use super::{GitksService, cache, into_status}; -async fn remote_commit_client( - svc: &GitksService, - header: Option<&RepositoryHeader>, - is_write: bool, -) -> Result>, tonic::Status> { - let header = match header { - Some(h) => h, - None => return Ok(None), - }; - let Some(route) = svc.route_repository(header, is_write).await? else { - return Ok(None); - }; - tracing::info!(storage_name = %route.storage_name, relative_path = %route.relative_path, actor_name = %route.actor_name, grpc_addr = %route.grpc_addr, "forwarding commit rpc"); - let endpoint = super::remote_endpoint(&route.grpc_addr).await?; - let client = CommitServiceClient::connect(endpoint) - .await - .map_err(|e| tonic::Status::unavailable(e.to_string()))?; - Ok(Some(client)) -} +remote_client!(remote_commit_client, CommitServiceClient, "commit"); #[tonic::async_trait] impl commit_service_server::CommitService for GitksService { @@ -36,7 +18,9 @@ impl commit_service_server::CommitService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_commit_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_commit_client(self, inner.repository.as_ref(), false).await? + { return client.list_commits(inner).await; } return Err(err); @@ -65,7 +49,9 @@ impl commit_service_server::CommitService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_commit_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_commit_client(self, inner.repository.as_ref(), false).await? + { return client.get_commit(inner).await; } return Err(err); @@ -93,7 +79,9 @@ impl commit_service_server::CommitService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_commit_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_commit_client(self, inner.repository.as_ref(), false).await? + { return client.get_commit_ancestors(inner).await; } return Err(err); @@ -123,7 +111,9 @@ impl commit_service_server::CommitService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_commit_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_commit_client(self, inner.repository.as_ref(), true).await? + { return client.create_commit(inner).await; } return Err(err); @@ -131,7 +121,9 @@ impl commit_service_server::CommitService for GitksService { Err(err) => return Err(err), }; let resp = gb.create_commit(inner).map_err(into_status)?; - let commit_hex = resp.commit.as_ref() + let commit_hex = resp + .commit + .as_ref() .and_then(|c| c.oid.as_ref().map(|o| o.hex.as_str()).or(Some("?"))) .unwrap_or("?"); tracing::info!(%repo, %branch, %commit_hex, "commit created"); @@ -151,7 +143,9 @@ impl commit_service_server::CommitService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_commit_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_commit_client(self, inner.repository.as_ref(), true).await? + { return client.revert_commit(inner).await; } return Err(err); @@ -176,7 +170,9 @@ impl commit_service_server::CommitService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_commit_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_commit_client(self, inner.repository.as_ref(), true).await? + { return client.cherry_pick_commit(inner).await; } return Err(err); @@ -200,7 +196,9 @@ impl commit_service_server::CommitService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_commit_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_commit_client(self, inner.repository.as_ref(), false).await? + { return client.compare_commits(inner).await; } return Err(err); diff --git a/server/diff.rs b/server/diff.rs index b0f4ba2..aff27e5 100644 --- a/server/diff.rs +++ b/server/diff.rs @@ -1,27 +1,9 @@ -use crate::pb::*; use crate::pb::diff_service_client::DiffServiceClient; +use crate::pb::*; use super::{GitksService, cache, into_status, into_stream}; -async fn remote_diff_client( - svc: &GitksService, - header: Option<&RepositoryHeader>, - is_write: bool, -) -> Result>, tonic::Status> { - let header = match header { - Some(h) => h, - None => return Ok(None), - }; - let Some(route) = svc.route_repository(header, is_write).await? else { - return Ok(None); - }; - tracing::info!(storage_name = %route.storage_name, relative_path = %route.relative_path, actor_name = %route.actor_name, grpc_addr = %route.grpc_addr, "forwarding diff rpc"); - let endpoint = super::remote_endpoint(&route.grpc_addr).await?; - let client = DiffServiceClient::connect(endpoint) - .await - .map_err(|e| tonic::Status::unavailable(e.to_string()))?; - Ok(Some(client)) -} +remote_client!(remote_diff_client, DiffServiceClient, "diff"); #[tonic::async_trait] impl diff_service_server::DiffService for GitksService { @@ -39,7 +21,9 @@ impl diff_service_server::DiffService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_diff_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_diff_client(self, inner.repository.as_ref(), false).await? + { return client.get_diff(inner).await; } return Err(err); @@ -68,7 +52,9 @@ impl diff_service_server::DiffService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_diff_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_diff_client(self, inner.repository.as_ref(), false).await? + { return client.get_commit_diff(inner).await; } return Err(err); @@ -97,7 +83,9 @@ impl diff_service_server::DiffService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_diff_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_diff_client(self, inner.repository.as_ref(), false).await? + { let resp = client.get_patch(inner).await?; let stream = super::bridge_server_stream(resp.into_inner()); return Ok(tonic::Response::new(stream)); @@ -127,7 +115,9 @@ impl diff_service_server::DiffService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_diff_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_diff_client(self, inner.repository.as_ref(), false).await? + { return client.get_diff_stats(inner).await; } return Err(err); diff --git a/server/merge.rs b/server/merge.rs index 11724b4..bc81392 100644 --- a/server/merge.rs +++ b/server/merge.rs @@ -1,27 +1,9 @@ -use crate::pb::*; use crate::pb::merge_service_client::MergeServiceClient; +use crate::pb::*; use super::{GitksService, into_status}; -async fn remote_merge_client( - svc: &GitksService, - header: Option<&RepositoryHeader>, - is_write: bool, -) -> Result>, tonic::Status> { - let header = match header { - Some(h) => h, - None => return Ok(None), - }; - let Some(route) = svc.route_repository(header, is_write).await? else { - return Ok(None); - }; - tracing::info!(storage_name = %route.storage_name, relative_path = %route.relative_path, actor_name = %route.actor_name, grpc_addr = %route.grpc_addr, "forwarding merge rpc"); - let endpoint = super::remote_endpoint(&route.grpc_addr).await?; - let client = MergeServiceClient::connect(endpoint) - .await - .map_err(|e| tonic::Status::unavailable(e.to_string()))?; - Ok(Some(client)) -} +remote_client!(remote_merge_client, MergeServiceClient, "merge"); #[tonic::async_trait] impl merge_service_server::MergeService for GitksService { @@ -36,7 +18,9 @@ impl merge_service_server::MergeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_merge_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_merge_client(self, inner.repository.as_ref(), false).await? + { return client.check_merge(inner).await; } return Err(err); @@ -60,7 +44,9 @@ impl merge_service_server::MergeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_merge_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_merge_client(self, inner.repository.as_ref(), true).await? + { return client.merge(inner).await; } return Err(err); @@ -84,7 +70,9 @@ impl merge_service_server::MergeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_merge_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_merge_client(self, inner.repository.as_ref(), false).await? + { return client.list_merge_conflicts(inner).await; } return Err(err); @@ -108,7 +96,9 @@ impl merge_service_server::MergeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_merge_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_merge_client(self, inner.repository.as_ref(), true).await? + { return client.resolve_merge_conflicts(inner).await; } return Err(err); @@ -133,7 +123,9 @@ impl merge_service_server::MergeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_merge_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_merge_client(self, inner.repository.as_ref(), true).await? + { return client.rebase(inner).await; } return Err(err); diff --git a/server/mod.rs b/server/mod.rs index 4d86f83..94ca4e7 100644 --- a/server/mod.rs +++ b/server/mod.rs @@ -1,3 +1,35 @@ +/// Generate a `remote__client` helper function that resolves a repository +/// route and returns a connected gRPC client for the given service. +macro_rules! remote_client { + ($fn_name:ident, $client:ty, $svc_label:literal) => { + async fn $fn_name( + svc: &super::GitksService, + header: Option<&crate::pb::RepositoryHeader>, + is_write: bool, + ) -> Result, tonic::Status> { + let header = match header { + Some(h) => h, + None => return Ok(None), + }; + let Some(route) = svc.route_repository(header, is_write).await? else { + return Ok(None); + }; + tracing::info!( + storage_name = %route.storage_name, + relative_path = %route.relative_path, + actor_name = %route.actor_name, + grpc_addr = %route.grpc_addr, + concat!("forwarding ", $svc_label, " rpc") + ); + let endpoint = super::remote_endpoint(&route.grpc_addr).await?; + let client = <$client>::connect(endpoint) + .await + .map_err(|e| tonic::Status::unavailable(e.to_string()))?; + Ok(Some(client)) + } + }; +} + mod archive; mod blame; mod branch; @@ -11,9 +43,9 @@ mod repository_maint; mod tag; mod tree; -use std::path::{Path, PathBuf}; use gix::discover::is_git; use ractor::{ActorCell, ActorRef}; +use std::path::{Path, PathBuf}; use tokio_stream::wrappers::ReceiverStream; use crate::actor::message::{GitNodeMessage, RouteDecision}; @@ -34,7 +66,11 @@ pub struct GitksService { impl GitksService { pub fn new(repo_prefix: PathBuf) -> Self { - Self { repo_prefix, node_actor: None, grpc_addr: String::new() } + Self { + repo_prefix, + node_actor: None, + grpc_addr: String::new(), + } } pub fn with_actor(mut self, node_actor: ActorRef) -> Self { @@ -74,20 +110,23 @@ impl GitksService { if local.as_ref().is_some_and(|actor| actor == &member) { continue; } - if let Some(decision) = query_find_primary(member.clone(), header.clone()).await? { - if decision.found && !decision.grpc_addr.is_empty() { - primary = Some(decision); - if is_write { - return Ok(primary); - } + if let Some(decision) = query_find_primary(member.clone(), header.clone()).await? + && decision.found + && !decision.grpc_addr.is_empty() + { + primary = Some(decision); + if is_write { + return Ok(primary); } } - if !is_write && replica.is_none() { - if let Some(decision) = query_find_replica(member.clone(), header.clone()).await? { - if decision.found && !decision.grpc_addr.is_empty() && decision.role == ROLE_REPLICA { - replica = Some(decision); - } - } + if !is_write + && replica.is_none() + && let Some(decision) = query_find_replica(member.clone(), header.clone()).await? + && decision.found + && !decision.grpc_addr.is_empty() + && decision.role == ROLE_REPLICA + { + replica = Some(decision); } } if let Some(p) = primary { @@ -142,20 +181,56 @@ impl GitksService { if relative_path.is_empty() { return Err(tonic::Status::invalid_argument("relative_path is required")); } + // Validate early to reject '..' and other traversal patterns + crate::sanitize::validate_relative_path(relative_path) + .map_err(|e| tonic::Status::invalid_argument(e.to_string()))?; + let candidate = self.repo_prefix.join(relative_path); - // Path traversal check - let canonical = candidate - .canonicalize() - .unwrap_or_else(|_| candidate.clone()); + // Canonicalize repo_prefix (which should exist) for a reliable check let prefix_canon = self .repo_prefix .canonicalize() .unwrap_or_else(|_| self.repo_prefix.clone()); + + // Unified path validation to avoid TOCTOU + let canonical = match candidate.canonicalize() { + Ok(canon) => { + // Path exists and was canonicalized + canon + } + Err(_) => { + // Path doesn't exist yet — validate via parent + let parent = candidate.parent().unwrap_or(&self.repo_prefix); + let filename = candidate.file_name().ok_or_else(|| { + tonic::Status::invalid_argument("invalid path: missing filename") + })?; + + let parent_canon = parent + .canonicalize() + .unwrap_or_else(|_| parent.to_path_buf()); + let constructed = parent_canon.join(filename); + + // String-level verification for non-existent paths + let constructed_str = constructed.to_string_lossy(); + let prefix_str = prefix_canon.to_string_lossy(); + + if !constructed_str.starts_with(&*prefix_str) { + return Err(tonic::Status::invalid_argument( + "path traversal detected: relative_path escapes repo prefix", + )); + } + + constructed + } + }; + + // Final check: canonical must be under prefix if !canonical.starts_with(&prefix_canon) { return Err(tonic::Status::invalid_argument( "path traversal detected: relative_path escapes repo prefix", )); } + Ok(canonical) } @@ -166,6 +241,9 @@ impl GitksService { old_oid: &str, new_oid: &str, ) { + // Invalidate caches that depend on this repository + crate::server::cache::invalidate_repo(relative_path); + if let Some(ref actor) = self.node_actor { let event = crate::actor::message::RefUpdateEvent { relative_path: relative_path.to_string(), @@ -189,14 +267,11 @@ impl GitksService { } } - - pub async fn remote_endpoint(addr: &str) -> Result { let uri: tonic::codegen::http::Uri = addr .parse() .map_err(|e| tonic::Status::invalid_argument(format!("invalid URI: {e}")))?; - tonic::transport::Endpoint::new(uri) - .map_err(|e| tonic::Status::internal(e.to_string())) + tonic::transport::Endpoint::new(uri).map_err(|e| tonic::Status::internal(e.to_string())) } pub(super) fn bridge_server_stream( diff --git a/server/pack.rs b/server/pack.rs index 1459a44..4be83e5 100644 --- a/server/pack.rs +++ b/server/pack.rs @@ -1,30 +1,12 @@ use tokio_stream::StreamExt; use tokio_stream::wrappers::ReceiverStream; -use crate::pb::*; use crate::pb::pack_service_client::PackServiceClient; +use crate::pb::*; use super::{GitksService, into_status}; -async fn remote_pack_client( - svc: &GitksService, - header: Option<&RepositoryHeader>, - is_write: bool, -) -> Result>, tonic::Status> { - let header = match header { - Some(h) => h, - None => return Ok(None), - }; - let Some(route) = svc.route_repository(header, is_write).await? else { - return Ok(None); - }; - tracing::info!(storage_name = %route.storage_name, relative_path = %route.relative_path, actor_name = %route.actor_name, grpc_addr = %route.grpc_addr, "forwarding pack rpc"); - let endpoint = super::remote_endpoint(&route.grpc_addr).await?; - let client = PackServiceClient::connect(endpoint) - .await - .map_err(|e| tonic::Status::unavailable(e.to_string()))?; - Ok(Some(client)) -} +remote_client!(remote_pack_client, PackServiceClient, "pack"); #[tonic::async_trait] impl pack_service_server::PackService for GitksService { @@ -43,7 +25,9 @@ impl pack_service_server::PackService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_pack_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_pack_client(self, inner.repository.as_ref(), false).await? + { return client.advertise_refs(inner).await; } return Err(err); @@ -70,19 +54,27 @@ impl pack_service_server::PackService for GitksService { let gb = match self.resolve(first.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_pack_client(self, first.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_pack_client(self, first.repository.as_ref(), false).await? + { let (tx, rx) = tokio::sync::mpsc::channel(16); let _ = tx.send(first).await; tokio::spawn(async move { use tokio_stream::StreamExt; while let Some(msg) = stream.next().await { match msg { - Ok(m) => { if tx.send(m).await.is_err() { break; } } + Ok(m) => { + if tx.send(m).await.is_err() { + break; + } + } Err(_) => break, } } }); - let resp = client.upload_pack(tokio_stream::wrappers::ReceiverStream::new(rx)).await?; + let resp = client + .upload_pack(tokio_stream::wrappers::ReceiverStream::new(rx)) + .await?; let out = super::bridge_server_stream(resp.into_inner()); return Ok(tonic::Response::new(out)); } @@ -123,19 +115,27 @@ impl pack_service_server::PackService for GitksService { let gb = match self.resolve(first.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_pack_client(self, first.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_pack_client(self, first.repository.as_ref(), false).await? + { let (tx, rx) = tokio::sync::mpsc::channel(16); let _ = tx.send(first).await; tokio::spawn(async move { use tokio_stream::StreamExt; while let Some(msg) = stream.next().await { match msg { - Ok(m) => { if tx.send(m).await.is_err() { break; } } + Ok(m) => { + if tx.send(m).await.is_err() { + break; + } + } Err(_) => break, } } }); - let resp = client.receive_pack(tokio_stream::wrappers::ReceiverStream::new(rx)).await?; + let resp = client + .receive_pack(tokio_stream::wrappers::ReceiverStream::new(rx)) + .await?; let out = super::bridge_server_stream(resp.into_inner()); return Ok(tonic::Response::new(out)); } @@ -172,7 +172,9 @@ impl pack_service_server::PackService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_pack_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_pack_client(self, inner.repository.as_ref(), false).await? + { let resp = client.pack_objects(inner).await?; let stream = super::bridge_server_stream(resp.into_inner()); return Ok(tonic::Response::new(stream)); @@ -201,7 +203,13 @@ impl pack_service_server::PackService for GitksService { let gb = match self.resolve(inputs.first().and_then(|r| r.repository.as_ref())) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_pack_client(self, inputs.first().and_then(|r| r.repository.as_ref()), false).await? { + if let Some(mut client) = remote_pack_client( + self, + inputs.first().and_then(|r| r.repository.as_ref()), + false, + ) + .await? + { return client.index_pack(tokio_stream::iter(inputs)).await; } return Err(err); @@ -224,7 +232,9 @@ impl pack_service_server::PackService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_pack_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_pack_client(self, inner.repository.as_ref(), false).await? + { return client.list_packfiles(inner).await; } return Err(err); @@ -247,7 +257,9 @@ impl pack_service_server::PackService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_pack_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_pack_client(self, inner.repository.as_ref(), false).await? + { return client.fsck(inner).await; } return Err(err); diff --git a/server/repository.rs b/server/repository.rs index 8865187..af55458 100644 --- a/server/repository.rs +++ b/server/repository.rs @@ -1,27 +1,9 @@ -use crate::pb::*; use crate::pb::repository_service_client::RepositoryServiceClient; +use crate::pb::*; -use super::{GitksService, git_cmd, into_status, repository_maint, remote_endpoint}; +use super::{GitksService, git_cmd, into_status, repository_maint}; -async fn remote_repository_client( - svc: &GitksService, - header: Option<&RepositoryHeader>, - is_write: bool, -) -> Result>, tonic::Status> { - let header = match header { - Some(h) => h, - None => return Ok(None), - }; - let Some(route) = svc.route_repository(header, is_write).await? else { - return Ok(None); - }; - tracing::info!(storage_name = %route.storage_name, relative_path = %route.relative_path, actor_name = %route.actor_name, grpc_addr = %route.grpc_addr, "forwarding repository rpc"); - let endpoint = remote_endpoint(&route.grpc_addr).await?; - let client = RepositoryServiceClient::connect(endpoint) - .await - .map_err(|e| tonic::Status::unavailable(e.to_string()))?; - Ok(Some(client)) -} +remote_client!(remote_repository_client, RepositoryServiceClient, "repository"); fn default_branch_name(gb: &crate::bare::GitBare) -> String { git_cmd(gb, &["symbolic-ref", "HEAD"]) @@ -48,7 +30,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), false).await? + { return client.get_repository(inner).await; } return Err(err); @@ -95,10 +79,11 @@ impl repository_service_server::RepositoryService for GitksService { let span = tracing::info_span!("repo.delete_repository", %repo); let _enter = span.enter(); let bare_dir = self.resolve_for_init(inner.repository.as_ref())?; - if !bare_dir.exists() { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), true).await? { - return client.delete_repository(inner).await; - } + if !bare_dir.exists() + && let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), true).await? + { + return client.delete_repository(inner).await; } tracing::warn!(%repo, path = %bare_dir.display(), "deleting repository"); std::fs::remove_dir_all(&bare_dir).map_err(|e| tonic::Status::internal(e.to_string()))?; @@ -117,10 +102,11 @@ impl repository_service_server::RepositoryService for GitksService { let _enter = span.enter(); let bare_dir = self.resolve_for_init(inner.repository.as_ref())?; let exists = bare_dir.exists() && bare_dir.is_dir() && bare_dir.join("HEAD").exists(); - if !exists { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), false).await? { - return client.repository_exists(inner).await; - } + if !exists + && let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), false).await? + { + return client.repository_exists(inner).await; } Ok(tonic::Response::new(RepositoryExistsResponse { exists })) } @@ -136,7 +122,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), false).await? + { return client.get_object_format(inner).await; } return Err(err); @@ -159,7 +147,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), false).await? + { return client.get_default_branch(inner).await; } return Err(err); @@ -183,7 +173,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), true).await? + { return client.set_default_branch(inner).await; } return Err(err); @@ -213,7 +205,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), false).await? + { return client.get_repository_config(inner).await; } return Err(err); @@ -238,6 +232,8 @@ impl repository_service_server::RepositoryService for GitksService { } } else { for key in &inner.keys { + crate::sanitize::validate_config_key(key) + .map_err(|e| tonic::Status::invalid_argument(e.to_string()))?; let out = git_cmd(&gb, &["config", "--get-all", key])?; if out.status.success() { let vals: Vec = String::from_utf8_lossy(&out.stdout) @@ -270,7 +266,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), true).await? + { return client.set_repository_config(inner).await; } return Err(err); @@ -278,6 +276,8 @@ impl repository_service_server::RepositoryService for GitksService { Err(err) => return Err(err), }; for entry in &inner.entries { + crate::sanitize::validate_config_key(&entry.key) + .map_err(|e| tonic::Status::invalid_argument(e.to_string()))?; if entry.values.is_empty() { git_cmd(&gb, &["config", "--unset-all", &entry.key])?; } else { @@ -305,7 +305,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), false).await? + { return client.get_repository_statistics(inner).await; } return Err(err); @@ -326,7 +328,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), false).await? + { return client.check_repository_health(inner).await; } return Err(err); @@ -349,7 +353,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), true).await? + { return client.garbage_collect(inner).await; } return Err(err); @@ -372,7 +378,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), true).await? + { return client.repack(inner).await; } return Err(err); @@ -400,7 +408,9 @@ impl repository_service_server::RepositoryService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_repository_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_repository_client(self, inner.repository.as_ref(), true).await? + { return client.write_commit_graph(inner).await; } return Err(err); diff --git a/server/repository_maint.rs b/server/repository_maint.rs index 893aa32..fc98047 100644 --- a/server/repository_maint.rs +++ b/server/repository_maint.rs @@ -39,17 +39,14 @@ fn dir_size(gb: &crate::bare::GitBare) -> u64 { } fn count_refs(gb: &crate::bare::GitBare) -> u64 { - let out = git_cmd(gb, &["for-each-ref", "--format=%(refname)"]).unwrap_or_else(|_| { - std::process::Output { - status: Default::default(), - stdout: Vec::new(), - stderr: Vec::new(), - } - }); - String::from_utf8_lossy(&out.stdout) - .lines() - .filter(|l| !l.is_empty()) - .count() as u64 + let out = git_cmd(gb, &["for-each-ref", "--format=%(refname)"]).ok(); + out.map(|o| { + String::from_utf8_lossy(&o.stdout) + .lines() + .filter(|l| !l.is_empty()) + .count() as u64 + }) + .unwrap_or(0) } fn file_len(path: &std::path::Path) -> u64 { diff --git a/server/tag.rs b/server/tag.rs index 9074c2e..2622b11 100644 --- a/server/tag.rs +++ b/server/tag.rs @@ -1,27 +1,9 @@ -use crate::pb::*; use crate::pb::tag_service_client::TagServiceClient; +use crate::pb::*; use super::{GitksService, into_status}; -async fn remote_tag_client( - svc: &GitksService, - header: Option<&RepositoryHeader>, - is_write: bool, -) -> Result>, tonic::Status> { - let header = match header { - Some(h) => h, - None => return Ok(None), - }; - let Some(route) = svc.route_repository(header, is_write).await? else { - return Ok(None); - }; - tracing::info!(storage_name = %route.storage_name, relative_path = %route.relative_path, actor_name = %route.actor_name, grpc_addr = %route.grpc_addr, "forwarding tag rpc"); - let endpoint = super::remote_endpoint(&route.grpc_addr).await?; - let client = TagServiceClient::connect(endpoint) - .await - .map_err(|e| tonic::Status::unavailable(e.to_string()))?; - Ok(Some(client)) -} +remote_client!(remote_tag_client, TagServiceClient, "tag"); #[tonic::async_trait] impl tag_service_server::TagService for GitksService { @@ -36,7 +18,9 @@ impl tag_service_server::TagService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tag_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_tag_client(self, inner.repository.as_ref(), false).await? + { return client.list_tags(inner).await; } return Err(err); @@ -60,7 +44,9 @@ impl tag_service_server::TagService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tag_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_tag_client(self, inner.repository.as_ref(), false).await? + { return client.get_tag(inner).await; } return Err(err); @@ -83,7 +69,9 @@ impl tag_service_server::TagService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tag_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_tag_client(self, inner.repository.as_ref(), true).await? + { return client.create_tag(inner).await; } return Err(err); @@ -108,7 +96,9 @@ impl tag_service_server::TagService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tag_client(self, inner.repository.as_ref(), true).await? { + if let Some(mut client) = + remote_tag_client(self, inner.repository.as_ref(), true).await? + { return client.delete_tag(inner).await; } return Err(err); @@ -133,7 +123,9 @@ impl tag_service_server::TagService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tag_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_tag_client(self, inner.repository.as_ref(), false).await? + { return client.verify_tag(inner).await; } return Err(err); diff --git a/server/tree.rs b/server/tree.rs index 9e111a6..862538d 100644 --- a/server/tree.rs +++ b/server/tree.rs @@ -1,27 +1,9 @@ -use crate::pb::*; use crate::pb::tree_service_client::TreeServiceClient; +use crate::pb::*; use super::{GitksService, cache, into_status, into_stream}; -async fn remote_tree_client( - svc: &GitksService, - header: Option<&RepositoryHeader>, - is_write: bool, -) -> Result>, tonic::Status> { - let header = match header { - Some(h) => h, - None => return Ok(None), - }; - let Some(route) = svc.route_repository(header, is_write).await? else { - return Ok(None); - }; - tracing::info!(storage_name = %route.storage_name, relative_path = %route.relative_path, actor_name = %route.actor_name, grpc_addr = %route.grpc_addr, "forwarding tree rpc"); - let endpoint = super::remote_endpoint(&route.grpc_addr).await?; - let client = TreeServiceClient::connect(endpoint) - .await - .map_err(|e| tonic::Status::unavailable(e.to_string()))?; - Ok(Some(client)) -} +remote_client!(remote_tree_client, TreeServiceClient, "tree"); #[tonic::async_trait] impl tree_service_server::TreeService for GitksService { @@ -39,7 +21,9 @@ impl tree_service_server::TreeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tree_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_tree_client(self, inner.repository.as_ref(), false).await? + { return client.list_tree(inner).await; } return Err(err); @@ -68,7 +52,9 @@ impl tree_service_server::TreeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tree_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_tree_client(self, inner.repository.as_ref(), false).await? + { return client.get_tree(inner).await; } return Err(err); @@ -97,7 +83,9 @@ impl tree_service_server::TreeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tree_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_tree_client(self, inner.repository.as_ref(), false).await? + { return client.get_blob(inner).await; } return Err(err); @@ -125,7 +113,9 @@ impl tree_service_server::TreeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tree_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_tree_client(self, inner.repository.as_ref(), false).await? + { let resp = client.get_raw_blob(inner).await?; let stream = super::bridge_server_stream(resp.into_inner()); return Ok(tonic::Response::new(stream)); @@ -159,7 +149,9 @@ impl tree_service_server::TreeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tree_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_tree_client(self, inner.repository.as_ref(), false).await? + { return client.get_file_metadata(inner).await; } return Err(err); @@ -187,7 +179,9 @@ impl tree_service_server::TreeService for GitksService { let gb = match self.resolve(inner.repository.as_ref()) { Ok(gb) => gb, Err(err) if err.code() == tonic::Code::NotFound => { - if let Some(mut client) = remote_tree_client(self, inner.repository.as_ref(), false).await? { + if let Some(mut client) = + remote_tree_client(self, inner.repository.as_ref(), false).await? + { return client.find_files(inner).await; } return Err(err); diff --git a/tag/create_tag.rs b/tag/create_tag.rs index 054ba6b..f34ce9f 100644 --- a/tag/create_tag.rs +++ b/tag/create_tag.rs @@ -4,9 +4,13 @@ use crate::pb::{CreateTagRequest, GetTagRequest, Tag}; impl GitBare { pub fn create_tag(&self, request: CreateTagRequest) -> GitResult { + crate::sanitize::validate_ref_name(&request.name)?; let target = match request.target.and_then(|s| s.selector) { Some(crate::pb::object_selector::Selector::Oid(oid)) => oid.hex, - Some(crate::pb::object_selector::Selector::Revision(name)) => name.revision, + Some(crate::pb::object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision + } None => "HEAD".into(), }; let mut args = vec![ diff --git a/tests/archive_test.rs b/tests/archive_test.rs index c02923d..ebe5ef7 100644 --- a/tests/archive_test.rs +++ b/tests/archive_test.rs @@ -13,7 +13,7 @@ fn hdr(name: &str) -> RepositoryHeader { #[tokio::test] async fn test_get_archive_tar() { - let (dir, gb) = common::setup_bare_repo(); + let (dir, _gb) = common::setup_bare_repo(); let svc = common::setup_service(dir.path()); let chunks = svc .get_archive(tonic::Request::new(ArchiveRequest { @@ -40,7 +40,7 @@ async fn test_get_archive_tar() { #[tokio::test] async fn test_get_archive_zip() { - let (dir, gb) = common::setup_bare_repo(); + let (dir, _gb) = common::setup_bare_repo(); let svc = common::setup_service(dir.path()); let chunks = svc .get_archive(tonic::Request::new(ArchiveRequest { @@ -70,7 +70,7 @@ async fn test_get_archive_zip() { #[tokio::test] async fn test_list_archive_entries() { - let (dir, gb) = common::setup_bare_repo(); + let (dir, _gb) = common::setup_bare_repo(); let svc = common::setup_service(dir.path()); let result = svc .list_archive_entries(tonic::Request::new(ListArchiveEntriesRequest { @@ -98,7 +98,7 @@ async fn test_list_archive_entries() { #[tokio::test] async fn test_get_archive_with_prefix() { - let (dir, gb) = common::setup_bare_repo(); + let (dir, _gb) = common::setup_bare_repo(); let svc = common::setup_service(dir.path()); let chunks = svc .get_archive(tonic::Request::new(ArchiveRequest { @@ -124,7 +124,7 @@ async fn test_get_archive_with_prefix() { #[tokio::test] async fn test_fsck_clean_repo() { - let (dir, gb) = common::setup_bare_repo(); + let (dir, _gb) = common::setup_bare_repo(); let svc = common::setup_service(dir.path()); let result = svc .fsck(tonic::Request::new(FsckRequest { @@ -209,7 +209,7 @@ async fn test_list_packfiles() { #[tokio::test] async fn test_advertise_refs() { - let (dir, gb) = common::setup_bare_repo(); + let (dir, _gb) = common::setup_bare_repo(); let svc = common::setup_service(dir.path()); let result = svc .advertise_refs(tonic::Request::new(AdvertiseRefsRequest { diff --git a/tests/bare_test.rs b/tests/bare_test.rs index 6af2e7b..870b785 100644 --- a/tests/bare_test.rs +++ b/tests/bare_test.rs @@ -6,7 +6,7 @@ use gitks::pb::RepositoryHeader; #[test] fn test_from_header_valid() { - let (dir, gb) = common::setup_bare_repo(); + let (_dir, gb) = common::setup_bare_repo(); let parent = gb.bare_dir.parent().unwrap().to_string_lossy().into_owned(); let name = gb .bare_dir diff --git a/tests/blame_test.rs b/tests/blame_test.rs index 41995db..ac0bb30 100644 --- a/tests/blame_test.rs +++ b/tests/blame_test.rs @@ -124,11 +124,11 @@ async fn test_blame_author_info() { let hunk = &result.hunks[0]; let commit = hunk.commit.as_ref().unwrap(); - if let Some(ref author) = commit.author { - if let Some(ref id) = author.identity { - assert_eq!(id.name, "Test", "author name should match"); - assert_eq!(id.email, "test@example.com"); - } + if let Some(ref author) = commit.author + && let Some(ref id) = author.identity + { + assert_eq!(id.name, "Test", "author name should match"); + assert_eq!(id.email, "test@example.com"); } } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 3fdd699..35043da 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,6 +1,7 @@ use gitks::bare::GitBare; use gitks::server::GitksService; +#[allow(dead_code)] pub fn setup_service(dir: &std::path::Path) -> GitksService { GitksService::new(dir.to_path_buf()) } @@ -104,6 +105,7 @@ pub fn setup_bare_repo() -> (tempfile::TempDir, GitBare) { (dir, GitBare::new(bare_dir)) } +#[allow(dead_code)] pub fn setup_bare_repo_with_conflict() -> (tempfile::TempDir, GitBare) { let dir = tempfile::tempdir().expect("create temp dir"); let bare_dir = dir.path().join("test-repo"); diff --git a/tests/macro_test.rs b/tests/macro_test.rs new file mode 100644 index 0000000..8bb1141 --- /dev/null +++ b/tests/macro_test.rs @@ -0,0 +1,184 @@ +use gitks::error::GitResult; +use gitks::pb::object_selector::Selector; +use gitks::pb::{ObjectName, ObjectSelector}; + +fn test_macro(selector: Option) -> GitResult { + let result = gitks::resolve_revision!(selector); + Ok(result) +} + +fn test_macro_with_default(selector: Option, default: &str) -> GitResult { + let result = gitks::resolve_revision!(selector, default); + Ok(result) +} + +#[test] +fn test_resolve_revision_with_oid() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Oid(gitks::pb::Oid { + hex: "abc1234567890123456789012345678901234567".to_string(), + value: vec![], + format: gitks::pb::ObjectFormat::Sha1 as i32, + })), + }); + + let result = test_macro(selector).unwrap(); + assert_eq!(result, "abc1234567890123456789012345678901234567"); +} + +#[test] +fn test_resolve_revision_with_valid_revision() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { + revision: "main".to_string(), + })), + }); + + let result = test_macro(selector).unwrap(); + assert_eq!(result, "main"); +} + +#[test] +fn test_resolve_revision_with_valid_branch() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { + revision: "feature/new-api".to_string(), + })), + }); + + let result = test_macro(selector).unwrap(); + assert_eq!(result, "feature/new-api"); +} + +#[test] +fn test_resolve_revision_with_head() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { + revision: "HEAD".to_string(), + })), + }); + + let result = test_macro(selector).unwrap(); + assert_eq!(result, "HEAD"); +} + +#[test] +fn test_resolve_revision_with_ancestry() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { + revision: "main~3".to_string(), + })), + }); + + let result = test_macro(selector).unwrap(); + assert_eq!(result, "main~3"); +} + +#[test] +fn test_resolve_revision_none_defaults_to_head() { + let result = test_macro(None).unwrap(); + assert_eq!(result, "HEAD"); +} + +#[test] +fn test_resolve_revision_with_custom_default() { + let result = test_macro_with_default(None, "develop").unwrap(); + assert_eq!(result, "develop"); +} + +#[test] +fn test_resolve_revision_empty_selector() { + let selector = Some(ObjectSelector { selector: None }); + let result = test_macro(selector).unwrap(); + assert_eq!(result, "HEAD"); +} + +#[test] +fn test_resolve_revision_empty_with_custom_default() { + let selector = Some(ObjectSelector { selector: None }); + let result = test_macro_with_default(selector, "custom-branch").unwrap(); + assert_eq!(result, "custom-branch"); +} + +#[test] +fn test_resolve_revision_rejects_dangerous() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { + revision: "branch;rm -rf /".to_string(), + })), + }); + + let result = test_macro(selector); + assert!(result.is_err()); +} + +#[test] +fn test_resolve_revision_rejects_traversal() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { + revision: "../etc/passwd".to_string(), + })), + }); + + let result = test_macro(selector); + assert!(result.is_err()); +} + +#[test] +fn test_resolve_revision_rejects_excessive_depth() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { + revision: "main~99999".to_string(), + })), + }); + + let result = test_macro(selector); + assert!(result.is_err()); +} + +#[test] +fn test_resolve_revision_rejects_too_long() { + let long_rev = "a".repeat(300); + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { revision: long_rev })), + }); + + let result = test_macro(selector); + assert!(result.is_err()); +} + +#[test] +fn test_resolve_revision_accepts_valid_hex() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { + revision: "deadbeef1234567890abcdef".to_string(), + })), + }); + + let result = test_macro(selector).unwrap(); + assert_eq!(result, "deadbeef1234567890abcdef"); +} + +#[test] +fn test_resolve_revision_accepts_ref_prefix() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { + revision: "ref:refs/heads/main".to_string(), + })), + }); + + let result = test_macro(selector).unwrap(); + assert_eq!(result, "ref:refs/heads/main"); +} + +#[test] +fn test_resolve_revision_accepts_tree_suffix() { + let selector = Some(ObjectSelector { + selector: Some(Selector::Revision(ObjectName { + revision: "main^{tree}".to_string(), + })), + }); + + let result = test_macro(selector).unwrap(); + assert_eq!(result, "main^{tree}"); +} diff --git a/tests/sanitize_test.rs b/tests/sanitize_test.rs new file mode 100644 index 0000000..fff8b77 --- /dev/null +++ b/tests/sanitize_test.rs @@ -0,0 +1,283 @@ +use gitks::sanitize::*; + +// ==================== validate_ref_name tests ==================== + +#[test] +fn test_validate_ref_name_accepts_valid_names() { + assert!(validate_ref_name("main").is_ok()); + assert!(validate_ref_name("master").is_ok()); + assert!(validate_ref_name("feature/new-api").is_ok()); + assert!(validate_ref_name("hotfix/bug-fix-123").is_ok()); + assert!(validate_ref_name("v1.0.0").is_ok()); + assert!(validate_ref_name("release-2024").is_ok()); + assert!(validate_ref_name("user/feature/branch").is_ok()); +} + +#[test] +fn test_validate_ref_name_rejects_empty() { + assert!(validate_ref_name("").is_err()); +} + +#[test] +fn test_validate_ref_name_rejects_dot_prefix_suffix() { + assert!(validate_ref_name(".branch").is_err()); + assert!(validate_ref_name("branch.").is_err()); + assert!(validate_ref_name(".branch.").is_err()); +} + +#[test] +fn test_validate_ref_name_rejects_slash_suffix() { + assert!(validate_ref_name("branch/").is_err()); +} + +#[test] +fn test_validate_ref_name_rejects_double_dot() { + assert!(validate_ref_name("feature..branch").is_err()); + assert!(validate_ref_name("..branch").is_err()); + assert!(validate_ref_name("branch..").is_err()); +} + +#[test] +fn test_validate_ref_name_rejects_at_brace() { + assert!(validate_ref_name("branch@{1}").is_err()); + assert!(validate_ref_name("@{upstream}").is_err()); + assert!(validate_ref_name("feature/@{branch}").is_err()); +} + +#[test] +fn test_validate_ref_name_rejects_forbidden_chars() { + assert!(validate_ref_name("branch~1").is_err()); + assert!(validate_ref_name("branch^1").is_err()); + assert!(validate_ref_name("branch:feature").is_err()); + assert!(validate_ref_name("branch?query").is_err()); + assert!(validate_ref_name("branch*glob").is_err()); + assert!(validate_ref_name("branch[0]").is_err()); + assert!(validate_ref_name("branch\\escape").is_err()); + assert!(validate_ref_name("branch name").is_err()); + assert!(validate_ref_name("branch\ttab").is_err()); + assert!(validate_ref_name("branch\nnewline").is_err()); + assert!(validate_ref_name("branch\rreturn").is_err()); + assert!(validate_ref_name("branch\0null").is_err()); +} + +#[test] +fn test_validate_ref_name_rejects_too_long() { + let long_name = "a".repeat(256); + assert!(validate_ref_name(&long_name).is_err()); + + let max_valid_name = "a".repeat(255); + assert!(validate_ref_name(&max_valid_name).is_ok()); +} + +// ==================== validate_revision tests ==================== + +#[test] +fn test_validate_revision_accepts_empty() { + assert!(validate_revision("").is_err()); +} + +#[test] +fn test_validate_revision_accepts_head() { + assert!(validate_revision("HEAD").is_ok()); +} + +#[test] +fn test_validate_revision_accepts_valid_hex() { + assert!(validate_revision("abc1234").is_ok()); + assert!(validate_revision("abc1234567890123456789012345678901234567890").is_ok()); + assert!(validate_revision("deadbeef").is_ok()); +} + +#[test] +fn test_validate_revision_rejects_invalid_hex_length() { + // "abc" is 3 chars - too short to be hex OID (requires 4-64), but valid as branch name + assert!(validate_revision("abc").is_ok()); + + let too_long = "a".repeat(65); + // 65 hex chars - too long to be hex OID, but might be valid as branch name + // However, it will fail ref name length check (> 255 chars would fail, but 65 is fine) + // Actually 65 chars of 'a' is a valid branch name, so it should pass + assert!(validate_revision(&too_long).is_ok()); +} + +#[test] +fn test_validate_revision_accepts_ref_prefix() { + assert!(validate_revision("ref:refs/heads/main").is_ok()); + assert!(validate_revision("ref:refs/tags/v1.0.0").is_ok()); +} + +#[test] +fn test_validate_revision_accepts_ancestry_operators() { + assert!(validate_revision("main~1").is_ok()); + assert!(validate_revision("main~10").is_ok()); + assert!(validate_revision("HEAD~10000").is_ok()); + assert!(validate_revision("main^1").is_ok()); + assert!(validate_revision("main^2").is_ok()); + assert!(validate_revision("HEAD^10000").is_ok()); +} + +#[test] +fn test_validate_revision_rejects_excessive_depth() { + assert!(validate_revision("main~10001").is_err()); + assert!(validate_revision("main~999999999999").is_err()); + assert!(validate_revision("main^10001").is_err()); + assert!(validate_revision("main^999999999999").is_err()); +} + +#[test] +fn test_validate_revision_accepts_tree_suffix() { + assert!(validate_revision("main^{tree}").is_ok()); + assert!(validate_revision("HEAD^{tree}").is_ok()); +} + +#[test] +fn test_validate_revision_rejects_too_long() { + // Test length limit (256 chars) - use a simple branch name to avoid depth checks + let long_rev = "a".repeat(257); + assert!(validate_revision(&long_rev).is_err()); + + // For non-hex revisions, the effective limit is 255 chars (ref name limit) + let max_valid = "a".repeat(255); + assert!(validate_revision(&max_valid).is_ok()); +} + +#[test] +fn test_validate_revision_accepts_valid_branch_names() { + assert!(validate_revision("main").is_ok()); + assert!(validate_revision("feature/new-api").is_ok()); + // v1.0.0 contains dots but they're not at start/end, so it's valid + assert!(validate_revision("v1.0.0").is_ok()); +} + +// ==================== validate_file_path tests ==================== + +#[test] +fn test_validate_file_path_accepts_valid_paths() { + assert!(validate_file_path("file.txt").is_ok()); + assert!(validate_file_path("src/main.rs").is_ok()); + assert!(validate_file_path("deep/nested/path/file.js").is_ok()); + assert!(validate_file_path("README.md").is_ok()); +} + +#[test] +fn test_validate_file_path_rejects_empty() { + assert!(validate_file_path("").is_err()); +} + +#[test] +fn test_validate_file_path_rejects_absolute_paths() { + assert!(validate_file_path("/etc/passwd").is_err()); + assert!(validate_file_path("/absolute/path").is_err()); + assert!(validate_file_path("/file.txt").is_err()); +} + +#[test] +fn test_validate_file_path_rejects_path_traversal() { + assert!(validate_file_path("../escape").is_err()); + assert!(validate_file_path("path/../escape").is_err()); + assert!(validate_file_path("path/../../escape").is_err()); + assert!(validate_file_path("..").is_err()); +} + +#[test] +fn test_validate_file_path_rejects_null_bytes() { + assert!(validate_file_path("file\0.txt").is_err()); + assert!(validate_file_path("path/\0escape").is_err()); +} + +#[test] +fn test_validate_file_path_rejects_git_directory() { + assert!(validate_file_path(".git").is_err()); + assert!(validate_file_path(".git/config").is_err()); + assert!(validate_file_path(".git/hooks/pre-commit").is_err()); + assert!(validate_file_path("path/.git/config").is_err()); + assert!(validate_file_path(".git/").is_err()); +} + +#[test] +fn test_validate_file_path_rejects_too_long() { + let long_path = "a".repeat(4097); + assert!(validate_file_path(&long_path).is_err()); + + let max_valid_path = "a".repeat(4096); + assert!(validate_file_path(&max_valid_path).is_ok()); +} + +#[cfg(windows)] +#[test] +fn test_validate_file_path_rejects_windows_reserved_names() { + assert!(validate_file_path("CON").is_err()); + assert!(validate_file_path("PRN").is_err()); + assert!(validate_file_path("AUX").is_err()); + assert!(validate_file_path("NUL").is_err()); + assert!(validate_file_path("COM1").is_err()); + assert!(validate_file_path("LPT1").is_err()); + assert!(validate_file_path("path/CON").is_err()); + assert!(validate_file_path("CON.txt").is_err()); +} + +// ==================== validate_relative_path tests ==================== + +#[test] +fn test_validate_relative_path_accepts_valid_paths() { + assert!(validate_relative_path("repo").is_ok()); + assert!(validate_relative_path("path/to/repo").is_ok()); + assert!(validate_relative_path("user/project").is_ok()); +} + +#[test] +fn test_validate_relative_path_rejects_empty() { + assert!(validate_relative_path("").is_err()); +} + +#[test] +fn test_validate_relative_path_rejects_absolute() { + assert!(validate_relative_path("/absolute/path").is_err()); + assert!(validate_relative_path("/etc").is_err()); +} + +#[test] +fn test_validate_relative_path_rejects_traversal() { + assert!(validate_relative_path("../escape").is_err()); + assert!(validate_relative_path("path/../escape").is_err()); + assert!(validate_relative_path("..").is_err()); + assert!(validate_relative_path("path/..").is_err()); +} + +// ==================== validate_config_key tests ==================== + +#[test] +fn test_validate_config_key_accepts_safe_keys() { + assert!(validate_config_key("user.name").is_ok()); + assert!(validate_config_key("user.email").is_ok()); + assert!(validate_config_key("core.editor").is_ok()); + assert!(validate_config_key("alias.co").is_ok()); +} + +#[test] +fn test_validate_config_key_rejects_empty() { + assert!(validate_config_key("").is_err()); +} + +#[test] +fn test_validate_config_key_rejects_dangerous_keys() { + assert!(validate_config_key("core.sshCommand").is_err()); + assert!(validate_config_key("core.hooksPath").is_err()); + assert!(validate_config_key("safe.directory").is_err()); +} + +#[test] +fn test_validate_config_key_rejects_wildcard_dangerous_keys() { + assert!(validate_config_key("remote.origin.url").is_err()); + assert!(validate_config_key("remote.upstream.url").is_err()); + assert!(validate_config_key("http.proxy").is_err()); + assert!(validate_config_key("https.proxy").is_err()); +} + +#[test] +fn test_validate_config_key_rejects_invalid_chars() { + assert!(validate_config_key("key with space").is_err()); + assert!(validate_config_key("key;rm -rf").is_err()); + assert!(validate_config_key("key$(command)").is_err()); + assert!(validate_config_key("key`command`").is_err()); +} diff --git a/tree/get_file_metadata.rs b/tree/get_file_metadata.rs index c15d761..cb0c036 100644 --- a/tree/get_file_metadata.rs +++ b/tree/get_file_metadata.rs @@ -2,17 +2,14 @@ use gix::object::tree::EntryKind; use crate::bare::GitBare; use crate::error::{GitError, GitResult}; -use crate::pb::{FileMetadata, GetFileMetadataRequest, ObjectType, object_selector}; +use crate::pb::{FileMetadata, GetFileMetadataRequest, ObjectType}; +use crate::resolve_revision; use crate::tree; impl GitBare { pub fn get_file_metadata(&self, request: GetFileMetadataRequest) -> GitResult { let repo = self.gix_repo()?; - let revision = match request.revision.and_then(|s| s.selector) { - Some(object_selector::Selector::Oid(oid)) => oid.hex, - Some(object_selector::Selector::Revision(name)) => name.revision, - None => "HEAD".into(), - }; + let revision = resolve_revision!(request.revision); let tree = repo .rev_parse_single(format!("{}^{{tree}}", revision).as_str())? .object()? diff --git a/tree/get_tree.rs b/tree/get_tree.rs index 30e9c24..5a33208 100644 --- a/tree/get_tree.rs +++ b/tree/get_tree.rs @@ -1,25 +1,19 @@ use crate::bare::GitBare; use crate::error::{GitError, GitResult}; use crate::pb::{GetTreeRequest, ListTreeRequest, Tree}; +use crate::resolve_revision; impl GitBare { pub fn get_tree(&self, request: GetTreeRequest) -> GitResult { let entries = self.list_tree(ListTreeRequest { - repository: request.repository, + repository: request.repository.clone(), revision: request.revision.clone(), path: request.path.clone(), recursive: false, pagination: None, })?; let repo = self.gix_repo()?; - let revision = request - .revision - .and_then(|s| s.selector) - .map(|s| match s { - crate::pb::object_selector::Selector::Oid(oid) => oid.hex, - crate::pb::object_selector::Selector::Revision(name) => name.revision, - }) - .unwrap_or_else(|| "HEAD".into()); + let revision = resolve_revision!(request.revision); let root = repo .rev_parse_single(format!("{}^{{tree}}", revision).as_str())? .object()? diff --git a/tree/list_tree.rs b/tree/list_tree.rs index 9e21a42..a318a1d 100644 --- a/tree/list_tree.rs +++ b/tree/list_tree.rs @@ -10,7 +10,10 @@ impl GitBare { let repo = self.gix_repo()?; let revision = match request.revision.clone().and_then(|s| s.selector) { Some(object_selector::Selector::Oid(oid)) => oid.hex, - Some(object_selector::Selector::Revision(name)) => name.revision, + Some(object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + name.revision + } None => "HEAD".into(), }; let mut tree = repo diff --git a/tree/mod.rs b/tree/mod.rs index e724976..bc42903 100644 --- a/tree/mod.rs +++ b/tree/mod.rs @@ -6,11 +6,16 @@ pub mod list_tree; use crate::bare::GitBare; use crate::pb::{self, RecentCommit, object_selector}; -pub(crate) fn resolve_revision(sel: &Option) -> String { +pub(crate) fn resolve_revision( + sel: &Option, +) -> Result { match sel.as_ref().and_then(|s| s.selector.as_ref()) { - Some(object_selector::Selector::Oid(oid)) => oid.hex.clone(), - Some(object_selector::Selector::Revision(name)) => name.revision.clone(), - None => "HEAD".into(), + Some(object_selector::Selector::Oid(oid)) => Ok(oid.hex.clone()), + Some(object_selector::Selector::Revision(name)) => { + crate::sanitize::validate_revision(&name.revision)?; + Ok(name.revision.clone()) + } + None => Ok("HEAD".into()), } }