🛡️ Sentinel: [CRITICAL] Fix Path Traversal in Snapshot and Idempotency Paths

.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-04-10 - Missing Path Validation on Snapshot and Idempotency Features
+**Vulnerability:** User-provided string parameters (`name` in snapshot save/restore, `profile` in context, and `idempotency_key`) were used directly in `Path::join()` to build file paths without character validation, allowing arbitrary filesystem read/write (Path Traversal via `../`).
+**Learning:** Functions that accept strings used for file paths must validate the contents of the string against a strict allowlist.
+**Prevention:** A `validate_file_name` function was added checking for `c.is_ascii_alphanumeric() || c == '_' || c == '-'`. Any user-provided identifier used as a filename component should be passed through this validation prior to use.

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/main.rs

@@ -19,6 +19,7 @@ mod wizard;
 use crate::cli::{Cli, Command, PolicyMode};
 use crate::config::*;
 use crate::error::AppError;
+use crate::utils::validate_file_name;
 use crate::utils::{env_bool, env_non_empty};
 use clap::Parser;
 use is_terminal::IsTerminal;
@@ -741,13 +742,12 @@ fn command_fingerprint(cli: &Cli) -> String {
     format!("{:016x}", hasher.finish())
 }
 
-fn idempotency_store_path(cli: &Cli) -> PathBuf {
-    crate::wallet_service::data_dir(&service_config(cli))
+fn idempotency_store_path(cli: &Cli) -> Result<PathBuf, AppError> {
+    let profile = cli.profile.as_deref().unwrap_or("default");
+    validate_file_name(profile)?;
+    Ok(crate::wallet_service::data_dir(&service_config(cli))
         .join("idempotency")
-        .join(format!(
-            "{}.json",
-            cli.profile.as_deref().unwrap_or("default")
-        ))
+        .join(format!("{}.json", profile)))
 }
 
 fn load_idempotency_store(path: &Path) -> Result<IdempotencyStore, AppError> {
@@ -791,7 +791,7 @@ fn try_replay_idempotent_result(
         return Ok(None);
     }
 
-    let path = idempotency_store_path(cli);
+    let path = idempotency_store_path(cli)?;
     let store = load_idempotency_store(&path)?;
     let Some(entry) = store.entries.get(key) else {
         return Ok(None);
@@ -824,7 +824,8 @@ fn record_idempotent_result(
         Some(k) => k.to_string(),
         None => return Ok(now_unix_ms()),
     };
-    let path = idempotency_store_path(cli);
+    validate_file_name(&key)?;
+    let path = idempotency_store_path(cli)?;
     let mut store = load_idempotency_store(&path)?;
     let fingerprint = command_fingerprint(cli);
 

src/utils.rs

@@ -3,6 +3,20 @@ 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()
+        || !name
+            .chars()
+            .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
+    {
+        return Err(AppError::Invalid(format!(
+            "invalid file name '{}': must contain only alphanumeric characters, underscores, and dashes",
+            name
+        )));
+    }
+    Ok(())
+}
+
 pub fn home_dir() -> PathBuf {
     if let Some(home) = std::env::var_os("HOME") {
         PathBuf::from(home)