diff --git a/.github/workflows/red-team-security.md b/.github/workflows/red-team-security.md index 9edd8fdf..156b7df0 100644 --- a/.github/workflows/red-team-security.md +++ b/.github/workflows/red-team-security.md @@ -68,7 +68,7 @@ grep -n 'replace\|replace_with_indent' src/compile/standalone.rs src/compile/one ### Category B: Path Traversal & File System -Audit `src/execute.rs`, `src/tools/create_pr.rs`, `src/tools/memory.rs`, `src/tools/upload_attachment.rs` for: +Audit `src/execute.rs`, `src/tools/create_pr.rs`, `src/tools/memory.rs`, `src/tools/upload_workitem_attachment.rs` for: - **Directory traversal**: Are `..` sequences fully blocked in all path inputs? Check safe output file paths, memory file paths, wiki page paths, attachment paths. - **Bounding directory escape**: Can the MCP server's bounding directory check be bypassed via symlinks, null bytes, or Unicode normalization? @@ -79,7 +79,7 @@ Focus files: ```bash cat src/tools/memory.rs cat src/tools/create_pr.rs -cat src/tools/upload_attachment.rs +cat src/tools/upload_workitem_attachment.rs grep -rn 'Path\|PathBuf\|canonicalize\|starts_with' src/tools/ grep -rn '\.\./' src/ ``` diff --git a/AGENTS.md b/AGENTS.md index b897dc79..e0033171 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -93,7 +93,7 @@ Every compiled pipeline runs as three sequential jobs: │ │ ├── update_pr.rs │ │ ├── update_wiki_page.rs │ │ ├── update_work_item.rs -│ │ └── upload_attachment.rs +│ │ └── upload_workitem_attachment.rs │ ├── runtimes/ # Runtime environment implementations (one dir per runtime) │ │ ├── mod.rs # Module entry point │ │ └── lean/ # Lean 4 theorem prover runtime diff --git a/README.md b/README.md index 8da1eab3..f1fb9d85 100644 --- a/README.md +++ b/README.md @@ -389,7 +389,7 @@ actions, and the executor processes them after threat analysis. | `create-git-tag` | Creates a git tag on a repository ref | | `create-branch` | Creates a new branch from an existing ref | | `add-build-tag` | Adds a tag to an ADO build | -| `upload-attachment` | Uploads a workspace file as an attachment to a work item | +| `upload-workitem-attachment` | Uploads a workspace file as an attachment to a work item | | `report-incomplete` | Reports that a task could not be completed | | `noop` | Reports no action was needed | | `missing-data` | Reports required data was unavailable | diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index 0f3f3ae7..4ba50349 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -400,7 +400,7 @@ safe-outputs: max: 1 # Maximum per run (default: 1) ``` -### upload-attachment +### upload-workitem-attachment Uploads a workspace file as an attachment to an Azure DevOps work item. **Agent parameters:** @@ -411,7 +411,7 @@ Uploads a workspace file as an attachment to an Azure DevOps work item. **Configuration options (front matter):** ```yaml safe-outputs: - upload-attachment: + upload-workitem-attachment: max-file-size: 5242880 # Maximum file size in bytes (default: 5 MB) allowed-extensions: [] # Optional — restrict file types (e.g., [".png", ".pdf"]) comment-prefix: "[Agent] " # Optional — prefix prepended to the comment diff --git a/prompts/create-ado-agentic-workflow.md b/prompts/create-ado-agentic-workflow.md index 7cb44c45..b7bb72ef 100644 --- a/prompts/create-ado-agentic-workflow.md +++ b/prompts/create-ado-agentic-workflow.md @@ -277,7 +277,7 @@ tools: | `update-work-item` | Update fields on existing work items (each field requires opt-in) | ✅ | | `comment-on-work-item` | Add comments to work items (requires `target` scoping) | ✅ | | `link-work-items` | Link two work items (parent/child, related, etc.) | ✅ | -| `upload-attachment` | Upload a workspace file to a work item | ✅ | +| `upload-workitem-attachment` | Upload a workspace file to a work item | ✅ | | **Pull Requests** | | | | `create-pull-request` | Create PRs from agent code changes | ✅ | | `add-pr-comment` | Add a comment thread to a PR | ✅ | diff --git a/prompts/update-ado-agentic-workflow.md b/prompts/update-ado-agentic-workflow.md index 7cb27043..622c1d0e 100644 --- a/prompts/update-ado-agentic-workflow.md +++ b/prompts/update-ado-agentic-workflow.md @@ -193,7 +193,7 @@ permissions: | Both | Agent reads; safe-outputs write | | Neither | No ADO tokens anywhere | -If adding write-requiring safe outputs (`create-pull-request`, `create-work-item`, `comment-on-work-item`, `update-work-item`, `create-wiki-page`, `update-wiki-page`, `link-work-items`, `upload-attachment`, `create-branch`, `create-git-tag`, `add-build-tag`, `add-pr-comment`, `reply-to-pr-comment`, `resolve-pr-thread`, `submit-pr-review`, `update-pr`, `queue-build`), you **must** also add `permissions.write`. The compiler will error otherwise. +If adding write-requiring safe outputs (`create-pull-request`, `create-work-item`, `comment-on-work-item`, `update-work-item`, `create-wiki-page`, `update-wiki-page`, `link-work-items`, `upload-workitem-attachment`, `create-branch`, `create-git-tag`, `add-build-tag`, `add-pr-comment`, `reply-to-pr-comment`, `resolve-pr-thread`, `submit-pr-review`, `update-pr`, `queue-build`), you **must** also add `permissions.write`. The compiler will error otherwise. ### Adding Pre/Post Steps diff --git a/src/execute.rs b/src/execute.rs index 49220ee6..4c7b1bf9 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -17,7 +17,7 @@ use crate::safeoutputs::{ ExecutionContext, ExecutionResult, Executor, LinkWorkItemsResult, MissingDataResult, MissingToolResult, NoopResult, QueueBuildResult, ReplyToPrCommentResult, ReportIncompleteResult, ResolvePrThreadResult, SubmitPrReviewResult, ToolResult, - UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, UploadAttachmentResult, + UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, UploadWorkitemAttachmentResult, }; // Re-export memory types for use by main.rs @@ -91,7 +91,7 @@ pub async fn execute_safe_outputs( AddBuildTagResult, CreateBranchResult, UpdatePrResult, - UploadAttachmentResult, + UploadWorkitemAttachmentResult, SubmitPrReviewResult, ReplyToPrCommentResult, ResolvePrThreadResult, @@ -262,7 +262,7 @@ pub async fn execute_safe_output( "add-build-tag" => AddBuildTagResult, "create-branch" => CreateBranchResult, "update-pr" => UpdatePrResult, - "upload-attachment" => UploadAttachmentResult, + "upload-workitem-attachment" => UploadWorkitemAttachmentResult, "submit-pr-review" => SubmitPrReviewResult, "reply-to-pr-review-comment" => ReplyToPrCommentResult, "resolve-pr-thread" => ResolvePrThreadResult, diff --git a/src/mcp.rs b/src/mcp.rs index 11fb59f3..5eaf12f8 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -27,7 +27,7 @@ use crate::safeoutputs::{ QueueBuildResult, SubmitPrReviewParams, SubmitPrReviewResult, ToolResult, UpdatePrParams, UpdatePrResult, UpdateWorkItemParams, UpdateWorkItemResult, - UploadAttachmentParams, UploadAttachmentResult, + UploadWorkitemAttachmentParams, UploadWorkitemAttachmentResult, anyhow_to_mcp_error, }; @@ -960,21 +960,21 @@ Changes will be applied during safe output processing." } #[tool( - name = "upload-attachment", + name = "upload-workitem-attachment", description = "Upload a file attachment to an Azure DevOps work item. The file will be \ uploaded and linked during safe output processing. File size and type restrictions may apply." )] - async fn upload_attachment( + async fn upload_workitem_attachment( &self, - params: Parameters, + params: Parameters, ) -> Result { info!( - "Tool called: upload-attachment - work item #{} file '{}'", + "Tool called: upload-workitem-attachment - work item #{} file '{}'", params.0.work_item_id, params.0.file_path ); let mut sanitized = params.0; sanitized.comment = sanitized.comment.map(|c| sanitize_text(&c)); - let result: UploadAttachmentResult = sanitized.try_into()?; + let result: UploadWorkitemAttachmentResult = sanitized.try_into()?; self.write_safe_output_file(&result).await .map_err(|e| anyhow_to_mcp_error(anyhow::anyhow!("Failed to write safe output: {}", e)))?; Ok(CallToolResult::success(vec![Content::text(format!( diff --git a/src/safeoutputs/mod.rs b/src/safeoutputs/mod.rs index 31eea77f..b91a2a8e 100644 --- a/src/safeoutputs/mod.rs +++ b/src/safeoutputs/mod.rs @@ -42,7 +42,7 @@ pub const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = tool_names![ AddBuildTagResult, CreateBranchResult, UpdatePrResult, - UploadAttachmentResult, + UploadWorkitemAttachmentResult, SubmitPrReviewResult, ReplyToPrCommentResult, ResolvePrThreadResult, @@ -75,7 +75,7 @@ pub const ALL_KNOWN_SAFE_OUTPUTS: &[&str] = all_safe_output_names![ AddBuildTagResult, CreateBranchResult, UpdatePrResult, - UploadAttachmentResult, + UploadWorkitemAttachmentResult, SubmitPrReviewResult, ReplyToPrCommentResult, ResolvePrThreadResult, @@ -262,7 +262,7 @@ mod submit_pr_review; mod update_pr; mod update_wiki_page; mod update_work_item; -mod upload_attachment; +mod upload_workitem_attachment; pub use add_build_tag::*; pub use add_pr_comment::*; @@ -287,7 +287,7 @@ pub use submit_pr_review::*; pub use update_pr::*; pub use update_wiki_page::*; pub use update_work_item::*; -pub use upload_attachment::*; +pub use upload_workitem_attachment::*; #[cfg(test)] mod tests { @@ -344,7 +344,7 @@ mod tests { assert!(AddBuildTagResult::REQUIRES_WRITE); assert!(CreateBranchResult::REQUIRES_WRITE); assert!(UpdatePrResult::REQUIRES_WRITE); - assert!(UploadAttachmentResult::REQUIRES_WRITE); + assert!(UploadWorkitemAttachmentResult::REQUIRES_WRITE); assert!(SubmitPrReviewResult::REQUIRES_WRITE); assert!(ReplyToPrCommentResult::REQUIRES_WRITE); assert!(ResolvePrThreadResult::REQUIRES_WRITE); diff --git a/src/safeoutputs/upload_attachment.rs b/src/safeoutputs/upload_workitem_attachment.rs similarity index 85% rename from src/safeoutputs/upload_attachment.rs rename to src/safeoutputs/upload_workitem_attachment.rs index 5342fb03..65976a3d 100644 --- a/src/safeoutputs/upload_attachment.rs +++ b/src/safeoutputs/upload_workitem_attachment.rs @@ -14,7 +14,7 @@ use anyhow::{Context, ensure}; /// Parameters for uploading an attachment to a work item #[derive(Deserialize, JsonSchema)] -pub struct UploadAttachmentParams { +pub struct UploadWorkitemAttachmentParams { /// The work item ID to attach the file to pub work_item_id: i64, @@ -25,7 +25,7 @@ pub struct UploadAttachmentParams { pub comment: Option, } -impl Validate for UploadAttachmentParams { +impl Validate for UploadWorkitemAttachmentParams { fn validate(&self) -> anyhow::Result<()> { ensure!(self.work_item_id > 0, "work_item_id must be positive"); ensure!(!self.file_path.is_empty(), "file_path must not be empty"); @@ -63,18 +63,18 @@ impl Validate for UploadAttachmentParams { } tool_result! { - name = "upload-attachment", + name = "upload-workitem-attachment", write = true, - params = UploadAttachmentParams, + params = UploadWorkitemAttachmentParams, /// Result of uploading an attachment to a work item - pub struct UploadAttachmentResult { + pub struct UploadWorkitemAttachmentResult { work_item_id: i64, file_path: String, comment: Option, } } -impl SanitizeContent for UploadAttachmentResult { +impl SanitizeContent for UploadWorkitemAttachmentResult { fn sanitize_content_fields(&mut self) { if let Some(comment) = &self.comment { self.comment = Some(sanitize_text(comment)); @@ -84,12 +84,12 @@ impl SanitizeContent for UploadAttachmentResult { const DEFAULT_MAX_FILE_SIZE: u64 = 5 * 1024 * 1024; // 5 MB -/// Configuration for the upload-attachment tool (specified in front matter) +/// Configuration for the upload-workitem-attachment tool (specified in front matter) /// /// Example front matter: /// ```yaml /// safe-outputs: -/// upload-attachment: +/// upload-workitem-attachment: /// max-file-size: 5242880 /// allowed-extensions: /// - .png @@ -98,7 +98,7 @@ const DEFAULT_MAX_FILE_SIZE: u64 = 5 * 1024 * 1024; // 5 MB /// comment-prefix: "[Agent] " /// ``` #[derive(Debug, Clone, SanitizeConfig, Serialize, Deserialize)] -pub struct UploadAttachmentConfig { +pub struct UploadWorkitemAttachmentConfig { /// Maximum file size in bytes (default: 5 MB) #[serde(default = "default_max_file_size", rename = "max-file-size")] pub max_file_size: u64, @@ -116,7 +116,7 @@ fn default_max_file_size() -> u64 { DEFAULT_MAX_FILE_SIZE } -impl Default for UploadAttachmentConfig { +impl Default for UploadWorkitemAttachmentConfig { fn default() -> Self { Self { max_file_size: DEFAULT_MAX_FILE_SIZE, @@ -127,7 +127,7 @@ impl Default for UploadAttachmentConfig { } #[async_trait::async_trait] -impl Executor for UploadAttachmentResult { +impl Executor for UploadWorkitemAttachmentResult { fn dry_run_summary(&self) -> String { format!("upload '{}' to work item #{}", self.file_path, self.work_item_id) } @@ -138,7 +138,7 @@ impl Executor for UploadAttachmentResult { self.file_path, self.work_item_id ); debug!( - "upload-attachment: work_item_id={}, file_path='{}'", + "upload-workitem-attachment: work_item_id={}, file_path='{}'", self.work_item_id, self.file_path ); @@ -156,7 +156,7 @@ impl Executor for UploadAttachmentResult { .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; debug!("ADO org: {}, project: {}", org_url, project); - let config: UploadAttachmentConfig = ctx.get_tool_config("upload-attachment"); + let config: UploadWorkitemAttachmentConfig = ctx.get_tool_config("upload-workitem-attachment"); debug!("Max file size: {} bytes", config.max_file_size); debug!("Allowed extensions: {:?}", config.allowed_extensions); @@ -365,14 +365,14 @@ mod tests { #[test] fn test_result_has_correct_name() { - assert_eq!(UploadAttachmentResult::NAME, "upload-attachment"); + assert_eq!(UploadWorkitemAttachmentResult::NAME, "upload-workitem-attachment"); } #[test] fn test_params_deserializes() { let json = r#"{"work_item_id": 42, "file_path": "output/report.pdf", "comment": "Weekly report"}"#; - let params: UploadAttachmentParams = serde_json::from_str(json).unwrap(); + let params: UploadWorkitemAttachmentParams = serde_json::from_str(json).unwrap(); assert_eq!(params.work_item_id, 42); assert_eq!(params.file_path, "output/report.pdf"); assert_eq!(params.comment, Some("Weekly report".to_string())); @@ -380,13 +380,13 @@ mod tests { #[test] fn test_params_converts_to_result() { - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 42, file_path: "output/report.pdf".to_string(), comment: Some("Weekly report".to_string()), }; - let result: UploadAttachmentResult = params.try_into().unwrap(); - assert_eq!(result.name, "upload-attachment"); + let result: UploadWorkitemAttachmentResult = params.try_into().unwrap(); + assert_eq!(result.name, "upload-workitem-attachment"); assert_eq!(result.work_item_id, 42); assert_eq!(result.file_path, "output/report.pdf"); assert_eq!(result.comment, Some("Weekly report".to_string())); @@ -394,124 +394,124 @@ mod tests { #[test] fn test_validation_rejects_zero_work_item_id() { - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 0, file_path: "output/report.pdf".to_string(), comment: None, }; - let result: Result = params.try_into(); + let result: Result = params.try_into(); assert!(result.is_err()); } #[test] fn test_validation_rejects_empty_file_path() { - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 42, file_path: "".to_string(), comment: None, }; - let result: Result = params.try_into(); + let result: Result = params.try_into(); assert!(result.is_err()); } #[test] fn test_validation_rejects_path_traversal() { - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 42, file_path: "../etc/passwd".to_string(), comment: None, }; - let result: Result = params.try_into(); + let result: Result = params.try_into(); assert!(result.is_err()); } #[test] fn test_validation_rejects_embedded_traversal() { // "src/../secret" has ".." as a standalone component - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 42, file_path: "src/../secret".to_string(), comment: None, }; - let result: Result = params.try_into(); + let result: Result = params.try_into(); assert!(result.is_err()); } #[test] fn test_validation_rejects_backslash_traversal() { - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 42, file_path: "src\\..\\secret.txt".to_string(), comment: None, }; - let result: Result = params.try_into(); + let result: Result = params.try_into(); assert!(result.is_err()); } #[test] fn test_validation_rejects_backslash_absolute_path() { - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 42, file_path: "\\etc\\passwd".to_string(), comment: None, }; - let result: Result = params.try_into(); + let result: Result = params.try_into(); assert!(result.is_err()); } #[test] fn test_validation_accepts_filename_with_dots_in_name() { // "report..v2.pdf" has ".." inside a filename, not as a standalone component - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 42, file_path: "report..v2.pdf".to_string(), comment: None, }; - let result: Result = params.try_into(); + let result: Result = params.try_into(); assert!(result.is_ok(), "report..v2.pdf should be a valid filename"); } #[test] fn test_validation_accepts_directory_with_dots_in_name() { // "v2..3/notes.md" — ".." inside a directory name, not a standalone component - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 42, file_path: "v2..3/notes.md".to_string(), comment: None, }; - let result: Result = params.try_into(); + let result: Result = params.try_into(); assert!(result.is_ok(), "v2..3/notes.md should be valid"); } #[test] fn test_validation_rejects_absolute_path() { - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 42, file_path: "/etc/passwd".to_string(), comment: None, }; - let result: Result = params.try_into(); + let result: Result = params.try_into(); assert!(result.is_err()); } #[test] fn test_result_serializes_correctly() { - let params = UploadAttachmentParams { + let params = UploadWorkitemAttachmentParams { work_item_id: 42, file_path: "output/report.pdf".to_string(), comment: Some("Test attachment".to_string()), }; - let result: UploadAttachmentResult = params.try_into().unwrap(); + let result: UploadWorkitemAttachmentResult = params.try_into().unwrap(); let json = serde_json::to_string(&result).unwrap(); - assert!(json.contains(r#""name":"upload-attachment""#)); + assert!(json.contains(r#""name":"upload-workitem-attachment""#)); assert!(json.contains(r#""work_item_id":42"#)); assert!(json.contains(r#""file_path":"output/report.pdf""#)); } #[test] fn test_config_defaults() { - let config = UploadAttachmentConfig::default(); + let config = UploadWorkitemAttachmentConfig::default(); assert_eq!(config.max_file_size, 5 * 1024 * 1024); assert!(config.allowed_extensions.is_empty()); assert!(config.comment_prefix.is_none()); @@ -527,7 +527,7 @@ allowed-extensions: - .log comment-prefix: "[Agent] " "#; - let config: UploadAttachmentConfig = serde_yaml::from_str(yaml).unwrap(); + let config: UploadWorkitemAttachmentConfig = serde_yaml::from_str(yaml).unwrap(); assert_eq!(config.max_file_size, 1_048_576); assert_eq!( config.allowed_extensions,