From 85748f4c95b69ff751685de29842f4e2020c89df Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 6 Apr 2026 10:41:03 +0000 Subject: [PATCH] Audit fixes: panic safety, path hardening, code quality - Replace 2 production unwrap() calls in workspace.rs with proper error handling to prevent panics on inconsistent state - Add AppState::save_config() helper to eliminate 11 duplicated save_to_file patterns in Tauri lib.rs - Log file watcher errors instead of silently swallowing them - Harden path traversal check in storage.rs: re-verify after canonicalization to catch symlink escapes - Add Windows reserved device name handling (CON, NUL, etc.) to sanitize_filename - Clean up stale .tmp files from interrupted atomic writes on startup All 107 core tests pass. https://claude.ai/code/session_01EnSrQsowc64rAwzD9BnJpc --- apps/tauri/src-tauri/src/lib.rs | 42 +++++++++-------- crates/onyx-cli/src/commands/workspace.rs | 8 +++- crates/onyx-core/src/storage.rs | 56 +++++++++++++++++++++-- 3 files changed, 79 insertions(+), 27 deletions(-) diff --git a/apps/tauri/src-tauri/src/lib.rs b/apps/tauri/src-tauri/src/lib.rs index e275932..b40718c 100644 --- a/apps/tauri/src-tauri/src/lib.rs +++ b/apps/tauri/src-tauri/src/lib.rs @@ -41,6 +41,13 @@ fn lock_state(state: &Mutex) -> Result Result<(), String> { + self.config.save_to_file(&self.config_path).map_err(|e| e.to_string()) + } +} + /// Validate that a workspace path is a reasonable directory and not a system path. fn validate_workspace_path(path: &str) -> Result<(), String> { let p = PathBuf::from(path); @@ -138,7 +145,7 @@ fn get_config(state: State<'_, Mutex>) -> Result { #[tauri::command] fn save_config(state: State<'_, Mutex>) -> Result<(), String> { let s = lock_state(&state)?; - s.config.save_to_file(&s.config_path).map_err(|e| e.to_string()) + s.save_config() } #[tauri::command] @@ -155,9 +162,7 @@ fn add_workspace( .set_current_workspace(id) .map_err(|e| e.to_string())?; s.repo = None; - s.config - .save_to_file(&s.config_path.clone()) - .map_err(|e| e.to_string()) + s.save_config() } #[tauri::command] @@ -170,9 +175,7 @@ fn set_current_workspace( .set_current_workspace(id) .map_err(|e| e.to_string())?; s.repo = None; - s.config - .save_to_file(&s.config_path.clone()) - .map_err(|e| e.to_string()) + s.save_config() } #[tauri::command] @@ -183,9 +186,7 @@ fn remove_workspace( let mut s = lock_state(&state)?; s.config.remove_workspace(&id); s.repo = None; - s.config - .save_to_file(&s.config_path.clone()) - .map_err(|e| e.to_string()) + s.save_config() } #[tauri::command] @@ -224,7 +225,7 @@ async fn rename_workspace( ws.path = new_path; } s.repo = None; - s.config.save_to_file(&s.config_path.clone()).map_err(|e| e.to_string())?; + s.save_config()?; } WorkspaceMode::Webdav => { // Rename the remote folder via WebDAV MOVE @@ -260,7 +261,7 @@ async fn rename_workspace( ws.webdav_path = Some(new_remote_path); } s.repo = None; - s.config.save_to_file(&s.config_path.clone()).map_err(|e| e.to_string())?; + s.save_config()?; } } @@ -521,9 +522,7 @@ fn set_webdav_config( if let Some(ws) = s.config.workspaces.get_mut(&workspace_id) { ws.webdav_url = Some(webdav_url); } - s.config - .save_to_file(&s.config_path.clone()) - .map_err(|e| e.to_string()) + s.save_config() } #[tauri::command] @@ -536,7 +535,7 @@ fn set_workspace_theme( if let Some(ws) = s.config.workspaces.get_mut(&workspace_id) { ws.theme = theme; } - s.config.save_to_file(&s.config_path.clone()).map_err(|e| e.to_string()) + s.save_config() } #[tauri::command] @@ -549,7 +548,7 @@ fn set_sync_interval( if let Some(ws) = s.config.workspaces.get_mut(&workspace_id) { ws.sync_interval_secs = interval_secs; } - s.config.save_to_file(&s.config_path.clone()).map_err(|e| e.to_string()) + s.save_config() } /// A remote folder entry returned to the frontend. @@ -693,7 +692,7 @@ fn add_webdav_workspace( .and_then(|rest| rest.split('/').next()) .unwrap_or("") .to_string(); - s.config.save_to_file(&s.config_path.clone()).map_err(|e| e.to_string())?; + s.save_config()?; drop(s); let creds = app_handle.state::>(); creds.store(&domain, &username, &password)?; @@ -785,7 +784,7 @@ async fn sync_workspace( if let Some(ws) = s.config.workspaces.get_mut(&workspace_id) { ws.last_sync = Some(Utc::now()); } - s.config.save_to_file(&s.config_path.clone()).map_err(|e| e.to_string())?; + s.save_config()?; } Ok(result.into()) @@ -803,7 +802,10 @@ fn start_watcher(handle: tauri::AppHandle, path: PathBuf) { let debouncer = new_debouncer( std::time::Duration::from_millis(500), move |events: Result, notify::Error>| { - let Ok(events) = events else { return }; + let Ok(events) = events else { + eprintln!("File watcher error: {:?}", events.unwrap_err()); + return; + }; // Only care about data file changes let has_data_change = events.iter().any(|e| { if e.kind != DebouncedEventKind::Any { return false; } diff --git a/crates/onyx-cli/src/commands/workspace.rs b/crates/onyx-cli/src/commands/workspace.rs index 5fc4925..ac10a94 100644 --- a/crates/onyx-cli/src/commands/workspace.rs +++ b/crates/onyx-cli/src/commands/workspace.rs @@ -126,7 +126,9 @@ pub fn retarget(name: String, path: String) -> Result<()> { let id = resolve_name(&config, &name)?; // Update path - config.workspaces.get_mut(&id).unwrap().path = path_buf.clone(); + config.workspaces.get_mut(&id) + .ok_or_else(|| anyhow::anyhow!("Workspace '{}' disappeared from config", name))? + .path = path_buf.clone(); save_config(&config)?; output::success(&format!("Workspace \"{}\" now points to {}", name, path_buf.display())); @@ -221,7 +223,9 @@ pub fn migrate(name: String, new_path: String) -> Result<()> { } // Update config - config.workspaces.get_mut(&id).unwrap().path = new_path_buf.clone(); + config.workspaces.get_mut(&id) + .ok_or_else(|| anyhow::anyhow!("Workspace '{}' disappeared from config", name))? + .path = new_path_buf.clone(); save_config(&config)?; output::success(&format!("Migrated {} items to {}", moved.len(), new_path_buf.display())); diff --git a/crates/onyx-core/src/storage.rs b/crates/onyx-core/src/storage.rs index 7369a69..c4af256 100644 --- a/crates/onyx-core/src/storage.rs +++ b/crates/onyx-core/src/storage.rs @@ -133,7 +133,33 @@ impl FileSystemStorage { if !root_path.exists() { return Err(Error::NotFound(format!("Path does not exist: {:?}", root_path))); } - Ok(Self { root_path }) + let storage = Self { root_path }; + storage.cleanup_stale_tmp_files(); + Ok(storage) + } + + /// Remove leftover .tmp files from interrupted atomic writes. + fn cleanup_stale_tmp_files(&self) { + let cleanup_dir = |dir: &Path| { + if let Ok(entries) = fs::read_dir(dir) { + for entry in entries.flatten() { + let path = entry.path(); + if path.extension().and_then(|e| e.to_str()) == Some("tmp") { + let _ = fs::remove_file(&path); + } + } + } + }; + // Clean root-level .tmp files + cleanup_dir(&self.root_path); + // Clean .tmp files inside list directories + if let Ok(entries) = fs::read_dir(&self.root_path) { + for entry in entries.flatten() { + if entry.path().is_dir() { + cleanup_dir(&entry.path()); + } + } + } } pub fn init(root_path: PathBuf) -> Result { @@ -181,11 +207,19 @@ impl FileSystemStorage { return Err(Error::InvalidData("Invalid list name: path traversal not allowed".to_string())); } let path = self.root_path.join(name); - // Verify resolved path stays within root + // Verify resolved path stays within root. + // Always build canonical_path from canonical_root + filename to avoid TOCTOU + // races and symlink escapes (canonicalize resolves symlinks, so a symlink + // pointing outside the workspace would be caught). let canonical_root = self.root_path.canonicalize() .map_err(Error::Io)?; let canonical_path = if path.exists() { - path.canonicalize().map_err(Error::Io)? + let resolved = path.canonicalize().map_err(Error::Io)?; + // Re-check after canonicalization to catch symlinks pointing outside + if !resolved.starts_with(&canonical_root) { + return Err(Error::InvalidData("Invalid list name: path escapes workspace".to_string())); + } + resolved } else { // Parent must exist and be canonicalizable (it's root_path) canonical_root.join(path.file_name().unwrap_or_default()) @@ -197,7 +231,7 @@ impl FileSystemStorage { } fn sanitize_filename(name: &str) -> String { - name.chars() + let sanitized: String = name.chars() .map(|c| match c { '/' | '\\' | ':' | '*' | '?' | '"' | '<' | '>' | '|' => '_', '\0'..='\x1f' => '_', @@ -205,7 +239,19 @@ impl FileSystemStorage { }) .collect::() .trim_matches(|c: char| c == '.' || c == ' ') - .to_string() + .to_string(); + // Reject Windows reserved device names (CON, PRN, AUX, NUL, COM0-9, LPT0-9) + let stem = sanitized.split('.').next().unwrap_or("").to_uppercase(); + let is_reserved = matches!(stem.as_str(), + "CON" | "PRN" | "AUX" | "NUL" + | "COM0" | "COM1" | "COM2" | "COM3" | "COM4" | "COM5" | "COM6" | "COM7" | "COM8" | "COM9" + | "LPT0" | "LPT1" | "LPT2" | "LPT3" | "LPT4" | "LPT5" | "LPT6" | "LPT7" | "LPT8" | "LPT9" + ); + if is_reserved { + format!("_{}", sanitized) + } else { + sanitized + } } fn task_file_path(&self, list_dir: &Path, task: &Task) -> PathBuf {