fix(drivers): reject whitespace in mount fields (#2086)

Signed-off-by: Evan Lezar <elezar@nvidia.com>

Author: elezar

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<String, String> {
-    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<String, String> {
-    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<String, String> {
-    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<String, String> {
     {
         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<String, String> {
-    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"
+        );
+    }
 }

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<Mount, S
             read_only,
         } => 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<Mount, S
             subpath,
         } => 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, Status>(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<Mount, S
             mode,
         } => 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}'"
             )));
         }
     }

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}'"
             ));
         }
     }