From 7eec8e22c8ed930d68aeb7a8d03be31c4d0f03e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 6 Apr 2026 11:09:35 +0000 Subject: [PATCH] Add tests for audit fixes: 15 new tests covering security and data integrity New sync tests: - validate_sync_path rejects backslashes and .. components - validate_sync_path allows valid paths - SyncLock prevents concurrent access - SyncLock released on drop, lock file cleaned up - SyncState save leaves no .tmp files - OfflineQueue save leaves no .tmp files - parse_frontmatter_for_conflict rejects oversized YAML (>64KB) New storage tests: - Deduplication with equal versions uses mtime tiebreaker - Frontmatter parsing rejects oversized YAML (>64KB) - Frontmatter parsing accepts normal-sized content - Version saturates at u64::MAX (no panic or wrap) - Delete task removes from metadata before file - atomic_write leaves no .tmp files https://claude.ai/code/session_01AJoK28N4vqLqzskq6ybGri --- crates/onyx-core/src/storage.rs | 143 +++++++++++++++++++++++++++++++- crates/onyx-core/src/sync.rs | 109 ++++++++++++++++++++++++ 2 files changed, 249 insertions(+), 3 deletions(-) diff --git a/crates/onyx-core/src/storage.rs b/crates/onyx-core/src/storage.rs index cef6487..e829bcc 100644 --- a/crates/onyx-core/src/storage.rs +++ b/crates/onyx-core/src/storage.rs @@ -1032,12 +1032,149 @@ mod tests { let tasks = storage.list_tasks(list.id).unwrap(); assert_eq!(tasks.len(), 1); assert_eq!(tasks[0].id, task_id); - // The winner should be the one written by write_task (version 1), not the manually created stale copy (also version 1 but alphabetically second) - // Actually both are version 1, so the first sorted wins — but the stale file should be cleaned up - // Let's verify only one .md file remains + // Both are version 1, so mtime tiebreaker picks the most recent file. + // Verify only one .md file remains. let md_count = fs::read_dir(&list_dir).unwrap() .filter(|e| e.as_ref().unwrap().path().extension().and_then(|s| s.to_str()) == Some("md")) .count(); assert_eq!(md_count, 1); } + + // --- Deduplication mtime tiebreaker --- + + #[test] + fn test_dedup_equal_versions_uses_mtime_tiebreaker() { + let temp_dir = TempDir::new().unwrap(); + let mut storage = init_storage(&temp_dir); + let list = storage.create_list("Dedup2".to_string()).unwrap(); + + let task_id = uuid::Uuid::new_v4(); + let list_dir = storage.list_dir_path(list.id).unwrap(); + + // Create two files with the same UUID and same version (1) + let content_a = format!( + "---\nid: {}\nstatus: backlog\nversion: 1\n---\n\nVersion A (older)", + task_id + ); + let content_b = format!( + "---\nid: {}\nstatus: backlog\nversion: 1\n---\n\nVersion B (newer)", + task_id + ); + + let path_a = list_dir.join("TaskA.md"); + let path_b = list_dir.join("TaskB.md"); + fs::write(&path_a, &content_a).unwrap(); + // Sleep briefly so mtime differs + std::thread::sleep(std::time::Duration::from_millis(50)); + fs::write(&path_b, &content_b).unwrap(); + + let tasks = storage.list_tasks(list.id).unwrap(); + assert_eq!(tasks.len(), 1); + assert_eq!(tasks[0].id, task_id); + // The newer file (B) should win the mtime tiebreaker + assert_eq!(tasks[0].description, "Version B (newer)"); + + // Verify only one .md file remains + let md_count = fs::read_dir(&list_dir).unwrap() + .filter(|e| e.as_ref().unwrap().path().extension().and_then(|s| s.to_str()) == Some("md")) + .count(); + assert_eq!(md_count, 1); + } + + // --- Frontmatter size limit --- + + #[test] + fn test_parse_frontmatter_rejects_oversized() { + let temp_dir = TempDir::new().unwrap(); + let storage = init_storage(&temp_dir); + + // Create content with frontmatter larger than 64KB + let huge = "x".repeat(70_000); + let content = format!( + "---\nid: 550e8400-e29b-41d4-a716-446655440000\nstatus: backlog\nversion: 1\nhuge: {}\n---\n\nBody", + huge + ); + let result = storage.parse_markdown_with_frontmatter(&content); + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("too large"), "Error should mention size: {}", err); + } + + #[test] + fn test_parse_frontmatter_accepts_normal_size() { + let temp_dir = TempDir::new().unwrap(); + let storage = init_storage(&temp_dir); + + let content = "---\nid: 550e8400-e29b-41d4-a716-446655440000\nstatus: backlog\nversion: 1\n---\n\nDescription"; + let result = storage.parse_markdown_with_frontmatter(content); + assert!(result.is_ok()); + } + + // --- Version saturating_add --- + + #[test] + fn test_version_saturates_at_max() { + let temp_dir = TempDir::new().unwrap(); + let mut storage = init_storage(&temp_dir); + let list = storage.create_list("MaxVer".to_string()).unwrap(); + + let mut task = Task::new("Saturate".to_string()); + task.version = u64::MAX - 1; + + storage.write_task(list.id, &task).unwrap(); + let read_back = storage.read_task(list.id, task.id).unwrap(); + assert_eq!(read_back.version, u64::MAX, "Version should saturate at u64::MAX"); + + // Writing again should not panic or wrap + storage.write_task(list.id, &read_back).unwrap(); + let read_again = storage.read_task(list.id, task.id).unwrap(); + assert_eq!(read_again.version, u64::MAX, "Version should stay at u64::MAX"); + } + + // --- Delete ordering: metadata before file --- + + #[test] + fn test_delete_task_removes_from_metadata_first() { + let temp_dir = TempDir::new().unwrap(); + let mut storage = init_storage(&temp_dir); + let list = storage.create_list("DelOrder".to_string()).unwrap(); + + let task = Task::new("ToDelete".to_string()); + let task_id = task.id; + storage.write_task(list.id, &task).unwrap(); + + // Verify task is in metadata + let meta = storage.read_list_metadata(list.id).unwrap(); + assert!(meta.task_order.contains(&task_id)); + + // Delete + storage.delete_task(list.id, task_id).unwrap(); + + // Verify metadata no longer contains the task + let meta_after = storage.read_list_metadata(list.id).unwrap(); + assert!(!meta_after.task_order.contains(&task_id)); + + // Verify file is also gone + let list_dir = storage.list_dir_path(list.id).unwrap(); + let md_count = fs::read_dir(&list_dir).unwrap() + .filter(|e| e.as_ref().unwrap().path().extension().and_then(|s| s.to_str()) == Some("md")) + .count(); + assert_eq!(md_count, 0); + } + + // --- Atomic write no leftover tmp --- + + #[test] + fn test_atomic_write_no_leftover_tmp() { + let temp_dir = TempDir::new().unwrap(); + let target = temp_dir.path().join("test.json"); + atomic_write(&target, b"hello").unwrap(); + + let tmp_files: Vec<_> = fs::read_dir(temp_dir.path()).unwrap() + .filter_map(|e| e.ok()) + .filter(|e| e.path().extension().and_then(|s| s.to_str()) == Some("tmp")) + .collect(); + assert!(tmp_files.is_empty(), "No .tmp files should remain after atomic_write"); + assert_eq!(fs::read_to_string(&target).unwrap(), "hello"); + } } diff --git a/crates/onyx-core/src/sync.rs b/crates/onyx-core/src/sync.rs index 8c2f7c9..b4afcc6 100644 --- a/crates/onyx-core/src/sync.rs +++ b/crates/onyx-core/src/sync.rs @@ -10,6 +10,7 @@ use crate::webdav::WebDavClient; /// File-based lock to prevent concurrent sync operations on the same workspace. /// The lock file is automatically removed on drop. +#[derive(Debug)] struct SyncLock { path: PathBuf, } @@ -1317,4 +1318,112 @@ mod tests { assert_eq!(path_parent("file.md"), None); assert_eq!(path_parent("a/b/c.md"), Some("a/b")); } + + // --- validate_sync_path tests --- + + #[test] + fn test_validate_sync_path_rejects_dotdot() { + assert!(validate_sync_path("../etc/passwd").is_err()); + assert!(validate_sync_path("list/../secret.md").is_err()); + } + + #[test] + fn test_validate_sync_path_rejects_backslash() { + assert!(validate_sync_path("list\\..\\secret.md").is_err()); + assert!(validate_sync_path("list\\file.md").is_err()); + assert!(validate_sync_path("a\\b").is_err()); + } + + #[test] + fn test_validate_sync_path_allows_valid_paths() { + assert!(validate_sync_path("list/task.md").is_ok()); + assert!(validate_sync_path(".onyx-workspace.json").is_ok()); + assert!(validate_sync_path("My Tasks/.listdata.json").is_ok()); + } + + // --- SyncLock tests --- + + #[test] + fn test_sync_lock_prevents_concurrent_access() { + let temp_dir = TempDir::new().unwrap(); + let _lock1 = SyncLock::acquire(temp_dir.path()).unwrap(); + let result = SyncLock::acquire(temp_dir.path()); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("already in progress")); + } + + #[test] + fn test_sync_lock_released_on_drop() { + let temp_dir = TempDir::new().unwrap(); + { + let _lock = SyncLock::acquire(temp_dir.path()).unwrap(); + // lock held here + } + // lock dropped — should be able to acquire again + let _lock2 = SyncLock::acquire(temp_dir.path()).unwrap(); + } + + #[test] + fn test_sync_lock_file_cleaned_up_on_drop() { + let temp_dir = TempDir::new().unwrap(); + let lock_path = temp_dir.path().join(".sync.lock"); + { + let _lock = SyncLock::acquire(temp_dir.path()).unwrap(); + assert!(lock_path.exists(), "Lock file should exist while held"); + } + assert!(!lock_path.exists(), "Lock file should be removed after drop"); + } + + // --- Sync state temp cleanup --- + + #[test] + fn test_sync_state_save_load_no_leftover_tmp() { + let temp_dir = TempDir::new().unwrap(); + let state = SyncState { + last_sync: Some(Utc::now()), + files: HashMap::new(), + }; + state.save(temp_dir.path()).unwrap(); + + // Verify no .tmp files remain + let tmp_files: Vec<_> = std::fs::read_dir(temp_dir.path()).unwrap() + .filter_map(|e| e.ok()) + .filter(|e| e.path().extension().and_then(|s| s.to_str()) == Some("tmp")) + .collect(); + assert!(tmp_files.is_empty(), "No .tmp files should remain after save"); + } + + // --- Queue save no leftover tmp --- + + #[test] + fn test_queue_save_no_leftover_tmp() { + let temp_dir = TempDir::new().unwrap(); + let queue = OfflineQueue { + operations: vec![QueuedOperation { + action_type: "upload".to_string(), + path: "test.md".to_string(), + queued_at: Utc::now(), + }], + }; + queue.save(temp_dir.path()).unwrap(); + + let tmp_files: Vec<_> = std::fs::read_dir(temp_dir.path()).unwrap() + .filter_map(|e| e.ok()) + .filter(|e| e.path().extension().and_then(|s| s.to_str()) == Some("tmp")) + .collect(); + assert!(tmp_files.is_empty(), "No .tmp files should remain after queue save"); + } + + // --- Frontmatter size limit in conflict parsing --- + + #[test] + fn test_parse_frontmatter_for_conflict_rejects_oversized() { + // Create content with frontmatter larger than 64KB + let huge_field = "x".repeat(70_000); + let content = format!("---\nid: 550e8400-e29b-41d4-a716-446655440000\nstatus: backlog\nversion: 1\nhuge: {}\n---\n\nBody", huge_field); + let result = parse_frontmatter_for_conflict(&content); + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("too large"), "Error should mention size: {}", err); + } }