fix: harden core data integrity — move_task rollback, path traversal, migration safety

Add rollback to move_task: if delete-from-source fails after write-to-
destination, clean up the duplicate. Reject list names with path separators
or '..' to prevent traversal; canonicalize() failures now return errors
instead of silently falling back to unchecked paths. Add validation and
rollback to CLI workspace migration: check destination is empty, track
moved files, and reverse on failure.

Cargo.lock

@@ -1000,6 +1000,7 @@ dependencies = [
  "tokio",
  "uuid",
  "wiremock",
+ "zeroize",
 ]
 
 [[package]]

crates/onyx-cli/src/commands/workspace.rs

@@ -168,33 +168,59 @@ pub fn migrate(name: String, new_path: String) -> Result<()> {
         return Ok(());
     }
 
+    // Validate destination
+    if old_path == new_path_buf {
+        anyhow::bail!("Source and destination paths are the same");
+    }
+    if new_path_buf.exists() && new_path_buf.read_dir()?.next().is_some() {
+        anyhow::bail!("Destination directory '{}' already contains files", new_path_buf.display());
+    }
+
     // Create destination directory
     std::fs::create_dir_all(&new_path_buf)?;
 
-    // Move files
+    // Move files, tracking what was moved for rollback
     output::info("Moving files...");
-    let entries = std::fs::read_dir(&old_path)?;
-    let mut count = 0;
+    let entries: Vec<_> = std::fs::read_dir(&old_path)?
+        .collect::<std::result::Result<Vec<_>, _>>()?;
+    let mut moved: Vec<(std::path::PathBuf, std::path::PathBuf)> = Vec::new();
 
-    for entry in entries {
-        let entry = entry?;
-        let file_name = entry.file_name();
-        let dest = new_path_buf.join(&file_name);
+    let move_result: Result<()> = (|| {
+        for entry in &entries {
+            let file_name = entry.file_name();
+            let dest = new_path_buf.join(&file_name);
 
-        if entry.path().is_dir() {
-            let mut options = fs_extra::dir::CopyOptions::new();
-            options.copy_inside = true;
-            fs_extra::dir::move_dir(entry.path(), &new_path_buf, &options)?;
-            output::item(&format!("Moved {}/", file_name.to_string_lossy()));
-        } else {
-            std::fs::rename(entry.path(), dest)?;
+            if entry.path().is_dir() {
+                let mut options = fs_extra::dir::CopyOptions::new();
+                options.copy_inside = true;
+                fs_extra::dir::move_dir(entry.path(), &new_path_buf, &options)?;
+            } else {
+                std::fs::rename(entry.path(), &dest)?;
+            }
+            moved.push((entry.path(), dest));
             output::item(&format!("Moved {}", file_name.to_string_lossy()));
         }
-        count += 1;
+        Ok(())
+    })();
+
+    if let Err(e) = move_result {
+        output::error(&format!("Migration failed: {}. Rolling back...", e));
+        for (src, dest) in moved.into_iter().rev() {
+            if dest.exists() {
+                if dest.is_dir() {
+                    let mut options = fs_extra::dir::CopyOptions::new();
+                    options.copy_inside = true;
+                    let _ = fs_extra::dir::move_dir(&dest, &old_path, &options);
+                } else {
+                    let _ = std::fs::rename(&dest, &src);
+                }
+            }
+        }
+        anyhow::bail!("Migration failed and was rolled back: {}", e);
     }
 
     // Remove old directory if empty
-    if old_path.read_dir()?.next().is_none() {
+    if old_path.exists() && old_path.read_dir()?.next().is_none() {
         std::fs::remove_dir(&old_path)?;
     }
 
@@ -202,7 +228,7 @@ pub fn migrate(name: String, new_path: String) -> Result<()> {
     config.add_workspace(name.clone(), WorkspaceConfig::new(new_path_buf.clone()));
     save_config(&config)?;
 
-    output::success(&format!("Migrated {} items to {}", count, new_path_buf.display()));
+    output::success(&format!("Migrated {} items to {}", moved.len(), new_path_buf.display()));
     output::success(&format!("Workspace \"{}\" now points to {}", name, new_path_buf.display()));
 
     Ok(())

crates/onyx-core/src/repository.rs

@@ -75,7 +75,11 @@ impl TaskRepository {
     pub fn move_task(&mut self, from_list_id: Uuid, to_list_id: Uuid, task_id: Uuid) -> Result<()> {
         let task = self.storage.read_task(from_list_id, task_id)?;
         self.storage.write_task(to_list_id, &task)?;
-        self.storage.delete_task(from_list_id, task_id)?;
+        // If delete from source fails, roll back by removing the copy from destination
+        if let Err(e) = self.storage.delete_task(from_list_id, task_id) {
+            let _ = self.storage.delete_task(to_list_id, task_id);
+            return Err(e);
+        }
         Ok(())
     }
 

crates/onyx-core/src/storage.rs

@@ -150,27 +150,22 @@ impl FileSystemStorage {
     }
 
     fn list_dir_path_by_name(&self, name: &str) -> Result<PathBuf> {
+        // Reject names containing path separators or traversal components
+        if name.contains('/') || name.contains('\\') || name == ".." || name.starts_with("../") || name.starts_with("..\\") {
+            return Err(Error::InvalidData("Invalid list name: path traversal not allowed".to_string()));
+        }
         let path = self.root_path.join(name);
-        // Prevent path traversal: resolved path must stay within root
+        // Verify resolved path stays within root
         let canonical_root = self.root_path.canonicalize()
-            .unwrap_or_else(|_| self.root_path.clone());
+            .map_err(Error::Io)?;
         let canonical_path = if path.exists() {
-            path.canonicalize().unwrap_or_else(|_| path.clone())
+            path.canonicalize().map_err(Error::Io)?
         } else {
-            // For non-existent paths, normalize by resolving the parent
-            if let Some(parent) = path.parent() {
-                let canonical_parent = if parent.exists() {
-                    parent.canonicalize().unwrap_or_else(|_| parent.to_path_buf())
-                } else {
-                    parent.to_path_buf()
-                };
-                canonical_parent.join(path.file_name().unwrap_or_default())
-            } else {
-                path.clone()
-            }
+            // Parent must exist and be canonicalizable (it's root_path)
+            canonical_root.join(path.file_name().unwrap_or_default())
         };
         if !canonical_path.starts_with(&canonical_root) {
-            return Err(Error::InvalidData(format!("Invalid list name: path escapes workspace")));
+            return Err(Error::InvalidData("Invalid list name: path escapes workspace".to_string()));
         }
         Ok(path)
     }