fix(nc/webdav): honour Overwrite on MOVE; restore-onto-existing → 412

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.
This commit is contained in:
Edouard Vanbelle
2026-06-16 23:24:00 +02:00
parent 9f2ebd0758
commit 5cb01b201d
3 changed files with 157 additions and 77 deletions
+34 -9
View File
@@ -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) ────────────────────
+67 -2
View File
@@ -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());
}