Reject non-ULID IDs in cache actionlog read command. (#2499)

Author: fnando

cmd/soroban-cli/src/commands/cache/actionlog/read.rs

@@ -1,4 +1,4 @@
-use std::{fs, io, path::PathBuf};
+use std::io;
 
 use crate::config::{data, locator};
 
@@ -10,8 +10,10 @@ pub enum Error {
     Data(#[from] data::Error),
     #[error("failed to find cache entry {0}")]
     NotFound(String),
+    #[error("invalid cache entry ID \"{0}\": expected a ULID")]
+    InvalidId(String),
     #[error(transparent)]
-    SerdeJson(#[from] serde_json::Error),
+    Io(#[from] std::io::Error),
 }
 
 #[derive(Debug, clap::Parser, Clone)]
@@ -24,15 +26,72 @@ pub struct Cmd {
 
 impl Cmd {
     pub fn run(&self) -> Result<(), Error> {
-        let file = self.file()?;
+        let id: ulid::Ulid = self
+            .id
+            .parse()
+            .map_err(|_| Error::InvalidId(self.id.clone()))?;
+        let file = data::actions_dir()?
+            .join(id.to_string())
+            .with_extension("json");
         tracing::debug!("reading file {}", file.display());
-        let mut file = fs::File::open(file).map_err(|_| Error::NotFound(self.id.clone()))?;
-        let mut stdout = io::stdout();
-        let _ = io::copy(&mut file, &mut stdout);
+        let mut f = std::fs::File::open(&file).map_err(|e| {
+            if e.kind() == io::ErrorKind::NotFound {
+                Error::NotFound(self.id.clone())
+            } else {
+                Error::Io(e)
+            }
+        })?;
+        io::copy(&mut f, &mut io::stdout())?;
         Ok(())
     }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::test_utils::EnvGuard;
+    use serial_test::serial;
+
+    #[test]
+    #[serial]
+    fn path_traversal_via_dotdot_is_rejected() {
+        let tmp = tempfile::tempdir().unwrap();
+        let _guard = EnvGuard::set("STELLAR_DATA_HOME", tmp.path());
+
+        let outside = tmp.path().join("outside.json");
+        std::fs::write(&outside, r#"{"leaked":true}"#).unwrap();
+
+        let cmd = Cmd {
+            id: "../outside".to_string(),
+        };
+
+        let err = cmd.run().expect_err("expected error for path-traversal ID");
+        assert!(
+            matches!(err, Error::InvalidId(_)),
+            "expected InvalidId, got {err:?}"
+        );
+    }
+
+    #[test]
+    #[serial]
+    fn absolute_path_id_is_rejected() {
+        let tmp = tempfile::tempdir().unwrap();
+        let _guard = EnvGuard::set("STELLAR_DATA_HOME", tmp.path());
+
+        let outside = tmp.path().join("outside.json");
+        std::fs::write(&outside, r#"{"leaked":true}"#).unwrap();
+
+        let abs_id = outside
+            .to_str()
+            .unwrap()
+            .trim_end_matches(".json")
+            .to_string();
+        let cmd = Cmd { id: abs_id };
 
-    pub fn file(&self) -> Result<PathBuf, Error> {
-        Ok(data::actions_dir()?.join(&self.id).with_extension("json"))
+        let err = cmd.run().expect_err("expected error for absolute-path ID");
+        assert!(
+            matches!(err, Error::InvalidId(_)),
+            "expected InvalidId, got {err:?}"
+        );
     }
 }

cmd/soroban-cli/src/config/locator.rs

@@ -888,28 +888,7 @@ mod tests {
         );
     }
 
-    struct EnvGuard(Vec<(String, Option<String>)>);
-
-    impl EnvGuard {
-        fn new(vars: &[&str]) -> Self {
-            let saved = vars
-                .iter()
-                .map(|k| (k.to_string(), std::env::var(k).ok()))
-                .collect();
-            Self(saved)
-        }
-    }
-
-    impl Drop for EnvGuard {
-        fn drop(&mut self) {
-            for (k, v) in &self.0 {
-                match v {
-                    Some(val) => std::env::set_var(k, val),
-                    None => std::env::remove_var(k),
-                }
-            }
-        }
-    }
+    use crate::test_utils::EnvGuard;
 
     #[test]
     #[serial]

cmd/soroban-cli/src/lib.rs

@@ -27,6 +27,9 @@ pub mod upgrade_check;
 pub mod utils;
 pub mod wasm;
 
+#[cfg(test)]
+pub mod test_utils;
+
 pub use commands::Root;
 
 pub fn parse_cmd<T>(s: &str) -> Result<T, clap::Error>

cmd/soroban-cli/src/test_utils.rs

@@ -0,0 +1,34 @@
+/// Saves and restores environment variables on drop.
+///
+/// Useful in tests that mutate env vars — guarantees cleanup even on panic.
+pub struct EnvGuard(Vec<(String, Option<String>)>);
+
+impl EnvGuard {
+    pub fn new(vars: &[&str]) -> Self {
+        let saved = vars
+            .iter()
+            .map(|k| (k.to_string(), std::env::var(k).ok()))
+            .collect();
+        for k in vars {
+            std::env::remove_var(k);
+        }
+        Self(saved)
+    }
+
+    pub fn set(key: &'static str, val: &std::path::Path) -> Self {
+        let prev = std::env::var(key).ok();
+        std::env::set_var(key, val);
+        Self(vec![(key.to_string(), prev)])
+    }
+}
+
+impl Drop for EnvGuard {
+    fn drop(&mut self) {
+        for (k, v) in &self.0 {
+            match v {
+                Some(val) => std::env::set_var(k, val),
+                None => std::env::remove_var(k),
+            }
+        }
+    }
+}