From e2d258089ea9f3ba569e5440557df39c9cebbf3e Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 19 Apr 2026 18:04:49 -0400 Subject: [PATCH] fix(server,workflow): close denylist-evasion gaps from security review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes from the Run Files security review (docs/agent/reviews/2026-04-19-run-files-security-review.md): Medium — Add `-c core.quotePath=false` to git invocations that feed the denylist. - git_diff_with_timeout (produces final_patch for the degraded fallback) — without this, a tracked file with non-ASCII chars, tabs, quotes, or backslashes in its name makes git emit a header like `diff --git "a/…" "b/…"`. The Run Files server's strip_denylisted_sections parser only recognizes unquoted `a/ b/` forms and would let the sensitive section pass through unfiltered. - GIT_HARDENED (the raw-diff / cat-file prefix used by the Run Files enumerator) — applied for symmetry so any future consumer parsing these invocations' output can't be tripped by the same quoted-path divergence. Low — is_sensitive path normalization switches to ASCII-only case fold. Full Unicode `to_lowercase()` can expand a codepoint into multiple chars (e.g. `İ` -> `i\u{307}`), which then silently fails to match an ASCII glob like `id_rsa`. ASCII-only folding makes the homoglyphic-path failure mode explicit — a path a reviewer can see is homoglyphic just doesn't match — rather than disguising it behind an opaque lowercase routine. All denylist globs are ASCII by design. Other findings in the review (denylist policy coverage gaps around id_rsa_backup / .netrc / .npmrc / etc.) are policy decisions, not matcher bugs, and are deferred. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../fabro-server/src/run_files_security.rs | 18 ++++++++++++------ lib/crates/fabro-workflow/src/sandbox_git.rs | 10 ++++++++-- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/lib/crates/fabro-server/src/run_files_security.rs b/lib/crates/fabro-server/src/run_files_security.rs index 287570b93..b4e6fa948 100644 --- a/lib/crates/fabro-server/src/run_files_security.rs +++ b/lib/crates/fabro-server/src/run_files_security.rs @@ -102,15 +102,21 @@ pub fn is_sensitive(path: &str) -> bool { 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. `to_lowercase()` can - // expand certain Unicode codepoints to multiple chars — the strip loop - // below runs against the fully-lowercased string so prefix matching is - // consistent regardless of the input's case. + // they ever leak through git output) match the same way. Case-folding + // is ASCII-only on purpose: all denylist globs are ASCII, and full + // Unicode `to_lowercase()` can expand a single codepoint into multiple + // chars (e.g. `İ` -> `i\u{307}`), which then silently can't match an + // ASCII glob. ASCII-only fold makes the failure mode explicit (a + // homoglyphic path just doesn't match) rather than disguised. let mut out = String::with_capacity(path.len()); for ch in path.chars() { let c = if ch == '\\' { '/' } else { ch }; - out.extend(c.to_lowercase()); + let lowered = if c.is_ascii() { + c.to_ascii_lowercase() + } else { + c + }; + out.push(lowered); } // Drop leading `./`, `../`, and bare-leading `/` prefixes until the // path has a meaningful first segment. Inner `..` components are left diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index 7d525c58f..ce8c9e9e4 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -200,7 +200,13 @@ pub(crate) async fn git_diff_with_timeout( base: &str, timeout_ms: u64, ) -> std::result::Result { - let cmd = format!("{GIT_REMOTE} diff {base} HEAD"); + // `-c core.quotePath=false` forces paths with non-ASCII, tabs, quotes, + // or backslashes to emit unquoted. The Run Files Changed endpoint's + // `strip_denylisted_sections` parser only recognizes unquoted + // `diff --git a/ b/` headers; without this flag git would + // wrap such paths in `"a/…"` / `"b/…"` and evade the denylist (see + // docs/agent/reviews/2026-04-19-run-files-security-review.md). + let cmd = format!("{GIT_REMOTE} -c core.quotePath=false diff {base} HEAD"); match sandbox .exec_command(&cmd, timeout_ms, None, None, None) .await @@ -264,7 +270,7 @@ pub async fn git_replace_worktree(sandbox: &dyn Sandbox, path: &str, branch: &st /// /// These invocations use [`sandbox_git_hardening_env`] via `exec_command` to /// disable terminal prompts and external diff drivers. -const GIT_HARDENED: &str = "git -c maintenance.auto=0 -c gc.auto=0 -c core.hooksPath=/dev/null -c core.fsmonitor=false -c protocol.file.allow=never"; +const GIT_HARDENED: &str = "git -c maintenance.auto=0 -c gc.auto=0 -c core.hooksPath=/dev/null -c core.fsmonitor=false -c protocol.file.allow=never -c core.quotePath=false"; /// Environment additions applied to every hardened sandbox-side git invocation. ///