Skip to content

Commit f9fc78f

Browse files
jamesadevineCopilot
andcommitted
fix(safeoutputs): trim() mode-line check and differentiate NotFound metadata errors
Two minor review fixes on PR #549. 1. validate_patch_paths now compares mode lines after a full trim() rather than trim_end(). trim_end() leaves leading whitespace intact, so a line like ' new file mode 120000' would silently bypass the check. Git's own format-patch never produces leading-indented mode lines so this was not a realistic attack path, but trim() costs nothing and closes the gap. The existing test now also covers the leading-whitespace and CRLF cases. 2. push_file_change_skipping_symlinks now distinguishes io::ErrorKind::NotFound from other metadata errors. NotFound is a normal transient condition (worktree mid-rebase, file pruned by git apply, etc.) and is logged at debug level. PermissionDenied and other unusual kinds remain at warn so triage isn't drowned by alert fatigue from benign races. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 6d55eed commit f9fc78f

1 file changed

Lines changed: 39 additions & 12 deletions

File tree

src/safeoutputs/create_pull_request.rs

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1879,10 +1879,19 @@ async fn push_file_change_skipping_symlinks(
18791879
// Not a regular file (e.g. directory, fifo, socket) — silently skip.
18801880
}
18811881
Err(e) => {
1882-
warn!(
1883-
"Failed to read metadata for {}: {} — skipping",
1884-
file_path, e
1885-
);
1882+
// NotFound is a normal transient condition (worktree mid-rebase, file
1883+
// already pruned by git apply, etc.). Anything else — most notably
1884+
// PermissionDenied — is unusual and worth flagging at warn for triage.
1885+
if e.kind() == std::io::ErrorKind::NotFound {
1886+
debug!("File no longer present for {} — skipping", file_path);
1887+
} else {
1888+
warn!(
1889+
"Failed to read metadata for {}: {} (kind={:?}) — skipping",
1890+
file_path,
1891+
e,
1892+
e.kind()
1893+
);
1894+
}
18861895
}
18871896
}
18881897
Ok(())
@@ -2024,7 +2033,7 @@ fn validate_patch_paths(patch_content: &str) -> anyhow::Result<()> {
20242033
let path = line.splitn(3, ' ').nth(2).unwrap_or("").trim_matches('"');
20252034
validate_single_path(path)?;
20262035
} else if {
2027-
let trimmed = line.trim_end();
2036+
let trimmed = line.trim();
20282037
trimmed == "new file mode 120000" || trimmed == "new mode 120000"
20292038
} {
20302039
// Reject patch lines that INTRODUCE a symlink (git mode 120000).
@@ -2040,9 +2049,10 @@ fn validate_patch_paths(patch_content: &str) -> anyhow::Result<()> {
20402049
// regular file, which is a legitimate cleanup operation and produces a
20412050
// safe worktree.
20422051
//
2043-
// The match is on the exact trimmed line (rather than `starts_with`) so
2044-
// we don't accidentally reject hypothetical future mode strings that
2045-
// happen to share the "120000" prefix.
2052+
// The match is on the fully trimmed line (rather than `starts_with` or
2053+
// `trim_end`) so neither leading nor trailing whitespace can bypass
2054+
// the check, and we don't accidentally reject hypothetical future mode
2055+
// strings that happen to share the "120000" prefix.
20462056
anyhow::bail!(
20472057
"Patch introduces a symlink (mode 120000), which is not allowed"
20482058
);
@@ -2466,16 +2476,33 @@ index 0000000..abcdefg
24662476

24672477
#[test]
24682478
fn test_validate_patch_paths_symlink_mode_suffix_not_bypass() {
2469-
// The mode check uses exact line equality (after trim_end), so a fake
2470-
// mode string with extra digits like "120000 1" or "1200001" must NOT
2471-
// be misclassified — but we still want any genuine "new file mode 120000"
2472-
// followed by trailing whitespace to be rejected.
2479+
// The mode check uses full trim() equality, so neither leading nor
2480+
// trailing whitespace can bypass the check, while genuine ASCII text
2481+
// before/after the keyword stays distinguishable.
24732482
let patch_trailing_ws = "diff --git a/x b/x\nnew file mode 120000 \n";
24742483
assert!(
24752484
validate_patch_paths(patch_trailing_ws).is_err(),
24762485
"trailing whitespace must not let a symlink mode line bypass the check"
24772486
);
24782487

2488+
// Git's own format-patch never produces leading-indented mode lines, but
2489+
// a hand-crafted patch could try to smuggle one through any matcher that
2490+
// anchored at the start of the line. Using trim() (not trim_end()) on the
2491+
// line ensures leading whitespace is no bypass either.
2492+
let patch_leading_ws = "diff --git a/x b/x\n new file mode 120000\n";
2493+
assert!(
2494+
validate_patch_paths(patch_leading_ws).is_err(),
2495+
"leading whitespace must not let a symlink mode line bypass the check"
2496+
);
2497+
2498+
// Carriage returns (Windows line endings) are also whitespace and must
2499+
// not bypass.
2500+
let patch_crlf = "diff --git a/x b/x\nnew file mode 120000\r\n";
2501+
assert!(
2502+
validate_patch_paths(patch_crlf).is_err(),
2503+
"trailing \\r must not let a symlink mode line bypass the check"
2504+
);
2505+
24792506
// A line that merely happens to share a "120000" prefix segment must
24802507
// not pass the check accidentally either — but it's also not a real
24812508
// git mode line. We just want to make sure exact-match doesn't reject

0 commit comments

Comments
 (0)