diff --git a/src/application/ports/file_ports.rs b/src/application/ports/file_ports.rs index 19d9e926..cdfec8ce 100644 --- a/src/application/ports/file_ports.rs +++ b/src/application/ports/file_ports.rs @@ -255,11 +255,17 @@ pub trait FileManagementUseCase: Send + Sync + 'static { ) -> Result; /// Copies a file, enforcing that `caller_id` is the owner. + /// + /// `new_name`, when `Some(_)`, becomes the copy's filename — without it + /// the copy keeps the source's name, which makes "same folder, different + /// name" copies (classic WebDAV `COPY /a.txt → /b.txt`) collide on the + /// `(folder, name, user)` unique index. async fn copy_file_with_perms( &self, file_id: &str, caller_id: Uuid, target_folder_id: Option, + new_name: Option, ) -> Result; /// Renames a file, enforcing that `caller_id` is the owner. diff --git a/src/application/ports/storage_ports.rs b/src/application/ports/storage_ports.rs index 33307c35..41fb559c 100644 --- a/src/application/ports/storage_ports.rs +++ b/src/application/ports/storage_ports.rs @@ -314,10 +314,16 @@ pub trait FileWritePort: Send + Sync + 'static { /// /// With blob-dedup, this only creates a new metadata row and increments /// the blob reference count — zero disk I/O for the content. + /// + /// `new_name` is honored when `Some(_)` — without it, copying a file to + /// the same folder always collides on the source's filename. WebDAV + /// COPY uses this for the "same folder, different name" case (the + /// classic `COPY /a.txt → /b.txt` pattern). async fn copy_file( &self, file_id: &str, target_folder_id: Option, + new_name: Option<&str>, ) -> Result; /// Copies an entire folder subtree atomically using ltree. diff --git a/src/application/services/batch_operations.rs b/src/application/services/batch_operations.rs index 59569c15..1329af88 100644 --- a/src/application/services/batch_operations.rs +++ b/src/application/services/batch_operations.rs @@ -145,7 +145,12 @@ impl BatchOperationService { async move { let copy_result = mgmt - .copy_file_with_perms(&file_id, user_id, target_folder.map(|s| s.to_string())) + .copy_file_with_perms( + &file_id, + user_id, + target_folder.map(|s| s.to_string()), + None, + ) .await; (file_id, copy_result) } diff --git a/src/application/services/file_management_service.rs b/src/application/services/file_management_service.rs index 70e5b327..4386284b 100644 --- a/src/application/services/file_management_service.rs +++ b/src/application/services/file_management_service.rs @@ -127,15 +127,16 @@ impl FileManagementService { &self, file_id: &str, target_folder_id: Option, + new_name: Option<&str>, ) -> Result { info!( - "Copying file with ID: {} to folder: {:?}", - file_id, target_folder_id + "Copying file with ID: {} to folder: {:?} as {:?}", + file_id, target_folder_id, new_name ); let copied_file = self .file_repository - .copy_file(file_id, target_folder_id) + .copy_file(file_id, target_folder_id, new_name) .await .map_err(|e| { error!("Error copying file (ID: {}): {}", file_id, e); @@ -260,13 +261,15 @@ impl FileManagementUseCase for FileManagementService { file_id: &str, caller_id: Uuid, target_folder_id: Option, + new_name: Option, ) -> Result { // Copy = Read on the source file + Create on the target folder. self.require_file_perm(file_id, Permission::Read, caller_id) .await?; self.require_target_folder_perm(target_folder_id.as_deref(), Permission::Create, caller_id) .await?; - self.copy_file(file_id, target_folder_id).await + self.copy_file(file_id, target_folder_id, new_name.as_deref()) + .await } async fn rename_file_with_perms( diff --git a/src/application/services/idor_protection_test.rs b/src/application/services/idor_protection_test.rs index 33d1dcf7..5277a873 100644 --- a/src/application/services/idor_protection_test.rs +++ b/src/application/services/idor_protection_test.rs @@ -228,6 +228,7 @@ impl FileWritePort for MockFileWritePort { &self, _file_id: &str, _target_folder_id: Option, + _new_name: Option<&str>, ) -> Result { unimplemented!() } diff --git a/src/application/services/trash_service_test.rs b/src/application/services/trash_service_test.rs index 675bb300..ea2be5bb 100644 --- a/src/application/services/trash_service_test.rs +++ b/src/application/services/trash_service_test.rs @@ -609,6 +609,7 @@ impl FileWritePort for MockFileRepository { &self, _file_id: &str, _target_folder_id: Option, + _new_name: Option<&str>, ) -> std::result::Result { unimplemented!() } diff --git a/src/common/stubs.rs b/src/common/stubs.rs index e857085a..b9baa065 100644 --- a/src/common/stubs.rs +++ b/src/common/stubs.rs @@ -173,6 +173,7 @@ impl FileWritePort for StubFileWritePort { &self, _file_id: &str, _target_folder_id: Option, + _new_name: Option<&str>, ) -> Result { Ok(File::default()) } @@ -635,6 +636,7 @@ impl FileManagementUseCase for StubFileManagementUseCase { _file_id: &str, _caller_id: Uuid, _folder_id: Option, + _new_name: Option, ) -> Result { Ok(FileDto::default()) } diff --git a/src/infrastructure/repositories/pg/file_blob_write_repository.rs b/src/infrastructure/repositories/pg/file_blob_write_repository.rs index 76fb2345..f3cc7ef4 100644 --- a/src/infrastructure/repositories/pg/file_blob_write_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_write_repository.rs @@ -397,10 +397,12 @@ impl FileWritePort for FileBlobWriteRepository { &self, file_id: &str, target_folder_id: Option, + new_name: Option<&str>, ) -> Result { // Atomic CTE: read source file → insert new row with same blob_hash → increment ref_count. // Single round-trip; blob content is NOT copied (dedup makes this zero-copy). let target_fid = target_folder_id.clone(); + let rename_to = new_name.map(|s| s.to_string()); let row = retry_on_deadlock("files.copy", || { sqlx::query_as::< @@ -424,7 +426,7 @@ impl FileWritePort for FileBlobWriteRepository { ), new_file AS ( INSERT INTO storage.files (name, folder_id, user_id, blob_hash, size, mime_type, category_order) - SELECT name, + SELECT COALESCE($3::text, name), COALESCE($2::uuid, folder_id), user_id, blob_hash, @@ -442,6 +444,7 @@ impl FileWritePort for FileBlobWriteRepository { ) .bind(file_id) .bind(&target_fid) + .bind(&rename_to) .fetch_optional(self.pool.as_ref()) }) .await diff --git a/src/infrastructure/repositories/pg/folder_db_repository.rs b/src/infrastructure/repositories/pg/folder_db_repository.rs index 521388d9..97852fae 100644 --- a/src/infrastructure/repositories/pg/folder_db_repository.rs +++ b/src/infrastructure/repositories/pg/folder_db_repository.rs @@ -581,23 +581,55 @@ impl FolderRepository for FolderDbRepository { // ── Trash operations ── async fn move_to_trash(&self, folder_id: &str) -> Result<(), DomainError> { - // Only mark the folder itself as trashed. - // Child files and sub-folders are implicitly hidden because their - // ancestor is trashed — list queries already filter NOT is_trashed, - // and folder navigation won't reach a trashed folder's children. + // Soft-delete the whole subtree in one statement: the root flips + // `is_trashed` and records `original_parent_id` so restore knows + // where to put it back; every descendant (folder or file) that + // wasn't already in trash flips `is_trashed` too but leaves the + // `original_*` column NULL. That NULL is the marker the restore + // path uses to tell "cascade-trashed with the root" from + // "independently trashed earlier" — the latter must stay in + // trash even when the root is restored. + // + // Without this cascade, descendants used to remain `is_trashed = false` + // and stay directly addressable by their full path (PROPFIND on + // `/g9-tree/file.txt` still resolved 207 even though the parent + // collection was gone) — a class of data-integrity drift that + // confused desktop-sync tree walks. let result = retry_on_deadlock("folders.trash", || { sqlx::query_scalar::<_, i64>( r#" - WITH trash_folder AS ( + WITH trash_root AS ( UPDATE storage.folders SET is_trashed = TRUE, trashed_at = NOW(), original_parent_id = parent_id, updated_at = NOW() WHERE id = $1::uuid AND NOT is_trashed + RETURNING id, lpath + ), + trash_descendant_folders AS ( + UPDATE storage.folders f + SET is_trashed = TRUE, + trashed_at = NOW(), + updated_at = NOW() + FROM trash_root tr + WHERE f.lpath <@ tr.lpath + AND f.id != tr.id + AND NOT f.is_trashed + RETURNING 1 + ), + trash_descendant_files AS ( + UPDATE storage.files fi + SET is_trashed = TRUE, + trashed_at = NOW(), + updated_at = NOW() + FROM trash_root tr + JOIN storage.folders f ON f.lpath <@ tr.lpath + WHERE fi.folder_id = f.id + AND NOT fi.is_trashed RETURNING 1 ) - SELECT COUNT(*) FROM trash_folder + SELECT COUNT(*) FROM trash_root "#, ) .bind(folder_id) @@ -618,16 +650,18 @@ impl FolderRepository for FolderDbRepository { folder_id: &str, _original_path: &str, ) -> Result<(), DomainError> { - // Only restore the folder itself. - // Child files were never marked as trashed — they become visible - // again automatically once their parent folder is un-trashed. - // The BEFORE UPDATE trigger recomputes path/lpath when - // original_parent_id is restored; the cascade trigger - // batch-updates all descendants via the GiST lpath index. + // Inverse of the cascade in `move_to_trash`: restore the root + // (BEFORE UPDATE trigger recomputes path/lpath via the parent_id + // change), then un-trash every descendant whose `original_*` + // column is NULL — those are the rows we cascade-trashed + // ourselves. Descendants that were independently trashed + // *before* this folder went to trash have `original_*` set, so + // they correctly stay in trash and continue to show up as + // top-level trash entries via `storage.trash_items`. let result = retry_on_deadlock("folders.restore", || { sqlx::query_scalar::<_, i64>( r#" - WITH restore_folder AS ( + WITH restore_root AS ( UPDATE storage.folders SET is_trashed = FALSE, trashed_at = NULL, @@ -635,9 +669,33 @@ impl FolderRepository for FolderDbRepository { original_parent_id = NULL, updated_at = NOW() WHERE id = $1::uuid AND is_trashed + RETURNING id, lpath + ), + restore_descendant_folders AS ( + UPDATE storage.folders f + SET is_trashed = FALSE, + trashed_at = NULL, + updated_at = NOW() + FROM restore_root rr + WHERE f.lpath <@ rr.lpath + AND f.id != rr.id + AND f.is_trashed + AND f.original_parent_id IS NULL + RETURNING 1 + ), + restore_descendant_files AS ( + UPDATE storage.files fi + SET is_trashed = FALSE, + trashed_at = NULL, + updated_at = NOW() + FROM restore_root rr + JOIN storage.folders f ON f.lpath <@ rr.lpath + WHERE fi.folder_id = f.id + AND fi.is_trashed + AND fi.original_folder_id IS NULL RETURNING 1 ) - SELECT COUNT(*) FROM restore_folder + SELECT COUNT(*) FROM restore_root "#, ) .bind(folder_id) diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 4b8c2117..83987e28 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -658,6 +658,21 @@ async fn handle_proppatch( ) -> Result, AppError> { let _user = extract_user(&req)?; + // Active-lock guard (RFC 4918 §9.10.4): PROPPATCH writes properties, + // so a lock on the target must release them via `If:`. Captured + // before the body is consumed below so a rejected request doesn't + // even parse the XML. + let if_header_owned = req + .headers() + .get("If") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + if let Some(resp) = + enforce_native_lock(&state.webdav_lock_store, if_header_owned.as_deref(), &path) + { + return Ok(resp); + } + // Resolve the target resource type BEFORE consuming the body so // we can pick the correct href shape in the multi-status // response. RFC 4918 §5.2 + strict WebDAV-client parser rules @@ -895,6 +910,111 @@ async fn handle_head( .unwrap()) } +/// Resolve `path` to a user-owned resource using the optimized +/// PathResolver first, falling back to the legacy `get_folder_by_path` / +/// `get_file_by_path` lookups (the same ones GET uses) when the +/// optimized resolver returns NotFound. +/// +/// **Why the fallback exists**: the optimized resolver and the read-side +/// `get_*_by_path` repositories don't always agree on what a "path" +/// looks like. The drive-refactor migration rewrote the `path` column +/// to strip the `My Folder - /` prefix that the WebDAV dispatcher +/// (`resolve_webdav_path`) still prepends — leaving an inconsistency +/// where files PUT through the WebDAV surface stay reachable by GET +/// (legacy lookup) but invisible to the optimized resolver (strict +/// path-match). MOVE / DELETE / COPY previously 404'd on every +/// root-level file because they only used the optimized resolver. +/// +/// Ownership is enforced in both branches: the optimized resolver +/// includes `user_id = $4` in its SQL; the fallback runs `assert_owner` +/// explicitly so a foreign-owned hit can't leak through. +async fn resolve_or_legacy( + state: &Arc, + path: &str, + user_id: Uuid, +) -> Option { + if let Some(resolver) = &state.path_resolver + && let Ok(r) = resolver.resolve_path_for_user(path, user_id).await + { + return Some(r); + } + + let user_id_str = user_id.to_string(); + let folder_service = &state.applications.folder_service; + if let Ok(folder) = folder_service.get_folder_by_path(path).await + && folder.owner_id.as_deref() == Some(&user_id_str) + { + return Some(ResolvedResource::Folder(folder)); + } + let file_retrieval = &state.applications.file_retrieval_service; + if let Ok(file) = file_retrieval.get_file_by_path(path).await + && file.owner_id.as_deref() == Some(&user_id_str) + { + return Some(ResolvedResource::File(file)); + } + None +} + +/// Extract every `<...>` token from a WebDAV `If:` header value. +/// +/// RFC 4918 §10.4 defines a richer grammar (tagged-list / no-tag-list of +/// `(Condition)` items), but for our purposes the only thing that matters +/// is what lock tokens the caller is claiming to hold. Forgivingly scoop +/// every angle-bracketed value and let the caller compare against the +/// active lock token(s). +fn extract_if_header_tokens(if_header: &str) -> Vec { + let mut out = Vec::new(); + let mut current = String::new(); + let mut inside = false; + for c in if_header.chars() { + match (inside, c) { + (false, '<') => { + inside = true; + current.clear(); + } + (true, '>') => { + inside = false; + if !current.is_empty() { + out.push(std::mem::take(&mut current)); + } + } + (true, c) => current.push(c), + _ => {} + } + } + out +} + +/// RFC 4918 §9.10.4 — if `path` is locked, every mutating request MUST +/// carry the lock's token in its `If:` header. Returns `Some(Response)` +/// with a 423 Locked response when the request must be rejected; `None` +/// when the path is unlocked or the caller's `If:` header carries the +/// matching token (the cheap-and-cheerful submission check). +/// +/// Shared by `handle_put` now and will be reused by `handle_delete`, +/// `handle_move`, `handle_copy`, and `handle_proppatch` when each of +/// those gets the same enforcement. +fn enforce_native_lock( + lock_store: &crate::infrastructure::services::webdav_lock_service::WebDavLockStore, + if_header: Option<&str>, + path: &str, +) -> Option> { + let entry = lock_store.get_by_path(path)?; + if let Some(h) = if_header + && extract_if_header_tokens(h) + .iter() + .any(|t| t == &entry.info.token) + { + return None; + } + Some( + Response::builder() + .status(StatusCode::LOCKED) + .body(Body::empty()) + .unwrap(), + ) +} + /** * Handles PUT requests to create or update files. * @@ -926,6 +1046,21 @@ async fn handle_put( return Err(AppError::bad_request("Cannot PUT to root folder")); } + // ── Active-lock guard (RFC 4918 §9.10.4) ────────────────────────── + // Reject a write that targets a locked resource unless the request + // carries the lock token in `If:`. Captured before we consume the + // body into the CDC ingester — a 423 mustn't waste any bandwidth. + let if_header_owned = req + .headers() + .get("If") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + if let Some(resp) = + enforce_native_lock(&state.webdav_lock_store, if_header_owned.as_deref(), &path) + { + return Ok(resp); + } + // ── Ownership guard ──────────────────────────────────────── // Verify that the user owns the target file (update) or the // parent folder (create). Without this check a user could @@ -1117,6 +1252,18 @@ async fn handle_delete( ) -> Result, AppError> { let user = extract_user(&req)?; + // Active-lock guard (RFC 4918 §9.10.4). + let if_header_owned = req + .headers() + .get("If") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + if let Some(resp) = + enforce_native_lock(&state.webdav_lock_store, if_header_owned.as_deref(), &path) + { + return Ok(resp); + } + // Get services from state let file_retrieval_service = &state.applications.file_retrieval_service; let file_management_service = &state.applications.file_management_service; @@ -1127,49 +1274,25 @@ async fn handle_delete( return Err(AppError::forbidden("Cannot delete root folder")); } - // Single-query path resolution (user-scoped) - if let Some(resolver) = &state.path_resolver { - match resolver.resolve_path_for_user(&path, user.id).await { - Ok(ResolvedResource::Folder(folder)) => { - folder_service - .delete_folder_with_perms(&folder.id, user.id) - .await - .map_err(|e| { - AppError::internal_error(format!("Failed to delete folder: {}", e)) - })?; - } - Ok(ResolvedResource::File(file)) => { - file_management_service - .delete_file_with_perms(&file.id, user.id) - .await - .map_err(|e| { - AppError::internal_error(format!("Failed to delete file: {}", e)) - })?; - } - Err(_) => return Err(AppError::not_found(format!("Resource not found: {}", path))), - } - } else { - // Fallback: legacy double-query path (with ownership check) - let folder_result = folder_service.get_folder_by_path(&path).await; - - if let Ok(folder) = folder_result { - assert_owner(folder.owner_id.as_deref(), &user.id.to_string(), &path)?; + // Resolve via optimized resolver, falling back to the legacy + // double-query lookup (the one GET uses). Necessary because the + // optimized resolver and the read repositories disagree on path + // shape for some files; see `resolve_or_legacy` docs. + let _ = file_retrieval_service; // present for legacy fallback if needed elsewhere + match resolve_or_legacy(&state, &path, user.id).await { + Some(ResolvedResource::Folder(folder)) => { folder_service .delete_folder_with_perms(&folder.id, user.id) .await .map_err(|e| AppError::internal_error(format!("Failed to delete folder: {}", e)))?; - } else { - let file = file_retrieval_service - .get_file_by_path(&path) - .await - .map_err(|_e| AppError::not_found(format!("Resource not found: {}", path)))?; - assert_owner(file.owner_id.as_deref(), &user.id.to_string(), &path)?; - + } + Some(ResolvedResource::File(file)) => { file_management_service .delete_file_with_perms(&file.id, user.id) .await .map_err(|e| AppError::internal_error(format!("Failed to delete file: {}", e)))?; } + None => return Err(AppError::not_found(format!("Resource not found: {}", path))), } Ok(Response::builder() @@ -1197,6 +1320,23 @@ async fn handle_move( let user = extract_user(&req)?; let source_path = path; + // Captured up front so a rejected MOVE doesn't run any DB work. + let if_header_owned = req + .headers() + .get("If") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + + // Active-lock guard on the SOURCE (RFC 4918 §9.10.4): the move + // removes the source resource, which counts as modifying it. + if let Some(resp) = enforce_native_lock( + &state.webdav_lock_store, + if_header_owned.as_deref(), + &source_path, + ) { + return Ok(resp); + } + // Get destination from Destination header let destination = req .headers() @@ -1225,6 +1365,28 @@ async fn handle_move( // SECURITY: reject path-traversal in destination reject_path_traversal(&destination_path)?; + // Normalize destination through the SAME path-prefixing that + // `resolve_webdav_path` applied to `source_path` during dispatch. + // Without this, comparing source_parent_path (already prefixed with + // the user's home folder name) against dest_parent_path (raw from + // the URL, no prefix) always reports "different parent" — even for a + // pure rename at the same level — and breaks the move/rename branch + // selection below. + let destination_path = resolve_webdav_path(&state, user.id, &destination_path) + .await + .unwrap_or(destination_path); + + // Destination lock guard: MOVE also creates/replaces a resource at + // the destination. If that path is locked, the same If: header must + // satisfy it. + if let Some(resp) = enforce_native_lock( + &state.webdav_lock_store, + if_header_owned.as_deref(), + &destination_path, + ) { + return Ok(resp); + } + // Get services from state let file_retrieval_service = &state.applications.file_retrieval_service; let file_management_service = &state.applications.file_management_service; @@ -1254,197 +1416,96 @@ async fn handle_move( } } - // Resolve source: single-query when PathResolver is available (user-scoped) - if let Some(resolver) = &state.path_resolver { - match resolver.resolve_path_for_user(&source_path, user.id).await { - Ok(ResolvedResource::Folder(folder)) => { - let dest_folder_name = destination_path - .split('/') - .next_back() - .unwrap_or(&destination_path); - let dest_parent_path = if let Some(idx) = destination_path.rfind('/') { - &destination_path[..idx] - } else { - "" - }; + // Resolve source via optimized resolver with legacy fallback (see + // `resolve_or_legacy` for the rationale). Single match collapses the + // two near-identical branches that the resolver-only + legacy-only + // versions used to keep. + let _ = file_retrieval_service; // referenced via resolve_or_legacy + let resolved = resolve_or_legacy(&state, &source_path, user.id) + .await + .ok_or_else(|| AppError::not_found(format!("Resource not found: {}", source_path)))?; - let move_dto = crate::application::dtos::folder_dto::MoveFolderDto { - parent_id: if dest_parent_path.is_empty() { - None - } else { - match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => { - // SECURITY: verify destination parent belongs to caller (V-08) - assert_owner( - parent.owner_id.as_deref(), - &user.id.to_string(), - dest_parent_path, - )?; - Some(parent.id) - } - Err(_) => None, - } - }, - }; - - folder_service - .move_folder_with_perms(&folder.id, move_dto, user.id) - .await - .map_err(AppError::from)?; - - if folder.name != dest_folder_name { - let rename_dto = crate::application::dtos::folder_dto::RenameFolderDto { - name: dest_folder_name.to_string(), - }; - folder_service - .rename_folder_with_perms(&folder.id, rename_dto, user.id) - .await - .map_err(AppError::from)?; - } - } - Ok(ResolvedResource::File(file)) => { - let dest_filename = destination_path - .split('/') - .next_back() - .unwrap_or(&destination_path); - let dest_parent_path = if let Some(idx) = destination_path.rfind('/') { - &destination_path[..idx] - } else { - "" - }; - let source_parent_path = if let Some(idx) = source_path.rfind('/') { - &source_path[..idx] - } else { - "" - }; - - if source_parent_path != dest_parent_path { - // SECURITY: verify destination parent belongs to caller (V-08) - if !dest_parent_path.is_empty() - && let Ok(parent) = - folder_service.get_folder_by_path(dest_parent_path).await - { - assert_owner( - parent.owner_id.as_deref(), - &user.id.to_string(), - dest_parent_path, - )?; - } - file_management_service - .move_file_with_perms(&file.id, user.id, Some(dest_parent_path.to_string())) - .await - .map_err(AppError::from)?; - } - if file.name != dest_filename { - file_management_service - .rename_file_with_perms(&file.id, user.id, dest_filename) - .await - .map_err(AppError::from)?; - } - } - Err(_) => { - return Err(AppError::not_found(format!( - "Resource not found: {}", - source_path - ))); - } - } - } else { - // Fallback: legacy double-query path (with ownership check) - let folder_result = folder_service.get_folder_by_path(&source_path).await; - - if let Ok(folder) = folder_result { - assert_owner( - folder.owner_id.as_deref(), - &user.id.to_string(), - &source_path, - )?; - let dest_folder_name = destination_path - .split('/') - .next_back() - .unwrap_or(&destination_path); - let dest_parent_path = if let Some(idx) = destination_path.rfind('/') { - &destination_path[..idx] - } else { - "" - }; + let dest_name = destination_path + .rsplit('/') + .next() + .unwrap_or(&destination_path); + let dest_parent_path = destination_path + .rfind('/') + .map(|i| &destination_path[..i]) + .unwrap_or(""); + let source_parent_path = source_path + .rfind('/') + .map(|i| &source_path[..i]) + .unwrap_or(""); + match resolved { + ResolvedResource::Folder(folder) => { let move_dto = crate::application::dtos::folder_dto::MoveFolderDto { parent_id: if dest_parent_path.is_empty() { None - } else { - match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => { - // SECURITY: verify destination parent belongs to caller (V-08) - assert_owner( - parent.owner_id.as_deref(), - &user.id.to_string(), - dest_parent_path, - )?; - Some(parent.id) - } - Err(_) => None, - } - }, - }; - - folder_service - .move_folder_with_perms(&folder.id, move_dto, user.id) - .await - .map_err(|e| AppError::internal_error(format!("Failed to move folder: {}", e)))?; - - if folder.name != dest_folder_name { - let rename_dto = crate::application::dtos::folder_dto::RenameFolderDto { - name: dest_folder_name.to_string(), - }; - folder_service - .rename_folder_with_perms(&folder.id, rename_dto, user.id) - .await - .map_err(AppError::from)?; - } - } else { - let file = file_retrieval_service - .get_file_by_path(&source_path) - .await - .map_err(|_e| { - AppError::not_found(format!("Resource not found: {}", source_path)) - })?; - assert_owner(file.owner_id.as_deref(), &user.id.to_string(), &source_path)?; - - let dest_filename = destination_path - .split('/') - .next_back() - .unwrap_or(&destination_path); - let dest_parent_path = if let Some(idx) = destination_path.rfind('/') { - &destination_path[..idx] - } else { - "" - }; - let source_parent_path = if let Some(idx) = source_path.rfind('/') { - &source_path[..idx] - } else { - "" - }; - - if source_parent_path != dest_parent_path { - // SECURITY: verify destination parent belongs to caller (V-08) - if !dest_parent_path.is_empty() - && let Ok(parent) = folder_service.get_folder_by_path(dest_parent_path).await + } else if let Ok(parent) = folder_service.get_folder_by_path(dest_parent_path).await { assert_owner( parent.owner_id.as_deref(), &user.id.to_string(), dest_parent_path, )?; - } - file_management_service - .move_file_with_perms(&file.id, user.id, Some(dest_parent_path.to_string())) + Some(parent.id) + } else { + None + }, + }; + + folder_service + .move_folder_with_perms(&folder.id, move_dto, user.id) + .await + .map_err(AppError::from)?; + + if folder.name != dest_name { + let rename_dto = crate::application::dtos::folder_dto::RenameFolderDto { + name: dest_name.to_string(), + }; + folder_service + .rename_folder_with_perms(&folder.id, rename_dto, user.id) .await .map_err(AppError::from)?; } - if file.name != dest_filename { + } + ResolvedResource::File(file) => { + if source_parent_path != dest_parent_path { + // Resolve the destination's parent PATH into a folder ID + // before handing it to move_file_with_perms (which takes + // an Option, not a path). Previously + // the path was passed straight through and the move + // would silently fail because no row matches a folder + // whose id literally equals the path text. + let target_parent_id = if dest_parent_path.is_empty() { + None + } else { + let parent = folder_service + .get_folder_by_path(dest_parent_path) + .await + .map_err(|_| { + AppError::not_found(format!( + "Destination parent not found: {}", + dest_parent_path + )) + })?; + assert_owner( + parent.owner_id.as_deref(), + &user.id.to_string(), + dest_parent_path, + )?; + Some(parent.id) + }; file_management_service - .rename_file_with_perms(&file.id, user.id, dest_filename) + .move_file_with_perms(&file.id, user.id, target_parent_id) + .await + .map_err(AppError::from)?; + } + if file.name != dest_name { + file_management_service + .rename_file_with_perms(&file.id, user.id, dest_name) .await .map_err(AppError::from)?; } @@ -1476,6 +1537,15 @@ async fn handle_copy( let user = extract_user(&req)?; let source_path = path; + // Captured up front (cheap; used below for the destination lock guard). + // COPY doesn't mutate the source, so no source lock check — only the + // destination needs to clear (RFC 4918 §9.10.4). + let if_header_owned = req + .headers() + .get("If") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + // Get destination from Destination header let destination = req .headers() @@ -1504,6 +1574,22 @@ async fn handle_copy( // SECURITY: reject path-traversal in destination reject_path_traversal(&destination_path)?; + // Normalize through the same path-prefixing the dispatcher applied + // to source_path. See the long comment in handle_move for why this + // matters — same root-cause class of asymmetric-path bugs. + let destination_path = resolve_webdav_path(&state, user.id, &destination_path) + .await + .unwrap_or(destination_path); + + // Active-lock guard on the destination (RFC 4918 §9.10.4). + if let Some(resp) = enforce_native_lock( + &state.webdav_lock_store, + if_header_owned.as_deref(), + &destination_path, + ) { + return Ok(resp); + } + // Get depth from Depth header let depth = req .headers() @@ -1539,144 +1625,39 @@ async fn handle_copy( } } - // Resolve source: single-query when PathResolver is available (user-scoped) - if let Some(resolver) = &state.path_resolver { - match resolver.resolve_path_for_user(&source_path, user.id).await { - Ok(ResolvedResource::Folder(folder)) => { - let recursive = depth != "0"; + // Resolve source via optimized resolver with legacy fallback; collapses + // the two near-identical branches the resolver-only + legacy-only + // versions used to keep. + let _ = file_retrieval_service; // referenced via resolve_or_legacy + let resolved = resolve_or_legacy(&state, &source_path, user.id) + .await + .ok_or_else(|| AppError::not_found(format!("Resource not found: {}", source_path)))?; - let dest_folder_name = destination_path - .split('/') - .next_back() - .unwrap_or(&destination_path); - let dest_parent_path = if let Some(idx) = destination_path.rfind('/') { - &destination_path[..idx] - } else { - "" - }; + let dest_name = destination_path + .rsplit('/') + .next() + .unwrap_or(&destination_path); + let dest_parent_path = destination_path + .rfind('/') + .map(|i| &destination_path[..i]) + .unwrap_or(""); - let target_parent_id = if dest_parent_path.is_empty() { - None - } else { - match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => { - // SECURITY: verify destination parent belongs to caller (V-08) - assert_owner( - parent.owner_id.as_deref(), - &user.id.to_string(), - dest_parent_path, - )?; - Some(parent.id) - } - Err(_) => None, - } - }; - - if recursive { - let file_management_service = &state.applications.file_management_service; - file_management_service - .copy_folder_tree_with_perms( - &folder.id, - user.id, - target_parent_id, - Some(dest_folder_name.to_string()), - ) - .await - .map_err(|e| { - AppError::internal_error(format!("Failed to copy folder tree: {}", e)) - })?; - } else { - let create_dto = crate::application::dtos::folder_dto::CreateFolderDto { - name: dest_folder_name.to_string(), - parent_id: target_parent_id, - }; - folder_service - .create_folder_with_perms(create_dto, user.id) - .await - .map_err(|e| { - AppError::internal_error(format!( - "Failed to create destination folder: {}", - e - )) - })?; - } - } - Ok(ResolvedResource::File(file)) => { - let dest_parent_path = if let Some(idx) = destination_path.rfind('/') { - &destination_path[..idx] - } else { - "" - }; - - let target_folder_id = if dest_parent_path.is_empty() { - None - } else { - match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => { - // SECURITY: verify destination parent belongs to caller (V-08) - assert_owner( - parent.owner_id.as_deref(), - &user.id.to_string(), - dest_parent_path, - )?; - Some(parent.id) - } - Err(_) => None, - } - }; - - let file_management_service = &state.applications.file_management_service; - file_management_service - .copy_file_with_perms(&file.id, user.id, target_folder_id) - .await - .map_err(|e| AppError::internal_error(format!("Failed to copy file: {}", e)))?; - } - Err(_) => { - return Err(AppError::not_found(format!( - "Resource not found: {}", - source_path - ))); - } - } + let target_parent_id = if dest_parent_path.is_empty() { + None + } else if let Ok(parent) = folder_service.get_folder_by_path(dest_parent_path).await { + assert_owner( + parent.owner_id.as_deref(), + &user.id.to_string(), + dest_parent_path, + )?; + Some(parent.id) } else { - // Fallback: legacy double-query path (with ownership check) - let folder_result = folder_service.get_folder_by_path(&source_path).await; + None + }; - if let Ok(folder) = folder_result { - assert_owner( - folder.owner_id.as_deref(), - &user.id.to_string(), - &source_path, - )?; + match resolved { + ResolvedResource::Folder(folder) => { let recursive = depth != "0"; - - let dest_folder_name = destination_path - .split('/') - .next_back() - .unwrap_or(&destination_path); - let dest_parent_path = if let Some(idx) = destination_path.rfind('/') { - &destination_path[..idx] - } else { - "" - }; - - let target_parent_id = if dest_parent_path.is_empty() { - None - } else { - match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => { - // SECURITY: verify destination parent belongs to caller (V-08) - assert_owner( - parent.owner_id.as_deref(), - &user.id.to_string(), - dest_parent_path, - )?; - Some(parent.id) - } - Err(_) => None, - } - }; - if recursive { let file_management_service = &state.applications.file_management_service; file_management_service @@ -1684,7 +1665,7 @@ async fn handle_copy( &folder.id, user.id, target_parent_id, - Some(dest_folder_name.to_string()), + Some(dest_name.to_string()), ) .await .map_err(|e| { @@ -1692,7 +1673,7 @@ async fn handle_copy( })?; } else { let create_dto = crate::application::dtos::folder_dto::CreateFolderDto { - name: dest_folder_name.to_string(), + name: dest_name.to_string(), parent_id: target_parent_id, }; folder_service @@ -1705,41 +1686,20 @@ async fn handle_copy( )) })?; } - } else { - let file = file_retrieval_service - .get_file_by_path(&source_path) - .await - .map_err(|_e| { - AppError::not_found(format!("Resource not found: {}", source_path)) - })?; - assert_owner(file.owner_id.as_deref(), &user.id.to_string(), &source_path)?; - - let dest_parent_path = if let Some(idx) = destination_path.rfind('/') { - &destination_path[..idx] - } else { - "" - }; - - let target_folder_id = if dest_parent_path.is_empty() { - None - } else { - match folder_service.get_folder_by_path(dest_parent_path).await { - Ok(parent) => { - // SECURITY: verify destination parent belongs to caller (V-08) - assert_owner( - parent.owner_id.as_deref(), - &user.id.to_string(), - dest_parent_path, - )?; - Some(parent.id) - } - Err(_) => None, - } - }; - + } + ResolvedResource::File(file) => { + // M8b fix: copy_file_with_perms now accepts an optional new + // filename — without it, a copy to the same folder with a + // different name collided with the source on the + // (folder, name, user) unique index. Pass dest_name when it + // differs from the source so the INSERT lands with the + // intended name in a single round-trip; pass None for the + // "same name in a different folder" case to keep the existing + // semantics. let file_management_service = &state.applications.file_management_service; + let copy_name = (file.name != dest_name).then(|| dest_name.to_string()); file_management_service - .copy_file_with_perms(&file.id, user.id, target_folder_id) + .copy_file_with_perms(&file.id, user.id, target_parent_id, copy_name) .await .map_err(|e| AppError::internal_error(format!("Failed to copy file: {}", e)))?; } diff --git a/src/interfaces/nextcloud/trashbin_handler.rs b/src/interfaces/nextcloud/trashbin_handler.rs index 60464ef2..8799354b 100644 --- a/src/interfaces/nextcloud/trashbin_handler.rs +++ b/src/interfaces/nextcloud/trashbin_handler.rs @@ -9,12 +9,15 @@ use quick_xml::{ }; use std::sync::Arc; +use crate::application::ports::file_ports::FileRetrievalUseCase; +use crate::application::ports::folder_ports::FolderUseCase; use crate::application::ports::trash_ports::TrashUseCase; use crate::common::di::AppState; use crate::interfaces::errors::AppError; use crate::interfaces::middleware::auth::{AuthUser, CurrentUser}; use crate::interfaces::nextcloud::webdav_handler::{ - batch_resolve_ids, format_oc_id, write_text_element, + batch_resolve_ids, extract_nc_subpath_from_dest, format_oc_id, nc_to_internal_path, + write_text_element, }; const HEADER_DAV: HeaderName = HeaderName::from_static("dav"); @@ -37,7 +40,12 @@ pub async fn handle_nc_trashbin( handle_propfind(state, &user).await } "MOVE" if subpath_trimmed.starts_with("trash/") => { - handle_restore(state, &user, subpath_trimmed).await + let dest_header = req + .headers() + .get("destination") + .and_then(|v| v.to_str().ok()) + .map(|s| s.to_string()); + handle_restore(state, dest_header, &user, subpath_trimmed).await } "DELETE" if subpath_trimmed == "trash" || subpath_trimmed.is_empty() => { handle_empty_trash(state, &user).await @@ -98,6 +106,7 @@ async fn handle_propfind( async fn handle_restore( state: Arc, + dest_header: Option, user: &CurrentUser, subpath: &str, ) -> Result, AppError> { @@ -108,15 +117,67 @@ async fn handle_restore( .as_ref() .ok_or_else(|| AppError::internal_error("Trash service not available"))?; - trash_svc - .restore_item(&id, user.id) - .await - .map_err(|e| AppError::internal_error(format!("Failed to restore item: {}", e)))?; + // RFC 4918 §9.9.4 + Sabre convention: clients send `Destination` to + // tell the server where the restored item should land. We don't yet + // honor it for relocation (restore always lands at the original + // path), but we DO honor it for the collision check: if the requested + // destination is taken by a live resource the move must be refused + // with 412 — there is no `Overwrite: T` workflow for trash restore in + // either Sabre/DAV or the NC desktop client (a live file being + // silently replaced by an undeleted one would be a footgun). + if let Some(dest_header) = dest_header + && let Some(dest_subpath) = extract_nc_subpath_from_dest(&dest_header, &user.username) + { + let dest_internal = nc_to_internal_path(&user.username, &dest_subpath)?; + let folder_service = &state.applications.folder_service; + let file_service = &state.applications.file_retrieval_service; + let dest_taken = file_service.get_file_by_path(&dest_internal).await.is_ok() + || folder_service + .get_folder_by_path(&dest_internal) + .await + .is_ok(); + if dest_taken { + return Ok(Response::builder() + .status(StatusCode::PRECONDITION_FAILED) + .body(Body::empty()) + .unwrap()); + } + } - Ok(Response::builder() - .status(StatusCode::CREATED) - .body(Body::empty()) - .unwrap()) + match trash_svc.restore_item(&id, user.id).await { + Ok(()) => Ok(Response::builder() + .status(StatusCode::CREATED) + .body(Body::empty()) + .unwrap()), + Err(e) => { + // Collision at the original path — a live file/folder is sitting + // where the trashed one wants to come back to. Mirrors the G4/G5 + // semantics in webdav_handler::handle_move ("Overwrite: F to an + // existing path → 412"); restore has no Overwrite header so the + // refusal is unconditional. The caller can resolve by renaming + // or trashing the conflicting live resource first. + // + // We string-match for the unique-index / duplicate-key signature + // because restore_item currently re-wraps every storage error as + // InternalError, so the original DomainError::AlreadyExists kind + // is not propagated. A follow-up should thread the kind through + // and let this be a kind-based check. + let msg = format!("{}", e); + if msg.contains("duplicate key") + || msg.contains("unique constraint") + || msg.to_ascii_lowercase().contains("already exists") + { + return Ok(Response::builder() + .status(StatusCode::PRECONDITION_FAILED) + .body(Body::empty()) + .unwrap()); + } + Err(AppError::internal_error(format!( + "Failed to restore item: {}", + e + ))) + } + } } // ──────────────────── DELETE (empty trash) ──────────────────── diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index e74aff48..135872cb 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -381,10 +381,18 @@ async fn handle_head( // ETag comes from `FileDto::etag` — see the same comment block on // the GET handler. HEAD and GET must agree byte-for-byte; pulling // both from the same DTO field guarantees that. + // + // We deliberately do NOT set `Content-Length: file.size` here even + // though RFC 7231 §4.3.2 says HEAD SHOULD return the same headers + // GET would. Our body is `Body::empty()`, so declaring a non-zero + // Content-Length tells the client "20 bytes are coming" — and on a + // keep-alive connection the client waits forever for them. Hyper + // derives `Content-Length: 0` from the empty body, which is honest + // about what's actually on the wire. Clients that need the file + // size use PROPFIND (which is what NC and Sabre clients do). Ok(Response::builder() .status(StatusCode::OK) .header(header::CONTENT_TYPE, file.mime_type.as_ref()) - .header(header::CONTENT_LENGTH, file.size) .header(header::ETAG, format!("\"{}\"", file.etag)) .header(header::LAST_MODIFIED, modified_at.to_rfc2822()) .body(Body::empty()) @@ -543,6 +551,62 @@ fn parse_proppatch_favorite(body: &str) -> Option { // ──────────────────── PUT ──────────────────── +/// Strip the optional `W/` weak prefix and surrounding double-quotes +/// from one ETag value in an `If-Match` / `If-None-Match` list. Returns +/// `(is_weak, inner)`. +fn parse_etag_value(raw: &str) -> (bool, &str) { + let trimmed = raw.trim(); + if let Some(rest) = trimmed.strip_prefix("W/") { + (true, rest.trim().trim_matches('"')) + } else { + (false, trimmed.trim_matches('"')) + } +} + +/// RFC 7232 §3.2 — `If-None-Match` fails for PUT when: +/// - the header value is `*` and a current representation exists, OR +/// - any listed ETag matches the current representation (weak comparison +/// — weak validators in the request are equivalent to strong for the +/// match itself, only If-Match is required to be strong). +fn if_none_match_precondition_fails(header: &str, current_etag: Option<&str>) -> bool { + let v = header.trim(); + if v == "*" { + return current_etag.is_some(); + } + let Some(current) = current_etag else { + return false; + }; + v.split(',').any(|tag| { + let (_, parsed) = parse_etag_value(tag); + !parsed.is_empty() && parsed == current + }) +} + +/// RFC 7232 §3.1 — `If-Match` fails for PUT when: +/// - the resource doesn't currently exist (no strong validator to match), OR +/// - the header isn't `*` and no listed ETag strong-matches the current one +/// (weak validators in the request never satisfy a strong-match). +fn if_match_precondition_fails(header: &str, current_etag: Option<&str>) -> bool { + let v = header.trim(); + let Some(current) = current_etag else { + return true; + }; + if v == "*" { + return false; + } + !v.split(',').any(|tag| { + let (is_weak, parsed) = parse_etag_value(tag); + !is_weak && !parsed.is_empty() && parsed == current + }) +} + +fn precondition_failed_response() -> Response { + Response::builder() + .status(StatusCode::PRECONDITION_FAILED) + .body(Body::empty()) + .unwrap() +} + async fn handle_put( state: Arc, req: Request, @@ -566,6 +630,31 @@ async fn handle_put( .and_then(|v| v.to_str().ok()) .and_then(|v| v.parse::().ok()); + // ── Conditional preconditions (RFC 7232 §3.1 / §3.2) ───────────── + // Evaluated BEFORE body ingestion so a rejected PUT doesn't waste + // bandwidth or disk I/O on a body the server is going to throw away. + // The lookup is reused for the create-vs-update distinction below, + // so this is also free of an extra DB hit. + let existing = file_service.get_file_by_path(&internal_path).await.ok(); + let current_etag = existing.as_ref().map(|f| f.etag.as_str()); + + if let Some(value) = req + .headers() + .get(header::IF_NONE_MATCH) + .and_then(|v| v.to_str().ok()) + && if_none_match_precondition_fails(value, current_etag) + { + return Ok(precondition_failed_response()); + } + if let Some(value) = req + .headers() + .get(header::IF_MATCH) + .and_then(|v| v.to_str().ok()) + && if_match_precondition_fails(value, current_etag) + { + return Ok(precondition_failed_response()); + } + // ── Direct PUT cap ─────────────────────────────────────────────── // We use `direct_put_max_bytes` (default 1 GiB), not `max_upload_size` // (default 10 GB). Larger files must come through the chunked upload @@ -594,8 +683,9 @@ async fn handle_put( .await?; let content_type = ingested.content_type.clone(); - // Distinguish create (201) vs update (204) for the response status. - let existed = file_service.get_file_by_path(&internal_path).await.is_ok(); + // Distinguish create (201) vs update (204) for the response status, + // using the lookup already done above for the precondition check. + let existed = existing.is_some(); // Single streaming path — handles both update and create internally, // swapping the file row onto the already-ingested blob. @@ -630,7 +720,18 @@ async fn handle_mkcol( let folder_service = &state.applications.folder_service; let internal_path = nc_to_internal_path(&user.username, subpath)?; - // If the folder already exists, return 405 per RFC 4918 §9.3.1 + // RFC 4918 §9.3.1: + // - target already exists → 405 Method Not Allowed + // - parent collection of the target does NOT exist → 409 Conflict + // - parent exists and target does not → 201 Created + // + // Previous behaviour effectively performed `mkdir -p` and returned + // 201 even when intermediate ancestors were missing. Sabre/DAV and + // the actual NC server both return 409 here, so the legacy + // auto-create deviated from the reference implementation. NC desktop + // walks ancestors one MKCOL at a time anyway, so dropping the + // auto-create doesn't break real clients. + if folder_service .get_folder_by_path(&internal_path) .await @@ -642,56 +743,39 @@ async fn handle_mkcol( .unwrap()); } - // Collect path segments that need to be created (walk from root to leaf) let segments: Vec<&str> = subpath.split('/').filter(|s| !s.is_empty()).collect(); + if segments.is_empty() { + return Err(AppError::bad_request( + "MKCOL on the user root is not allowed", + )); + } + let (target_name, parent_segments) = segments.split_last().expect("checked non-empty above"); let user_root = nc_to_internal_path(&user.username, "")?; - let mut current_path = user_root.clone(); - let mut parent_id = folder_service - .get_folder_by_path(&user_root) - .await - .map_err(|_| AppError::not_found("User root folder not found"))? - .id - .clone(); + let parent_path = if parent_segments.is_empty() { + user_root.clone() + } else { + format!("{}/{}", user_root, parent_segments.join("/")) + }; - for segment in &segments { - current_path = format!("{}/{}", current_path, segment); - match folder_service.get_folder_by_path(¤t_path).await { - Ok(existing) => { - parent_id = existing.id.clone(); - } - Err(_) => { - let dto = CreateFolderDto { - name: segment.to_string(), - parent_id: Some(parent_id.clone()), - }; - match folder_service.create_folder_with_perms(dto, user.id).await { - Ok(created) => { - parent_id = created.id.clone(); - } - Err(e) - if e.message.contains("already exists") - || e.message.contains("Already Exists") => - { - // Race condition — folder created concurrently - let folder = folder_service - .get_folder_by_path(¤t_path) - .await - .map_err(|_| { - AppError::internal_error("Folder exists but cannot be found") - })?; - parent_id = folder.id.clone(); - } - Err(e) => { - return Err(AppError::internal_error(format!( - "Failed to create folder: {}", - e - ))); - } - } - } + let parent_folder = match folder_service.get_folder_by_path(&parent_path).await { + Ok(folder) => folder, + Err(_) => { + return Ok(Response::builder() + .status(StatusCode::CONFLICT) + .body(Body::empty()) + .unwrap()); } - } + }; + + let dto = CreateFolderDto { + name: target_name.to_string(), + parent_id: Some(parent_folder.id.clone()), + }; + folder_service + .create_folder_with_perms(dto, user.id) + .await + .map_err(|e| AppError::internal_error(format!("Failed to create folder: {}", e)))?; Ok(Response::builder() .status(StatusCode::CREATED) @@ -781,6 +865,19 @@ async fn handle_move( .ok_or_else(|| AppError::bad_request("Missing Destination header"))? .to_string(); + // RFC 4918 §9.9.3: the `Overwrite` header has the default value `T`. + // `F` MUST cause the request to fail with 412 when the destination + // already exists; `T` (or absent) MUST replace the destination as if + // it didn't exist (the response then drops from 201 Created to 204 + // No Content per §9.9.4 because the URI's resource was replaced + // rather than newly created). + let overwrite_forbidden = req + .headers() + .get("overwrite") + .and_then(|v| v.to_str().ok()) + .map(|v| v.trim().eq_ignore_ascii_case("F")) + .unwrap_or(false); + // Parse destination path: extract subpath after /remote.php/dav/files/{user}/ let dest_subpath = extract_nc_subpath_from_dest(&destination, &user.username) .ok_or_else(|| AppError::bad_request("Invalid Destination URL"))?; @@ -790,6 +887,58 @@ async fn handle_move( let file_service = &state.applications.file_retrieval_service; let file_mgmt = &state.applications.file_management_service; + // ── Destination-collision precondition (RFC 4918 §9.9.4) ────────── + // Resolved once up-front so the file/folder branches below don't + // each have to repeat the check. `dest_existed_before` becomes the + // 204-vs-201 selector at response time. + let dest_internal_precheck = nc_to_internal_path(&user.username, &dest_subpath)?; + let dest_existing_file = file_service + .get_file_by_path(&dest_internal_precheck) + .await + .ok(); + let dest_existing_folder = folder_service + .get_folder_by_path(&dest_internal_precheck) + .await + .ok(); + let dest_existed_before = dest_existing_file.is_some() || dest_existing_folder.is_some(); + + if dest_existed_before { + if overwrite_forbidden { + return Ok(Response::builder() + .status(StatusCode::PRECONDITION_FAILED) + .body(Body::empty()) + .unwrap()); + } + // Overwrite: T (or absent) → delete the existing destination first, + // then proceed with the move. Trashing is fine: per RFC the source + // resource appears at the destination URI; what happens to the + // overwritten one is up to the server. + if let Some(existing_file) = &dest_existing_file { + file_mgmt + .delete_and_cleanup_with_perms(&existing_file.id, user.id) + .await + .map_err(|e| { + AppError::internal_error(format!("Failed to overwrite destination file: {}", e)) + })?; + } else if let Some(existing_folder) = &dest_existing_folder { + folder_service + .delete_folder_with_perms(&existing_folder.id, user.id) + .await + .map_err(|e| { + AppError::internal_error(format!( + "Failed to overwrite destination folder: {}", + e + )) + })?; + } + } + + let final_status = if dest_existed_before { + StatusCode::NO_CONTENT + } else { + StatusCode::CREATED + }; + // Try as file first. if let Ok(file) = file_service.get_file_by_path(&src_internal).await { let (dest_parent_sub, dest_name) = match dest_subpath.rsplit_once('/') { @@ -833,7 +982,7 @@ async fn handle_move( // Return ETag and OC-ETag so Nextcloud clients can track the moved file. let dest_internal = nc_to_internal_path(&user.username, &dest_subpath)?; - let mut builder = Response::builder().status(StatusCode::CREATED); + let mut builder = Response::builder().status(final_status); if let Ok(moved) = file_service.get_file_by_path(&dest_internal).await { // Route through `FileDto::etag` so the MOVE response // matches what a subsequent PROPFIND on the destination @@ -909,7 +1058,7 @@ async fn handle_move( } return Ok(Response::builder() - .status(StatusCode::CREATED) + .status(final_status) .body(Body::empty()) .unwrap()); } @@ -922,7 +1071,7 @@ async fn handle_move( /// Only accepts relative paths or absolute URLs whose path starts with the /// expected DAV prefix. For full URLs the host is ignored — the path alone is /// used — so an attacker cannot redirect the server to a different host. -fn extract_nc_subpath_from_dest(dest: &str, username: &str) -> Option { +pub fn extract_nc_subpath_from_dest(dest: &str, username: &str) -> Option { let prefix = format!("/remote.php/dav/files/{}/", username); // For full URLs, extract the path portion (everything after the authority). let path = if dest.starts_with("http://") || dest.starts_with("https://") { diff --git a/tests/api/public_shares.hurl b/tests/api/public_shares.hurl index 34f49657..81b02edc 100644 --- a/tests/api/public_shares.hurl +++ b/tests/api/public_shares.hurl @@ -136,15 +136,45 @@ body contains "{{shared_file_id}}" # ───────────────────────────────────────────────────────────── -# 8 — Direct file share: mint a share on the FILE itself -# (item_type=file) and access it via /api/s/{token}. -# -# KNOWN BUG: GET /api/s/{folder-token}/file/{file_id} (the -# "fetch a file from inside a shared folder" route at -# share_handler.rs:653) currently returns 500. We sidestep -# it here by sharing the file directly. When the folder-file -# path is fixed, add a new scenario asserting it returns -# 200 + body, and back-link this comment. +# 8 — Fetch a file from inside the FOLDER share via +# /api/s/{folder-token}/file/{file_id}. This is the path NC +# desktop and web clients use to download a single file out +# of a shared folder without zipping the whole tree. The +# handler must (a) accept the file_id only when the file +# lives in the share's subtree, and (b) refuse with 404 for +# any file outside the subtree (anti-enumeration: the same +# status as "file doesn't exist", so the caller can't probe +# for foreign file ids). +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/s/{{share_token}}/file/{{shared_file_id}} + +HTTP 200 +[Asserts] +header "Content-Disposition" contains "hello.txt" + + +# A file the caller owns but that isn't inside the shared +# folder MUST 404 — same shape as "no such file", so the +# response can't be used to enumerate file ids. +POST {{base_url}}/api/files/upload +Authorization: Bearer {{admin_token}} +[MultipartFormData] +folder_id: {{admin_home_id}} +file: file,fixtures/hello.txt; text/plain + +HTTP 201 +[Captures] +outsider_file_id: jsonpath "$.id" + + +GET {{base_url}}/api/s/{{share_token}}/file/{{outsider_file_id}} + +HTTP 404 + + +# ───────────────────────────────────────────────────────────── +# 8b — Direct file share: mint a share on the FILE itself +# (item_type=file) and access it via /api/s/{token}. # ───────────────────────────────────────────────────────────── POST {{base_url}}/api/shares Authorization: Bearer {{admin_token}} @@ -256,6 +286,14 @@ DELETE {{base_url}}/api/shares/{{file_share_id}} Authorization: Bearer {{admin_token}} HTTP 204 +# The "outsider" hello.txt sits in admin's home folder, not under the +# shared subtree — delete it explicitly so the next test in the +# runner (permissions.hurl) can upload its own hello.txt to the same +# folder without hitting the live-name unique index (409). +DELETE {{base_url}}/api/files/{{outsider_file_id}} +Authorization: Bearer {{admin_token}} +HTTP 204 + DELETE {{base_url}}/api/folders/{{share_folder_id}} Authorization: Bearer {{admin_token}} HTTP 204 diff --git a/tests/webdav/lib/dav_helpers.sh b/tests/webdav/lib/dav_helpers.sh index f5911269..d6840b4e 100755 --- a/tests/webdav/lib/dav_helpers.sh +++ b/tests/webdav/lib/dav_helpers.sh @@ -84,6 +84,14 @@ dav_curl() { curl -s -H "Authorization: Bearer $TOKEN" "$@" } +# Return the HTTP status code of a `PROPFIND Depth: 0` against the +# given NC URL. Used by existence assertions ("did this collection +# silently get auto-created?") where the only thing the caller cares +# about is the status (404 → absent, 207 → present). +nc_status_propfind_depth0() { + nc_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$1" +} + # Count `` (or ``) children in a multistatus # body. Case-insensitive on the namespace prefix because OxiCloud's # two DAV surfaces use different cases: the NC handler emits diff --git a/tests/webdav/test_native_webdav_lifecycle.sh b/tests/webdav/test_native_webdav_lifecycle.sh index 828d22bf..48284d86 100755 --- a/tests/webdav/test_native_webdav_lifecycle.sh +++ b/tests/webdav/test_native_webdav_lifecycle.sh @@ -180,39 +180,18 @@ pass "M4: Range bytes=0-9 → 206 + 10 bytes" # code path where it should actually work: root-level MOVE of a # file PUT at root. If even this 404s, the bug is broader and # native MOVE is unusable, not just nested. -echo " M5: MOVE /webdav/m3-sample.txt → /webdav/m5-moved.txt" +echo " M5: MOVE /webdav/m3-sample.txt → /webdav/m5-moved.txt → 201/204" STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X MOVE \ -H "Destination: $DAV_BASE/m5-moved.txt" \ "$DAV_BASE/m3-sample.txt") -case "$STATUS" in - 201|204) - pass "M5: root-level MOVE → $STATUS" - ;; - 404) - # KNOWN BUG: native MOVE returns 404 on a file that was - # PUT at the same path, even at root level. The strict - # `resolve_path_for_user` SQL query doesn't match what - # the PUT's `save_file_from_temp_with_dedup` stored — - # most likely because the WebDAV dispatcher's path - # prepending (`resolve_webdav_path` → "My Folder - X/foo") - # doesn't match the user's actual home folder path - # field in the DB. Same root cause makes nested MOVE - # (see M3 comment) unusable too. - # - # Where the fix lives: - # `interfaces/api/handlers/webdav_handler.rs::handle_move` - # currently calls `resolver.resolve_path_for_user`. It - # should either: - # (a) fall back to `file_retrieval_service.get_file_by_path` - # (the same lookup GET uses successfully), or - # (b) normalise the source path through the same - # transformer the PUT writes through. - pass "M5: root-level MOVE → 404 (KNOWN BUG: resolve_path_for_user mismatch — pinned)" - ;; - *) - fail "M5: unexpected status $STATUS" - ;; -esac +[[ "$STATUS" == "201" || "$STATUS" == "204" ]] \ + || fail "M5: root-level MOVE expected 201/204, got $STATUS" +# Source is gone, destination present. +[[ "$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/m3-sample.txt")" == "404" ]] \ + || fail "M5: source still resolvable after MOVE" +[[ "$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/m5-moved.txt")" == "207" ]] \ + || fail "M5: destination not found after MOVE" +pass "M5: root-level MOVE → $STATUS, source gone, destination present" # ───────────────────────────────────────────────────────────── # M6 — MKCOL sub/ → 201 @@ -228,16 +207,11 @@ pass "M6: native MKCOL → 201" # ───────────────────────────────────────────────────────────── echo " M7: DELETE /webdav/m6-sub/ → 204" STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X DELETE "$DAV_BASE/m6-sub/") -case "$STATUS" in - 204) pass "M7: native DELETE → 204" ;; - 404) - # If DELETE also hits the resolve_path_for_user 404 trap - # (it uses the same resolver), pin as same root-cause - # KNOWN BUG. - pass "M7: native DELETE → 404 (KNOWN BUG: same resolve_path_for_user mismatch as M5 — pinned)" - ;; - *) fail "M7: unexpected status $STATUS" ;; -esac +[[ "$STATUS" == "204" ]] \ + || fail "M7: native DELETE expected 204, got $STATUS" +[[ "$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/m6-sub/")" == "404" ]] \ + || fail "M7: folder still resolvable after DELETE" +pass "M7: native DELETE → 204, folder gone" # ───────────────────────────────────────────────────────────── # M8 — COPY a.txt → b.txt (pin whatever current behaviour is) @@ -245,57 +219,25 @@ esac # M8 source depends on whether M5 MOVE actually worked. If M5 was # pinned as KNOWN BUG (404), the source for M8 is still # m3-sample.txt at root, not m5-moved.txt. -echo " M8: COPY native source → /webdav/m8-copy.txt" -M8_SOURCE_URL="$DAV_BASE/m3-sample.txt" -# If M5 actually moved the file, the source name changed. -if dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/m5-moved.txt" | grep -q "207"; then - M8_SOURCE_URL="$DAV_BASE/m5-moved.txt" +echo " M8: COPY /webdav/m5-moved.txt → /webdav/m8-copy.txt" +# M5 now succeeds, so the source is at m5-moved.txt. (Kept fallback +# to m3-sample.txt to surface a clear error if M5 regressed.) +M8_SOURCE_URL="$DAV_BASE/m5-moved.txt" +if ! dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/m5-moved.txt" | grep -q "207"; then + M8_SOURCE_URL="$DAV_BASE/m3-sample.txt" fi STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X COPY \ -H "Destination: $DAV_BASE/m8-copy.txt" \ "$M8_SOURCE_URL") -case "$STATUS" in - 201|204) - # Confirm source still exists (COPY != MOVE). - SRC_STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$M8_SOURCE_URL") - DST_STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/m8-copy.txt") - [[ "$SRC_STATUS" == "207" ]] \ - || fail "M8: COPY removed source ($SRC_STATUS instead of 207) — that's MOVE behaviour, not COPY" - [[ "$DST_STATUS" == "207" ]] \ - || fail "M8: destination not present after COPY ($DST_STATUS)" - pass "M8: native COPY → $STATUS, source preserved, destination present" - ;; - 405) - pass "M8: native COPY → 405 METHOD_NOT_ALLOWED — handler not implemented, pinned" - ;; - 404) - pass "M8: native COPY → 404 (KNOWN BUG: same resolve_path_for_user mismatch as M5/M7 — pinned)" - ;; - 500) - # KNOWN BUG: the COPY file branch at - # `interfaces/api/handlers/webdav_handler.rs::handle_copy` - # line ~1639 passes `(file.id, user.id, target_folder_id)` - # to `copy_file_with_perms` — no destination NAME. The - # copy therefore lands in the target folder under the - # SOURCE's name, ignoring the rename the client requested. - # When source and destination resolve to the same folder - # (common for root-level COPY), this collides with the - # source itself → AlreadyExists → leaks as 500. - # - # Where the fix lives: same handler — either - # (a) extend `copy_file_with_perms` to accept an - # optional new name (the folder-tree branch on - # line ~1591 already passes a name into - # `copy_folder_tree_with_perms`), or - # (b) follow the copy with a `rename_file_with_perms` - # call if `dest_filename != source.name` (mirrors - # what MOVE does at line ~1347). - pass "M8: native COPY → 500 (KNOWN BUG: dest filename discarded, collides with source — pinned)" - ;; - *) - fail "M8: unexpected COPY status $STATUS" - ;; -esac +[[ "$STATUS" == "201" || "$STATUS" == "204" ]] \ + || fail "M8: native COPY expected 201/204, got $STATUS" +SRC_STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$M8_SOURCE_URL") +DST_STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/m8-copy.txt") +[[ "$SRC_STATUS" == "207" ]] \ + || fail "M8: COPY removed source ($SRC_STATUS instead of 207) — that's MOVE behaviour, not COPY" +[[ "$DST_STATUS" == "207" ]] \ + || fail "M8: destination not present after COPY ($DST_STATUS)" +pass "M8: native COPY → $STATUS, source preserved, destination renamed correctly" # ═════════════════════════════════════════════════════════════ # Group N — LOCK / UNLOCK @@ -332,44 +274,101 @@ LOCK_TOKEN=$(grep -i '^lock-token:' <<< "$HEADERS" | awk '{print $2}' | tr -d '\ pass "N1: LOCK → 200 + Lock-Token=$LOCK_TOKEN" # ───────────────────────────────────────────────────────────── -# N2 — PUT without the lock token → 423 Locked -# ───────────────────────────────────────────────────────────── -# ───────────────────────────────────────────────────────────── -# N2 — PUT to a locked file without the token +# N2 — PUT to a locked file without the token → 423 Locked # # RFC 4918 §9.10.4 + §6: a writeable resource under an # exclusive lock MUST reject conflicting writes with 423 -# Locked. OxiCloud's native handler currently does NOT consult -# the lock store before writing — LOCK just produces a token, -# and any PUT/DELETE/MOVE/PROPPATCH succeeds regardless. The -# class-2 DAV advertisement in M1 is therefore aspirational: -# the protocol surface exists, the enforcement doesn't. -# -# Where the fix lives: -# `interfaces/api/handlers/webdav_handler.rs::handle_put` (and -# the mutator paths in handle_delete / handle_move / handle_copy / -# handle_proppatch) — each needs to check the WebDAV lock service -# for an active lock on the target path and reject with 423 if -# the request doesn't carry a matching `If: ()` header. -# The lock store itself already records tokens — confirmed by N1 -# capturing one — so the gap is purely on the read-side check. +# Locked unless the request submits the lock token in `If:`. +# N2b verifies the inverse: same PUT with the correct +# `If: ()` header succeeds, proving the gate isn't +# blocking legitimate updates from the lock owner. # ───────────────────────────────────────────────────────────── -echo " N2: PUT /webdav/n-locked.txt without If:() — pinned: lock not enforced (RFC would 423)" +echo " N2: PUT /webdav/n-locked.txt without If:() → 423" STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PUT \ -H "Content-Type: text/plain" \ --data-binary 'tampered contents' \ "$DAV_BASE/n-locked.txt") -case "$STATUS" in - 204) - pass "N2: PUT succeeded despite active lock → 204 (KNOWN BUG: lock not enforced — pinned)" - ;; - 423) - fail "N2: server now returns 423 Locked. Lock enforcement was added — update this pin to assert == 423." - ;; - *) - fail "N2: unexpected status $STATUS" - ;; -esac +[[ "$STATUS" == "423" ]] \ + || fail "N2: expected 423 Locked for PUT to locked path without token, got $STATUS" +pass "N2: PUT to locked path without token → 423" + +echo " N2b: PUT /webdav/n-locked.txt WITH If:() → 204" +STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PUT \ + -H "Content-Type: text/plain" \ + -H "If: (<$LOCK_TOKEN>)" \ + --data-binary 'authorised update' \ + "$DAV_BASE/n-locked.txt") +[[ "$STATUS" == "204" ]] \ + || fail "N2b: expected 204 No Content for PUT with correct lock token, got $STATUS" +pass "N2b: PUT with matching If:() → 204" + +# ───────────────────────────────────────────────────────────── +# N2c–N2f — Lock enforcement on the other mutator methods +# +# RFC 4918 §9.10.4: a lock binds every mutating method, not just +# PUT. The native handler's `enforce_native_lock` helper was +# designed to be called by handle_delete / handle_move / +# handle_copy / handle_proppatch as well — these tests prove the +# wire is in. Each case uses the n-locked.txt resource locked +# above and a `WITHOUT If:` request, expecting 423. Positive +# (with-token) coverage is implicit: the M-series above already +# exercises each method on unlocked resources and asserts the +# success codes, so a regression that hard-rejected every call +# would fail there. +# +# Order matters: each must run while the lock is still held, +# i.e. before N3 below releases it. +# ───────────────────────────────────────────────────────────── +echo " N2c: DELETE /webdav/n-locked.txt without If:() → 423" +STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X DELETE \ + "$DAV_BASE/n-locked.txt") +[[ "$STATUS" == "423" ]] \ + || fail "N2c: expected 423 Locked for DELETE on locked path without token, got $STATUS" +# The file must still be present after a rejected DELETE. +[[ "$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/n-locked.txt")" == "207" ]] \ + || fail "N2c: file removed after rejected DELETE (423 was advisory only?)" +pass "N2c: DELETE on locked path without token → 423, resource preserved" + +echo " N2d: MOVE /webdav/n-locked.txt without If:() → 423 (source-side lock)" +STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X MOVE \ + -H "Destination: $DAV_BASE/n-locked-moved.txt" \ + "$DAV_BASE/n-locked.txt") +[[ "$STATUS" == "423" ]] \ + || fail "N2d: expected 423 Locked for MOVE on locked source without token, got $STATUS" +[[ "$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/n-locked.txt")" == "207" ]] \ + || fail "N2d: source disappeared after rejected MOVE" +[[ "$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/n-locked-moved.txt")" == "404" ]] \ + || fail "N2d: destination created after rejected MOVE" +pass "N2d: MOVE with locked source and no token → 423, no state mutated" + +echo " N2e: COPY into /webdav/n-locked.txt (locked destination) without If:() → 423" +# Set up a fresh unlocked source for the COPY. +dav_curl -o /dev/null -X PUT -H "Content-Type: text/plain" \ + --data-binary 'n2e copy source' \ + "$DAV_BASE/n2e-copy-src.txt" > /dev/null +STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X COPY \ + -H "Destination: $DAV_BASE/n-locked.txt" \ + "$DAV_BASE/n2e-copy-src.txt") +[[ "$STATUS" == "423" ]] \ + || fail "N2e: expected 423 Locked for COPY into locked destination without token, got $STATUS" +# The locked destination's content must not have been replaced. +BODY=$(dav_curl -s "$DAV_BASE/n-locked.txt") +[[ "$BODY" == "authorised update" ]] \ + || fail "N2e: locked destination's content was overwritten (got '$BODY')" +pass "N2e: COPY into locked destination without token → 423, target untouched" + +echo " N2f: PROPPATCH /webdav/n-locked.txt without If:() → 423" +PROPPATCH_BODY=' + + tampered +' +STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PROPPATCH \ + -H "Content-Type: application/xml" \ + --data "$PROPPATCH_BODY" \ + "$DAV_BASE/n-locked.txt") +[[ "$STATUS" == "423" ]] \ + || fail "N2f: expected 423 Locked for PROPPATCH on locked path without token, got $STATUS" +pass "N2f: PROPPATCH on locked path without token → 423" # ───────────────────────────────────────────────────────────── # N3 — UNLOCK with token → 204; subsequent PUT succeeds diff --git a/tests/webdav/test_nc_move_copy_delete_trash.sh b/tests/webdav/test_nc_move_copy_delete_trash.sh index b3057d3b..4a26930c 100755 --- a/tests/webdav/test_nc_move_copy_delete_trash.sh +++ b/tests/webdav/test_nc_move_copy_delete_trash.sh @@ -108,64 +108,65 @@ STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MOVE \ pass "G3: URL-encoded destination decoded correctly" # ───────────────────────────────────────────────────────────── -# G4 / G5 — Overwrite header behaviour (pinned: not honoured) +# G4 / G5 / G5b — Overwrite header (RFC 4918 §9.9.4) +# +# G4 : Overwrite: F + destination exists → 412 (refuse) +# G5 : Overwrite: T + destination exists → 204 (replace) +# G5b : Overwrite header absent → default T per spec → 204 +# G5c : Overwrite: F + destination ABSENT → 201 (normal create) # ───────────────────────────────────────────────────────────── -echo " G4: MOVE with Overwrite: F to an existing path (pinned: SERVER BUG — leaks 500)" +echo " G4: MOVE with Overwrite: F to an existing path → 412" put_nc_file "g4-src.txt" "G4 source" put_nc_file "g4-dest.txt" "G4 destination (should remain)" STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MOVE \ -H "Destination: $NC_FILES_BASE/g4-dest.txt" \ -H "Overwrite: F" \ "$NC_FILES_BASE/g4-src.txt") -case "$STATUS" in - 500) - # KNOWN BUG: the NC MOVE handler doesn't intercept - # `Overwrite: F` and doesn't map the domain-layer - # `AlreadyExists` to 412. It tries to rename, the - # storage layer 409s "name already taken", and the - # handler bubbles that up as 500. NC desktop will - # interpret 500 as "server transient error" and - # retry, which masks the real conflict. - # - # The right fix is in `interfaces/nextcloud/webdav_handler.rs::handle_move`: - # check `Overwrite: F` BEFORE attempting the rename, return - # 412 on collision; OR when Overwrite is omitted/T, delete - # the destination first (replace semantics, → 204). - pass "G4: Overwrite: F → 500 (KNOWN BUG: should be 412 per RFC 4918 §9.9.4 — pinned)" - ;; - 412) - fail "G4: server now correctly returns 412 for Overwrite: F. Bug is fixed — update this pin to assert == 412." - ;; - 201|204) - fail "G4: server now silently overwrites despite Overwrite: F (status $STATUS) — this would be a *different* bug; RFC requires 412." - ;; - *) - fail "G4: unexpected status $STATUS" - ;; -esac +[[ "$STATUS" == "412" ]] \ + || fail "G4: expected 412 Precondition Failed for Overwrite: F + collision, got $STATUS" +# Source and destination must both still exist with original contents. +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g4-src.txt")" == "207" ]] \ + || fail "G4: source disappeared after 412 (move should have been refused, not partially applied)" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g4-dest.txt")" == "207" ]] \ + || fail "G4: destination disappeared after 412" +pass "G4: Overwrite: F + collision → 412, source and destination intact" -echo " G5: MOVE with Overwrite: T to an existing path (pinned: SERVER BUG — leaks 500)" +echo " G5: MOVE with Overwrite: T to an existing path → 204" put_nc_file "g5-src.txt" "G5 source" put_nc_file "g5-dest.txt" "G5 destination (to be replaced)" STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MOVE \ -H "Destination: $NC_FILES_BASE/g5-dest.txt" \ -H "Overwrite: T" \ "$NC_FILES_BASE/g5-src.txt") -case "$STATUS" in - 500) - # Same root cause as G4: the handler doesn't consider the - # `Overwrite` header at all. With `Overwrite: T` it SHOULD - # delete the destination first and proceed (→ 204), but - # today it bubbles up the storage-layer "Already Exists". - pass "G5: Overwrite: T → 500 (KNOWN BUG: should be 204 per RFC 4918 §9.9.4 — pinned)" - ;; - 204) - fail "G5: server now correctly returns 204 for Overwrite: T. Bug is fixed — update this pin to assert == 204." - ;; - *) - fail "G5: unexpected status $STATUS" - ;; -esac +[[ "$STATUS" == "204" ]] \ + || fail "G5: expected 204 No Content for Overwrite: T + collision, got $STATUS" +# Source gone, destination now has the source's content. +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g5-src.txt")" == "404" ]] \ + || fail "G5: source still present after successful overwrite move" +DEST_BODY=$(nc_curl -s "$NC_FILES_BASE/g5-dest.txt") +[[ "$DEST_BODY" == "G5 source" ]] \ + || fail "G5: destination content not replaced; got '$DEST_BODY'" +pass "G5: Overwrite: T + collision → 204, destination replaced" + +echo " G5b: MOVE with no Overwrite header to an existing path → 204 (default T)" +put_nc_file "g5b-src.txt" "G5b source" +put_nc_file "g5b-dest.txt" "G5b destination (default-overwrite target)" +STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MOVE \ + -H "Destination: $NC_FILES_BASE/g5b-dest.txt" \ + "$NC_FILES_BASE/g5b-src.txt") +[[ "$STATUS" == "204" ]] \ + || fail "G5b: expected 204 No Content for missing Overwrite header (default T), got $STATUS" +pass "G5b: absent Overwrite defaults to T → 204" + +echo " G5c: MOVE with Overwrite: F to a NEW path → 201 (no collision to refuse)" +put_nc_file "g5c-src.txt" "G5c source" +STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MOVE \ + -H "Destination: $NC_FILES_BASE/g5c-fresh-dest.txt" \ + -H "Overwrite: F" \ + "$NC_FILES_BASE/g5c-src.txt") +[[ "$STATUS" == "201" ]] \ + || fail "G5c: expected 201 Created for Overwrite: F + no collision, got $STATUS" +pass "G5c: Overwrite: F + new destination → 201" # ───────────────────────────────────────────────────────────── # G6 — MOVE a folder (subtree) @@ -255,7 +256,7 @@ pass "G8: DELETE → 204 + GET 404" # descendant assertions below will trip and you can flip them # to strict 404. # ───────────────────────────────────────────────────────────── -echo " G9: DELETE folder (pinned: descendants currently orphan — KNOWN BUG)" +echo " G9: DELETE folder cascades soft-delete to descendants" nc_curl -o /dev/null -X MKCOL "$NC_FILES_BASE/g9-tree/" > /dev/null nc_curl -o /dev/null -X MKCOL "$NC_FILES_BASE/g9-tree/inner/" > /dev/null put_nc_file "g9-tree/file.txt" "G9 file" @@ -264,22 +265,49 @@ STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X DELETE "$NC_FILES_BASE/g9-tre [[ "$STATUS" == "204" ]] \ || fail "G9: folder DELETE expected 204, got $STATUS" -# Folder itself: correctly 404. +# Folder itself: 404. [[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/")" == "404" ]] \ - || fail "G9: folder still present after DELETE — that part should always be 404" + || fail "G9: folder still resolvable after DELETE" -# Descendants: pin the current (buggy) "still alive" status. -# Either current 207 (bug) or future 404 (fix) is acceptable; -# anything else means something has drifted unexpectedly. -CHILD_STATUS=$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/file.txt") -DEEP_STATUS=$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/inner/deep.txt") -if [[ "$CHILD_STATUS" == "207" && "$DEEP_STATUS" == "207" ]]; then - pass "G9: descendants still reachable (file=207, deep=207) — KNOWN BUG pinned: move_to_trash isn't recursive at the row level" -elif [[ "$CHILD_STATUS" == "404" && "$DEEP_STATUS" == "404" ]]; then - fail "G9: descendants now correctly 404 (file=$CHILD_STATUS, deep=$DEEP_STATUS) — bug is fixed, flip this case to strict 404 assertions." -else - fail "G9: mixed/unexpected descendant statuses (file=$CHILD_STATUS, deep=$DEEP_STATUS) — pin needs review" -fi +# Descendants must now also be 404 (cascade soft-delete reaches the +# whole subtree). Previous behaviour left them reachable at their +# full path while the parent was gone — a data-integrity drift that +# confused desktop-sync tree walks. +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/file.txt")" == "404" ]] \ + || fail "G9: direct-child file still resolvable after parent DELETE — cascade not working" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/inner/")" == "404" ]] \ + || fail "G9: descendant folder still resolvable after parent DELETE — cascade not working" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/inner/deep.txt")" == "404" ]] \ + || fail "G9: descendant file still resolvable after parent DELETE — cascade not working" +pass "G9: DELETE folder → 204, descendants all 404 (cascade reaches the whole subtree)" + +# G9b — restore the trashed root and verify cascade-restore brings +# every descendant back with the same paths. Cascade-trashed +# descendants (original_parent_id IS NULL) get un-trashed; rows that +# were independently trashed before the folder went to trash stay +# trashed. +echo " G9b: restore the trashed g9-tree → cascade-restore reaches descendants" +BODY=$(nc_curl -X PROPFIND -H "Depth: 1" "$NC_TRASH_BASE/") +G9_TRASHED_HREF=$(extract_response_href_containing "$BODY" "g9-tree") +G9_TRASHED_ID=$(basename "$G9_TRASHED_HREF") +[[ -n "$G9_TRASHED_ID" ]] || fail "G9b: trashed g9-tree not found via PROPFIND" +STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MOVE \ + -H "Destination: $NC_FILES_BASE/g9-tree-restored/" \ + "$NC_TRASH_BASE/$G9_TRASHED_ID") +[[ "$STATUS" == "201" || "$STATUS" == "204" ]] \ + || fail "G9b: restore expected 201/204, got $STATUS" +# The folder and ALL its descendants are reachable again at their +# original paths (restore goes to original location, not the +# Destination header). +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/")" == "207" ]] \ + || fail "G9b: root folder not back after restore" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/file.txt")" == "207" ]] \ + || fail "G9b: direct-child file not restored alongside parent" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/inner/")" == "207" ]] \ + || fail "G9b: descendant folder not restored alongside parent" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/g9-tree/inner/deep.txt")" == "207" ]] \ + || fail "G9b: descendant file not restored alongside parent" +pass "G9b: restored g9-tree carries the whole subtree back" # ═════════════════════════════════════════════════════════════ # Group K — Trashbin DAV (depends on G8's deletion above) @@ -408,28 +436,17 @@ TRASHED_ID=$(basename "$TRASHED_HREF") STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MOVE \ -H "Destination: $NC_FILES_BASE/k5-conflict.txt" \ "$NC_TRASH_BASE/$TRASHED_ID") -case "$STATUS" in - 201|204) - pass "K5: restore-onto-existing → $STATUS (current behaviour pinned: collision NOT prevented at this layer)" - ;; - 412) - pass "K5: restore-onto-existing → 412 (current behaviour pinned: precondition-style refusal)" - ;; - 409) - pass "K5: restore-onto-existing → 409 (current behaviour pinned: name conflict)" - ;; - 500) - # Same shape as the G4/G5 bug — restore is a MOVE under - # the hood, and the handler doesn't catch the storage- - # layer "Already Exists" before it becomes an internal - # error. Pinned because that's the actual current - # behaviour, not because it's correct. - pass "K5: restore-onto-existing → 500 (KNOWN BUG: same root cause as G4/G5 — pinned)" - ;; - *) - fail "K5: unexpected status $STATUS — pin needs reviewing" - ;; -esac +[[ "$STATUS" == "412" ]] \ + || fail "K5: expected 412 Precondition Failed for restore-onto-existing, got $STATUS" +# The trashed item must still be in the trash (refused restore mustn't +# half-delete the trash row). +[[ -n "$(extract_response_href_containing "$(nc_curl -X PROPFIND -H "Depth: 1" "$NC_TRASH_BASE/")" "k5-doomed")" ]] \ + || fail "K5: trash entry vanished after a refused restore" +# The conflicting live file must still be there with its original content. +LIVE_BODY=$(nc_curl -s "$NC_FILES_BASE/k5-conflict.txt") +[[ "$LIVE_BODY" == "k5 original (stays)" ]] \ + || fail "K5: conflicting live file mutated; got '$LIVE_BODY'" +pass "K5: restore-onto-existing → 412, trash row and live file intact" # ── Cleanup ────────────────────────────────────────────────────────────────── echo " cleanup: empty trash + remove residual fixtures" diff --git a/tests/webdav/test_nc_put_mkcol_blake3.sh b/tests/webdav/test_nc_put_mkcol_blake3.sh index ad7fe7b3..8c2e064e 100755 --- a/tests/webdav/test_nc_put_mkcol_blake3.sh +++ b/tests/webdav/test_nc_put_mkcol_blake3.sh @@ -145,27 +145,83 @@ ACTUAL=$(nc_curl "$NC_FILES_BASE/f1-small.txt") pass "F4: GET after overwrite serves the new bytes (no stale-cache)" # ───────────────────────────────────────────────────────────── -# F5 / F6 — Conditional PUT (pinned: currently no-op) +# F5 / F6 — Conditional PUT (RFC 7232 §3.1/§3.2, RFC 4918 §10) +# +# F5 covers `If-None-Match: *`: server MUST refuse the PUT with +# 412 when the target representation already exists (used by +# clients to do "create only if absent"). The mirror case — same +# header on a NEW path — must succeed; covered by F5b. +# +# F6 covers `If-Match: ""`: server MUST refuse the PUT with +# 412 when the supplied ETag doesn't strong-match the current +# representation (used by clients to do "update only if +# unchanged"). The mirror case — correct ETag → success — is +# covered by F6b. # ───────────────────────────────────────────────────────────── -echo " F5: PUT with If-None-Match: * on existing path (pinned current: 204, RFC-4918 would be 412)" +echo " F5: PUT with If-None-Match: * on existing path → 412" HEADERS=$(nc_curl -D - -o /dev/null -X PUT \ -H "If-None-Match: *" -H "Content-Type: text/plain" \ --data-binary 'F5-payload' \ "$NC_FILES_BASE/f1-small.txt") STATUS=$(awk 'NR==1{print $2}' <<< "$HEADERS" | tr -d '\r') -[[ "$STATUS" == "204" || "$STATUS" == "201" ]] \ - || fail "F5: unexpected status $STATUS (expected 204 — current ignore-conditional behaviour)" -pass "F5: PUT honours no conditional headers today — pinned" +[[ "$STATUS" == "412" ]] \ + || fail "F5: expected 412 Precondition Failed for If-None-Match: * on existing path, got $STATUS" +pass "F5: If-None-Match: * on existing path → 412" -echo " F6: PUT with If-Match: \"wrong-etag\" (pinned current: succeeds, RFC-4918 would be 412)" +echo " F5b: PUT with If-None-Match: * on NEW path → 201/204" +HEADERS=$(nc_curl -D - -o /dev/null -X PUT \ + -H "If-None-Match: *" -H "Content-Type: text/plain" \ + --data-binary 'F5b-payload' \ + "$NC_FILES_BASE/f5b-new.txt") +STATUS=$(awk 'NR==1{print $2}' <<< "$HEADERS" | tr -d '\r') +[[ "$STATUS" == "201" || "$STATUS" == "204" ]] \ + || fail "F5b: expected 201/204 for If-None-Match: * on new path, got $STATUS" +pass "F5b: If-None-Match: * on new path → $STATUS" + +echo " F6: PUT with If-Match: \"wrong-etag\" → 412" HEADERS=$(nc_curl -D - -o /dev/null -X PUT \ -H 'If-Match: "deadbeef-never-matches"' -H "Content-Type: text/plain" \ --data-binary 'F6-payload' \ "$NC_FILES_BASE/f1-small.txt") STATUS=$(awk 'NR==1{print $2}' <<< "$HEADERS" | tr -d '\r') -[[ "$STATUS" == "204" || "$STATUS" == "201" ]] \ - || fail "F6: unexpected status $STATUS (expected 204 — current ignore-conditional behaviour)" -pass "F6: PUT honours no If-Match today — pinned" +[[ "$STATUS" == "412" ]] \ + || fail "F6: expected 412 Precondition Failed for non-matching If-Match, got $STATUS" +pass "F6: If-Match with non-matching ETag → 412" + +echo " F6b: PUT with correct If-Match → 204" +# Fetch the current ETag of f1-small.txt via PROPFIND-ish HEAD, +# then re-PUT with that exact value as If-Match. Must succeed. +CURRENT_ETAG=$(nc_curl -D - -o /dev/null -X HEAD "$NC_FILES_BASE/f1-small.txt" \ + | awk 'BEGIN{IGNORECASE=1} /^etag:/ {print $2}' | tr -d '\r') +[[ -n "$CURRENT_ETAG" ]] || fail "F6b: could not read current ETag via HEAD" +HEADERS=$(nc_curl -D - -o /dev/null -X PUT \ + -H "If-Match: $CURRENT_ETAG" -H "Content-Type: text/plain" \ + --data-binary 'F6b-payload' \ + "$NC_FILES_BASE/f1-small.txt") +STATUS=$(awk 'NR==1{print $2}' <<< "$HEADERS" | tr -d '\r') +[[ "$STATUS" == "204" ]] \ + || fail "F6b: expected 204 for If-Match with correct ETag, got $STATUS" +pass "F6b: If-Match with current ETag → 204" + +echo " F6c: PUT with If-Match: * on existing path → 204 (catch-all)" +HEADERS=$(nc_curl -D - -o /dev/null -X PUT \ + -H 'If-Match: *' -H "Content-Type: text/plain" \ + --data-binary 'F6c-payload' \ + "$NC_FILES_BASE/f1-small.txt") +STATUS=$(awk 'NR==1{print $2}' <<< "$HEADERS" | tr -d '\r') +[[ "$STATUS" == "204" ]] \ + || fail "F6c: expected 204 for If-Match: * on existing path, got $STATUS" +pass "F6c: If-Match: * on existing path → 204" + +echo " F6d: PUT with If-Match on NEW path → 412 (resource absent → cannot match)" +HEADERS=$(nc_curl -D - -o /dev/null -X PUT \ + -H 'If-Match: "anything"' -H "Content-Type: text/plain" \ + --data-binary 'F6d-payload' \ + "$NC_FILES_BASE/f6d-new.txt") +STATUS=$(awk 'NR==1{print $2}' <<< "$HEADERS" | tr -d '\r') +[[ "$STATUS" == "412" ]] \ + || fail "F6d: expected 412 for If-Match on absent path, got $STATUS" +pass "F6d: If-Match on absent path → 412" # ───────────────────────────────────────────────────────────── # F7 — PUT a "large" file → succeeds, GET returns exact bytes @@ -263,38 +319,43 @@ grep -q '' <<< "$BODY" \ pass "F10: MKCOL creates folder, PROPFIND sees it as a collection" # ───────────────────────────────────────────────────────────── -# F11 — MKCOL with missing intermediate parent +# F11 / F11b / F11c — MKCOL parent semantics (RFC 4918 §9.3.1) # -# Pinned current behaviour: OxiCloud's MKCOL auto-creates -# missing intermediate parents (effectively `mkdir -p` -# semantics). Sending MKCOL on `/a/b/c/` where neither `a` nor -# `b` exists succeeds with 201 — both intermediates are -# silently created. +# F11 : missing intermediate parent → 409 Conflict +# F11b : parent exists, target new → 201 Created (positive case) +# F11c : target already exists → 405 Method Not Allowed # -# Strict RFC 4918 §9.3.1 requires 409 Conflict here ("when the -# parent collection does not exist"). NC desktop tolerates -# either behaviour (it always MKCOLs ancestors one at a time -# during sync), so the auto-create behaviour is harmless in -# practice — but if you ever want strict mode, the fix lives -# in `interfaces/nextcloud/webdav_handler.rs::handle_mkcol`: -# look up the parent path before creating; 409 if missing. +# Sabre/DAV and the actual NC server both 409 on a missing +# intermediate; our previous `mkdir -p` behaviour deviated. NC +# desktop walks ancestors one MKCOL at a time during sync so +# nothing real breaks from dropping the auto-create. # ───────────────────────────────────────────────────────────── -echo " F11: MKCOL with missing parent (pinned: auto-creates parents, RFC-4918 would 409)" +echo " F11: MKCOL with missing intermediate parent → 409" STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MKCOL \ "$NC_FILES_BASE/f11-nonexistent-parent/inner/") -case "$STATUS" in - 201) - pass "F11: MKCOL auto-created intermediate parents (201) — pinned current behaviour" - ;; - 409) - fail "F11: server now returns 409 (RFC-4918 strict). Bug? Improvement? — review and update pin to strict assertion." - ;; - *) - fail "F11: unexpected status $STATUS" - ;; -esac -# Cleanup the auto-created parent so subsequent tests don't see it. -nc_curl -o /dev/null -X DELETE "$NC_FILES_BASE/f11-nonexistent-parent/" > /dev/null 2>&1 || true +[[ "$STATUS" == "409" ]] \ + || fail "F11: expected 409 Conflict for MKCOL with missing parent, got $STATUS" +# The non-existent parent must NOT have been auto-created either. +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/f11-nonexistent-parent/")" == "404" ]] \ + || fail "F11: intermediate parent was silently created — auto-create still happening" +pass "F11: MKCOL with missing parent → 409, parent not silently created" + +echo " F11b: MKCOL with existing parent + new target → 201" +nc_curl -o /dev/null -X MKCOL "$NC_FILES_BASE/f11b-parent/" > /dev/null +STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MKCOL \ + "$NC_FILES_BASE/f11b-parent/child/") +[[ "$STATUS" == "201" ]] \ + || fail "F11b: expected 201 Created for MKCOL with existing parent, got $STATUS" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/f11b-parent/child/")" == "207" ]] \ + || fail "F11b: target collection not visible via PROPFIND after MKCOL" +pass "F11b: MKCOL with existing parent → 201, target reachable" + +echo " F11c: MKCOL with target that already exists → 405" +STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MKCOL \ + "$NC_FILES_BASE/f11b-parent/child/") +[[ "$STATUS" == "405" ]] \ + || fail "F11c: expected 405 Method Not Allowed for MKCOL on existing collection, got $STATUS" +pass "F11c: MKCOL on existing target → 405" # ───────────────────────────────────────────────────────────── # F12 — MKCOL on existing folder → 405