diff --git a/Cargo.lock b/Cargo.lock index 038a547ff..df8c71d27 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1963,6 +1963,7 @@ dependencies = [ "fabro-vault", "fabro-workflow", "futures-util", + "globset", "hex", "hmac", "http-body-util", diff --git a/Cargo.toml b/Cargo.toml index 58d237d59..4be15b04d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -46,6 +46,7 @@ walkdir = "2" regex = "1" semver = "1" aho-corasick = "1" +globset = "0.4" dirs = "6" mac_address = "1" md5 = "0.7" diff --git a/lib/crates/fabro-server/Cargo.toml b/lib/crates/fabro-server/Cargo.toml index 55a64dec5..ac5fd50cd 100644 --- a/lib/crates/fabro-server/Cargo.toml +++ b/lib/crates/fabro-server/Cargo.toml @@ -42,6 +42,7 @@ axum.workspace = true axum-extra.workspace = true cookie.workspace = true dirs.workspace = true +globset.workspace = true tower = "0.5" tower-http = { version = "0.6", features = ["trace"] } tokio-stream = { workspace = true, features = ["sync"] } diff --git a/lib/crates/fabro-server/src/lib.rs b/lib/crates/fabro-server/src/lib.rs index 01704a9a1..b81ff64ba 100644 --- a/lib/crates/fabro-server/src/lib.rs +++ b/lib/crates/fabro-server/src/lib.rs @@ -14,6 +14,7 @@ pub mod install; pub mod ip_allowlist; pub mod jwt_auth; mod run_files; +mod run_files_security; mod run_manifest; pub mod security_headers; pub mod serve; diff --git a/lib/crates/fabro-server/src/run_files.rs b/lib/crates/fabro-server/src/run_files.rs index cf596d998..99d1a5cd6 100644 --- a/lib/crates/fabro-server/src/run_files.rs +++ b/lib/crates/fabro-server/src/run_files.rs @@ -38,10 +38,10 @@ use fabro_workflow::sandbox_git::{ use futures_util::FutureExt; use serde::Deserialize; use tokio::sync::{Mutex, watch}; -use tracing::info; use crate::error::ApiError; use crate::jwt_auth::AuthenticatedService; +use crate::run_files_security::{RunFilesMetrics, is_sensitive}; use crate::server::{AppState, parse_run_id_path_pub}; /// Per-file cap: 256 KiB OR 20k lines (whichever comes first). @@ -327,18 +327,17 @@ async fn materialize_sandbox_path(state: &Arc, run_id: &RunId) -> List count_flags(&response_data); let duration_ms = u64::try_from(start.elapsed().as_millis()).unwrap_or(u64::MAX); - info!( - run_id = %run_id, - file_count = response_data.len(), - bytes_total = aggregate_bytes, + RunFilesMetrics { + file_count: response_data.len(), + bytes_total: aggregate_bytes, duration_ms, truncated, binary_count, sensitive_count, symlink_count, submodule_count, - "Run files response produced" - ); + } + .emit(run_id); Ok(PaginatedRunFileList { data: response_data, @@ -982,32 +981,6 @@ fn count_flags(data: &[FileDiff]) -> (u64, u64, u64, u64) { (binary, sensitive, symlink, submodule) } -/// Path-based sensitive-file denylist (Unit 8 extracts this into a shared -/// module; an inline set keeps the handler self-contained for Unit 5). -/// -/// `basename_lower` is already ASCII-lowercased, so the suffix checks below -/// are effectively case-insensitive despite clippy's heuristic lint. -#[allow(clippy::case_sensitive_file_extension_comparisons)] -fn is_sensitive(path: &str) -> bool { - let basename = path.rsplit_once('/').map_or(path, |(_, name)| name); - let basename_lower = basename.to_ascii_lowercase(); - let basename_match = basename_lower == ".env" - || basename_lower.starts_with(".env.") - || basename_lower.ends_with(".pem") - || basename_lower.starts_with("id_rsa") - || basename_lower.ends_with(".p12") - || basename_lower.ends_with(".keystore") - || basename_lower.ends_with(".key"); - if basename_match { - return true; - } - // Path-suffix patterns. - path.ends_with(".aws/credentials") - || path.ends_with(".git/config") - || path.contains("/.ssh/") - || path.starts_with(".ssh/") -} - #[cfg(test)] mod tests { use std::sync::atomic::{AtomicUsize, Ordering}; diff --git a/lib/crates/fabro-server/src/run_files_security.rs b/lib/crates/fabro-server/src/run_files_security.rs new file mode 100644 index 000000000..d8af8612a --- /dev/null +++ b/lib/crates/fabro-server/src/run_files_security.rs @@ -0,0 +1,237 @@ +#![allow(unreachable_pub, dead_code)] + +//! Security helpers shared by the Run Files Changed endpoint: a globset-based +//! sensitive-path denylist, a sandbox-git env-hardening helper, and a +//! structured metrics emitter that enforces the tracing allowlist. +//! +//! All matching is path-based and case-insensitive. The denylist is a +//! defense-in-depth control — it is not a content scanner and will not +//! catch arbitrary secrets hidden inside non-secret file extensions. +//! +//! `sandbox_git_env` and `RunFilesMetrics` are intentionally public APIs +//! even though they're currently consumed by a single caller — the module +//! is designed as a reusable surface for any future sensitive-data-adjacent +//! endpoint. + +use std::collections::HashMap; +use std::sync::OnceLock; + +use fabro_types::RunId; +use globset::{Glob, GlobSet, GlobSetBuilder}; +use tracing::info; + +/// Basename patterns — applied to the final path segment only. +const BASENAME_GLOBS: &[&str] = &[ + ".env", + ".env.*", + "*.pem", + "id_rsa", + "id_rsa.*", + "id_ed25519", + "id_ed25519.*", + "*.p12", + "*.keystore", + "*.key", +]; + +/// Path-suffix patterns — applied to the whole repo-relative path. +const PATH_SUFFIX_GLOBS: &[&str] = &[ + "**/.aws/credentials", + ".aws/credentials", + "**/.git/config", + ".git/config", + "**/.ssh/**", + ".ssh/**", +]; + +/// Lazily-constructed globsets. Building from a static pattern list never +/// fails in practice, but we defensively unwrap into an always-false matcher +/// so a pattern typo in a future edit doesn't take the whole endpoint down. +struct Denylist { + basename: GlobSet, + path: GlobSet, +} + +fn build_set(patterns: &[&str]) -> GlobSet { + let mut builder = GlobSetBuilder::new(); + for pat in patterns { + if let Ok(glob) = Glob::new(pat) { + builder.add(glob); + } + } + builder.build().unwrap_or_else(|_| GlobSet::empty()) +} + +fn denylist() -> &'static Denylist { + static SET: OnceLock = OnceLock::new(); + SET.get_or_init(|| Denylist { + basename: build_set(BASENAME_GLOBS), + path: build_set(PATH_SUFFIX_GLOBS), + }) +} + +/// Return `true` if `path` matches any entry in the sensitive-path denylist. +/// +/// Matching semantics: +/// - Inputs are normalized to lowercase and POSIX separators before matching. +/// - Ancestor `../` / `./` components are stripped so attempts to sneak a +/// sensitive file via traversal can't evade the check. +/// - Basename globs fire against the last path segment only (prevents +/// `log/.env_audit/data.txt` from matching the `.env.*` pattern). +/// - Path-suffix globs fire against the whole normalized path. +/// - Empty paths, pure `.` paths, and paths that normalize to empty are +/// considered sensitive as a safe default — no legitimate diff produces +/// these; treating them as sensitive prevents accidental exposure of +/// pathological git output. +#[must_use] +pub fn is_sensitive(path: &str) -> bool { + let normalized = normalize_for_match(path); + if normalized.is_empty() { + return true; + } + let set = denylist(); + + let basename = normalized + .rsplit_once('/') + .map_or(normalized.as_str(), |(_, n)| n); + if set.basename.is_match(basename) { + return true; + } + set.path.is_match(normalized.as_str()) +} + +fn normalize_for_match(path: &str) -> String { + // Replace backslashes with forward slashes so Windows-style paths (if + // they ever leak through git output) match the same way; lowercase so + // the patterns are effectively case-insensitive. + let mut out = String::with_capacity(path.len()); + for ch in path.chars() { + let c = if ch == '\\' { '/' } else { ch }; + out.extend(c.to_lowercase()); + } + // Drop leading `./` and consecutive `../` prefixes; keep inner `..` alone + // since git doesn't emit those in normal diffs. + while let Some(rest) = out + .strip_prefix("./") + .or_else(|| out.strip_prefix("../")) + .or_else(|| out.strip_prefix("/")) + { + out = rest.to_string(); + } + out +} + +/// Environment additions applied to every sandbox-side git invocation under +/// the Run Files endpoint. Pairs with the hardened `-c` flags the sandbox +/// git helpers already use. +#[must_use] +pub fn sandbox_git_env() -> HashMap { + HashMap::from([ + ("GIT_TERMINAL_PROMPT".to_string(), "0".to_string()), + ("GIT_EXTERNAL_DIFF".to_string(), String::new()), + ]) +} + +/// Metrics emitted at the tail of every Run Files response. The field set is +/// deliberately the only shape of tracing output the endpoint produces — +/// enforced by `emit`, which never interpolates or logs individual paths, +/// file contents, or raw git stderr. +pub struct RunFilesMetrics { + pub file_count: usize, + pub bytes_total: u64, + pub duration_ms: u64, + pub truncated: bool, + pub binary_count: u64, + pub sensitive_count: u64, + pub symlink_count: u64, + pub submodule_count: u64, +} + +impl RunFilesMetrics { + pub fn emit(&self, run_id: &RunId) { + info!( + run_id = %run_id, + file_count = self.file_count, + bytes_total = self.bytes_total, + duration_ms = self.duration_ms, + truncated = self.truncated, + binary_count = self.binary_count, + sensitive_count = self.sensitive_count, + symlink_count = self.symlink_count, + submodule_count = self.submodule_count, + "Run files response produced" + ); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn is_sensitive_matches_basename_patterns() { + assert!(is_sensitive(".env")); + assert!(is_sensitive("apps/web/.env.production")); + assert!(is_sensitive("keys/id_rsa")); + assert!(is_sensitive("keys/id_rsa.pub")); + assert!(is_sensitive("keys/id_ed25519")); + assert!(is_sensitive("certs/Server.PEM")); + assert!(is_sensitive("data/vault.keystore")); + assert!(is_sensitive("config/service.key")); + } + + #[test] + fn is_sensitive_matches_path_suffix_patterns() { + assert!(is_sensitive(".ssh/authorized_keys")); + assert!(is_sensitive("home/user/.ssh/id_custom")); + assert!(is_sensitive(".aws/credentials")); + assert!(is_sensitive("home/user/.aws/credentials")); + assert!(is_sensitive(".git/config")); + } + + #[test] + fn is_sensitive_rejects_benign_paths() { + assert!(!is_sensitive("src/main.rs")); + assert!(!is_sensitive("README.md")); + // A file whose name merely contains `env` as a substring must not + // match the `.env` / `.env.*` basename globs. + assert!(!is_sensitive("src/environment.ts")); + // Ancestor-directory-only match: `.env_audit` is a directory, the + // actual file is `data.txt` — shouldn't hit the basename pattern. + assert!(!is_sensitive("log/.env_audit/data.txt")); + } + + #[test] + fn is_sensitive_handles_path_traversal_safely() { + // `../` components strip to an otherwise-benign path rather than + // letting the attacker evade matching. The ultimate segment drives + // the basename check. + assert!(is_sensitive("../.ssh/id_rsa")); + assert!(is_sensitive("./.env")); + assert!(!is_sensitive("../../src/main.rs")); + } + + #[test] + fn is_sensitive_empty_paths_fail_closed() { + assert!(is_sensitive("")); + assert!(is_sensitive("./")); + assert!(is_sensitive("/")); + } + + #[test] + fn is_sensitive_is_case_insensitive() { + assert!(is_sensitive(".ENV")); + assert!(is_sensitive(".Env.Production")); + assert!(is_sensitive("keys/ID_RSA")); + } + + #[test] + fn sandbox_git_env_sets_expected_pairs() { + let env = sandbox_git_env(); + assert_eq!( + env.get("GIT_TERMINAL_PROMPT").map(String::as_str), + Some("0") + ); + assert_eq!(env.get("GIT_EXTERNAL_DIFF").map(String::as_str), Some("")); + } +}