🛡️ Sentinel: [CRITICAL] Fix path traversal in snapshot commands

.jules/sentinel.md

@@ -7,3 +7,7 @@
 **Vulnerability:** The `maybe_write_text` utility function was using `std::fs::write`, which resulted in sensitive data (like PSBT files and offers) being saved with insecure default file permissions, making them readable by other users on a shared system.
 **Learning:** Even generic utility functions used for saving user-requested command outputs must use secure file permissions (`0o600`) if the data they handle (like PSBTs and offers) is sensitive.
 **Prevention:** Always use `crate::paths::write_secure_file` instead of `std::fs::write` for all file writing operations that might contain sensitive material in this codebase.
+## 2024-05-03 - Prevent Path Traversal in Snapshot Commands
+**Vulnerability:** The snapshot command constructs file paths using `snap_dir.join(format!("{name}.json"))` where `name` is directly sourced from user input (CLI argument). If the user provides a path starting with `/` or `../`, `Path::join` resolves it absolutely or traverses up, allowing arbitrary file read/write across the system instead of isolating it within `snap_dir`.
+**Learning:** In Rust, `Path::join` replaces the base path if the appended segment is absolute (e.g., `/tmp/malicious`). Unvalidated user inputs used in path construction can lead to severe path traversal vulnerabilities.
+**Prevention:** Implement strict file name validation functions ensuring that user-provided names consist only of safe characters (e.g., alphanumeric, underscores, dashes) before constructing file paths.

src/commands/snapshot.rs

@@ -1,6 +1,7 @@
 use crate::cli::{Cli, SnapshotAction, SnapshotArgs};
 use crate::error::AppError;
 use crate::output::CommandOutput;
+use crate::utils::validate_file_name;
 use crate::{confirm, profile_path, read_profile, snapshot_dir, write_bytes_atomic};
 use std::fs;
 
@@ -12,6 +13,7 @@ pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result<CommandOutput, AppErr
 
     match &args.action {
         SnapshotAction::Save { name, overwrite } => {
+            validate_file_name(name)?;
             let source = read_profile(&profile_path)?;
             let destination = snap_dir.join(format!("{name}.json"));
             if destination.exists() && !(*overwrite || cli.yes) {
@@ -27,6 +29,7 @@ pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result<CommandOutput, AppErr
             })
         }
         SnapshotAction::Restore { name } => {
+            validate_file_name(name)?;
             if !confirm(&format!("Are you sure you want to restore snapshot '{name}'? This will overwrite your current profile."), cli) {
                 return Err(AppError::Internal("aborted by user".to_string()));
             }

src/utils.rs

@@ -3,6 +3,21 @@ use crate::error::AppError;
 use std::env;
 use std::path::{Path, PathBuf};
 
+pub fn validate_file_name(name: &str) -> Result<(), AppError> {
+    if name.is_empty() {
+        return Err(AppError::Invalid("file name cannot be empty".to_string()));
+    }
+    for c in name.chars() {
+        if !c.is_ascii_alphanumeric() && c != '_' && c != '-' {
+            return Err(AppError::Invalid(format!(
+                "invalid character in file name: '{}'",
+                c
+            )));
+        }
+    }
+    Ok(())
+}
+
 pub fn home_dir() -> PathBuf {
     if let Some(home) = std::env::var_os("HOME") {
         PathBuf::from(home)
@@ -219,3 +234,37 @@ pub fn parse_indices(s: Option<&str>) -> Result<Vec<usize>, AppError> {
     }
     Ok(indices)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::error::AppError;
+
+    #[test]
+    fn test_validate_file_name() {
+        assert!(validate_file_name("valid_name-123").is_ok());
+        assert!(validate_file_name("SNAPSHOT_name").is_ok());
+
+        assert!(matches!(validate_file_name(""), Err(AppError::Invalid(_))));
+        assert!(matches!(
+            validate_file_name("../passwd"),
+            Err(AppError::Invalid(_))
+        ));
+        assert!(matches!(
+            validate_file_name("/etc/passwd"),
+            Err(AppError::Invalid(_))
+        ));
+        assert!(matches!(
+            validate_file_name("foo.bar"),
+            Err(AppError::Invalid(_))
+        ));
+        assert!(matches!(
+            validate_file_name("a/b"),
+            Err(AppError::Invalid(_))
+        ));
+        assert!(matches!(
+            validate_file_name("invalid chars!"),
+            Err(AppError::Invalid(_))
+        ));
+    }
+}