🛡️ Sentinel: [CRITICAL] Fix path traversal in profile and snapshot paths

.jules/sentinel.md

@@ -7,3 +7,8 @@
 **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-04-13 - Path Traversal on Profile and Snapshot Names
+**Vulnerability:** The application was constructing file paths for profiles (`profiles/{profile_name}.json`) and snapshots (`snapshots/{profile_name}/{snapshot_name}.json`) without sanitizing or validating the `profile_name` or `snapshot_name` variables. This could allow an attacker to read, write, or overwrite arbitrary files on the system using path traversal payloads like `../../`.
+**Learning:** Any user-provided or configurable string that is used to construct a file path must be strictly validated to prevent path traversal, even if the application otherwise handles the actual file read/write operations securely.
+**Prevention:** Use a dedicated validation function (e.g., `crate::utils::validate_file_name`) to enforce a strict allowlist of characters (such as alphanumeric, underscores, and dashes) on file and directory names before using them in `Path::join` operations.

src/commands/snapshot.rs

@@ -7,11 +7,10 @@ use std::fs;
 pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result<CommandOutput, AppError> {
     let profile_path = profile_path(cli)?;
     let snap_dir = snapshot_dir(cli)?;
-    fs::create_dir_all(&snap_dir)
-        .map_err(|e| AppError::Config(format!("failed to create snapshot dir: {e}")))?;
 
     match &args.action {
         SnapshotAction::Save { name, overwrite } => {
+            crate::utils::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 +26,7 @@ pub async fn run(cli: &Cli, args: &SnapshotArgs) -> Result<CommandOutput, AppErr
             })
         }
         SnapshotAction::Restore { name } => {
+            crate::utils::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/paths.rs

@@ -56,6 +56,7 @@ pub fn home_dir() -> PathBuf {
 }
 
 pub fn profile_path(config: &crate::config::ServiceConfig<'_>) -> Result<PathBuf, AppError> {
+    crate::utils::validate_file_name(config.profile)?;
     let root = data_dir(config);
     let profiles = root.join("profiles");
     if !profiles.exists() {
@@ -70,6 +71,7 @@ pub fn profile_lock_path(config: &crate::config::ServiceConfig<'_>) -> Result<Pa
 }
 
 pub fn snapshot_dir(config: &crate::config::ServiceConfig<'_>) -> Result<PathBuf, AppError> {
+    crate::utils::validate_file_name(config.profile)?;
     let root = data_dir(config);
     let directory = root.join("snapshots").join(config.profile);
     create_secure_dir_all(&directory)

src/utils.rs

@@ -219,3 +219,19 @@ pub fn parse_indices(s: Option<&str>) -> Result<Vec<usize>, AppError> {
     }
     Ok(indices)
 }
+
+pub fn validate_file_name(name: &str) -> Result<(), AppError> {
+    if name.is_empty() {
+        return Err(AppError::Invalid("file name cannot be empty".to_string()));
+    }
+    if !name
+        .chars()
+        .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
+    {
+        return Err(AppError::Invalid(
+            "file name can only contain alphanumeric characters, underscores, and dashes"
+                .to_string(),
+        ));
+    }
+    Ok(())
+}