From aaefca1fa1b4097d2c0457dbbaf360a72c94d8ff Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 11 May 2026 06:11:33 +0000 Subject: [PATCH 1/2] Initial plan From 576d9b2e561e05da51f90a00497c3d16f6f0b7dd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 11 May 2026 06:17:36 +0000 Subject: [PATCH 2/2] fix(safeoutputs): block VSO command injection in upload file paths and stdout messages Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/e5c9bc47-654c-4611-9ab5-62a953014ad5 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/execute.rs | 11 ++++++- src/safeoutputs/upload_build_attachment.rs | 22 ++++++++++++++ src/safeoutputs/upload_pipeline_artifact.rs | 22 ++++++++++++++ src/safeoutputs/upload_workitem_attachment.rs | 30 ++++++++++++++++--- 4 files changed, 80 insertions(+), 5 deletions(-) diff --git a/src/execute.rs b/src/execute.rs index ca03eec..0d0e535 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -214,7 +214,8 @@ fn log_and_print_entry_result(i: usize, total: usize, tool_name: &str, result: & warn!("[{}/{}] {} failed: {}", i + 1, total, tool_name, result.message); } let symbol = if result.is_warning() { "⚠" } else if result.success { "✓" } else { "✗" }; - println!("[{}/{}] {} - {} - {}", i + 1, total, tool_name, symbol, result.message); + let safe_msg = neutralize_pipeline_commands(&result.message); + println!("[{}/{}] {} - {} - {}", i + 1, total, tool_name, symbol, safe_msg); } /// Parse a JSON entry as `T` and run it through `execute_sanitized`. @@ -486,6 +487,14 @@ mod tests { assert_eq!(extract_entry_context(&entry), " (work item #42)"); } + #[test] + fn test_stdout_print_neutralizes_result_message_pipeline_commands() { + let message = "Uploaded '##vso[task.setvariable variable=X]y.txt'"; + let safe = neutralize_pipeline_commands(message); + assert!(!safe.contains("##vso[task")); + assert!(safe.contains("`##vso[`")); + } + #[tokio::test] async fn test_execute_unknown_tool_fails() { let entry = serde_json::json!({"name": "unknown_tool", "foo": "bar"}); diff --git a/src/safeoutputs/upload_build_attachment.rs b/src/safeoutputs/upload_build_attachment.rs index 5773440..99c44a0 100644 --- a/src/safeoutputs/upload_build_attachment.rs +++ b/src/safeoutputs/upload_build_attachment.rs @@ -89,6 +89,10 @@ impl Validate for UploadBuildAttachmentParams { !self.file_path.contains(':'), "file_path must not contain ':'" ); + ensure!( + !self.file_path.contains("##vso[") && !self.file_path.contains("##["), + "file_path must not contain Azure DevOps pipeline command sequences" + ); for component in self.file_path.split(['/', '\\']) { ensure!( is_safe_path_segment(component), @@ -729,6 +733,24 @@ mod tests { .is_err()); } + #[test] + fn test_validation_rejects_pipeline_command_sequences_in_file_path() { + assert!( + make_params( + Some(1), + "agent-report", + "##vso[task.setvariable variable=EXPLOIT]value.txt" + ) + .validate() + .is_err() + ); + assert!( + make_params(Some(1), "agent-report", "##[error]value.txt") + .validate() + .is_err() + ); + } + #[test] fn test_result_serializes_correctly_with_build_id() { let result = UploadBuildAttachmentResult::new( diff --git a/src/safeoutputs/upload_pipeline_artifact.rs b/src/safeoutputs/upload_pipeline_artifact.rs index 60d1414..b017a23 100644 --- a/src/safeoutputs/upload_pipeline_artifact.rs +++ b/src/safeoutputs/upload_pipeline_artifact.rs @@ -111,6 +111,10 @@ impl Validate for UploadPipelineArtifactParams { !self.file_path.contains(':'), "file_path must not contain ':'" ); + ensure!( + !self.file_path.contains("##vso[") && !self.file_path.contains("##["), + "file_path must not contain Azure DevOps pipeline command sequences" + ); for component in self.file_path.split(['/', '\\']) { ensure!( is_safe_path_segment(component), @@ -776,6 +780,24 @@ mod tests { .is_err()); } + #[test] + fn test_validation_rejects_pipeline_command_sequences_in_file_path() { + assert!( + make_params( + None, + "report", + "##vso[task.setvariable variable=EXPLOIT]value.txt" + ) + .validate() + .is_err() + ); + assert!( + make_params(None, "report", "##[error]value.txt") + .validate() + .is_err() + ); + } + #[test] fn test_dry_run_summary() { let result = UploadPipelineArtifactResult::new( diff --git a/src/safeoutputs/upload_workitem_attachment.rs b/src/safeoutputs/upload_workitem_attachment.rs index 269565f..5423053 100644 --- a/src/safeoutputs/upload_workitem_attachment.rs +++ b/src/safeoutputs/upload_workitem_attachment.rs @@ -50,6 +50,10 @@ impl Validate for UploadWorkitemAttachmentParams { !contains_newline(&self.file_path), "file_path must not contain newlines or carriage returns" ); + ensure!( + !self.file_path.contains("##vso[") && !self.file_path.contains("##["), + "file_path must not contain Azure DevOps pipeline command sequences" + ); ensure!( !self .file_path @@ -222,10 +226,10 @@ impl Executor for UploadWorkitemAttachmentResult { // acceptable because the injection risk from binary attachments is negligible. if let Ok(text) = std::str::from_utf8(&file_bytes) { if text.contains("##vso[") { - return Ok(ExecutionResult::failure(format!( - "File '{}' contains '##vso[' command injection sequence", - self.file_path - ))); + return Ok(ExecutionResult::failure( + "Uploaded file contains '##vso[' command injection sequence — upload rejected" + .to_string(), + )); } } @@ -521,6 +525,24 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_validation_rejects_pipeline_command_sequences_in_file_path() { + let vso = UploadWorkitemAttachmentParams { + work_item_id: 42, + file_path: "##vso[task.setvariable variable=EXPLOIT]value.txt".to_string(), + comment: None, + }; + let shorthand = UploadWorkitemAttachmentParams { + work_item_id: 42, + file_path: "##[error]value.txt".to_string(), + comment: None, + }; + let vso_result: Result = vso.try_into(); + let shorthand_result: Result = shorthand.try_into(); + assert!(vso_result.is_err()); + assert!(shorthand_result.is_err()); + } + #[test] fn test_result_serializes_correctly() { let params = UploadWorkitemAttachmentParams {