Skip to content

Commit 08e1fc2

Browse files
Copilotjamesadevine
andcommitted
fix(safeoutputs): prevent symlink exfiltration in create-pr Stage 3
Replace is_file() (follows symlinks) with symlink_metadata() checks in collect_changes_from_worktree and collect_changes_from_diff_tree so that symlinks in the applied patch are silently skipped rather than followed to arbitrary filesystem paths (e.g. /proc/self/environ). Also add symlink mode (120000) detection to validate_patch_paths as a belt-and-suspenders defence: patches that create or convert files to symlinks are rejected before git apply is attempted. Adds two unit tests covering the new rejection paths. Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/99d4d7f5-eaea-4a34-87e3-ac34bab53ddb Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
1 parent 202685d commit 08e1fc2

1 file changed

Lines changed: 131 additions & 16 deletions

File tree

src/safeoutputs/create_pull_request.rs

Lines changed: 131 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1656,14 +1656,32 @@ async fn collect_changes_from_worktree(
16561656
}
16571657
// New/untracked files
16581658
"??" | "A " | " A" | "AM" => {
1659-
if full_path.is_file() {
1660-
changes.push(read_file_change("add", file_path, &full_path).await?);
1659+
match tokio::fs::symlink_metadata(&full_path).await {
1660+
Ok(meta) if meta.file_type().is_file() => {
1661+
changes.push(read_file_change("add", file_path, &full_path).await?);
1662+
}
1663+
Ok(meta) if meta.file_type().is_symlink() => {
1664+
warn!(
1665+
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
1666+
file_path
1667+
);
1668+
}
1669+
_ => {}
16611670
}
16621671
}
16631672
// Modified files
16641673
" M" | "M " | "MM" => {
1665-
if full_path.is_file() {
1666-
changes.push(read_file_change("edit", file_path, &full_path).await?);
1674+
match tokio::fs::symlink_metadata(&full_path).await {
1675+
Ok(meta) if meta.file_type().is_file() => {
1676+
changes.push(read_file_change("edit", file_path, &full_path).await?);
1677+
}
1678+
Ok(meta) if meta.file_type().is_symlink() => {
1679+
warn!(
1680+
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
1681+
file_path
1682+
);
1683+
}
1684+
_ => {}
16671685
}
16681686
}
16691687
// Renamed files - format is "R old_path -> new_path"
@@ -1687,16 +1705,34 @@ async fn collect_changes_from_worktree(
16871705
// If status is "RM" (renamed + modified), also emit content
16881706
if status_code == "RM" {
16891707
let new_full_path = worktree_path.join(new_path.trim());
1690-
if new_full_path.is_file() {
1691-
changes.push(read_file_change("edit", new_path.trim(), &new_full_path).await?);
1708+
match tokio::fs::symlink_metadata(&new_full_path).await {
1709+
Ok(meta) if meta.file_type().is_file() => {
1710+
changes.push(read_file_change("edit", new_path.trim(), &new_full_path).await?);
1711+
}
1712+
Ok(meta) if meta.file_type().is_symlink() => {
1713+
warn!(
1714+
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
1715+
new_path.trim()
1716+
);
1717+
}
1718+
_ => {}
16921719
}
16931720
}
16941721
}
16951722
}
16961723
// Other statuses - try to handle as edit if file exists
16971724
_ => {
1698-
if full_path.is_file() {
1699-
changes.push(read_file_change("edit", file_path, &full_path).await?);
1725+
match tokio::fs::symlink_metadata(&full_path).await {
1726+
Ok(meta) if meta.file_type().is_file() => {
1727+
changes.push(read_file_change("edit", file_path, &full_path).await?);
1728+
}
1729+
Ok(meta) if meta.file_type().is_symlink() => {
1730+
warn!(
1731+
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
1732+
file_path
1733+
);
1734+
}
1735+
_ => {}
17001736
}
17011737
}
17021738
}
@@ -1744,8 +1780,17 @@ async fn collect_changes_from_diff_tree(
17441780
}));
17451781
} else if status_code == "A" {
17461782
// Added file
1747-
if full_path.is_file() {
1748-
changes.push(read_file_change("add", file_path, &full_path).await?);
1783+
match tokio::fs::symlink_metadata(&full_path).await {
1784+
Ok(meta) if meta.file_type().is_file() => {
1785+
changes.push(read_file_change("add", file_path, &full_path).await?);
1786+
}
1787+
Ok(meta) if meta.file_type().is_symlink() => {
1788+
warn!(
1789+
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
1790+
file_path
1791+
);
1792+
}
1793+
_ => {}
17491794
}
17501795
} else if status_code.starts_with('R') && parts.len() >= 3 {
17511796
// Renamed file: R100\told_path\tnew_path
@@ -1765,22 +1810,51 @@ async fn collect_changes_from_diff_tree(
17651810

17661811
// If the file was also modified (similarity < 100), emit an edit with content
17671812
let new_full_path = worktree_path.join(new_path);
1768-
if status_code != "R100" && new_full_path.is_file() {
1769-
changes.push(read_file_change("edit", new_path, &new_full_path).await?);
1813+
if status_code != "R100" {
1814+
match tokio::fs::symlink_metadata(&new_full_path).await {
1815+
Ok(meta) if meta.file_type().is_file() => {
1816+
changes.push(read_file_change("edit", new_path, &new_full_path).await?);
1817+
}
1818+
Ok(meta) if meta.file_type().is_symlink() => {
1819+
warn!(
1820+
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
1821+
new_path
1822+
);
1823+
}
1824+
_ => {}
1825+
}
17701826
}
17711827
} else if status_code.starts_with('C') && parts.len() >= 3 {
17721828
// Copied file: C100\tsrc_path\tdest_path
17731829
let dest_path = parts[2];
17741830
validate_single_path(dest_path)?;
17751831

17761832
let dest_full_path = worktree_path.join(dest_path);
1777-
if dest_full_path.is_file() {
1778-
changes.push(read_file_change("add", dest_path, &dest_full_path).await?);
1833+
match tokio::fs::symlink_metadata(&dest_full_path).await {
1834+
Ok(meta) if meta.file_type().is_file() => {
1835+
changes.push(read_file_change("add", dest_path, &dest_full_path).await?);
1836+
}
1837+
Ok(meta) if meta.file_type().is_symlink() => {
1838+
warn!(
1839+
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
1840+
dest_path
1841+
);
1842+
}
1843+
_ => {}
17791844
}
17801845
} else {
17811846
// Modified or other — read current content
1782-
if full_path.is_file() {
1783-
changes.push(read_file_change("edit", file_path, &full_path).await?);
1847+
match tokio::fs::symlink_metadata(&full_path).await {
1848+
Ok(meta) if meta.file_type().is_file() => {
1849+
changes.push(read_file_change("edit", file_path, &full_path).await?);
1850+
}
1851+
Ok(meta) if meta.file_type().is_symlink() => {
1852+
warn!(
1853+
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
1854+
file_path
1855+
);
1856+
}
1857+
_ => {}
17841858
}
17851859
}
17861860
}
@@ -1833,6 +1907,7 @@ async fn read_file_change(
18331907
/// - No .git directory modifications
18341908
/// - No absolute paths
18351909
/// - No null bytes
1910+
/// - No symlink entries (mode 120000)
18361911
fn validate_patch_paths(patch_content: &str) -> anyhow::Result<()> {
18371912
let mut in_diff = false;
18381913
for line in patch_content.lines() {
@@ -1872,6 +1947,15 @@ fn validate_patch_paths(patch_content: &str) -> anyhow::Result<()> {
18721947
{
18731948
let path = line.splitn(3, ' ').nth(2).unwrap_or("").trim_matches('"');
18741949
validate_single_path(path)?;
1950+
} else if line.starts_with("new file mode 120000")
1951+
|| line.starts_with("old mode 120000")
1952+
|| line.starts_with("new mode 120000")
1953+
{
1954+
// Reject symlink entries (git mode 120000 = symbolic link).
1955+
// Symlinks in a patch could be used to make Stage 3 follow them to
1956+
// arbitrary filesystem paths (e.g. /proc/self/environ) when collecting
1957+
// file changes to upload to ADO.
1958+
anyhow::bail!("Patch contains a symlink entry (mode 120000), which is not allowed");
18751959
}
18761960
}
18771961
Ok(())
@@ -2225,6 +2309,37 @@ new file mode 100755
22252309
);
22262310
}
22272311

2312+
#[test]
2313+
fn test_validate_patch_paths_symlink_rejected() {
2314+
// A patch that creates a new symlink (mode 120000) must be rejected.
2315+
// This is the primary attack vector for symlink exfiltration of Stage 3 secrets.
2316+
let patch = r#"diff --git a/secrets.txt b/secrets.txt
2317+
new file mode 120000
2318+
index 0000000..abcdefg
2319+
--- /dev/null
2320+
+++ b/secrets.txt
2321+
@@ -0,0 +1 @@
2322+
+/proc/self/environ
2323+
\ No newline at end of file
2324+
"#;
2325+
assert!(
2326+
validate_patch_paths(patch).is_err(),
2327+
"patch with symlink mode 120000 should be rejected"
2328+
);
2329+
}
2330+
2331+
#[test]
2332+
fn test_validate_patch_paths_symlink_mode_change_rejected() {
2333+
// A patch that changes a file to a symlink (old mode → new mode 120000) is rejected.
2334+
let patch = "diff --git a/file.txt b/file.txt\n\
2335+
old mode 100644\n\
2336+
new mode 120000\n";
2337+
assert!(
2338+
validate_patch_paths(patch).is_err(),
2339+
"patch that introduces symlink via mode change should be rejected"
2340+
);
2341+
}
2342+
22282343
#[test]
22292344
fn test_validate_single_path_valid() {
22302345
assert!(validate_single_path("src/main.rs").is_ok());

0 commit comments

Comments
 (0)