From 6a58db585f7e00d740013f014021dd47897ee3e1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 1 May 2026 10:41:32 +0000 Subject: [PATCH 1/2] feat(safe-outputs): add upload-build-artifact safe output Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/96219dfc-20c8-4f51-a26f-5cce43192f51 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- docs/safe-outputs.md | 34 + src/execute.rs | 10 +- src/mcp.rs | 113 ++++ src/safeoutputs/mod.rs | 5 + src/safeoutputs/upload_build_artifact.rs | 754 +++++++++++++++++++++++ 5 files changed, 912 insertions(+), 4 deletions(-) create mode 100644 src/safeoutputs/upload_build_artifact.rs diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index 0f3f3ae7..121dd7c5 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -418,6 +418,40 @@ safe-outputs: max: 1 # Maximum per run (default: 1) ``` +### upload-build-artifact +Attaches a workspace file to an existing Azure DevOps build (typically one that has **already finished**) using the [build attachments REST API](https://learn.microsoft.com/en-us/rest/api/azure/devops/build/attachments/) (`PUT /_apis/build/builds/{buildId}/attachments/{type}/{name}`). Use this when an agent needs to decorate a previously-finished build run with a generated report, screenshot, or log bundle. Unlike `upload-artifact` (which targets the *current* run via a `##vso[artifact.upload]` logging command), `upload-build-artifact` can target *any* build the executor's token has access to. + +**Agent parameters:** +- `build_id` - The ID of the build to attach the file to (required, must be positive) +- `artifact_name` - Name to attach the file under (required; 1-100 chars; alphanumerics, `-`, `_`, `.`; must not start with `.`) +- `file_path` - Relative path to the file in the workspace (required; no directory traversal, no absolute paths, no `.git` segments) + +**Configuration options (front matter):** +```yaml +safe-outputs: + upload-build-artifact: + max-file-size: 52428800 # Maximum file size in bytes (default: 50 MB) + allowed-extensions: [] # Optional — restrict file types (e.g., [".png", ".pdf", ".log"]) + allowed-artifact-names: [] # Optional — allow-list of artifact names; entries ending with `*` match by prefix + allowed-build-ids: [] # Optional — allow-list of build IDs the agent may attach to + name-prefix: "agent-" # Optional — prefix prepended to the agent-supplied artifact name + attachment-type: "agent-artifact" # Optional — value used for the `{type}` segment of the attachments URL (default: "agent-artifact") + max: 3 # Maximum per run (default: 3) +``` + +**Validation performed at Stage 1 (MCP / sandbox):** +- `file_path` is resolved against the agent's workspace, canonicalized, and rejected if it escapes via symlinks. +- Directories are rejected — only single files are supported. +- The accepted file is **copied** into the safe-outputs working directory under a generated unique name. This staged copy is what Stage 3 reads — the agent's sandbox workspace is no longer accessible at execution time, mirroring how `create-pull-request` stages its patch file. + +**Validation performed at Stage 3:** +- The staged file is re-canonicalized inside the safe-outputs working directory (defense in depth). +- When `allowed-build-ids` is non-empty, the requested `build_id` must match an entry. +- Files larger than `max-file-size` are rejected. +- When `allowed-extensions` is non-empty, the original `file_path` extension must match (case-insensitive). +- When `allowed-artifact-names` is non-empty, the resolved artifact name (after `name-prefix`) must match an allow-list entry. +- The configured `attachment-type` is re-validated against the same charset rules as an artifact name before being used in the URL. + ### cache-memory (moved to `tools:`) Memory is now configured as a first-class tool under `tools: cache-memory:` instead of `safe-outputs: memory:`. See the [Cache Memory section](./tools.md#cache-memory-cache-memory) in `docs/tools.md` for details. diff --git a/src/execute.rs b/src/execute.rs index 49220ee6..8aa73279 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -12,10 +12,10 @@ use std::path::Path; use crate::ndjson::{self, SAFE_OUTPUT_FILENAME}; use crate::safeoutputs::{ - AddBuildTagResult, AddPrCommentResult, CreateBranchResult, CreateGitTagResult, - CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, CommentOnWorkItemResult, - ExecutionContext, ExecutionResult, Executor, LinkWorkItemsResult, MissingDataResult, - MissingToolResult, NoopResult, QueueBuildResult, ReplyToPrCommentResult, + AddBuildTagResult, AddPrCommentResult, UploadBuildArtifactResult, CreateBranchResult, + CreateGitTagResult, CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, + CommentOnWorkItemResult, ExecutionContext, ExecutionResult, Executor, LinkWorkItemsResult, + MissingDataResult, MissingToolResult, NoopResult, QueueBuildResult, ReplyToPrCommentResult, ReportIncompleteResult, ResolvePrThreadResult, SubmitPrReviewResult, ToolResult, UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, UploadAttachmentResult, }; @@ -92,6 +92,7 @@ pub async fn execute_safe_outputs( CreateBranchResult, UpdatePrResult, UploadAttachmentResult, + UploadBuildArtifactResult, SubmitPrReviewResult, ReplyToPrCommentResult, ResolvePrThreadResult, @@ -263,6 +264,7 @@ pub async fn execute_safe_output( "create-branch" => CreateBranchResult, "update-pr" => UpdatePrResult, "upload-attachment" => UploadAttachmentResult, + "upload-build-artifact" => UploadBuildArtifactResult, "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..303316a2 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -13,6 +13,7 @@ use crate::sanitize::{SanitizeContent, sanitize as sanitize_text}; use crate::safeoutputs::{ AddBuildTagParams, AddBuildTagResult, AddPrCommentParams, AddPrCommentResult, + UploadBuildArtifactParams, UploadBuildArtifactResult, CommentOnWorkItemParams, CommentOnWorkItemResult, CreateBranchParams, CreateBranchResult, CreateGitTagParams, CreateGitTagResult, @@ -983,6 +984,118 @@ uploaded and linked during safe output processing. File size and type restrictio ))])) } + #[tool( + name = "upload-build-artifact", + description = "Attach a workspace file to an existing Azure DevOps build (typically one \ +that has already finished) as a build attachment. The file will be staged now and uploaded to \ +the build via the ADO build attachments REST API during safe output processing. File size, \ +extension, artifact-name and build-id restrictions may apply per the workflow's safe-outputs \ +config." + )] + async fn upload_build_artifact( + &self, + params: Parameters, + ) -> Result { + info!( + "Tool called: upload-build-artifact - artifact '{}' file '{}' build #{}", + params.0.artifact_name, params.0.file_path, params.0.build_id + ); + + // Validate the agent-supplied params (build id, artifact name charset, + // path traversal / absolute / null bytes, etc.) before touching the + // filesystem. + crate::safeoutputs::Validate::validate(¶ms.0).map_err(anyhow_to_mcp_error)?; + + // Resolve the agent-supplied file path against the bounding directory + // (the agent's workspace root inside the sandbox) and verify it + // canonicalises to a location *inside* that directory — guarding + // against symlink escapes. + let resolved = self.bounding_directory.join(¶ms.0.file_path); + let canonical = resolved.canonicalize().map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!( + "File '{}' could not be located inside the workspace: {}", + params.0.file_path, + e + )) + })?; + let canonical_root = self.bounding_directory.canonicalize().map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!( + "Failed to canonicalize bounding directory: {}", + e + )) + })?; + if !canonical.starts_with(&canonical_root) { + return Err(anyhow_to_mcp_error(anyhow::anyhow!( + "File '{}' resolves outside the workspace (symlink escape)", + params.0.file_path + ))); + } + + // Reject directories — upload-build-artifact is single-file only. + let metadata = tokio::fs::metadata(&canonical).await.map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!("Failed to stat '{}': {}", params.0.file_path, e)) + })?; + if metadata.is_dir() { + return Err(anyhow_to_mcp_error(anyhow::anyhow!( + "File '{}' is a directory; upload-build-artifact only supports single files", + params.0.file_path + ))); + } + let file_size = metadata.len(); + + // Generate a unique staged filename and copy the file into the + // safe-outputs directory. Stage 3 reads it back from there because + // the agent's sandbox workspace is no longer accessible by then. + // The staged name preserves the original extension and embeds a + // short random suffix to avoid collisions across multiple calls. + let extension = std::path::Path::new(¶ms.0.file_path) + .extension() + .and_then(|s| s.to_str()) + .map(|s| { + s.chars() + .filter(|c| c.is_ascii_alphanumeric()) + .take(16) + .collect::() + }) + .unwrap_or_default(); + let staged_filename = if extension.is_empty() { + format!( + "upload-build-artifact-{}-{}", + params.0.artifact_name, + generate_short_id() + ) + } else { + format!( + "upload-build-artifact-{}-{}.{}", + params.0.artifact_name, + generate_short_id(), + extension + ) + }; + let staged_path = self.output_directory.join(&staged_filename); + tokio::fs::copy(&canonical, &staged_path).await.map_err(|e| { + anyhow_to_mcp_error(anyhow::anyhow!( + "Failed to stage file '{}' into safe-outputs directory: {}", + params.0.file_path, + e + )) + })?; + + let result = UploadBuildArtifactResult::new( + params.0.build_id, + params.0.artifact_name.clone(), + params.0.file_path.clone(), + staged_filename.clone(), + file_size, + ); + 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!( + "Artifact '{}' queued from file '{}' ({} bytes) for build #{}. The file will be uploaded to the build during safe output processing.", + result.artifact_name, result.file_path, file_size, result.build_id + ))])) + } + #[tool( name = "submit-pr-review", description = "Submit a pull request review with a decision (approve, request-changes, \ diff --git a/src/safeoutputs/mod.rs b/src/safeoutputs/mod.rs index 31eea77f..62aa7a99 100644 --- a/src/safeoutputs/mod.rs +++ b/src/safeoutputs/mod.rs @@ -43,6 +43,7 @@ pub const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = tool_names![ CreateBranchResult, UpdatePrResult, UploadAttachmentResult, + UploadBuildArtifactResult, SubmitPrReviewResult, ReplyToPrCommentResult, ResolvePrThreadResult, @@ -76,6 +77,7 @@ pub const ALL_KNOWN_SAFE_OUTPUTS: &[&str] = all_safe_output_names![ CreateBranchResult, UpdatePrResult, UploadAttachmentResult, + UploadBuildArtifactResult, SubmitPrReviewResult, ReplyToPrCommentResult, ResolvePrThreadResult, @@ -243,6 +245,7 @@ pub(crate) fn validate_git_ref_name(name: &str, label: &str) -> anyhow::Result<( mod add_build_tag; mod add_pr_comment; +mod upload_build_artifact; mod comment_on_work_item; mod create_branch; mod create_git_tag; @@ -266,6 +269,7 @@ mod upload_attachment; pub use add_build_tag::*; pub use add_pr_comment::*; +pub use upload_build_artifact::*; pub use comment_on_work_item::*; pub use create_branch::*; pub use create_git_tag::*; @@ -345,6 +349,7 @@ mod tests { assert!(CreateBranchResult::REQUIRES_WRITE); assert!(UpdatePrResult::REQUIRES_WRITE); assert!(UploadAttachmentResult::REQUIRES_WRITE); + assert!(UploadBuildArtifactResult::REQUIRES_WRITE); assert!(SubmitPrReviewResult::REQUIRES_WRITE); assert!(ReplyToPrCommentResult::REQUIRES_WRITE); assert!(ResolvePrThreadResult::REQUIRES_WRITE); diff --git a/src/safeoutputs/upload_build_artifact.rs b/src/safeoutputs/upload_build_artifact.rs new file mode 100644 index 00000000..152a87dc --- /dev/null +++ b/src/safeoutputs/upload_build_artifact.rs @@ -0,0 +1,754 @@ +//! Upload build artifact safe output tool. +//! +//! Lets an agent propose attaching a workspace file to an existing (typically +//! already-finished) Azure DevOps build via the **build attachments** REST API +//! (`PUT /_apis/build/builds/{buildId}/attachments/{type}/{name}`). Unlike +//! `upload-artifact` (which emits a `##vso[artifact.upload]` logging command +//! tied to the *current* run), this tool can attach a file to **any** build +//! the executor's token has access to — useful for posthumously decorating a +//! build with a generated report, screenshot, or log bundle. +//! +//! The flow mirrors `upload-artifact` and `create-pull-request`: +//! +//! * **Stage 1 (MCP, in the agent sandbox):** the MCP server validates the +//! agent-supplied params, resolves `file_path` against the agent's workspace, +//! rejects symlink escapes / directories, and **copies the file** into the +//! safe-outputs `output_directory` under a generated unique name. The +//! sandbox workspace is no longer accessible at execution time, so the file +//! has to be staged where Stage 3 can find it. +//! * **Stage 3 (executor, outside the sandbox):** reads the staged file from +//! `ctx.working_directory.join(staged_file)`, applies operator-supplied +//! limits (`max-file-size`, `allowed-extensions`, `allowed-artifact-names`, +//! `allowed-build-ids`, `name-prefix`, `attachment-type`), and PUTs the +//! bytes to the ADO build attachments endpoint. + +use ado_aw_derive::SanitizeConfig; +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use crate::sanitize::SanitizeContent; +use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::tool_result; +use crate::validate::{is_safe_path_segment, is_valid_version}; +use anyhow::{Context, ensure}; + +/// Parameters for attaching a workspace file to an existing ADO build. +#[derive(Deserialize, JsonSchema)] +pub struct UploadBuildArtifactParams { + /// The build ID to attach the file to. Must be positive. The build does + /// not need to be running — this tool is designed to attach files + /// posthumously to already-finished builds. + pub build_id: i64, + + /// The artifact name to attach the file under. Used as the `{name}` + /// segment of the ADO build attachments URL. ADO requires a non-empty + /// name made of alphanumerics, `-`, `_`, or `.`. Must be 1-100 characters + /// and must not start with `.`. + pub artifact_name: String, + + /// Path to the file in the workspace to attach. Must be a relative path + /// with no directory traversal, no absolute prefix, and no `.git` + /// segments. + pub file_path: String, +} + +impl Validate for UploadBuildArtifactParams { + fn validate(&self) -> anyhow::Result<()> { + ensure!(self.build_id > 0, "build_id must be positive"); + + // artifact_name: ADO requires non-empty, ≤100 chars, charset + // [A-Za-z0-9._-], and (per our hardening) no leading `.`. + ensure!( + self.artifact_name.len() <= 100, + "artifact_name must be at most 100 characters" + ); + ensure!( + !self.artifact_name.starts_with('.'), + "artifact_name must not start with '.'" + ); + ensure!( + is_valid_version(&self.artifact_name), + "artifact_name must be non-empty and contain only alphanumeric characters, '-', '_' or '.'" + ); + + // file_path: must be relative, with no traversal, no absolute prefix, + // no `.git`/hidden segments, no null bytes, no drive-letter colons. + ensure!(!self.file_path.is_empty(), "file_path must not be empty"); + ensure!( + !self.file_path.contains('\0'), + "file_path must not contain null bytes" + ); + ensure!( + !self.file_path.contains(':'), + "file_path must not contain ':'" + ); + for component in self.file_path.split(['/', '\\']) { + ensure!( + is_safe_path_segment(component), + "file_path component '{}' is not a safe path segment (no empty, '..', or leading '.' allowed)", + component + ); + } + Ok(()) + } +} + +/// Internal params struct mirroring `UploadBuildArtifactResult` fields for the +/// `tool_result!` macro's `TryFrom` plumbing. The actual MCP parameters come +/// from `UploadBuildArtifactParams`; this struct only exists so the macro can +/// wire up `Validate`/`TryFrom` while the real construction happens in MCP via +/// `UploadBuildArtifactResult::new()` after the file is staged. +#[derive(Deserialize, JsonSchema)] +struct UploadBuildArtifactResultFields { + build_id: i64, + artifact_name: String, + file_path: String, + staged_file: String, + file_size: u64, +} + +impl Validate for UploadBuildArtifactResultFields {} + +tool_result! { + name = "upload-build-artifact", + write = true, + params = UploadBuildArtifactResultFields, + default_max = 3, + /// Result of attaching a workspace file to an existing ADO build. + pub struct UploadBuildArtifactResult { + /// Build ID the file should be attached to. + build_id: i64, + /// Artifact name as proposed by the agent (pre-prefix). + artifact_name: String, + /// Original file path proposed by the agent (used for display and the + /// extension-allowlist check). + file_path: String, + /// Filename of the staged copy inside the safe-outputs directory. + /// Stage 1 (MCP) copies the agent's file into the safe-outputs dir + /// under this name; Stage 3 reads it back from + /// `ctx.working_directory.join(staged_file)` because the agent's + /// sandbox workspace is no longer accessible by then. + staged_file: String, + /// Size in bytes of the staged file at copy time. + file_size: u64, + } +} + +impl SanitizeContent for UploadBuildArtifactResult { + fn sanitize_content_fields(&mut self) { + // All textual fields are strictly validated to safe charsets; no + // additional textual sanitization is required. + } +} + +impl UploadBuildArtifactResult { + /// Construct a result after the agent's file has been staged into the + /// safe-outputs directory. + pub fn new( + build_id: i64, + artifact_name: String, + file_path: String, + staged_file: String, + file_size: u64, + ) -> Self { + Self { + name: ::NAME.to_string(), + build_id, + artifact_name, + file_path, + staged_file, + file_size, + } + } +} + +const DEFAULT_MAX_FILE_SIZE: u64 = 50 * 1024 * 1024; // 50 MB +const DEFAULT_ATTACHMENT_TYPE: &str = "agent-artifact"; + +/// Configuration for the upload-build-artifact tool (specified in front +/// matter). +/// +/// Example front matter: +/// ```yaml +/// safe-outputs: +/// upload-build-artifact: +/// max-file-size: 52428800 +/// allowed-extensions: +/// - .png +/// - .pdf +/// - .log +/// allowed-artifact-names: +/// - agent-* +/// allowed-build-ids: +/// - 12345 +/// - 67890 +/// name-prefix: "agent-" +/// attachment-type: "agent-artifact" +/// max: 5 +/// ``` +#[derive(Debug, Clone, SanitizeConfig, Serialize, Deserialize)] +pub struct UploadBuildArtifactConfig { + /// Maximum file size in bytes (default: 50 MB). + #[serde(default = "default_max_file_size", rename = "max-file-size")] + pub max_file_size: u64, + + /// Allowed file extensions (e.g., `[".png", ".pdf"]`). Empty means all + /// extensions are allowed. + #[serde(default, rename = "allowed-extensions")] + pub allowed_extensions: Vec, + + /// Restrict which artifact names may be published. Empty means any name + /// (subject to charset rules) is allowed. Entries ending with `*` match + /// by prefix; otherwise the comparison is exact. + #[serde(default, rename = "allowed-artifact-names")] + pub allowed_artifact_names: Vec, + + /// Restrict which build IDs the agent may attach to. Empty means any + /// build ID accessible to the executor's token is allowed. + #[serde(default, rename = "allowed-build-ids")] + pub allowed_build_ids: Vec, + + /// Prefix prepended to the agent-supplied artifact name before publishing. + #[serde(default, rename = "name-prefix")] + pub name_prefix: Option, + + /// Value to use for the `{type}` segment of the build attachments URL. + /// Defaults to `agent-artifact`. Must satisfy the same charset rules as + /// an artifact name. + #[serde(default, rename = "attachment-type")] + pub attachment_type: Option, +} + +fn default_max_file_size() -> u64 { + DEFAULT_MAX_FILE_SIZE +} + +impl Default for UploadBuildArtifactConfig { + fn default() -> Self { + Self { + max_file_size: DEFAULT_MAX_FILE_SIZE, + allowed_extensions: Vec::new(), + allowed_artifact_names: Vec::new(), + allowed_build_ids: Vec::new(), + name_prefix: None, + attachment_type: None, + } + } +} + +#[async_trait::async_trait] +impl Executor for UploadBuildArtifactResult { + fn dry_run_summary(&self) -> String { + format!( + "attach '{}' as artifact '{}' to build #{}", + self.file_path, self.artifact_name, self.build_id + ) + } + + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + info!( + "Attaching '{}' as artifact '{}' to build #{}", + self.file_path, self.artifact_name, self.build_id + ); + debug!( + "upload-build-artifact: build_id={}, artifact_name='{}', file_path='{}'", + self.build_id, self.artifact_name, self.file_path + ); + + let config: UploadBuildArtifactConfig = ctx.get_tool_config("upload-build-artifact"); + debug!("Max file size: {} bytes", config.max_file_size); + debug!("Allowed extensions: {:?}", config.allowed_extensions); + debug!("Allowed artifact names: {:?}", config.allowed_artifact_names); + debug!("Allowed build IDs: {:?}", config.allowed_build_ids); + + // Check build-id allow-list (if configured). + if !config.allowed_build_ids.is_empty() + && !config.allowed_build_ids.contains(&self.build_id) + { + return Ok(ExecutionResult::failure(format!( + "Build ID {} is not in the allowed-build-ids list", + self.build_id + ))); + } + + // Apply name-prefix and re-validate the resulting name's charset (the + // prefix itself is operator-controlled and sanitized at config load, + // but we still defensively check the joined string). + let final_name = match &config.name_prefix { + Some(prefix) => format!("{}{}", prefix, self.artifact_name), + None => self.artifact_name.clone(), + }; + if final_name.starts_with('.') || final_name.len() > 100 || !is_valid_version(&final_name) + { + return Ok(ExecutionResult::failure(format!( + "Resolved artifact name '{}' is not a valid Azure DevOps artifact name", + final_name + ))); + } + debug!("Final artifact name (after prefix): {}", final_name); + + // Check artifact-name allow-list (if configured). + if !config.allowed_artifact_names.is_empty() { + let allowed = config.allowed_artifact_names.iter().any(|pattern| { + if let Some(prefix) = pattern.strip_suffix('*') { + final_name.starts_with(prefix) + } else { + *pattern == final_name + } + }); + if !allowed { + return Ok(ExecutionResult::failure(format!( + "Artifact name '{}' is not in the allowed list", + final_name + ))); + } + } + + // Validate file extension against allowed-extensions (if configured). + if !config.allowed_extensions.is_empty() { + let has_valid_ext = config.allowed_extensions.iter().any(|ext| { + self.file_path + .to_lowercase() + .ends_with(&ext.to_lowercase()) + }); + if !has_valid_ext { + return Ok(ExecutionResult::failure(format!( + "File '{}' has an extension not in the allowed list: {:?}", + self.file_path, config.allowed_extensions + ))); + } + } + + // Resolve the attachment type. Operator config wins; otherwise use the + // default. Re-validate the charset defensively even though + // `SanitizeConfig` strips control characters, because the type is + // interpolated into a URL path segment. + let attachment_type = config + .attachment_type + .as_deref() + .unwrap_or(DEFAULT_ATTACHMENT_TYPE); + if attachment_type.is_empty() + || attachment_type.len() > 100 + || !is_valid_version(attachment_type) + { + return Ok(ExecutionResult::failure(format!( + "attachment-type '{}' is not a valid value (must be non-empty, ≤100 chars, alphanumeric/'-'/'_'/'.')", + attachment_type + ))); + } + debug!("Attachment type: {}", attachment_type); + + // Resolve the staged file inside the safe-outputs working directory. + // Stage 1 (MCP) copied the agent's file there under `self.staged_file`; + // the sandbox workspace where the original lived is no longer + // accessible. Canonicalize and verify it stays inside + // `working_directory` so a malicious staged_file value can't escape + // (defense in depth — MCP generates the name itself). + let staged_path = ctx.working_directory.join(&self.staged_file); + debug!("Staged file path: {}", staged_path.display()); + + let canonical = staged_path.canonicalize().context( + "Failed to canonicalize staged file path — file may be missing or contains broken symlinks", + )?; + let canonical_base = ctx + .working_directory + .canonicalize() + .context("Failed to canonicalize working directory")?; + if !canonical.starts_with(&canonical_base) { + return Ok(ExecutionResult::failure(format!( + "Staged file '{}' resolves outside the safe-outputs directory", + self.staged_file + ))); + } + + // Reject directories defensively — the staged entry must always be a + // single file (Stage 1 only copies single files). + let metadata = std::fs::metadata(&canonical).context("Failed to read file metadata")?; + if metadata.is_dir() { + return Ok(ExecutionResult::failure(format!( + "Staged path '{}' is a directory; upload-build-artifact only supports single files", + self.staged_file + ))); + } + let file_size = metadata.len(); + debug!("File size: {} bytes", file_size); + if file_size > config.max_file_size { + return Ok(ExecutionResult::failure(format!( + "File size ({} bytes) exceeds maximum allowed size ({} bytes)", + file_size, config.max_file_size + ))); + } + + // Read the file bytes for upload. + let file_bytes = std::fs::read(&canonical).context("Failed to read file contents")?; + + if ctx.dry_run { + return Ok(ExecutionResult::success(format!( + "[dry-run] would attach '{}' ({} bytes) as artifact '{}' to build #{}", + self.file_path, file_size, final_name, self.build_id + ))); + } + + // Resolve the ADO API context (org URL, project, token). + let org_url = ctx + .ado_org_url + .as_ref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_ref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_ref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + debug!("ADO org: {}, project: {}", org_url, project); + + // Build the build-attachments URL. + // PUT {org_url}/{project}/_apis/build/builds/{buildId}/attachments/{type}/{name}?api-version=7.1-preview.1 + let url = format!( + "{}/{}/_apis/build/builds/{}/attachments/{}/{}?api-version=7.1-preview.1", + org_url.trim_end_matches('/'), + utf8_percent_encode(project, PATH_SEGMENT), + self.build_id, + utf8_percent_encode(attachment_type, PATH_SEGMENT), + utf8_percent_encode(&final_name, PATH_SEGMENT), + ); + debug!("Attachment URL: {}", url); + + let client = reqwest::Client::new(); + info!( + "Uploading {} bytes to build #{} as attachment '{}/{}'", + file_size, self.build_id, attachment_type, final_name + ); + let response = client + .put(&url) + .header("Content-Type", "application/octet-stream") + .basic_auth("", Some(token)) + .body(file_bytes) + .send() + .await + .context("Failed to send attachment upload request to Azure DevOps")?; + + if response.status().is_success() { + let resp_body: serde_json::Value = + response.json().await.unwrap_or(serde_json::Value::Null); + let attachment_url = resp_body + .get("url") + .and_then(|v| v.as_str()) + .map(|s| s.to_string()); + info!( + "Attached '{}' to build #{} as '{}'", + self.file_path, self.build_id, final_name + ); + + Ok(ExecutionResult::success_with_data( + format!( + "Attached '{}' to build #{} as artifact '{}'", + self.file_path, self.build_id, final_name + ), + serde_json::json!({ + "build_id": self.build_id, + "artifact_name": final_name, + "attachment_type": attachment_type, + "file_path": self.file_path, + "size_bytes": file_size, + "attachment_url": attachment_url, + "project": project, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + Ok(ExecutionResult::failure(format!( + "Failed to attach artifact to build #{} (HTTP {}): {}", + self.build_id, status, error_body + ))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::safeoutputs::ToolResult; + + #[test] + fn test_result_has_correct_name() { + assert_eq!(UploadBuildArtifactResult::NAME, "upload-build-artifact"); + } + + fn make_params(build_id: i64, artifact_name: &str, file_path: &str) -> UploadBuildArtifactParams { + UploadBuildArtifactParams { + build_id, + artifact_name: artifact_name.to_string(), + file_path: file_path.to_string(), + } + } + + #[test] + fn test_params_deserializes() { + let json = r#"{"build_id": 1234, "artifact_name": "agent-report", "file_path": "out/report.pdf"}"#; + let params: UploadBuildArtifactParams = serde_json::from_str(json).unwrap(); + assert_eq!(params.build_id, 1234); + assert_eq!(params.artifact_name, "agent-report"); + assert_eq!(params.file_path, "out/report.pdf"); + } + + #[test] + fn test_params_validate_accepts_valid() { + assert!(make_params(1, "agent-report", "out/report.pdf").validate().is_ok()); + } + + #[test] + fn test_validation_rejects_zero_build_id() { + assert!(make_params(0, "agent-report", "out/report.pdf").validate().is_err()); + } + + #[test] + fn test_validation_rejects_negative_build_id() { + assert!(make_params(-1, "agent-report", "out/report.pdf").validate().is_err()); + } + + #[test] + fn test_validation_rejects_empty_artifact_name() { + assert!(make_params(1, "", "out/report.pdf").validate().is_err()); + } + + #[test] + fn test_validation_rejects_artifact_name_starting_with_dot() { + assert!(make_params(1, ".hidden", "out/report.pdf").validate().is_err()); + } + + #[test] + fn test_validation_rejects_artifact_name_with_space() { + assert!(make_params(1, "my artifact", "out/report.pdf").validate().is_err()); + } + + #[test] + fn test_validation_rejects_artifact_name_with_slash() { + assert!(make_params(1, "my/artifact", "out/report.pdf").validate().is_err()); + } + + #[test] + fn test_validation_accepts_dotted_artifact_name() { + assert!(make_params(1, "agent.report.v2", "out/report.pdf").validate().is_ok()); + } + + #[test] + fn test_validation_rejects_empty_file_path() { + assert!(make_params(1, "agent-report", "").validate().is_err()); + } + + #[test] + fn test_validation_rejects_path_traversal() { + assert!(make_params(1, "agent-report", "../etc/passwd").validate().is_err()); + } + + #[test] + fn test_validation_rejects_absolute_path() { + assert!(make_params(1, "agent-report", "/etc/passwd").validate().is_err()); + } + + #[test] + fn test_validation_rejects_backslash_traversal() { + assert!(make_params(1, "agent-report", "src\\..\\secret.txt").validate().is_err()); + } + + #[test] + fn test_validation_rejects_dotgit_component() { + assert!(make_params(1, "agent-report", ".git/config").validate().is_err()); + } + + #[test] + fn test_result_serializes_correctly() { + let result = UploadBuildArtifactResult::new( + 1234, + "agent-report".to_string(), + "out/report.pdf".to_string(), + "upload-build-artifact-agent-report-1234.bin".to_string(), + 42, + ); + let json = serde_json::to_string(&result).unwrap(); + assert!(json.contains(r#""name":"upload-build-artifact""#)); + assert!(json.contains(r#""build_id":1234"#)); + assert!(json.contains(r#""artifact_name":"agent-report""#)); + assert!(json.contains(r#""file_path":"out/report.pdf""#)); + assert!(json.contains(r#""staged_file":"upload-build-artifact-agent-report-1234.bin""#)); + } + + #[test] + fn test_config_defaults() { + let config = UploadBuildArtifactConfig::default(); + assert_eq!(config.max_file_size, 50 * 1024 * 1024); + assert!(config.allowed_extensions.is_empty()); + assert!(config.allowed_artifact_names.is_empty()); + assert!(config.allowed_build_ids.is_empty()); + assert!(config.name_prefix.is_none()); + assert!(config.attachment_type.is_none()); + } + + #[test] + fn test_config_deserializes_from_yaml() { + let yaml = r#" +max-file-size: 1048576 +allowed-extensions: + - .png + - .pdf +allowed-artifact-names: + - agent-* + - report +allowed-build-ids: + - 100 + - 200 +name-prefix: "agent-" +attachment-type: "agent-artifact" +"#; + let config: UploadBuildArtifactConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.max_file_size, 1_048_576); + assert_eq!(config.allowed_extensions, vec![".png", ".pdf"]); + assert_eq!(config.allowed_artifact_names, vec!["agent-*", "report"]); + assert_eq!(config.allowed_build_ids, vec![100, 200]); + assert_eq!(config.name_prefix, Some("agent-".to_string())); + assert_eq!(config.attachment_type, Some("agent-artifact".to_string())); + } + + fn make_ctx(working_directory: std::path::PathBuf, dry_run: bool) -> ExecutionContext { + ExecutionContext { + ado_org_url: None, + ado_organization: None, + ado_project: None, + access_token: None, + source_directory: working_directory.clone(), + working_directory, + tool_configs: std::collections::HashMap::new(), + repository_id: None, + repository_name: None, + allowed_repositories: std::collections::HashMap::new(), + agent_stats: None, + dry_run, + } + } + + #[tokio::test] + async fn test_executor_reads_staged_file_from_working_directory() { + let dir = tempfile::tempdir().unwrap(); + let staged = "upload-build-artifact-agent-report-deadbeef.pdf"; + std::fs::write(dir.path().join(staged), b"%PDF-1.4 hello").unwrap(); + + let result = UploadBuildArtifactResult::new( + 1234, + "agent-report".to_string(), + "out/report.pdf".to_string(), + staged.to_string(), + 14, + ); + let ctx = make_ctx(dir.path().to_path_buf(), true); + let outcome = result.execute_impl(&ctx).await.unwrap(); + assert!(outcome.success, "expected success, got: {:?}", outcome); + assert!(outcome.message.contains("[dry-run]")); + assert!(outcome.message.contains("agent-report")); + assert!(outcome.message.contains("#1234")); + } + + #[tokio::test] + async fn test_executor_rejects_missing_staged_file() { + let dir = tempfile::tempdir().unwrap(); + let result = UploadBuildArtifactResult::new( + 1234, + "agent-report".to_string(), + "out/report.pdf".to_string(), + "does-not-exist.pdf".to_string(), + 0, + ); + let ctx = make_ctx(dir.path().to_path_buf(), true); + let err = result.execute_impl(&ctx).await.unwrap_err(); + assert!( + err.to_string().contains("canonicalize"), + "expected canonicalize error, got: {}", + err + ); + } + + #[tokio::test] + async fn test_executor_rejects_disallowed_build_id() { + let dir = tempfile::tempdir().unwrap(); + let staged = "upload-build-artifact-agent-report-cafef00d.pdf"; + std::fs::write(dir.path().join(staged), b"hello").unwrap(); + + let result = UploadBuildArtifactResult::new( + 999, + "agent-report".to_string(), + "out/report.pdf".to_string(), + staged.to_string(), + 5, + ); + let mut ctx = make_ctx(dir.path().to_path_buf(), true); + ctx.tool_configs.insert( + "upload-build-artifact".to_string(), + serde_json::json!({ "allowed-build-ids": [100, 200] }), + ); + let outcome = result.execute_impl(&ctx).await.unwrap(); + assert!(!outcome.success); + assert!( + outcome.message.contains("allowed-build-ids"), + "expected allowed-build-ids rejection, got: {}", + outcome.message + ); + } + + #[tokio::test] + async fn test_executor_accepts_allowed_build_id() { + let dir = tempfile::tempdir().unwrap(); + let staged = "upload-build-artifact-agent-report-feedf00d.pdf"; + std::fs::write(dir.path().join(staged), b"hello").unwrap(); + + let result = UploadBuildArtifactResult::new( + 100, + "agent-report".to_string(), + "out/report.pdf".to_string(), + staged.to_string(), + 5, + ); + let mut ctx = make_ctx(dir.path().to_path_buf(), true); + ctx.tool_configs.insert( + "upload-build-artifact".to_string(), + serde_json::json!({ "allowed-build-ids": [100, 200] }), + ); + let outcome = result.execute_impl(&ctx).await.unwrap(); + assert!(outcome.success, "expected success, got: {:?}", outcome); + } + + #[tokio::test] + async fn test_executor_rejects_oversized_file() { + let dir = tempfile::tempdir().unwrap(); + let staged = "upload-build-artifact-agent-big-deadbeef.bin"; + std::fs::write(dir.path().join(staged), vec![0u8; 1024]).unwrap(); + + let result = UploadBuildArtifactResult::new( + 1, + "agent-big".to_string(), + "out/big.bin".to_string(), + staged.to_string(), + 1024, + ); + let mut ctx = make_ctx(dir.path().to_path_buf(), true); + ctx.tool_configs.insert( + "upload-build-artifact".to_string(), + serde_json::json!({ "max-file-size": 100 }), + ); + let outcome = result.execute_impl(&ctx).await.unwrap(); + assert!(!outcome.success); + assert!( + outcome.message.contains("exceeds maximum"), + "expected size rejection, got: {}", + outcome.message + ); + } +} From 6e3d82dcdef73be4be1d9c16646e2542b46e9735 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 1 May 2026 10:46:27 +0000 Subject: [PATCH 2/2] log warning when ADO build-attachment response JSON cannot be parsed Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/96219dfc-20c8-4f51-a26f-5cce43192f51 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/safeoutputs/upload_build_artifact.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/safeoutputs/upload_build_artifact.rs b/src/safeoutputs/upload_build_artifact.rs index 152a87dc..e7481a71 100644 --- a/src/safeoutputs/upload_build_artifact.rs +++ b/src/safeoutputs/upload_build_artifact.rs @@ -23,7 +23,7 @@ //! bytes to the ADO build attachments endpoint. use ado_aw_derive::SanitizeConfig; -use log::{debug, info}; +use log::{debug, info, warn}; use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -434,8 +434,13 @@ impl Executor for UploadBuildArtifactResult { .context("Failed to send attachment upload request to Azure DevOps")?; if response.status().is_success() { - let resp_body: serde_json::Value = - response.json().await.unwrap_or(serde_json::Value::Null); + let resp_body: serde_json::Value = response.json().await.unwrap_or_else(|e| { + warn!( + "Build attachment uploaded for build #{} but the response JSON could not be parsed: {} — proceeding without attachment URL", + self.build_id, e + ); + serde_json::Value::Null + }); let attachment_url = resp_body .get("url") .and_then(|v| v.as_str())