cjw/Oxicloud
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(nc/webdav): trash restore refuses MOVE onto a live destination

Browse Source 
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<Body>` 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.
This commit is contained in:
Edouard Vanbelle
2026-06-17 01:44:24 +02:00
parent f9de7ac596
commit f62cf0b65f
2 changed files with 42 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/interfaces/nextcloud/trashbin_handler.rs
+38 -2
View File
@@ -9,12 +9,15 @@ use quick_xml::{
}; };
use std::sync::Arc; 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::application::ports::trash_ports::TrashUseCase;
use crate::common::di::AppState; use crate::common::di::AppState;
use crate::interfaces::errors::AppError; use crate::interfaces::errors::AppError;
use crate::interfaces::middleware::auth::{AuthUser, CurrentUser}; use crate::interfaces::middleware::auth::{AuthUser, CurrentUser};
use crate::interfaces::nextcloud::webdav_handler::{ 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"); const HEADER_DAV: HeaderName = HeaderName::from_static("dav");
@@ -37,7 +40,12 @@ pub async fn handle_nc_trashbin(
handle_propfind(state, &user).await handle_propfind(state, &user).await
} }
"MOVE" if subpath_trimmed.starts_with("trash/") => { "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() => { "DELETE" if subpath_trimmed == "trash" || subpath_trimmed.is_empty() => {
handle_empty_trash(state, &user).await handle_empty_trash(state, &user).await
@@ -98,6 +106,7 @@ async fn handle_propfind(
async fn handle_restore( async fn handle_restore(
state: Arc<AppState>, state: Arc<AppState>,
dest_header: Option<String>,
user: &CurrentUser, user: &CurrentUser,
subpath: &str, subpath: &str,
) -> Result<Response<Body>, AppError> { ) -> Result<Response<Body>, AppError> {
@@ -108,6 +117,33 @@ async fn handle_restore(
.as_ref() .as_ref()
.ok_or_else(|| AppError::internal_error("Trash service not available"))?; .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 { match trash_svc.restore_item(&id, user.id).await {
Ok(()) => Ok(Response::builder() Ok(()) => Ok(Response::builder()
.status(StatusCode::CREATED) .status(StatusCode::CREATED)
src/interfaces/nextcloud/webdav_handler.rs
+4 -2
View File
@@ -737,7 +737,9 @@ async fn handle_mkcol(
let segments: Vec<&str> = subpath.split('/').filter(|s| !s.is_empty()).collect(); let segments: Vec<&str> = subpath.split('/').filter(|s| !s.is_empty()).collect();
if segments.is_empty() { 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"); 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 /// 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 /// 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. /// used — so an attacker cannot redirect the server to a different host.
fn extract_nc_subpath_from_dest(dest: &str, username: &str) -> Option<String> { pub fn extract_nc_subpath_from_dest(dest: &str, username: &str) -> Option<String> {
let prefix = format!("/remote.php/dav/files/{}/", username); let prefix = format!("/remote.php/dav/files/{}/", username);
// For full URLs, extract the path portion (everything after the authority). // For full URLs, extract the path portion (everything after the authority).
let path = if dest.starts_with("http://") || dest.starts_with("https://") { let path = if dest.starts_with("http://") || dest.starts_with("https://") {