From 2fd6fad8c4c2da7e87a9f5ae166076006a09ed9c Mon Sep 17 00:00:00 2001 From: Giorgos Miliaras Date: Mon, 7 Sep 2026 10:08:49 +0000 Subject: [PATCH] Strip leading backslashes from AppSpec paths on Windows On Windows the "\" separator in an AppSpec files.source or hooks.location was treated as a filesystem root, so PathBuf::join discarded the archive directory and resolved the path to the drive root. Strip every leading separator via paths::APPSPEC_PATH_SEPARATORS (['/', '\\'] on Windows, ['/'] on Unix) so such paths always resolve inside the deployment archive. Bump agent version to 2.0.1 --- Cargo.toml | 2 +- crates/codedeploy-commands/Cargo.toml | 2 +- src/installer/core.rs | 212 +++++++++++++++++++++++++- src/lifecycle_event/executor.rs | 79 +++++++++- src/paths.rs | 33 ++++ 5 files changed, 318 insertions(+), 10 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 03c7478..1806f53 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,7 +7,7 @@ edition = "2024" rust-version = "1.91" homepage = "https://aws.amazon.com/codedeploy/" repository = "https://github.com/aws/aws-codedeploy-agent" -version = "2.0.0" +version = "2.0.1" description = "AWS CodeDeploy agent responsible for deploying software on an individual EC2/On-prem instance" license = "Apache-2.0" authors = ["Amazon Web Services"] diff --git a/crates/codedeploy-commands/Cargo.toml b/crates/codedeploy-commands/Cargo.toml index be08a5c..435c0e5 100644 --- a/crates/codedeploy-commands/Cargo.toml +++ b/crates/codedeploy-commands/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codedeploy-commands" -version = "2.0.0" +version = "2.0.1" edition = "2024" rust-version = "1.91" publish = false diff --git a/src/installer/core.rs b/src/installer/core.rs index f286bf0..1146572 100644 --- a/src/installer/core.rs +++ b/src/installer/core.rs @@ -9,6 +9,7 @@ use super::{ error::{InstallerError, Result}, }; use crate::application_specification::{AppSpec, FileExistsBehavior}; +use crate::paths::APPSPEC_PATH_SEPARATORS; use crate::system::write_file_secure; use serde_json::json; use std::fs; @@ -194,11 +195,9 @@ impl Installer { file_mapping: &crate::application_specification::FileMapping, spec: &AppSpec, ) -> Result<()> { - // Strip leading '/' so that an absolute-looking `source` stays relative to - // the archive dir: a leading slash must not discard the base path (as - // Rust's PathBuf::join would). - let source_relative = - file_mapping.source().strip_prefix('/').unwrap_or(file_mapping.source()); + // A leading separator means "the entire revision", not a filesystem root. + // Strip every one of them so `join` cannot discard the archive dir. + let source_relative = file_mapping.source().trim_start_matches(APPSPEC_PATH_SEPARATORS); let source = self.deployment_archive_dir.join(source_relative); debug!("Processing file mapping from source: {}", source.display()); @@ -813,6 +812,209 @@ files: assert!(dest_dir.path().join("index.html").exists()); } + /// A bare separator copies every file in the archive, `appspec.yml` included. + #[test] + fn install_bare_slash_source_copies_whole_revision() { + let archive_dir = TempDir::new().unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + fs::write(archive_dir.path().join("appspec.yml"), "version: 0.0\n").unwrap(); + fs::write(archive_dir.path().join("my-file.txt"), "one").unwrap(); + fs::create_dir(archive_dir.path().join("my-folder")).unwrap(); + fs::write(archive_dir.path().join("my-folder/my-file-2.txt"), "two").unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ); + + let appspec_yaml = format!( + r" +version: 0.0 +os: linux +files: + - source: / + destination: {} +", + dest_dir.path().display() + ); + let spec = AppSpec::parse(&appspec_yaml).unwrap(); + installer.install("test-group", &spec).unwrap(); + + assert!(dest_dir.path().join("appspec.yml").exists()); + assert!(dest_dir.path().join("my-file.txt").exists()); + assert!(dest_dir.path().join("my-folder/my-file-2.txt").exists()); + } + + /// Repeated leading separators must be fully stripped. + #[test] + fn install_repeated_leading_slash_source_resolves_inside_archive() { + let archive_dir = TempDir::new().unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + fs::write(archive_dir.path().join("index.html"), "").unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ); + + let appspec_yaml = format!( + r" +version: 0.0 +os: linux +files: + - source: //index.html + destination: {} +", + dest_dir.path().display() + ); + let spec = AppSpec::parse(&appspec_yaml).unwrap(); + installer.install("test-group", &spec).unwrap(); + + assert!(dest_dir.path().join("index.html").exists()); + } + + /// On Unix `\` is a filename character, not a separator. The lint assumes it + /// is a separator on every platform, which is what this test disproves. + #[cfg(unix)] + #[allow(clippy::join_absolute_paths)] + #[test] + fn install_backslash_source_is_a_filename_on_unix() { + let archive_dir = TempDir::new().unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + fs::write(archive_dir.path().join(r"\index.html"), "").unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ); + + let appspec_yaml = format!( + r" +version: 0.0 +os: linux +files: + - source: \index.html + destination: {} +", + dest_dir.path().display() + ); + let spec = AppSpec::parse(&appspec_yaml).unwrap(); + installer.install("test-group", &spec).unwrap(); + + assert!(dest_dir.path().join(r"\index.html").exists()); + } + + #[cfg(windows)] + #[test] + fn install_leading_backslash_source_resolves_inside_archive() { + let archive_dir = TempDir::new().unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + fs::write(archive_dir.path().join("index.html"), "").unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ); + + let appspec_yaml = format!( + r" +version: 0.0 +os: windows +files: + - source: \index.html + destination: {} +", + dest_dir.path().display() + ); + let spec = AppSpec::parse(&appspec_yaml).unwrap(); + installer.install("test-group", &spec).unwrap(); + + assert!(dest_dir.path().join("index.html").exists()); + } + + /// A bare `\` copies every file in the archive, `appspec.yml` included. + #[cfg(windows)] + #[test] + fn install_bare_backslash_source_copies_whole_revision() { + let archive_dir = TempDir::new().unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + fs::write(archive_dir.path().join("appspec.yml"), "version: 0.0\n").unwrap(); + fs::write(archive_dir.path().join("my-file.txt"), "one").unwrap(); + fs::create_dir(archive_dir.path().join("my-folder")).unwrap(); + fs::write(archive_dir.path().join(r"my-folder\my-file-2.txt"), "two").unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ); + + let appspec_yaml = format!( + r" +version: 0.0 +os: windows +files: + - source: \ + destination: {} +", + dest_dir.path().display() + ); + let spec = AppSpec::parse(&appspec_yaml).unwrap(); + installer.install("test-group", &spec).unwrap(); + + assert!(dest_dir.path().join("appspec.yml").exists()); + assert!(dest_dir.path().join("my-file.txt").exists()); + assert!(dest_dir.path().join(r"my-folder\my-file-2.txt").exists()); + } + + /// A leading separator followed by further components still resolves inside + /// the archive, not at the drive root. + #[cfg(windows)] + #[test] + fn install_leading_backslash_subdirectory_resolves_inside_archive() { + let archive_dir = TempDir::new().unwrap(); + let instructions_dir = TempDir::new().unwrap(); + let dest_dir = TempDir::new().unwrap(); + + fs::create_dir(archive_dir.path().join("my-folder")).unwrap(); + fs::write(archive_dir.path().join(r"my-folder\my-file.txt"), "one").unwrap(); + + let installer = Installer::new( + archive_dir.path().to_path_buf(), + instructions_dir.path().to_path_buf(), + FileExistsBehavior::Overwrite, + ); + + let appspec_yaml = format!( + r" +version: 0.0 +os: windows +files: + - source: \my-folder + destination: {} +", + dest_dir.path().display() + ); + let spec = AppSpec::parse(&appspec_yaml).unwrap(); + installer.install("test-group", &spec).unwrap(); + + assert!(dest_dir.path().join("my-file.txt").exists()); + } + #[test] fn install_path_traversal_allowed_when_flag_off() { let archive_dir = TempDir::new().unwrap(); diff --git a/src/lifecycle_event/executor.rs b/src/lifecycle_event/executor.rs index f74ce7c..e457950 100644 --- a/src/lifecycle_event/executor.rs +++ b/src/lifecycle_event/executor.rs @@ -16,6 +16,7 @@ use super::script::{HookEnvPolicy, Script}; use super::script_run_log::ScriptRunLog; use crate::application_specification::AppSpec; use crate::deployment_specification::types::{DeploymentSpec, RevisionLocation, RevisionSource}; +use crate::paths::APPSPEC_PATH_SEPARATORS; use crate::system::file_ops::ensure_executable; use std::collections::HashMap; use std::path::{Path, PathBuf}; @@ -195,9 +196,9 @@ impl LifecycleEventExecutor { log: &Arc>, ) -> Result<(), ScriptError> { let location = script_info.location().to_string(); - // Strip leading '/' so a leading slash in the location does not discard - // the base path (unlike Rust's PathBuf::join). - let location_relative = location.strip_prefix('/').unwrap_or(&location); + // Strip leading separators so `join` cannot discard the archive dir. + // Mirrors the `files.source` strip in installer/core.rs. + let location_relative = location.trim_start_matches(APPSPEC_PATH_SEPARATORS); let script_path = archive_dir.join(location_relative); // `entries()` is the bounded stdout/stderr tail the stream tasks buffered; // it becomes the diagnostic's log so the service sees the script's output. @@ -824,6 +825,78 @@ hooks: assert!(entries.iter().any(|e| e.contains("Script - /scripts/ok.sh"))); } + /// Repeated leading separators must be fully stripped. + #[cfg(unix)] + #[test] + fn execute_repeated_leading_slash_location_resolves_inside_archive() { + use std::os::unix::fs::PermissionsExt; + + let dir = TempDir::new().unwrap(); + let appspec = r" +version: 0.0 +os: linux +hooks: + AfterInstall: + - location: //scripts/ok.sh + timeout: 10 +"; + setup_appspec(dir.path(), appspec); + + let scripts_dir = dir.path().join("deployment-archive/scripts"); + std::fs::create_dir_all(&scripts_dir).unwrap(); + std::fs::write(scripts_dir.join("ok.sh"), "#!/bin/sh\necho done\n").unwrap(); + std::fs::set_permissions(scripts_dir.join("ok.sh"), std::fs::Permissions::from_mode(0o755)) + .unwrap(); + + let spec = s3_spec(); + let he = LifecycleEventExecutor::new( + LifecycleEventType::AfterInstall, + &spec, + dir.path(), + None, + None, + ) + .unwrap(); + let entries = he.execute().unwrap(); + assert!(entries.iter().any(|e| e.contains("Script - //scripts/ok.sh"))); + } + + /// Uses a missing script and asserts the resolved path in the error message, + /// so nothing has to be executed to check resolution. + #[cfg(windows)] + #[test] + fn execute_leading_backslash_location_resolves_inside_archive() { + let dir = TempDir::new().unwrap(); + let appspec = r" +version: 0.0 +os: windows +hooks: + AfterInstall: + - location: \scripts\missing.cmd + timeout: 10 +"; + setup_appspec(dir.path(), appspec); + + let spec = s3_spec(); + let he = LifecycleEventExecutor::new( + LifecycleEventType::AfterInstall, + &spec, + dir.path(), + None, + None, + ) + .unwrap(); + let err = he.execute().unwrap_err(); + + assert_eq!(err.error_code, ErrorCode::ScriptMissing); + let expected = dir.path().join(r"deployment-archive\scripts\missing.cmd"); + assert!( + err.message.contains(&expected.display().to_string()), + "location must resolve inside the archive, got: {}", + err.message + ); + } + #[cfg(unix)] #[test] fn execute_failing_script() { diff --git a/src/paths.rs b/src/paths.rs index de7e5d3..9e929b9 100644 --- a/src/paths.rs +++ b/src/paths.rs @@ -9,6 +9,12 @@ use std::path::PathBuf; +/// Leading separators to strip from an `AppSpec` path (`files: source`, +/// `hooks: location`) before resolving it against the deployment archive. +/// +/// `\` is a separator on Windows only; on Unix it is a legal filename character. +pub const APPSPEC_PATH_SEPARATORS: &[char] = if cfg!(windows) { &['/', '\\'] } else { &['/'] }; + /// Returns the Windows base directory: `%PROGRAMDATA%\Amazon\CodeDeploy`. /// /// Read from `%PROGRAMDATA%`, falling back to `C:\ProgramData`. @@ -156,6 +162,33 @@ pub fn updater_log_path() -> PathBuf { mod tests { use super::*; + #[cfg(unix)] + #[test] + fn appspec_separators_exclude_backslash_on_unix() { + assert_eq!(APPSPEC_PATH_SEPARATORS, &['/']); + assert_eq!(r"\app".trim_start_matches(APPSPEC_PATH_SEPARATORS), r"\app"); + } + + #[cfg(windows)] + #[test] + fn appspec_separators_include_backslash_on_windows() { + assert_eq!(APPSPEC_PATH_SEPARATORS, &['/', '\\']); + assert_eq!(r"\app".trim_start_matches(APPSPEC_PATH_SEPARATORS), "app"); + assert_eq!(r"\".trim_start_matches(APPSPEC_PATH_SEPARATORS), ""); + } + + #[test] + fn appspec_separators_strip_repeated_slashes() { + assert_eq!("//app".trim_start_matches(APPSPEC_PATH_SEPARATORS), "app"); + assert_eq!("/".trim_start_matches(APPSPEC_PATH_SEPARATORS), ""); + } + + #[test] + fn appspec_separators_leave_relative_paths_alone() { + assert_eq!("app/sub".trim_start_matches(APPSPEC_PATH_SEPARATORS), "app/sub"); + assert_eq!("./app".trim_start_matches(APPSPEC_PATH_SEPARATORS), "./app"); + } + #[cfg(unix)] #[test] fn config_file_returns_unix_path() {