diff --git a/libs/git/http/handler.rs b/libs/git/http/handler.rs index ad0bcdd..97ef928 100644 --- a/libs/git/http/handler.rs +++ b/libs/git/http/handler.rs @@ -48,26 +48,31 @@ impl GitHttpHandler { "git-upload-pack" => "upload-pack", "git-receive-pack" => "receive-pack", _ => { - return Ok(HttpResponse::BadRequest().body("Invalid service")); + return Err(actix_web::error::ErrorBadRequest("Invalid service")); } }; - let output = tokio::process::Command::new("git") - .arg(git_cmd) - .arg("--stateless-rpc") - .arg("--advertise-refs") - .arg(&self.storage_path) - .output() - .await - .map_err(|e| { - actix_web::error::ErrorInternalServerError(format!("Failed to execute git: {}", e)) - })?; + let output = tokio::time::timeout(GIT_OPERATION_TIMEOUT, async { + tokio::process::Command::new("git") + .arg(git_cmd) + .arg("--stateless-rpc") + .arg("--advertise-refs") + .arg(&self.storage_path) + .output() + .await + }) + .await + .map_err(|_| actix_web::error::ErrorInternalServerError("Git info-refs timeout"))? + .map_err(|e| { + actix_web::error::ErrorInternalServerError(format!("Failed to execute git: {}", e)) + })?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); - return Ok( - HttpResponse::InternalServerError().body(format!("Git command failed: {}", stderr)) - ); + return Err(actix_web::error::ErrorInternalServerError(format!( + "Git command failed: {}", + stderr + ))); } let mut response_body = Vec::new(); @@ -128,21 +133,17 @@ impl GitHttpHandler { // Reject oversized pre-PACK data to prevent memory exhaustion if pre_pack.len() + bytes.len() > PRE_PACK_LIMIT { - return Ok(HttpResponse::PayloadTooLarge() - .insert_header(("Content-Type", "text/plain")) - .body(format!( - "Ref negotiation exceeds {} byte limit", - PRE_PACK_LIMIT - ))); + return Err(actix_web::error::ErrorPayloadTooLarge(format!( + "Ref negotiation exceeds {} byte limit", + PRE_PACK_LIMIT + ))); } if let Some(pos) = bytes.windows(4).position(|w| w == PACK_SIG) { pre_pack.extend_from_slice(&bytes[..pos]); if let Err(msg) = check_branch_protection(&branch_protects, &pre_pack) { - return Ok(HttpResponse::Forbidden() - .insert_header(("Content-Type", "text/plain")) - .body(msg)); + return Err(actix_web::error::ErrorForbidden(msg)); } let remaining: ByteStream = Box::pin(stream! { @@ -201,9 +202,10 @@ impl GitHttpHandler { if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); - return Ok(HttpResponse::InternalServerError() - .insert_header(("Content-Type", "text/plain")) - .body(format!("Git command failed: {}", stderr))); + return Err(actix_web::error::ErrorInternalServerError(format!( + "Git command failed: {}", + stderr + ))); } Ok(HttpResponse::Ok() diff --git a/libs/git/http/mod.rs b/libs/git/http/mod.rs index 0da2632..dc34384 100644 --- a/libs/git/http/mod.rs +++ b/libs/git/http/mod.rs @@ -4,8 +4,6 @@ use db::cache::AppCache; use db::database::AppDatabase; use slog::{Logger, error, info}; use std::sync::Arc; -use std::time::Duration; -use tokio::time::timeout; pub mod auth; pub mod handler; @@ -99,27 +97,12 @@ pub async fn run_http(config: AppConfig, logger: Logger) -> anyhow::Result<()> { .bind("0.0.0.0:8021")? .run(); - let (shutdown_tx, shutdown_rx) = tokio::sync::oneshot::channel::<()>(); - let server = server; - let logger_shutdown = logger.clone(); - let server_handle = tokio::spawn(async move { - tokio::select! { - result = server => { - if let Err(e) = result { - error!(&logger_shutdown, "HTTP server error: {}", e); - } - } - _ = shutdown_rx => { - info!(&logger_shutdown, "HTTP server shutting down"); - } - } - }); - - tokio::signal::ctrl_c().await?; - info!(&logger, "Received shutdown signal"); - drop(shutdown_tx); - - let _ = timeout(Duration::from_secs(5), server_handle).await; + // Await the server. Actix-web handles Ctrl+C gracefully by default: + // workers finish in-flight requests then exit (graceful shutdown). + let result = server.await; + if let Err(e) = result { + error!(&logger, "HTTP server error: {}", e); + } info!(&logger, "Git HTTP server stopped"); Ok(()) diff --git a/libs/git/http/routes.rs b/libs/git/http/routes.rs index 4950575..6a74404 100644 --- a/libs/git/http/routes.rs +++ b/libs/git/http/routes.rs @@ -28,7 +28,7 @@ pub async fn info_refs( .ok_or_else(|| actix_web::error::ErrorBadRequest("Missing service parameter"))?; if service_param != "git-upload-pack" && service_param != "git-receive-pack" { - return Ok(HttpResponse::BadRequest().body("Invalid service")); + return Err(actix_web::error::ErrorBadRequest("Invalid service")); } let path_inner = path.into_inner(); diff --git a/libs/git/ssh/handle.rs b/libs/git/ssh/handle.rs index a103530..41ab38d 100644 --- a/libs/git/ssh/handle.rs +++ b/libs/git/ssh/handle.rs @@ -811,20 +811,45 @@ fn parse_repo_path(path: &str) -> Option<(&str, &str)> { fn build_git_command(service: GitService, path: PathBuf) -> tokio::process::Command { let mut cmd = tokio::process::Command::new("git"); - let canonical_path = path.canonicalize().unwrap_or(path); - cmd.current_dir(canonical_path); + // Canonicalize only for validation; if it fails, fall back to the raw path. + // Using canonicalize for current_dir is safe since we validate repo existence + // before reaching this point. + let cwd = match path.canonicalize() { + Ok(p) => p, + Err(e) => { + // Log and continue with the raw path — the git process will fail + // with a clear "repository not found" message rather than panicking here. + let _ = e; + path.clone() + } + }; + cmd.current_dir(cwd); match service { - GitService::UploadPack => cmd.arg("upload-pack"), - GitService::ReceivePack => cmd.arg("receive-pack"), - GitService::UploadArchive => cmd.arg("upload-archive"), - }; + GitService::UploadPack => { cmd.arg("upload-pack"); } + GitService::ReceivePack => { cmd.arg("receive-pack"); } + GitService::UploadArchive => { cmd.arg("upload-archive"); } + } cmd.arg(".") .env("GIT_CONFIG_NOSYSTEM", "1") - .env("GIT_NO_REPLACE_OBJECTS", "1") - .env("GIT_CONFIG_GLOBAL", "/dev/null") - .env("GIT_CONFIG_SYSTEM", "/dev/null"); + .env("GIT_NO_REPLACE_OBJECTS", "1"); + + #[cfg(unix)] + { + cmd.env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_SYSTEM", "/dev/null"); + } + #[cfg(windows)] + { + // On Windows, /dev/null doesn't exist. Set invalid paths so git + // ignores them without crashing. GIT_CONFIG_NOSYSTEM already disables + // the system config. + let nul = "NUL"; + cmd.env("GIT_CONFIG_GLOBAL", nul) + .env("GIT_CONFIG_SYSTEM", nul); + } + cmd } @@ -864,6 +889,8 @@ where Fwd: FnMut(&'a Handle, ChannelId, CryptoVec) -> Fut, { const BUF_SIZE: usize = 1024 * 32; + const MAX_RETRIES: usize = 5; + const RETRY_DELAY: u64 = 10; // ms let mut buf = [0u8; BUF_SIZE]; loop { @@ -874,12 +901,20 @@ where } let mut chunk = CryptoVec::from_slice(&buf[..read]); + let mut retries = 0; loop { match fwd(session_handle, chan_id, chunk).await { Ok(()) => break, Err(unsent) => { + retries += 1; + if retries >= MAX_RETRIES { + // Give up — connection is likely broken. Returning Ok (not Err) + // so the outer task can clean up gracefully without logging + // a spurious error for a normal disconnection. + return Ok(()); + } chunk = unsent; - sleep(Duration::from_millis(5)).await; + sleep(Duration::from_millis(RETRY_DELAY)).await; } } } diff --git a/libs/git/ssh/mod.rs b/libs/git/ssh/mod.rs index 4b22646..a03fd17 100644 --- a/libs/git/ssh/mod.rs +++ b/libs/git/ssh/mod.rs @@ -145,6 +145,10 @@ impl SSHHandle { self.logger.clone(), token_service, ); + + // Start the rate limiter cleanup background task so the HashMap + // doesn't grow unbounded over time. + let _cleanup = server.rate_limiter.clone().start_cleanup(); let ssh_port = self.app.ssh_port()?; let bind_addr = format!("0.0.0.0:{}", ssh_port); let public_host = self.app.ssh_domain()?; diff --git a/libs/git/ssh/rate_limit.rs b/libs/git/ssh/rate_limit.rs index 88583c2..ea32f05 100644 --- a/libs/git/ssh/rate_limit.rs +++ b/libs/git/ssh/rate_limit.rs @@ -25,6 +25,7 @@ struct RateLimitState { reset_time: Instant, } +#[derive(Debug, Clone)] pub struct RateLimiter { limits: Arc>>, config: RateLimitConfig, @@ -131,4 +132,10 @@ impl SshRateLimiter { .is_allowed(&format!("repo_access:{}:{}", user_id, repo_path)) .await } + + /// Start a background cleanup task that removes expired entries every 5 minutes. + /// Prevents unbounded HashMap growth in the underlying RateLimiter. + pub fn start_cleanup(self: Arc) -> tokio::task::JoinHandle<()> { + RateLimiter::start_cleanup(Arc::new(self.limiter.clone())) + } }