Sanitize asset paths before passing them to the loader (#223)

Signed-off-by: Michael X. Grey <[email protected]>

rmf_site_editor/src/site/drawing.rs

@@ -103,7 +103,19 @@ pub fn add_drawing_visuals(
             )),
             _ => source.clone(),
         };
-        let texture_handle: Handle<Image> = asset_server.load(&String::from(&asset_source));
+        let asset_path = match String::try_from(&asset_source) {
+            Ok(asset_path) => asset_path,
+            Err(err) => {
+                error!(
+                    "Invalid syntax while creating asset path for a drawing: {err}. \
+                    Check that your asset information was input correctly. \
+                    Current value:\n{:?}",
+                    asset_source,
+                );
+                continue;
+            }
+        };
+        let texture_handle: Handle<Image> = asset_server.load(asset_path);
         commands.entity(e).insert(LoadingDrawing(texture_handle));
     }
 }
@@ -194,7 +206,7 @@ pub fn handle_loaded_drawing(
                     .remove::<LoadingDrawing>();
             }
             LoadState::Failed => {
-                error!("Failed loading drawing {:?}", String::from(source));
+                error!("Failed loading drawing {:?}", source);
                 commands.entity(entity).remove::<LoadingDrawing>();
             }
             _ => {}

rmf_site_editor/src/site/model.rs

@@ -230,7 +230,19 @@ pub fn update_model_scenes(
             }
             _ => source.clone(),
         };
-        let handle = asset_server.load_untyped(&String::from(&asset_source));
+        let asset_path = match String::try_from(&asset_source) {
+            Ok(asset_path) => asset_path,
+            Err(err) => {
+                error!(
+                    "Invalid syntax while creating asset path for a model: {err}. \
+                    Check that your asset information was input correctly. \
+                    Current value:\n{:?}",
+                    asset_source,
+                );
+                return;
+            }
+        };
+        let handle = asset_server.load_untyped(asset_path);
         commands
             .insert(PreventDeletion::because(
                 "Waiting for model to spawn".to_string(),
@@ -305,8 +317,19 @@ pub fn update_model_tentative_formats(
                     continue;
                 }
             }
-            let path = String::from(source);
-            let model_ext = path
+            let asset_path = match String::try_from(source) {
+                Ok(asset_path) => asset_path,
+                Err(err) => {
+                    error!(
+                        "Invalid syntax while creating asset path to load a model: {err}. \
+                        Check that your asset information was input correctly. \
+                        Current value:\n{:?}",
+                        source,
+                    );
+                    continue;
+                }
+            };
+            let model_ext = asset_path
                 .rsplit_once('.')
                 .map(|s| s.1.to_owned())
                 .unwrap_or_else(|| tentative_format.to_string(""));
@@ -323,7 +346,10 @@ pub fn update_model_tentative_formats(
                     _ => "Failed parsing file".to_owned(),
                 }
             };
-            warn!("Failed loading Model with source {}: {}", path, reason);
+            warn!(
+                "Failed loading Model with source {}: {}",
+                asset_path, reason
+            );
             cmd.remove::<TentativeModelFormat>();
         }
     }

rmf_site_editor/src/site/texture.rs

@@ -28,10 +28,23 @@ pub fn fetch_image_for_texture(
     asset_server: Res<AssetServer>,
 ) {
     for (e, image, texture) in &mut changed_textures {
+        let asset_path = match String::try_from(&texture.source) {
+            Ok(asset_path) => asset_path,
+            Err(err) => {
+                error!(
+                    "Invalid syntax while creating asset path: {err}. \
+                    Check that your asset information was input correctly. \
+                    Current value:\n{:?}",
+                    texture.source,
+                );
+                continue;
+            }
+        };
+
         if let Some(mut image) = image {
-            *image = asset_server.load(String::from(&texture.source));
+            *image = asset_server.load(asset_path);
         } else {
-            let image: Handle<Image> = asset_server.load(String::from(&texture.source));
+            let image: Handle<Image> = asset_server.load(asset_path);
             commands.entity(e).insert(image);
         }
     }

rmf_site_editor/src/workcell/urdf_package_exporter/generate_package.rs

@@ -82,7 +82,7 @@ fn convert_and_copy_meshes(
 fn get_path_to_asset_file(asset_source: &AssetSource) -> Result<PathBuf, Box<dyn Error>> {
     match asset_source {
         AssetSource::Package(_) => Ok(urdf_rs::utils::expand_package_path(
-            &(String::from(asset_source)),
+            &(String::try_from(asset_source)?),
             None,
         )
         .to_string()

rmf_site_format/src/asset_source.rs

@@ -17,7 +17,10 @@
 
 use crate::*;
 #[cfg(feature = "bevy")]
-use bevy::prelude::{Component, Reflect, ReflectComponent};
+use bevy::{
+    asset::{AssetPath, ParseAssetPathError},
+    prelude::{Component, Reflect, ReflectComponent},
+};
 use pathdiff::diff_paths;
 use serde::{Deserialize, Serialize};
 use std::path::{Path, PathBuf};
@@ -75,6 +78,17 @@ impl AssetSource {
 
         Ok(())
     }
+
+    /// Convert the asset source into an asset path without attempting to validate
+    /// whether the asset path has valid syntax.
+    pub unsafe fn as_unvalidated_asset_path(&self) -> String {
+        match self {
+            AssetSource::Remote(uri) => String::from("rmf-server://") + uri,
+            AssetSource::Local(filename) => String::from("file://") + filename,
+            AssetSource::Search(name) => String::from("search://") + name,
+            AssetSource::Package(path) => String::from("package://") + path,
+        }
+    }
 }
 
 impl Default for AssetSource {
@@ -83,14 +97,19 @@ impl Default for AssetSource {
     }
 }
 
-impl From<&AssetSource> for String {
-    fn from(asset_source: &AssetSource) -> String {
-        match asset_source {
-            AssetSource::Remote(uri) => String::from("rmf-server://") + uri,
-            AssetSource::Local(filename) => String::from("file://") + filename,
-            AssetSource::Search(name) => String::from("search://") + name,
-            AssetSource::Package(path) => String::from("package://") + path,
-        }
+#[cfg(feature = "bevy")]
+impl TryFrom<&AssetSource> for String {
+    type Error = ParseAssetPathError;
+    fn try_from(asset_source: &AssetSource) -> Result<String, ParseAssetPathError> {
+        // SAFETY: After we get this string, we immediately validate it before
+        // returning it.
+        let result = unsafe { asset_source.as_unvalidated_asset_path() };
+
+        // Verify that the string can be parsed as an asset path before we
+        // return it.
+        AssetPath::try_parse(&result)
+            .map(|_| ()) // drop the borrowing of result
+            .map(|_| result)
     }
 }
 

rmf_site_format/src/legacy/building_map.rs

@@ -630,7 +630,11 @@ impl BuildingMap {
         let textures = textures
             .into_iter()
             .map(|(id, texture)| {
-                let name: String = (&texture.source).into();
+                // SAFETY: We're picking the string apart to automatically generate
+                // a name for the texture. We don't need to validate the syntax
+                // because what we produce here will only exist to be viewed by
+                // humans.
+                let name: String = unsafe { (&texture.source).as_unvalidated_asset_path() };
                 let name = Path::new(&name)
                     .file_stem()
                     .map(|s| s.to_str().map(|s| s.to_owned()))

rmf_site_format/src/workcell.rs

@@ -494,7 +494,10 @@ impl From<Geometry> for urdf_rs::Geometry {
     fn from(geometry: Geometry) -> Self {
         match geometry {
             Geometry::Mesh { source, scale } => urdf_rs::Geometry::Mesh {
-                filename: (&source).into(),
+                // SAFETY: We don't need to validate the syntax of the asset
+                // path because that will be done later when we attempt to load
+                // this as an asset.
+                filename: unsafe { (&source).as_unvalidated_asset_path() },
                 scale: scale.map(|v| urdf_rs::Vec3([v.x as f64, v.y as f64, v.z as f64])),
             },
             Geometry::Primitive(PrimitiveShape::Box { size: [x, y, z] }) => {