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/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index ff9d5718..83987e28 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -1365,6 +1365,17 @@ 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. @@ -1462,17 +1473,33 @@ async fn handle_move( } ResolvedResource::File(file) => { if source_parent_path != dest_parent_path { - if !dest_parent_path.is_empty() - && let Ok(parent) = folder_service.get_folder_by_path(dest_parent_path).await - { + // 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 - .move_file_with_perms(&file.id, user.id, Some(dest_parent_path.to_string())) + .move_file_with_perms(&file.id, user.id, target_parent_id) .await .map_err(AppError::from)?; } @@ -1547,6 +1574,13 @@ 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, @@ -1654,24 +1688,20 @@ async fn handle_copy( } } ResolvedResource::File(file) => { - // M8b fix: copy_file_with_perms takes (file_id, caller, target_folder) - // but no rename — so a copy to a different name in the SAME folder - // (typical for root-level "duplicate" pattern) collided with the - // source filename and 500'd. Mirror MOVE: copy first, then rename - // if the dest filename differs from the source's name. + // 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 copied = file_management_service - .copy_file_with_perms(&file.id, user.id, target_parent_id) + let copy_name = (file.name != dest_name).then(|| dest_name.to_string()); + file_management_service + .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)))?; - if file.name != dest_name { - file_management_service - .rename_file_with_perms(&copied.id, user.id, dest_name) - .await - .map_err(|e| { - AppError::internal_error(format!("Failed to rename copied file: {}", e)) - })?; - } } }