Skip to content

Commit 45cd552

Browse files
authored
fix(safeoutputs): neutralize Stage 3 upload message command injection paths (#501)
1 parent f9d3550 commit 45cd552

4 files changed

Lines changed: 80 additions & 5 deletions

File tree

src/execute.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,8 @@ fn log_and_print_entry_result(i: usize, total: usize, tool_name: &str, result: &
214214
warn!("[{}/{}] {} failed: {}", i + 1, total, tool_name, result.message);
215215
}
216216
let symbol = if result.is_warning() { "⚠" } else if result.success { "✓" } else { "✗" };
217-
println!("[{}/{}] {} - {} - {}", i + 1, total, tool_name, symbol, result.message);
217+
let safe_msg = neutralize_pipeline_commands(&result.message);
218+
println!("[{}/{}] {} - {} - {}", i + 1, total, tool_name, symbol, safe_msg);
218219
}
219220

220221
/// Parse a JSON entry as `T` and run it through `execute_sanitized`.
@@ -486,6 +487,14 @@ mod tests {
486487
assert_eq!(extract_entry_context(&entry), " (work item #42)");
487488
}
488489

490+
#[test]
491+
fn test_stdout_print_neutralizes_result_message_pipeline_commands() {
492+
let message = "Uploaded '##vso[task.setvariable variable=X]y.txt'";
493+
let safe = neutralize_pipeline_commands(message);
494+
assert!(!safe.contains("##vso[task"));
495+
assert!(safe.contains("`##vso[`"));
496+
}
497+
489498
#[tokio::test]
490499
async fn test_execute_unknown_tool_fails() {
491500
let entry = serde_json::json!({"name": "unknown_tool", "foo": "bar"});

src/safeoutputs/upload_build_attachment.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,10 @@ impl Validate for UploadBuildAttachmentParams {
8989
!self.file_path.contains(':'),
9090
"file_path must not contain ':'"
9191
);
92+
ensure!(
93+
!self.file_path.contains("##vso[") && !self.file_path.contains("##["),
94+
"file_path must not contain Azure DevOps pipeline command sequences"
95+
);
9296
for component in self.file_path.split(['/', '\\']) {
9397
ensure!(
9498
is_safe_path_segment(component),
@@ -729,6 +733,24 @@ mod tests {
729733
.is_err());
730734
}
731735

736+
#[test]
737+
fn test_validation_rejects_pipeline_command_sequences_in_file_path() {
738+
assert!(
739+
make_params(
740+
Some(1),
741+
"agent-report",
742+
"##vso[task.setvariable variable=EXPLOIT]value.txt"
743+
)
744+
.validate()
745+
.is_err()
746+
);
747+
assert!(
748+
make_params(Some(1), "agent-report", "##[error]value.txt")
749+
.validate()
750+
.is_err()
751+
);
752+
}
753+
732754
#[test]
733755
fn test_result_serializes_correctly_with_build_id() {
734756
let result = UploadBuildAttachmentResult::new(

src/safeoutputs/upload_pipeline_artifact.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,10 @@ impl Validate for UploadPipelineArtifactParams {
111111
!self.file_path.contains(':'),
112112
"file_path must not contain ':'"
113113
);
114+
ensure!(
115+
!self.file_path.contains("##vso[") && !self.file_path.contains("##["),
116+
"file_path must not contain Azure DevOps pipeline command sequences"
117+
);
114118
for component in self.file_path.split(['/', '\\']) {
115119
ensure!(
116120
is_safe_path_segment(component),
@@ -776,6 +780,24 @@ mod tests {
776780
.is_err());
777781
}
778782

783+
#[test]
784+
fn test_validation_rejects_pipeline_command_sequences_in_file_path() {
785+
assert!(
786+
make_params(
787+
None,
788+
"report",
789+
"##vso[task.setvariable variable=EXPLOIT]value.txt"
790+
)
791+
.validate()
792+
.is_err()
793+
);
794+
assert!(
795+
make_params(None, "report", "##[error]value.txt")
796+
.validate()
797+
.is_err()
798+
);
799+
}
800+
779801
#[test]
780802
fn test_dry_run_summary() {
781803
let result = UploadPipelineArtifactResult::new(

src/safeoutputs/upload_workitem_attachment.rs

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,10 @@ impl Validate for UploadWorkitemAttachmentParams {
5050
!contains_newline(&self.file_path),
5151
"file_path must not contain newlines or carriage returns"
5252
);
53+
ensure!(
54+
!self.file_path.contains("##vso[") && !self.file_path.contains("##["),
55+
"file_path must not contain Azure DevOps pipeline command sequences"
56+
);
5357
ensure!(
5458
!self
5559
.file_path
@@ -222,10 +226,10 @@ impl Executor for UploadWorkitemAttachmentResult {
222226
// acceptable because the injection risk from binary attachments is negligible.
223227
if let Ok(text) = std::str::from_utf8(&file_bytes) {
224228
if text.contains("##vso[") {
225-
return Ok(ExecutionResult::failure(format!(
226-
"File '{}' contains '##vso[' command injection sequence",
227-
self.file_path
228-
)));
229+
return Ok(ExecutionResult::failure(
230+
"Uploaded file contains '##vso[' command injection sequence — upload rejected"
231+
.to_string(),
232+
));
229233
}
230234
}
231235

@@ -521,6 +525,24 @@ mod tests {
521525
assert!(result.is_err());
522526
}
523527

528+
#[test]
529+
fn test_validation_rejects_pipeline_command_sequences_in_file_path() {
530+
let vso = UploadWorkitemAttachmentParams {
531+
work_item_id: 42,
532+
file_path: "##vso[task.setvariable variable=EXPLOIT]value.txt".to_string(),
533+
comment: None,
534+
};
535+
let shorthand = UploadWorkitemAttachmentParams {
536+
work_item_id: 42,
537+
file_path: "##[error]value.txt".to_string(),
538+
comment: None,
539+
};
540+
let vso_result: Result<UploadWorkitemAttachmentResult, _> = vso.try_into();
541+
let shorthand_result: Result<UploadWorkitemAttachmentResult, _> = shorthand.try_into();
542+
assert!(vso_result.is_err());
543+
assert!(shorthand_result.is_err());
544+
}
545+
524546
#[test]
525547
fn test_result_serializes_correctly() {
526548
let params = UploadWorkitemAttachmentParams {

0 commit comments

Comments
 (0)