refactor(server): extract run_files_security with globset denylist (P3-2)

Moves the sensitive-path denylist, sandbox-git env helper, and metrics
emitter into a dedicated run_files_security module so the Run Files
Changed endpoint has a single, testable surface for security controls.

Denylist upgrades to globset::GlobSet with two explicit lists:
- Basename globs: .env, .env.*, *.pem, id_rsa, id_rsa.*, id_ed25519*,
  *.p12, *.keystore, *.key
- Path-suffix globs: .aws/credentials, .git/config, .ssh/**

Matching semantics explicitly pinned:
- Case-insensitive via lowercased normalization
- Path traversal (`../`, `./`, leading `/`) stripped before match
- Basename globs match the final segment only — prevents
  `log/.env_audit/data.txt` from matching `.env.*`
- Empty/pathological paths fail closed (sensitive=true safe default)

Also ships:
- sandbox_git_env() returning the env-hardening map
- RunFilesMetrics struct + emit() so tracing never leaks paths/contents

Handler migrates to consume the new module; inline denylist and inline
info!() call removed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Cargo.lock

@@ -1963,6 +1963,7 @@ dependencies = [
  "fabro-vault",
  "fabro-workflow",
  "futures-util",
+ "globset",
  "hex",
  "hmac",
  "http-body-util",

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"

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"] }

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;

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<AppState>, 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};

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<Denylist> = 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<String, String> {
+    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(""));
+    }
+}