Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
52 changes: 49 additions & 3 deletions src/safeoutputs/add_pr_comment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(())
}
}
Expand All @@ -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()
Expand Down Expand Up @@ -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<AddPrCommentResult, _> = params.try_into();
assert!(result.is_err());
}

#[test]
fn test_validation_rejects_line_without_file_path() {
let params = AddPrCommentParams {
Expand Down Expand Up @@ -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
);
}
}
35 changes: 35 additions & 0 deletions src/safeoutputs/create_branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(())
}
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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<CreateBranchResult, _> = params.try_into();
assert!(result.is_err());
}

#[test]
fn test_validation_rejects_branch_starting_with_dash() {
let params = CreateBranchParams {
Expand Down Expand Up @@ -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
);
}
}
40 changes: 36 additions & 4 deletions src/safeoutputs/create_git_tag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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(())
}
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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<CreateGitTagResult, _> = params.try_into();
assert!(result.is_err());
}

#[test]
fn test_result_serializes_correctly() {
let params = CreateGitTagParams {
Expand Down Expand Up @@ -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
);
}
}
36 changes: 36 additions & 0 deletions src/safeoutputs/create_pr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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(())
}
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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();
Expand Down
37 changes: 36 additions & 1 deletion src/safeoutputs/reply_to_pr_comment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(())
}
}
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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<ReplyToPrCommentResult, _> = params.try_into();
assert!(result.is_err());
}

#[test]
fn test_result_serializes_correctly() {
let params = ReplyToPrCommentParams {
Expand Down Expand Up @@ -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
);
}
}
Loading