diff --git a/src/execute.rs b/src/execute.rs index 307aa0b3..743af2fd 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -11,6 +11,7 @@ use std::collections::HashMap; use std::path::Path; use crate::ndjson::{self, SAFE_OUTPUT_FILENAME}; +use crate::sanitize::neutralize_pipeline_commands; use crate::safeoutputs::{ AddBuildTagResult, AddPrCommentResult, CreateBranchResult, CreateGitTagResult, CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, CommentOnWorkItemResult, @@ -362,7 +363,7 @@ fn resolve_max(ctx: &ExecutionContext, tool_name: &str, default_max: u32) -> usi /// Extract a human-readable context identifier from a safe-output entry for log messages. /// Called before sanitization, so all string values are stripped of control characters -/// to prevent log injection. +/// and ADO pipeline commands are neutralized to prevent log injection via stdout. fn extract_entry_context(entry: &Value) -> String { if let Some(id) = entry.get("id").and_then(|v| v.as_u64()) { return format!(" (work item #{})", id); @@ -372,6 +373,7 @@ fn extract_entry_context(entry: &Value) -> String { } if let Some(title) = entry.get("title").and_then(|v| v.as_str()) { let clean: String = title.chars().filter(|c| !c.is_control()).collect(); + let clean = neutralize_pipeline_commands(&clean); let truncated: &str = if clean.chars().count() > 40 { &clean[..clean.char_indices().nth(40).map(|(i, _)| i).unwrap_or(clean.len())] } else { @@ -381,6 +383,7 @@ fn extract_entry_context(entry: &Value) -> String { } if let Some(path) = entry.get("path").and_then(|v| v.as_str()) { let clean: String = path.chars().filter(|c| !c.is_control()).collect(); + let clean = neutralize_pipeline_commands(&clean); return format!(" (path: {})", clean); } String::new() @@ -433,6 +436,52 @@ mod tests { use std::collections::HashMap; use std::path::PathBuf; + // ── extract_entry_context ───────────────────────────────────────────────── + + #[test] + fn test_extract_entry_context_neutralizes_vso_in_title() { + let entry = serde_json::json!({ + "title": "##vso[task.complete result=Failed]" + }); + let ctx = extract_entry_context(&entry); + assert!( + !ctx.contains("##vso[task."), + "VSO command in title should be neutralized; got: {ctx}" + ); + assert!( + ctx.contains("`##vso[`"), + "VSO command should be wrapped in backticks; got: {ctx}" + ); + } + + #[test] + fn test_extract_entry_context_neutralizes_vso_in_path() { + let entry = serde_json::json!({ + "path": "##vso[task.setvariable variable=X]injected" + }); + let ctx = extract_entry_context(&entry); + assert!( + !ctx.contains("##vso[task."), + "VSO command in path should be neutralized; got: {ctx}" + ); + assert!( + ctx.contains("`##vso[`"), + "VSO command should be wrapped in backticks; got: {ctx}" + ); + } + + #[test] + fn test_extract_entry_context_preserves_normal_title() { + let entry = serde_json::json!({"title": "Fix login bug"}); + assert_eq!(extract_entry_context(&entry), " (\"Fix login bug\")"); + } + + #[test] + fn test_extract_entry_context_prefers_id_over_title() { + let entry = serde_json::json!({"id": 42, "title": "should be ignored"}); + assert_eq!(extract_entry_context(&entry), " (work item #42)"); + } + #[tokio::test] async fn test_execute_unknown_tool_fails() { let entry = serde_json::json!({"name": "unknown_tool", "foo": "bar"}); diff --git a/src/safeoutputs/create_wiki_page.rs b/src/safeoutputs/create_wiki_page.rs index 67fdea5e..1146e3b5 100644 --- a/src/safeoutputs/create_wiki_page.rs +++ b/src/safeoutputs/create_wiki_page.rs @@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize}; use super::PATH_SEGMENT; use super::resolve_wiki_branch; use ado_aw_derive::SanitizeConfig; -use crate::sanitize::{SanitizeContent, sanitize as sanitize_text}; +use crate::sanitize::{SanitizeContent, neutralize_pipeline_commands, sanitize as sanitize_text}; use crate::tool_result; use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; @@ -70,13 +70,17 @@ tool_result! { impl SanitizeContent for CreateWikiPageResult { fn sanitize_content_fields(&mut self) { - // Path is a structural identifier — sanitize lightly (remove control chars) - // but do not escape HTML or neutralize patterns that are valid in wiki paths. - self.path = self - .path - .chars() - .filter(|c| !c.is_control() || *c == '\t') - .collect(); + // Path is a structural identifier — remove control characters and + // neutralize ADO pipeline commands to prevent VSO command injection + // via log output, without escaping HTML or corrupting valid path + // characters like slashes, spaces, or angle brackets. + self.path = neutralize_pipeline_commands( + &self + .path + .chars() + .filter(|c| !c.is_control() || *c == '\t') + .collect::(), + ); self.content = sanitize_text(&self.content); self.comment = self.comment.as_ref().map(|c| sanitize_text(c)); } @@ -676,6 +680,27 @@ wiki-name: "MyProject.wiki" assert_eq!(result.path, "/Folder/My Page"); } + #[test] + fn test_sanitize_neutralizes_vso_command_in_path() { + let params = CreateWikiPageParams { + path: "/##vso[task.setvariable variable=X]injected".to_string(), + content: "Some valid content here.".to_string(), + comment: None, + }; + let mut result: CreateWikiPageResult = params.try_into().unwrap(); + result.sanitize_content_fields(); + assert!( + !result.path.contains("##vso[task."), + "VSO command in path should be neutralized; got: {}", + result.path + ); + assert!( + result.path.contains("`##vso[`"), + "VSO command should be wrapped in backticks; got: {}", + result.path + ); + } + // ── Executor (no-token failure) ─────────────────────────────────────────── #[tokio::test] diff --git a/src/safeoutputs/update_wiki_page.rs b/src/safeoutputs/update_wiki_page.rs index c0824b54..7528b362 100644 --- a/src/safeoutputs/update_wiki_page.rs +++ b/src/safeoutputs/update_wiki_page.rs @@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize}; use super::PATH_SEGMENT; use super::resolve_wiki_branch; use ado_aw_derive::SanitizeConfig; -use crate::sanitize::{SanitizeContent, sanitize as sanitize_text}; +use crate::sanitize::{SanitizeContent, neutralize_pipeline_commands, sanitize as sanitize_text}; use crate::tool_result; use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; @@ -66,13 +66,17 @@ tool_result! { impl SanitizeContent for UpdateWikiPageResult { fn sanitize_content_fields(&mut self) { - // Path is a structural identifier — sanitize lightly (remove control chars) - // but do not escape HTML or neutralize patterns that are valid in wiki paths. - self.path = self - .path - .chars() - .filter(|c| !c.is_control() || *c == '\t') - .collect(); + // Path is a structural identifier — remove control characters and + // neutralize ADO pipeline commands to prevent VSO command injection + // via log output, without escaping HTML or corrupting valid path + // characters like slashes, spaces, or angle brackets. + self.path = neutralize_pipeline_commands( + &self + .path + .chars() + .filter(|c| !c.is_control() || *c == '\t') + .collect::(), + ); self.content = sanitize_text(&self.content); self.comment = self.comment.as_ref().map(|c| sanitize_text(c)); } @@ -648,6 +652,27 @@ wiki-name: "MyProject.wiki" assert_eq!(result.path, "/Folder/My Page"); } + #[test] + fn test_sanitize_neutralizes_vso_command_in_path() { + let params = UpdateWikiPageParams { + path: "/##vso[task.setvariable variable=X]injected".to_string(), + content: "Some valid content here.".to_string(), + comment: None, + }; + let mut result: UpdateWikiPageResult = params.try_into().unwrap(); + result.sanitize_content_fields(); + assert!( + !result.path.contains("##vso[task."), + "VSO command in path should be neutralized; got: {}", + result.path + ); + assert!( + result.path.contains("`##vso[`"), + "VSO command should be wrapped in backticks; got: {}", + result.path + ); + } + // ── Executor (no-token failure) ─────────────────────────────────────────── #[tokio::test] diff --git a/src/sanitize.rs b/src/sanitize.rs index b5cf0977..fe2de1c8 100644 --- a/src/sanitize.rs +++ b/src/sanitize.rs @@ -142,7 +142,7 @@ fn remove_control_characters(input: &str) -> String { /// /// Also handles `##[` (the shorthand form used for `##[section]`, `##[error]`, /// etc.) which ADO pipelines also interpret. -fn neutralize_pipeline_commands(input: &str) -> String { +pub(crate) fn neutralize_pipeline_commands(input: &str) -> String { let mut result = String::with_capacity(input.len() + 32); let mut rest = input;