From 3ae3e698c2ebe181838abb54eaa761442cc17069 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 1 Jul 2026 11:09:45 +0200 Subject: [PATCH] fix(drivers): reject whitespace in mount fields Signed-off-by: Evan Lezar --- crates/openshell-core/src/driver_mounts.rs | 81 +++++++++++-------- crates/openshell-driver-docker/src/lib.rs | 56 ++++--------- .../openshell-driver-podman/src/container.rs | 32 +++++--- 3 files changed, 84 insertions(+), 85 deletions(-) diff --git a/crates/openshell-core/src/driver_mounts.rs b/crates/openshell-core/src/driver_mounts.rs index 0b27e0a3b..138fbdf1d 100644 --- a/crates/openshell-core/src/driver_mounts.rs +++ b/crates/openshell-core/src/driver_mounts.rs @@ -13,32 +13,36 @@ const RESERVED_MOUNT_TARGETS: &[&str] = &[ ]; /// Validate a non-empty driver mount source. -pub fn validate_mount_source(source: &str, field: &str) -> Result { - let source = source.trim(); +pub fn validate_mount_source(source: &str, field: &str) -> Result<(), String> { if source.is_empty() { return Err(format!("{field} must not be empty")); } + if source != source.trim() { + return Err(format!("{field} must not contain surrounding whitespace")); + } if source.as_bytes().contains(&0) { return Err(format!("{field} must not contain NUL bytes")); } - Ok(source.to_string()) + Ok(()) } /// Validate a bind mount source as an absolute host path. -pub fn validate_absolute_mount_source(source: &str, field: &str) -> Result { - let source = validate_mount_source(source, field)?; - if !Path::new(&source).is_absolute() { +pub fn validate_absolute_mount_source(source: &str, field: &str) -> Result<(), String> { + validate_mount_source(source, field)?; + if !Path::new(source).is_absolute() { return Err(format!("{field} must be an absolute host path")); } - Ok(source) + Ok(()) } /// Validate a relative subpath inside a runtime-managed mount source. -pub fn validate_mount_subpath(subpath: &str) -> Result { - let subpath = subpath.trim(); +pub fn validate_mount_subpath(subpath: &str) -> Result<(), String> { if subpath.is_empty() { return Err("mount subpath must not be empty".to_string()); } + if subpath != subpath.trim() { + return Err("mount subpath must not contain surrounding whitespace".to_string()); + } if subpath.as_bytes().contains(&0) { return Err("mount subpath must not contain NUL bytes".to_string()); } @@ -50,57 +54,56 @@ pub fn validate_mount_subpath(subpath: &str) -> Result { { return Err("mount subpath must be relative and must not contain '..'".to_string()); } - Ok(subpath.to_string()) + Ok(()) } /// Validate a container-side mount target for user-supplied driver mounts. -pub fn validate_container_mount_target(target: &str) -> Result { - let target = normalize_container_mount_target(target); +pub fn validate_container_mount_target(target: &str) -> Result<(), String> { if target.is_empty() { return Err("mount target must not be empty".to_string()); } + if target != target.trim() { + return Err("mount target must not contain surrounding whitespace".to_string()); + } if target.as_bytes().contains(&0) { return Err("mount target must not contain NUL bytes".to_string()); } if !target.starts_with('/') { return Err("mount target must be an absolute container path".to_string()); } - if target == "/" { + let path = Path::new(target); + if path == Path::new("/") { return Err("mount target must not be the container root".to_string()); } - let path = Path::new(&target); if path .components() .any(|component| matches!(component, std::path::Component::ParentDir)) { return Err("mount target must not contain '..'".to_string()); } - if target == "/sandbox" { + if path == Path::new("/sandbox") { return Err("mount target '/sandbox' is reserved for the OpenShell workspace".to_string()); } for reserved in RESERVED_MOUNT_TARGETS { - if path_is_or_under(&target, reserved) { + if path_is_or_under(path, Path::new(reserved)) { return Err(format!( "mount target '{target}' conflicts with reserved OpenShell path '{reserved}'" )); } } - Ok(target) + Ok(()) } -fn normalize_container_mount_target(target: &str) -> String { - let target = target.trim(); +/// Normalize a validated container-side mount target for semantic comparison. +pub fn normalize_mount_target(target: &str) -> String { if target == "/" { return target.to_string(); } target.trim_end_matches('/').to_string() } -fn path_is_or_under(path: &str, parent: &str) -> bool { - path == parent - || path - .strip_prefix(parent) - .is_some_and(|rest| rest.starts_with('/')) +fn path_is_or_under(path: &Path, parent: &Path) -> bool { + path == parent || path.starts_with(parent) } #[cfg(test)] @@ -109,10 +112,8 @@ mod tests { #[test] fn container_target_allows_paths_under_workspace() { - assert_eq!( - validate_container_mount_target("/sandbox/work/").unwrap(), - "/sandbox/work" - ); + validate_container_mount_target("/sandbox/work/").unwrap(); + assert_eq!(normalize_mount_target("/sandbox/work/"), "/sandbox/work"); } #[test] @@ -138,16 +139,30 @@ mod tests { #[test] fn container_target_does_not_prefix_match_unrelated_paths() { - assert_eq!( - validate_container_mount_target("/etc/openshell-tools").unwrap(), - "/etc/openshell-tools" - ); + validate_container_mount_target("/etc/openshell-tools").unwrap(); } #[test] fn mount_subpath_must_be_relative_without_parent_dirs() { - assert_eq!(validate_mount_subpath(" project/a ").unwrap(), "project/a"); + assert!(validate_mount_subpath("project/a").is_ok()); + assert!(validate_mount_subpath(" project/a ").is_err()); assert!(validate_mount_subpath("/project").is_err()); assert!(validate_mount_subpath("../project").is_err()); } + + #[test] + fn mount_values_reject_surrounding_whitespace() { + assert_eq!( + validate_mount_source(" volume ", "volume source").unwrap_err(), + "volume source must not contain surrounding whitespace" + ); + assert_eq!( + validate_absolute_mount_source(" /host/path", "bind source").unwrap_err(), + "bind source must not contain surrounding whitespace" + ); + assert_eq!( + validate_container_mount_target("/sandbox/work ").unwrap_err(), + "mount target must not contain surrounding whitespace" + ); + } } diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 913054934..a59352018 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -555,20 +555,18 @@ impl DockerComputeDriver { ) -> Result<(), Status> { for mount in &driver_config.mounts { if let DockerDriverMountConfig::Volume { source, .. } = mount { - match self.docker.inspect_volume(source.trim()).await { + match self.docker.inspect_volume(source).await { Ok(volume) => { if !self.config.enable_bind_mounts && docker_volume_is_bind_backed(&volume) { return Err(Status::failed_precondition(format!( - "docker volume '{}' is backed by a host bind mount and requires enable_bind_mounts = true in [openshell.drivers.docker]", - source.trim() + "docker volume '{source}' is backed by a host bind mount and requires enable_bind_mounts = true in [openshell.drivers.docker]" ))); } } Err(err) if is_not_found_error(&err) => { return Err(Status::failed_precondition(format!( - "docker volume '{}' does not exist", - source.trim() + "docker volume '{source}' does not exist" ))); } Err(err) => { @@ -1775,14 +1773,8 @@ fn docker_mount_from_config(config: &DockerDriverMountConfig) -> Result Ok(Mount { typ: Some(MountTypeEnum::BIND), - source: Some( - driver_mounts::validate_absolute_mount_source(source, "bind source") - .map_err(Status::failed_precondition)?, - ), - target: Some( - driver_mounts::validate_container_mount_target(target) - .map_err(Status::failed_precondition)?, - ), + source: Some(source.clone()), + target: Some(target.clone()), read_only: Some(*read_only), ..Default::default() }), @@ -1793,27 +1785,13 @@ fn docker_mount_from_config(config: &DockerDriverMountConfig) -> Result Ok(Mount { typ: Some(MountTypeEnum::VOLUME), - source: Some( - driver_mounts::validate_mount_source(source, "volume source") - .map_err(Status::failed_precondition)?, - ), - target: Some( - driver_mounts::validate_container_mount_target(target) - .map_err(Status::failed_precondition)?, - ), + source: Some(source.clone()), + target: Some(target.clone()), read_only: Some(*read_only), - volume_options: subpath - .as_ref() - .map(|subpath| { - Ok::(MountVolumeOptions { - subpath: Some( - driver_mounts::validate_mount_subpath(subpath) - .map_err(Status::failed_precondition)?, - ), - ..Default::default() - }) - }) - .transpose()?, + volume_options: subpath.as_ref().map(|subpath| MountVolumeOptions { + subpath: Some(subpath.clone()), + ..Default::default() + }), ..Default::default() }), DockerDriverMountConfig::Tmpfs { @@ -1823,10 +1801,7 @@ fn docker_mount_from_config(config: &DockerDriverMountConfig) -> Result Ok(Mount { typ: Some(MountTypeEnum::TMPFS), - target: Some( - driver_mounts::validate_container_mount_target(target) - .map_err(Status::failed_precondition)?, - ), + target: Some(target.clone()), tmpfs_options: Some(MountTmpfsOptions { size_bytes: validate_optional_positive_integral_i64( *size_bytes, @@ -1906,11 +1881,12 @@ fn validate_docker_driver_mounts( )); } }; - let target = driver_mounts::validate_container_mount_target(target) + driver_mounts::validate_container_mount_target(target) .map_err(Status::failed_precondition)?; - if !targets.insert(target.clone()) { + let normalized_target = driver_mounts::normalize_mount_target(target); + if !targets.insert(normalized_target.clone()) { return Err(Status::failed_precondition(format!( - "duplicate docker driver_config mount target '{target}'" + "duplicate docker driver_config mount target '{normalized_target}'" ))); } } diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 66d0d9d90..ba47e4eaa 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -493,7 +493,7 @@ pub fn podman_driver_volume_mount_sources( .mounts .into_iter() .filter_map(|mount| match mount { - PodmanDriverMountConfig::Volume { source, .. } => Some(source.trim().to_string()), + PodmanDriverMountConfig::Volume { source, .. } => Some(source), _ => None, }) .collect()) @@ -515,7 +515,7 @@ pub fn podman_driver_image_mount_sources( .mounts .into_iter() .filter_map(|mount| match mount { - PodmanDriverMountConfig::Image { source, .. } => Some(source.trim().to_string()), + PodmanDriverMountConfig::Image { source, .. } => Some(source), _ => None, }) .collect()) @@ -541,10 +541,12 @@ fn podman_user_mounts( target, read_only, } => { + driver_mounts::validate_absolute_mount_source(&source, "bind source")?; + driver_mounts::validate_container_mount_target(&target)?; result.mounts.push(Mount { kind: "bind".into(), - source: driver_mounts::validate_absolute_mount_source(&source, "bind source")?, - destination: driver_mounts::validate_container_mount_target(&target)?, + source, + destination: target, options: vec![ if read_only { "ro" } else { "rw" }.to_string(), "rbind".to_string(), @@ -558,9 +560,11 @@ fn podman_user_mounts( subpath, } => { reject_subpath(subpath.as_deref(), "podman volume mounts")?; + driver_mounts::validate_mount_source(&source, "volume source")?; + driver_mounts::validate_container_mount_target(&target)?; result.volumes.push(NamedVolume { - name: driver_mounts::validate_mount_source(&source, "volume source")?, - dest: driver_mounts::validate_container_mount_target(&target)?, + name: source, + dest: target, options: vec![if read_only { "ro" } else { "rw" }.to_string()], }); } @@ -583,10 +587,11 @@ fn podman_user_mounts( { options.push(format!("mode={mode:o}")); } + driver_mounts::validate_container_mount_target(&target)?; result.mounts.push(Mount { kind: "tmpfs".into(), source: "tmpfs".into(), - destination: driver_mounts::validate_container_mount_target(&target)?, + destination: target, options, }); } @@ -597,9 +602,11 @@ fn podman_user_mounts( subpath, } => { reject_subpath(subpath.as_deref(), "podman image mounts")?; + driver_mounts::validate_mount_source(&source, "image source")?; + driver_mounts::validate_container_mount_target(&target)?; result.image_volumes.push(ImageVolume { - source: driver_mounts::validate_mount_source(&source, "image source")?, - destination: driver_mounts::validate_container_mount_target(&target)?, + source, + destination: target, rw: !read_only, }); } @@ -671,10 +678,11 @@ fn validate_podman_driver_mounts( target } }; - let target = driver_mounts::validate_container_mount_target(target)?; - if !targets.insert(target.clone()) { + driver_mounts::validate_container_mount_target(target)?; + let normalized_target = driver_mounts::normalize_mount_target(target); + if !targets.insert(normalized_target.clone()) { return Err(format!( - "duplicate podman driver_config mount target '{target}'" + "duplicate podman driver_config mount target '{normalized_target}'" )); } }