feat(safe-outputs): add upload-artifact safe output#363
Conversation
Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/0d0cdabb-69cb-46a4-b5b8-b2dba48abf00 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
|
/rust-review |
|
✅ Rust PR Reviewer completed successfully! |
🔍 Rust PR ReviewSummary: Good security-conscious design overall, but has one meaningful security gap and two configuration footguns worth addressing before merge. Findings🔒 Security Concerns
|
| )) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
@copilot we have many built in validators - try and leverage them
There was a problem hiding this comment.
Refactored in 63b598c to use the built-in validators from src/validate.rs:
artifact_namecharset →is_valid_version([A-Za-z0-9._-], non-empty)file_pathper-component →is_safe_path_segment(covers empty/absolute,.., embedded separators, leading.— including.git)- File-content injection scan →
contains_pipeline_command(now also catches##[, not just##vso[) abs_pathnewline guard →contains_newline
Inline checks now only remain for things validators don't cover (length cap, leading . on artifact name, null byte / : in path, and the ] guard for the logging command).
Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/7dfd610b-d6ae-4e0b-9a68-3ea5db5540e1 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
| "Artifact '{}' queued from file '{}'. The file will be published during safe output processing.", | ||
| result.artifact_name, result.file_path | ||
| ))])) | ||
| } |
There was a problem hiding this comment.
@copilot right now this will do nothing, in the end. We need to save the agent's provided file to the output_directory, and then retrieve it in the executor phase. This is a similar idea to create-pull-request tool.
There was a problem hiding this comment.
Restructured in fb479e5 to mirror the create-pull-request staging pattern:
Stage 1 (MCP, mcp.rs) now:
- Validates
UploadArtifactParams. - Resolves
file_pathagainstbounding_directory(workspace root) and canonicalizes to guard against symlink escape. - Rejects directories.
- Copies the file into
self.output_directoryunder a generated unique nameupload-artifact-<artifact_name>-<short-id>.<ext>(extension preserved for the Stage 3 allow-list check). - Constructs the result via
UploadArtifactResult::new(artifact_name, file_path, staged_file, file_size)and writes the NDJSON record.
Stage 3 (upload_artifact.rs::execute_impl) now reads from ctx.working_directory.join(&self.staged_file) and re-canonicalizes (defense in depth), then runs the existing size / extension / name-allow-list / name-prefix / ##vso[-injection / abs-path-charset checks before emitting ##vso[artifact.upload artifactname=...]<staged-abs-path>.
The result struct gained staged_file and file_size fields and an explicit ::new() constructor (an internal UploadArtifactResultFields mirror keeps the tool_result! macro happy). Added 3 new executor tests (success, missing-file, ##vso[-rejection) on top of the existing 16 — all 1034 tests + clippy pass.
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…ectory in Stage 3 Agent-Logs-Url: https://github.com/githubnext/ado-aw/sessions/c3b3f8ed-5b9d-45ed-aeaa-8d69a4dcb450 Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
/rust-review |
|
✅ Rust PR Reviewer completed successfully! |
🔍 Rust PR ReviewSummary: Looks good overall — solid security posture with defense-in-depth, good test coverage, and clean integration with existing patterns. Two issues worth addressing before merge. Findings🐛 Bugs / Logic Issues
|
* feat(safe-outputs): add unified upload-build-artifact safe output Combines the concepts from #363 (upload-artifact, current build) and #373 (upload-build-artifact, arbitrary build) into a single tool that handles both use cases via the ADO build attachments REST API. Key design: - build_id is Optional — omit to target the current pipeline run (resolved from BUILD_BUILDID in Stage 3) - REST API (PUT .../builds/{buildId}/attachments/{type}/{name}) for all cases, replacing the ##vso[artifact.upload] logging command approach - allowed-build-ids check is skipped when targeting the current build (implicitly trusted) - Full Stage 1 → Stage 3 file staging contract: validate, canonicalize, bounds-check, stage file with extension preservation, re-canonicalize and re-verify in Stage 3 Supersedes #363 and #373. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: move file read after dry-run guard and use Path::extension() for allowlist check - Move std::fs::read after the dry_run early return to avoid reading up to 50 MB into memory only to discard it in dry-run mode. - Switch extension allowlist from suffix matching on the full path to Path::extension() comparison, preventing false matches like 'catalog' matching allowed-extensions: ['log']. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: restore cache-memory heading in safe-outputs.md The heading was accidentally dropped when the upload-build-artifact section was inserted, leaving the cache-memory paragraph as a stray note under the wrong section. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use async file read, add staged-file integrity check, clarify comments - Switch std::fs::read to tokio::fs::read().await to avoid blocking the tokio runtime for files up to 50 MB. - Add a warn!() when the live staged-file size differs from the size recorded at Stage 1, catching potential file tampering between stages. - Add comments on is_valid_version() call sites noting intentional reuse for artifact-name and attachment-type charset validation. - Add comment on staged filename explaining max-length reasoning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: make staged-file size mismatch a hard failure A size mismatch between Stage 1 and Stage 3 indicates the staged file was modified between stages — fail hard rather than warning and uploading potentially tampered content. Adds a test for the integrity check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: add attachment-type dot guard, Stage 1 size cap, executor tests - Add starts_with('.') guard to attachment-type validation, matching the artifact_name check for consistency. - Add defense-in-depth file size cap at Stage 1 (MCP) using the default 50 MB limit to prevent staging-disk exhaustion before Stage 3 enforces the operator's configured limit. - Add three missing executor tests: extension allowlist rejection, artifact-name allowlist rejection, and name-prefix application. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * feat: add SHA-256 cross-stage integrity verification Add SHA-256 hash recording at Stage 1 and verification at Stage 3 for both file-staging safe outputs: - upload-build-artifact: staged_sha256 field recorded after copying the file, verified before uploading to ADO. Catches same-size file swaps that the size-only check would miss. - create-pull-request: patch_sha256 field recorded after writing the patch, verified before applying. Field is Optional for backward compatibility with existing NDJSON records. Also adds sha256_hex() helper in upload_build_artifact module, reused by both tools and the MCP handlers. A separate issue (#381) tracks adding the full staging pattern (including SHA-256) to upload-workitem-attachment, which currently reads directly from source_directory without staging. Also fixes: name-prefix length cap (50 chars), attachment-type leading-dot guard, comment typo in MCP handler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: make patch_sha256 a required field on CreatePrResult Pipelines are versioned with their YAML — no need for backward compatibility with old NDJSON records. Making the hash optional created a security hole where a missing hash would skip verification entirely. Now patch_sha256 is a required String and verification is unconditional. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: move sha256_hex to src/hash.rs, fix prefix slice panic - Move sha256_hex() from upload_build_artifact.rs to a dedicated src/hash.rs module, removing the coupling between create_pr, mcp.rs and the upload_build_artifact module. - Fix potential panic in name-prefix error path: &prefix[..20] can panic on multi-byte UTF-8 boundaries. Use prefix.chars().take(20).collect() instead. - Revert upload_build_artifact module visibility back to private. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: add SHA-256 mismatch tests, fix TOCTOU, extract is_valid_artifact_name - Add non-dry-run SHA-256 mismatch tests for both upload-build-artifact and create-pull-request (the mismatch path fires before any HTTP call, so no credentials are needed). - Fix TOCTOU in MCP handler: hash the source bytes before writing to the staging directory, instead of re-reading the staged copy. - Extract is_valid_artifact_name() in validate.rs to decouple artifact name validation from version string validation, preventing silent breakage if is_valid_version is ever tightened. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: import DEFAULT_MAX_FILE_SIZE in MCP handler Replace the floating STAGE1_MAX_FILE_SIZE copy with a direct import of DEFAULT_MAX_FILE_SIZE from upload_build_artifact, ensuring the Stage 1 cap stays in sync if the default is ever changed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds a new
upload-artifactsafe output that lets agents publish a workspace file as an Azure DevOps pipeline artifact attached to the current build run — the closest ADO equivalent to GitHub Actions'actions/upload-artifact.Summary
Today an agent has no clean way to surface a build deliverable (generated report, screenshot, log bundle, compiled binary).
upload-attachmentonly attaches files to a work item; there is no analogue for the run-summary Artifacts list. gh-aw'supload-assetsolves the same problem for GitHub by committing files to an orphaned branch, and it points users atactions/upload-artifactfor the run-scoped variant. ADO needs its own first-class tool because the agent runs inside the AWF network-isolated sandbox and can't invokePublishPipelineArtifact@1directly.The flow is split across two stages, mirroring how
create-pull-requeststages its patch file:Stage 1 (MCP, in the agent sandbox):
UploadArtifactParams—artifact_name(charset, length, no leading.) andfile_path(no traversal, no absolute paths, no.git, no symlink escape outside the workspace). Validation leverages the shared primitives insrc/validate.rs(is_valid_versionfor the artifact-name charset,is_safe_path_segmentper file-path component) instead of inlining char allowlists.file_pathagainstbounding_directory(the agent's workspace root), canonicalises it, and rejects symlink escapes.output_directoryunder a generated unique name (upload-artifact-<artifact_name>-<short-id>.<ext>, extension preserved for the Stage 3 allow-list check). This is essential because the agent's sandbox workspace is no longer accessible at execution time.UploadArtifactResult::new(artifact_name, file_path, staged_file, file_size)and writes the NDJSON record.Stage 3 (executor, outside the sandbox):
ctx.working_directory.join(staged_file)and re-canonicalises it inside the working directory (defense in depth).name-prefix, then enforcesallowed-artifact-names,allowed-extensions, andmax-file-sizelimits.##vso[or##[) usingcontains_pipeline_command; binary files skip that scan.\n/\r/](usingcontains_newline) so the logging command can't be malformed/escaped.##vso[artifact.upload artifactname=NAME]<staged-abs-path>on stdout. The build agent picks this up and asynchronously attaches the file to the current run.Dry-run summarises the action without emitting the command.
Front-matter shape
Files changed
src/safeoutputs/upload_artifact.rs— new tool:UploadArtifactParams,UploadArtifactResult(withartifact_name,file_path,staged_file,file_sizefields and an explicit::new()constructor), internalUploadArtifactResultFieldsmirror to satisfy thetool_result!macro, validation, config, and Stage 3 executor (+19 unit tests, including 3 executor tests covering staged-file success, missing-file failure, and##vso[-injection rejection). Uses shared validators fromsrc/validate.rsfor charset, path-segment, pipeline-command and newline checks.src/safeoutputs/mod.rs— module, re-export,WRITE_REQUIRING_SAFE_OUTPUTS,ALL_KNOWN_SAFE_OUTPUTS, consistency test.src/execute.rs— dispatcher entry and per-run budget registration.src/mcp.rs— MCP tool method (upload-artifact) now performs Stage 1 validation, workspace-bounded canonicalisation, directory rejection, and copies the file intooutput_directorybefore writing the NDJSON record.docs/safe-outputs.md— reference section under the existingupload-attachmententry, updated to describe the Stage 1 staging and Stage 3 validation split.Notes / future work
containerfolder.PublishPipelineArtifactlater without changing the agent-facing API if needed.Test plan
cargo build✅cargo test✅ (1034 tests, 0 failures, including 19 new tests for this tool covering name/path validation, config defaults, YAML round-trip, serialization, and Stage 3 executor behaviour against a staged file in a temporary working directory)cargo clippy --all-targets --all-features✅ (no warnings on the new file)to_string_lossy()).src/validate.rs(is_valid_version,is_safe_path_segment,contains_pipeline_command,contains_newline) instead of inlining char-allowlist / injection-detection logic. The##vso[scan now also catches##[, a strict improvement.output_directoryduring the MCP call (Stage 1) and read it back fromctx.working_directoryin Stage 3, mirroring thecreate-pull-requestpattern. Without this, the Stage 3 executor would have had nothing to publish because the sandbox workspace is no longer accessible by then.