diff --git a/src/safeoutputs/add_pr_comment.rs b/src/safeoutputs/add_pr_comment.rs index aebdc0a..86e49b7 100644 --- a/src/safeoutputs/add_pr_comment.rs +++ b/src/safeoutputs/add_pr_comment.rs @@ -7,9 +7,10 @@ use serde::{Deserialize, Serialize}; use super::PATH_SEGMENT; use ado_aw_derive::SanitizeConfig; -use crate::sanitize::{SanitizeContent, sanitize as sanitize_text}; +use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; use crate::tool_result; use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::validate::reject_pipeline_injection; use anyhow::{Context, ensure}; /// Parameters for adding a comment thread on a pull request @@ -89,6 +90,7 @@ impl Validate for AddPrCommentParams { if let Some(fp) = &self.file_path { validate_file_path(fp)?; } + reject_pipeline_injection(&self.repository, "repository")?; Ok(()) } } @@ -112,8 +114,8 @@ tool_result! { impl SanitizeContent for AddPrCommentResult { fn sanitize_content_fields(&mut self) { self.content = sanitize_text(&self.content); - // Strip control characters from structural fields for defense-in-depth - self.repository = self.repository.chars().filter(|c| !c.is_control()).collect(); + self.repository = sanitize_config(&self.repository); + // Strip control characters from remaining structural fields for defense-in-depth self.status = self.status.chars().filter(|c| !c.is_control()).collect(); self.file_path = self.file_path.as_ref().map(|fp| { fp.chars().filter(|c| !c.is_control()).collect() @@ -463,6 +465,21 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_validation_rejects_repository_pipeline_command() { + let params = AddPrCommentParams { + pull_request_id: 42, + content: "This is a valid comment body text.".to_string(), + repository: "##vso[task.setvariable variable=x]y".to_string(), + file_path: None, + start_line: None, + line: None, + status: "active".to_string(), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + #[test] fn test_validation_rejects_line_without_file_path() { let params = AddPrCommentParams { @@ -632,4 +649,33 @@ allowed-statuses: "uppercase 'Active' should match config value 'active'" ); } + + #[test] + fn test_sanitize_content_neutralizes_repository_pipeline_command() { + let params = AddPrCommentParams { + pull_request_id: 42, + content: "This is a valid comment body text.".to_string(), + repository: "##vso[task.setvariable variable=x]y".to_string(), + file_path: None, + start_line: None, + line: None, + status: "active".to_string(), + }; + let mut result = AddPrCommentResult { + name: "add-pr-comment".to_string(), + pull_request_id: params.pull_request_id, + content: params.content, + repository: params.repository, + file_path: params.file_path, + start_line: params.start_line, + line: params.line, + status: params.status, + }; + result.sanitize_content_fields(); + assert!( + result.repository.contains("`##vso[`"), + "repository pipeline command should be neutralized with backticks: {}", + result.repository + ); + } } diff --git a/src/safeoutputs/create_branch.rs b/src/safeoutputs/create_branch.rs index 5d26f98..a996edd 100644 --- a/src/safeoutputs/create_branch.rs +++ b/src/safeoutputs/create_branch.rs @@ -10,6 +10,7 @@ use super::{PATH_SEGMENT, validate_git_ref_name}; use crate::sanitize::{SanitizeContent, sanitize_config}; use crate::tool_result; use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::validate::reject_pipeline_injection; use anyhow::{Context, ensure}; /// Parameters for creating a branch @@ -71,6 +72,9 @@ impl Validate for CreateBranchParams { ); validate_git_ref_name(branch, "source_branch")?; } + if let Some(repository) = &self.repository { + reject_pipeline_injection(repository, "repository")?; + } Ok(()) } @@ -92,6 +96,7 @@ tool_result! { impl SanitizeContent for CreateBranchResult { fn sanitize_content_fields(&mut self) { self.branch_name = sanitize_config(&self.branch_name); + self.repository = self.repository.as_deref().map(sanitize_config); } } @@ -459,6 +464,18 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_validation_rejects_repository_pipeline_command() { + let params = CreateBranchParams { + branch_name: "feature/new-work".to_string(), + source_branch: Some("main".to_string()), + source_commit: None, + repository: Some("##vso[task.setvariable variable=x]y".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + #[test] fn test_validation_rejects_branch_starting_with_dash() { let params = CreateBranchParams { @@ -546,4 +563,22 @@ allowed-source-branches: assert_eq!(config.allowed_repositories, vec!["self", "other-repo"]); assert_eq!(config.allowed_source_branches, vec!["main", "develop"]); } + + #[test] + fn test_sanitize_content_neutralizes_repository_pipeline_command() { + let mut result = CreateBranchResult { + name: "create-branch".to_string(), + branch_name: "feature/new-work".to_string(), + source_branch: Some("main".to_string()), + source_commit: None, + repository: Some("##vso[task.setvariable variable=x]y".to_string()), + }; + result.sanitize_content_fields(); + let repository = result.repository.as_deref().unwrap_or(""); + assert!( + repository.contains("`##vso[`"), + "repository pipeline command should be neutralized with backticks: {}", + repository + ); + } } diff --git a/src/safeoutputs/create_git_tag.rs b/src/safeoutputs/create_git_tag.rs index 5f24193..a8ef8dd 100644 --- a/src/safeoutputs/create_git_tag.rs +++ b/src/safeoutputs/create_git_tag.rs @@ -7,9 +7,10 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use super::{PATH_SEGMENT, validate_git_ref_name}; -use crate::sanitize::{SanitizeContent, sanitize as sanitize_text}; +use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; use crate::tool_result; use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::validate::reject_pipeline_injection; use anyhow::{Context, ensure}; /// Parameters for creating a git tag (agent-provided) @@ -75,6 +76,9 @@ impl Validate for CreateGitTagParams { "message must be at least 5 characters" ); } + if let Some(repository) = &self.repository { + reject_pipeline_injection(repository, "repository")?; + } Ok(()) } @@ -106,9 +110,7 @@ impl SanitizeContent for CreateGitTagResult { self.commit = self.commit.as_ref().map(|c| { c.chars().filter(|ch| !ch.is_control()).collect() }); - self.repository = self.repository.as_ref().map(|r| { - r.chars().filter(|ch| !ch.is_control()).collect() - }); + self.repository = self.repository.as_deref().map(sanitize_config); } } @@ -477,6 +479,18 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_validation_rejects_repository_pipeline_command() { + let params = CreateGitTagParams { + tag_name: "v1.0.0".to_string(), + commit: None, + message: Some("Valid message".to_string()), + repository: Some("##vso[task.setvariable variable=x]y".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + #[test] fn test_result_serializes_correctly() { let params = CreateGitTagParams { @@ -517,4 +531,22 @@ message-prefix: "[release] " assert_eq!(config.allowed_repositories, vec!["self", "my-lib"]); assert_eq!(config.message_prefix.as_deref(), Some("[release] ")); } + + #[test] + fn test_sanitize_content_neutralizes_repository_pipeline_command() { + let mut result = CreateGitTagResult { + name: "create-git-tag".to_string(), + tag_name: "v1.0.0".to_string(), + commit: None, + message: Some("Valid message".to_string()), + repository: Some("##vso[task.setvariable variable=x]y".to_string()), + }; + result.sanitize_content_fields(); + let repository = result.repository.as_deref().unwrap_or(""); + assert!( + repository.contains("`##vso[`"), + "repository pipeline command should be neutralized with backticks: {}", + repository + ); + } } diff --git a/src/safeoutputs/create_pr.rs b/src/safeoutputs/create_pr.rs index b45cd74..9831217 100644 --- a/src/safeoutputs/create_pr.rs +++ b/src/safeoutputs/create_pr.rs @@ -9,6 +9,7 @@ use ado_aw_derive::SanitizeConfig; use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, ToolResult, Validate}; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; use crate::tool_result; +use crate::validate::reject_pipeline_injection; use anyhow::{Context, ensure}; /// Maximum allowed patch file size (5 MB) @@ -254,6 +255,9 @@ impl Validate for CreatePrParams { self.description.len() >= 10, "PR description must be at least 10 characters" ); + if let Some(repository) = &self.repository { + reject_pipeline_injection(repository, "repository")?; + } Ok(()) } } @@ -320,6 +324,7 @@ impl SanitizeContent for CreatePrResult { fn sanitize_content_fields(&mut self) { self.title = sanitize_text(&self.title); self.description = sanitize_text(&self.description); + self.repository = sanitize_config(&self.repository); for label in &mut self.agent_labels { *label = sanitize_config(label); } @@ -2092,6 +2097,37 @@ mod tests { assert!(params.validate().is_err()); } + #[test] + fn test_validate_params_rejects_repository_pipeline_command() { + let params = CreatePrParams { + title: "Fix bug in parser".to_string(), + description: "This PR fixes a critical bug in the parser module.".to_string(), + repository: Some("##vso[task.setvariable variable=x]y".to_string()), + labels: vec![], + }; + assert!(params.validate().is_err()); + } + + #[test] + fn test_sanitize_content_neutralizes_repository_pipeline_command() { + let mut result = CreatePrResult::new( + "Fix bug in parser".to_string(), + "This PR fixes a critical bug in the parser module.".to_string(), + "agent/fix-parser".to_string(), + "/tmp/test.patch".to_string(), + "##vso[task.setvariable variable=x]y".to_string(), + vec![], + None, + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef".to_string(), + ); + result.sanitize_content_fields(); + assert!( + result.repository.contains("`##vso[`"), + "repository pipeline command should be neutralized with backticks: {}", + result.repository + ); + } + #[test] fn test_config_default_target_branch() { let config = CreatePrConfig::default(); diff --git a/src/safeoutputs/reply_to_pr_comment.rs b/src/safeoutputs/reply_to_pr_comment.rs index fdcae6e..3579f31 100644 --- a/src/safeoutputs/reply_to_pr_comment.rs +++ b/src/safeoutputs/reply_to_pr_comment.rs @@ -7,9 +7,10 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use super::PATH_SEGMENT; -use crate::sanitize::{SanitizeContent, sanitize as sanitize_text}; +use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; use crate::tool_result; use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::validate::reject_pipeline_injection; use anyhow::{Context, ensure}; /// Parameters for replying to an existing review comment thread on a pull request @@ -42,6 +43,9 @@ impl Validate for ReplyToPrCommentParams { self.content.len() >= 10, "content must be at least 10 characters" ); + if let Some(repository) = &self.repository { + reject_pipeline_injection(repository, "repository")?; + } Ok(()) } } @@ -62,6 +66,7 @@ tool_result! { impl SanitizeContent for ReplyToPrCommentResult { fn sanitize_content_fields(&mut self) { self.content = sanitize_text(&self.content); + self.repository = self.repository.as_deref().map(sanitize_config); } } @@ -318,6 +323,18 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_validation_rejects_repository_pipeline_command() { + let params = ReplyToPrCommentParams { + pull_request_id: 42, + thread_id: 7, + content: "This is a valid reply body text.".to_string(), + repository: Some("##vso[task.setvariable variable=x]y".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + #[test] fn test_result_serializes_correctly() { let params = ReplyToPrCommentParams { @@ -353,4 +370,22 @@ allowed-repositories: assert_eq!(config.comment_prefix, Some("[Agent] ".to_string())); assert_eq!(config.allowed_repositories, vec!["self", "other-repo"]); } + + #[test] + fn test_sanitize_content_neutralizes_repository_pipeline_command() { + let mut result = ReplyToPrCommentResult { + name: "reply-to-pr-review-comment".to_string(), + pull_request_id: 42, + thread_id: 7, + content: "This is a valid reply body text.".to_string(), + repository: Some("##vso[task.setvariable variable=x]y".to_string()), + }; + result.sanitize_content_fields(); + let repository = result.repository.as_deref().unwrap_or(""); + assert!( + repository.contains("`##vso[`"), + "repository pipeline command should be neutralized with backticks: {}", + repository + ); + } } diff --git a/src/safeoutputs/resolve_pr_thread.rs b/src/safeoutputs/resolve_pr_thread.rs index 10e291e..6989772 100644 --- a/src/safeoutputs/resolve_pr_thread.rs +++ b/src/safeoutputs/resolve_pr_thread.rs @@ -11,6 +11,7 @@ use super::PATH_SEGMENT; use crate::sanitize::{SanitizeContent, sanitize_config}; use crate::tool_result; use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::validate::reject_pipeline_injection; use anyhow::{Context, ensure}; /// All valid thread status strings (lowercase, agent-facing) @@ -70,6 +71,9 @@ impl Validate for ResolvePrThreadParams { self.status, VALID_STATUSES.join(", ") ); + if let Some(repository) = &self.repository { + reject_pipeline_injection(repository, "repository")?; + } Ok(()) } } @@ -364,6 +368,18 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_validation_rejects_repository_pipeline_command() { + let params = ResolvePrThreadParams { + pull_request_id: 42, + thread_id: 7, + status: "fixed".to_string(), + repository: Some("##vso[task.setvariable variable=x]y".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + #[test] fn test_result_serializes_correctly() { let params = ResolvePrThreadParams { diff --git a/src/safeoutputs/submit_pr_review.rs b/src/safeoutputs/submit_pr_review.rs index 12f4461..ad0e165 100644 --- a/src/safeoutputs/submit_pr_review.rs +++ b/src/safeoutputs/submit_pr_review.rs @@ -10,6 +10,7 @@ use super::{PATH_SEGMENT, resolve_repo_name}; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; use crate::tool_result; use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::validate::reject_pipeline_injection; use anyhow::{Context, ensure}; /// Valid event values for submit-pr-review @@ -57,6 +58,9 @@ impl Validate for SubmitPrReviewParams { self.pull_request_id > 0, "pull_request_id must be a positive integer" ); + if let Some(repository) = &self.repository { + reject_pipeline_injection(repository, "repository")?; + } ensure!( VALID_EVENTS.contains(&self.event.as_str()), "event must be one of: {}", @@ -511,6 +515,18 @@ mod tests { assert!(result.is_ok()); } + #[test] + fn test_validation_rejects_repository_pipeline_command() { + let params = SubmitPrReviewParams { + pull_request_id: 1, + event: "approve".to_string(), + body: None, + repository: Some("##vso[task.setvariable variable=x]y".to_string()), + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + #[test] fn test_result_serializes_correctly() { let params = SubmitPrReviewParams { diff --git a/src/safeoutputs/update_pr.rs b/src/safeoutputs/update_pr.rs index 7b7e01c..9efa154 100644 --- a/src/safeoutputs/update_pr.rs +++ b/src/safeoutputs/update_pr.rs @@ -10,6 +10,7 @@ use super::{PATH_SEGMENT, resolve_repo_name}; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; use crate::tool_result; use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::validate::reject_pipeline_injection; use anyhow::{Context, ensure}; /// Valid operation names for update-pr @@ -77,6 +78,9 @@ impl Validate for UpdatePrParams { self.pull_request_id > 0, "pull_request_id must be a positive integer" ); + if let Some(repository) = &self.repository { + reject_pipeline_injection(repository, "repository")?; + } ensure!( VALID_OPERATIONS.contains(&self.operation.as_str()), "operation must be one of: {}", @@ -127,7 +131,6 @@ impl Validate for UpdatePrParams { } _ => {} // set-auto-complete has no extra required fields } - Ok(()) } } @@ -1062,6 +1065,21 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_validation_rejects_repository_pipeline_command() { + let params = UpdatePrParams { + pull_request_id: 1, + repository: Some("##vso[task.setvariable variable=x]y".to_string()), + operation: "set-auto-complete".to_string(), + reviewers: None, + labels: None, + vote: None, + description: None, + }; + let result: Result = params.try_into(); + assert!(result.is_err()); + } + #[test] fn test_result_serializes_correctly() { let params = UpdatePrParams {