mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-07 03:00:29 +00:00
fix(server,workflow): close denylist-evasion gaps from security review
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/<old> b/<new>` 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) <noreply@anthropic.com>
This commit is contained in:
parent
0c77a184ab
commit
e2d258089e
2 changed files with 20 additions and 8 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -200,7 +200,13 @@ pub(crate) async fn git_diff_with_timeout(
|
|||
base: &str,
|
||||
timeout_ms: u64,
|
||||
) -> std::result::Result<String, String> {
|
||||
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/<old> b/<new>` 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.
|
||||
///
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue