Skip to content

fix(safeoutputs): prevent symlink exfiltration in create-pr Stage 3 #549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 2 commits into
base: main
Choose a base branch
from
+131 −16
Draft

fix(safeoutputs): prevent symlink exfiltration in create-pr Stage 3 #549

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c7b0940
Initial plan
Copilot May 15, 2026
711e24f
fix(safeoutputs): prevent symlink exfiltration in create-pr Stage 3
Copilot May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 131 additions & 16 deletions src/safeoutputs/create_pr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1656,14 +1656,32 @@ async fn collect_changes_from_worktree(
}
// New/untracked files
"??" | "A " | " A" | "AM" => {
if full_path.is_file() {
changes.push(read_file_change("add", file_path, &full_path).await?);
match tokio::fs::symlink_metadata(&full_path).await {
Ok(meta) if meta.file_type().is_file() => {
changes.push(read_file_change("add", file_path, &full_path).await?);
}
Ok(meta) if meta.file_type().is_symlink() => {
warn!(
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
file_path
);
}
_ => {}
}
}
// Modified files
" M" | "M " | "MM" => {
if full_path.is_file() {
changes.push(read_file_change("edit", file_path, &full_path).await?);
match tokio::fs::symlink_metadata(&full_path).await {
Ok(meta) if meta.file_type().is_file() => {
changes.push(read_file_change("edit", file_path, &full_path).await?);
}
Ok(meta) if meta.file_type().is_symlink() => {
warn!(
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
file_path
);
}
_ => {}
}
}
// Renamed files - format is "R old_path -> new_path"
Expand All @@ -1687,16 +1705,34 @@ async fn collect_changes_from_worktree(
// If status is "RM" (renamed + modified), also emit content
if status_code == "RM" {
let new_full_path = worktree_path.join(new_path.trim());
if new_full_path.is_file() {
changes.push(read_file_change("edit", new_path.trim(), &new_full_path).await?);
match tokio::fs::symlink_metadata(&new_full_path).await {
Ok(meta) if meta.file_type().is_file() => {
changes.push(read_file_change("edit", new_path.trim(), &new_full_path).await?);
}
Ok(meta) if meta.file_type().is_symlink() => {
warn!(
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
new_path.trim()
);
}
_ => {}
}
}
}
}
// Other statuses - try to handle as edit if file exists
_ => {
if full_path.is_file() {
changes.push(read_file_change("edit", file_path, &full_path).await?);
match tokio::fs::symlink_metadata(&full_path).await {
Ok(meta) if meta.file_type().is_file() => {
changes.push(read_file_change("edit", file_path, &full_path).await?);
}
Ok(meta) if meta.file_type().is_symlink() => {
warn!(
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
file_path
);
}
_ => {}
}
}
}
Expand Down Expand Up @@ -1744,8 +1780,17 @@ async fn collect_changes_from_diff_tree(
}));
} else if status_code == "A" {
// Added file
if full_path.is_file() {
changes.push(read_file_change("add", file_path, &full_path).await?);
match tokio::fs::symlink_metadata(&full_path).await {
Ok(meta) if meta.file_type().is_file() => {
changes.push(read_file_change("add", file_path, &full_path).await?);
}
Ok(meta) if meta.file_type().is_symlink() => {
warn!(
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
file_path
);
}
_ => {}
}
} else if status_code.starts_with('R') && parts.len() >= 3 {
// Renamed file: R100\told_path\tnew_path
Expand All @@ -1765,22 +1810,51 @@ async fn collect_changes_from_diff_tree(

// If the file was also modified (similarity < 100), emit an edit with content
let new_full_path = worktree_path.join(new_path);
if status_code != "R100" && new_full_path.is_file() {
changes.push(read_file_change("edit", new_path, &new_full_path).await?);
if status_code != "R100" {
match tokio::fs::symlink_metadata(&new_full_path).await {
Ok(meta) if meta.file_type().is_file() => {
changes.push(read_file_change("edit", new_path, &new_full_path).await?);
}
Ok(meta) if meta.file_type().is_symlink() => {
warn!(
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
new_path
);
}
_ => {}
}
}
} else if status_code.starts_with('C') && parts.len() >= 3 {
// Copied file: C100\tsrc_path\tdest_path
let dest_path = parts[2];
validate_single_path(dest_path)?;

let dest_full_path = worktree_path.join(dest_path);
if dest_full_path.is_file() {
changes.push(read_file_change("add", dest_path, &dest_full_path).await?);
match tokio::fs::symlink_metadata(&dest_full_path).await {
Ok(meta) if meta.file_type().is_file() => {
changes.push(read_file_change("add", dest_path, &dest_full_path).await?);
}
Ok(meta) if meta.file_type().is_symlink() => {
warn!(
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
dest_path
);
}
_ => {}
}
} else {
// Modified or other — read current content
if full_path.is_file() {
changes.push(read_file_change("edit", file_path, &full_path).await?);
match tokio::fs::symlink_metadata(&full_path).await {
Ok(meta) if meta.file_type().is_file() => {
changes.push(read_file_change("edit", file_path, &full_path).await?);
}
Ok(meta) if meta.file_type().is_symlink() => {
warn!(
"Skipping symlink in worktree: {} (symlink-following attack prevention)",
file_path
);
}
_ => {}
}
}
}
Expand Down Expand Up @@ -1833,6 +1907,7 @@ async fn read_file_change(
/// - No .git directory modifications
/// - No absolute paths
/// - No null bytes
/// - No symlink entries (mode 120000)
fn validate_patch_paths(patch_content: &str) -> anyhow::Result<()> {
let mut in_diff = false;
for line in patch_content.lines() {
Expand Down Expand Up @@ -1872,6 +1947,15 @@ fn validate_patch_paths(patch_content: &str) -> anyhow::Result<()> {
{
let path = line.splitn(3, ' ').nth(2).unwrap_or("").trim_matches('"');
validate_single_path(path)?;
} else if line.starts_with("new file mode 120000")
|| line.starts_with("old mode 120000")
|| line.starts_with("new mode 120000")
{
// Reject symlink entries (git mode 120000 = symbolic link).
// Symlinks in a patch could be used to make Stage 3 follow them to
// arbitrary filesystem paths (e.g. /proc/self/environ) when collecting
// file changes to upload to ADO.
anyhow::bail!("Patch contains a symlink entry (mode 120000), which is not allowed");
}
}
Ok(())
Expand Down Expand Up @@ -2225,6 +2309,37 @@ new file mode 100755
);
}

#[test]
fn test_validate_patch_paths_symlink_rejected() {
// A patch that creates a new symlink (mode 120000) must be rejected.
// This is the primary attack vector for symlink exfiltration of Stage 3 secrets.
let patch = r#"diff --git a/secrets.txt b/secrets.txt
new file mode 120000
index 0000000..abcdefg
--- /dev/null
+++ b/secrets.txt
@@ -0,0 +1 @@
+/proc/self/environ
\ No newline at end of file
"#;
assert!(
validate_patch_paths(patch).is_err(),
"patch with symlink mode 120000 should be rejected"
);
}

#[test]
fn test_validate_patch_paths_symlink_mode_change_rejected() {
// A patch that changes a file to a symlink (old mode → new mode 120000) is rejected.
let patch = "diff --git a/file.txt b/file.txt\n\
old mode 100644\n\
new mode 120000\n";
assert!(
validate_patch_paths(patch).is_err(),
"patch that introduces symlink via mode change should be rejected"
);
}

#[test]
fn test_validate_single_path_valid() {
assert!(validate_single_path("src/main.rs").is_ok());
Expand Down