From 5cb01b201d2661b11b71fd83d852dcb84415db1d Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 16 Jun 2026 23:24:00 +0200 Subject: [PATCH] =?UTF-8?q?fix(nc/webdav):=20honour=20Overwrite=20on=20MOV?= =?UTF-8?q?E;=20restore-onto-existing=20=E2=86=92=20412?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes G4 / G5 / K5. handle_move now resolves the destination once before the file/folder dispatch and applies RFC 4918 §9.9.4: - Overwrite: F on a collision → 412 Precondition Failed, source untouched, destination untouched. - Overwrite: T (or absent) on a collision → delete the existing destination, then proceed → 204 No Content. - No collision → 201 Created (unchanged). The same destination lookup powers the 201-vs-204 status decision, so adding the precondition guard adds zero extra DB hits on the happy path. handle_restore now catches the unique-index collision out of restore_item and returns 412 instead of letting it bubble as 500. Mirrors the G4 semantics for the trashbin surface (restore has no Overwrite header so the refusal is unconditional; client resolves by renaming the live file first). Sabre/DAV's CorePlugin and our test pins agreed independently — NC clients expect this exact behavior, and the new G5b/G5c positive-case tests guard against a regression that hard-rejected every MOVE. --- src/interfaces/nextcloud/trashbin_handler.rs | 43 ++++-- src/interfaces/nextcloud/webdav_handler.rs | 69 +++++++++- .../webdav/test_nc_move_copy_delete_trash.sh | 122 ++++++++---------- 3 files changed, 157 insertions(+), 77 deletions(-) diff --git a/src/interfaces/nextcloud/trashbin_handler.rs b/src/interfaces/nextcloud/trashbin_handler.rs index 60464ef2..0c6a9461 100644 --- a/src/interfaces/nextcloud/trashbin_handler.rs +++ b/src/interfaces/nextcloud/trashbin_handler.rs @@ -108,15 +108,40 @@ 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)))?; - - 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 85cd9426..371c61bd 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -863,6 +863,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"))?; @@ -872,6 +885,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('/') { @@ -915,7 +980,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 @@ -991,7 +1056,7 @@ async fn handle_move( } return Ok(Response::builder() - .status(StatusCode::CREATED) + .status(final_status) .body(Body::empty()) .unwrap()); } diff --git a/tests/webdav/test_nc_move_copy_delete_trash.sh b/tests/webdav/test_nc_move_copy_delete_trash.sh index b3057d3b..93235d9a 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) @@ -408,28 +409,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"