/rust-review

✅ Rust PR Reviewer completed successfully!

🔍 Rust PR Review

Summary: Good security-conscious design overall, but has one meaningful security gap and two configuration footguns worth addressing before merge.

Findings

🔒 Security Concerns

• src/safeoutputs/upload_artifact.rs:216-228 — Extension filter uses agent-supplied path, not canonical path (symlink bypass)

  The allowed-extensions check runs against self.file_path (the agent's unresolved string) before symlink resolution. An agent can create a symlink inside the workspace — e.g. report.pdf → malware.exe — and the filter sees .pdf while the pipeline artifact receives .exe content. The fix is to apply the extension check against canonical (already computed at line 235) rather than self.file_path:

  // after computing `canonical`:
  let has_valid_ext = config.allowed_extensions.iter().any(|ext| {
      canonical
          .to_string_lossy()
          .to_lowercase()
          .ends_with(&ext.to_lowercase())
  });

  Or more robustly, check canonical.extension(). Since canonical is always valid UTF-8 at this point (checked at line 284), this is straightforward.

⚠️ Suggestions

• src/safeoutputs/upload_artifact.rs:217-221 — ends_with extension matching without enforcing a leading dot

  allowed_extensions entries are not validated to start with .. An operator who writes allowed-extensions: ["pdf"] instead of [".pdf"] gets a suffix check: "apdf" and "logpdf" both pass. Should either (a) normalise entries to start with . at config load, or (b) validate that each entry starts with . and surface a clear error if not.

• src/safeoutputs/upload_artifact.rs:52-76 — \n/\r not rejected in file_path during validate()

  Newlines are only checked in abs_path after canonicalization (line 300). Paths containing \n are technically valid on Linux; source_directory.join(path_with_newline) and canonicalize() can succeed if the file exists, putting a newline in the log output at line 168. Adding ensure!(!self.file_path.contains('\n') && !self.file_path.contains('\r'), ...) in validate() is cheap defense-in-depth consistent with blocking \0.

✅ What Looks Good

• The symlink-escape guard using canonical.starts_with(&canonical_base) is correct — Rust's Path::starts_with does component-wise comparison, not string prefix, so /workspace-evil/file can't spoof /workspace.
• Re-validating final_name's charset (line 184-195) after applying the operator-controlled name-prefix is the right defensive pattern; a malicious prefix containing ] or ##vso[ is caught here before the logging command is emitted.
• Explicit UTF-8 path check (line 284) and ]/\n/\r guard on abs_path (line 300) are well-targeted; this forecloses the logging-command injection vectors correctly.
• Whole-file read for ##vso[ scanning is bounded by max_file_size (checked first at line 261), so worst-case memory usage is deterministic.
• The max: N budget field flows through the executor dispatch machinery rather than being duplicated in the config struct — consistent with all other safe-output tools.

Generated by Rust PR Reviewer for issue #363 · ● 1.1M · ◷

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

/rust-review

✅ Rust PR Reviewer completed successfully!

🔍 Rust PR Review

Summary: 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

• src/safeoutputs/upload_artifact.rs:362 — The emitted ADO logging command is missing the containerfolder property. The [documented format]((learn.microsoft.com/redacted) is:

  ##vso[artifact.upload containerfolder=FOLDER;artifactname=NAME]PATH
  ```
  The current code emits:
  ```
  ##vso[artifact.upload artifactname=NAME]PATH
  

  Without containerfolder, behavior is agent-version-dependent — the file may end up in an unnamed container root or the artifact may not appear as expected in the run's Artifacts tab. A reasonable default is to use final_name as both (i.e. containerfolder=NAME;artifactname=NAME), which is the pattern most ADO examples use for single-artifact uploads.

⚠️ Suggestions

• src/safeoutputs/upload_artifact.rs:296–340 — The file content (up to 50 MB) is read into memory for the contains_pipeline_command scan before the ctx.dry_run early-exit check at line 345. In dry-run mode the scan result is discarded, so the read is wasted. Moving the dry_run guard to just before the file read (after the size check) would avoid allocating up to 50 MB unnecessarily. Note: upload_attachment.rs has the same ordering, so this isn't a new regression — but this new tool operates on potentially much larger files.

• src/safeoutputs/upload_artifact.rs:256–265 — The extension allowlist check uses self.file_path (the agent-supplied value stored in the NDJSON record) rather than the staged file's actual extension. Stage 1 encodes the source extension in the staged filename (e.g. upload-artifact-name-<id>.pdf), so comparing against Path::new(&self.staged_file).extension() would be equivalent and slightly more tamper-resistant — if the NDJSON file_path field is modified between stages, the extension check could be bypassed while the staged file points to a different type. The staged file is still bounded to working_directory, limiting practical impact, but the staged extension is already available.

✅ What Looks Good

• The two-stage copy-then-emit pattern (mirroring create-pull-request) is the right approach; the sandbox workspace genuinely isn't accessible at execution time.
• Canonicalize + starts_with bounding checks in both Stage 1 (MCP) and Stage 3 are solid defense-in-depth against symlink escapes, even if the NDJSON staged_file field is tampered with.
• Using shared is_valid_version, is_safe_path_segment, contains_pipeline_command, and contains_newline validators rather than inlining char allowlists is exactly the right convention for this codebase.
• The ##[ catch (in addition to ##vso[) in contains_pipeline_command is a strict improvement over upload_attachment.rs.
• 19 tests with executor tests that use tempfile and actually exercise the staged-file path are well above the bar for this kind of tool.

Generated by Rust PR Reviewer for issue #363 · ● 940.5K · ◷