From fa1125bfebd026fc01e86d9f4bcf9e0141ef771a Mon Sep 17 00:00:00 2001 From: Tristan Michael Date: Thu, 2 Apr 2026 08:58:27 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20harden=20core=20data=20integrity=20?= =?UTF-8?q?=E2=80=94=20move=5Ftask=20rollback,=20path=20traversal,=20migra?= =?UTF-8?q?tion=20safety?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 1 + crates/onyx-cli/src/commands/workspace.rs | 60 ++++++++++++++++------- crates/onyx-core/src/repository.rs | 6 ++- crates/onyx-core/src/storage.rs | 25 ++++------ 4 files changed, 59 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 194cd5f..b55af2b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1000,6 +1000,7 @@ dependencies = [ "tokio", "uuid", "wiremock", + "zeroize", ] [[package]] diff --git a/crates/onyx-cli/src/commands/workspace.rs b/crates/onyx-cli/src/commands/workspace.rs index 4467752..cdf8b18 100644 --- a/crates/onyx-cli/src/commands/workspace.rs +++ b/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::, _>>()?; + 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(()) diff --git a/crates/onyx-core/src/repository.rs b/crates/onyx-core/src/repository.rs index 3ba3ea2..2298cc0 100644 --- a/crates/onyx-core/src/repository.rs +++ b/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(()) } diff --git a/crates/onyx-core/src/storage.rs b/crates/onyx-core/src/storage.rs index 9a41991..fec62dc 100644 --- a/crates/onyx-core/src/storage.rs +++ b/crates/onyx-core/src/storage.rs @@ -150,27 +150,22 @@ impl FileSystemStorage { } fn list_dir_path_by_name(&self, name: &str) -> Result { + // 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) }