From f62cf0b65f93f8aed353ceeaece68cb94d183496 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Wed, 17 Jun 2026 01:44:24 +0200 Subject: [PATCH] fix(nc/webdav): trash restore refuses MOVE onto a live destination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the client sends `MOVE /trashbin/{id}` with a `Destination` header, handle_restore now resolves the destination path and returns 412 Precondition Failed if a live file or folder already sits there — matching Sabre/DAV and the NC desktop client's expectation. There is no `Overwrite: T` workflow for trash restore in either reference implementation (silently replacing a live file with an undeleted one is a footgun), so the refusal is unconditional. The destination header is extracted at the dispatch site as an owned String so the future stays Send-compatible (`&Request` is not Sync because the body trait object is Send-only). `extract_nc_subpath_from_dest` is promoted to `pub` so trashbin_handler can share the same URL parser as handle_move. --- src/interfaces/nextcloud/trashbin_handler.rs | 40 +++++++++++++++++++- src/interfaces/nextcloud/webdav_handler.rs | 6 ++- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/src/interfaces/nextcloud/trashbin_handler.rs b/src/interfaces/nextcloud/trashbin_handler.rs index 0c6a9461..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,6 +117,33 @@ async fn handle_restore( .as_ref() .ok_or_else(|| AppError::internal_error("Trash service not available"))?; + // 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()); + } + } + match trash_svc.restore_item(&id, user.id).await { Ok(()) => Ok(Response::builder() .status(StatusCode::CREATED) diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index a13dc874..204c9bce 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -737,7 +737,9 @@ async fn handle_mkcol( 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")); + 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"); @@ -1061,7 +1063,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://") {