From eb895b0412c798a8891c7c03017e7caf94b616eb Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 6 Jun 2026 18:18:13 +0200 Subject: [PATCH] fix(webdav): fix trailing / on collection --- src/interfaces/api/handlers/webdav_handler.rs | 144 +++++++++++++++++- 1 file changed, 137 insertions(+), 7 deletions(-) diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index b4b93ada..1fd985c5 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -69,6 +69,37 @@ pub(crate) fn encode_uri_path(path: &str) -> String { .join("/") } +/// Build the `` value for a non-collection (file) resource. +/// +/// RFC 4918 §5.2 distinguishes collection (folder) URLs from +/// non-collection URLs by a trailing `/`. Files use NO trailing +/// slash. Mirror of [`webdav_collection_href`] — keep both arms +/// of the choice on the same screen so an "is it a file or a +/// folder?" reviewer can verify both branches at once. +fn webdav_href(path: &str) -> String { + format!("/webdav/{}", encode_uri_path(path)) +} + +/// Build the `` value for a collection (folder) resource. +/// +/// Always terminates with `/` — RFC 4918 §5.2 requires collection +/// URLs to end in a slash, and strict WebDAV clients (notably the +/// NextCloud desktop sync engine, which also speaks to this +/// endpoint) abort multi-status parses with +/// `Invalid href "<…>" expected starting with ""` +/// when the response's own-entry href is missing the trailing `/`. +/// PROPPATCH and LOCK responses on folders MUST use this — using +/// [`webdav_href`] for a folder is the bug class this helper +/// exists to prevent. +fn webdav_collection_href(path: &str) -> String { + let h = webdav_href(path); + if h.ends_with('/') { + h + } else { + format!("{}/", h) + } +} + // Create a custom DAV header since it's not in the standard headers const HEADER_DAV: HeaderName = HeaderName::from_static("dav"); const HEADER_LOCK_TOKEN: HeaderName = HeaderName::from_static("lock-token"); @@ -605,12 +636,35 @@ async fn build_streaming_propfind_response( * @return XML response with property modification results */ async fn handle_proppatch( - _state: Arc, + state: Arc, req: Request, path: String, ) -> Result, AppError> { let _user = extract_user(&req)?; + // Resolve the target resource type BEFORE consuming the body so + // we can pick the correct href shape in the multi-status + // response. RFC 4918 §5.2 + strict WebDAV-client parser rules + // require a trailing `/` for collection hrefs; emitting + // `/webdav/foo` for a folder breaks NC-desktop / Cyberduck / + // other multi-status consumers the same way the NC PROPFIND + // bug did. An empty / `/` path is the root, always a + // collection. A path that resolves to neither file nor folder + // (e.g. PROPPATCH on a resource that doesn't exist) defaults + // to non-collection — matches the request-line shape the + // client used, since collection paths conventionally arrive + // with trailing `/` already trimmed by routing. + let is_collection = if path.is_empty() || path == "/" { + true + } else { + state + .applications + .folder_service + .get_folder_by_path(&path) + .await + .is_ok() + }; + // Read request body (XML — bounded to 1 MB) let body_bytes = body::to_bytes(req.into_body(), MAX_XML_BODY) .await @@ -636,8 +690,12 @@ async fn handle_proppatch( results.push((prop, true)); } - // Generate response - let href = format!("/webdav/{}", encode_uri_path(&path)); + // Generate response — collection vs file href chosen above. + let href = if is_collection { + webdav_collection_href(&path) + } else { + webdav_href(&path) + }; let mut response_body = Vec::new(); WebDavAdapter::generate_proppatch_response(&mut response_body, &href, &results).map_err( |e| AppError::internal_error(format!("Failed to generate PROPPATCH response: {}", e)), @@ -1715,6 +1773,24 @@ async fn handle_lock( ) -> Result, AppError> { let user = extract_user(&req)?; + // Determine collection-vs-file for href shape. Root + known + // folders → collection; everything else (existing files, + // lock-null on a non-existent path) → file. RFC 4918 §9.10.1 + // allows LOCK on a non-existent resource (the "lock-null + // resource" pattern used by Office save flows) — that arm + // falls through to the file href shape, matching the + // request-line shape clients send. + let is_collection = if path.is_empty() || path == "/" { + true + } else { + state + .applications + .folder_service + .get_folder_by_path(&path) + .await + .is_ok() + }; + // Get the headers that we need let depth = req .headers() @@ -1764,8 +1840,12 @@ async fn handle_lock( AppError::precondition_failed(format!("Lock token not found or expired: {}", token)) })?; - // Generate response - let href = format!("/webdav/{}", encode_uri_path(&path)); + // Generate response — collection vs file href chosen above. + let href = if is_collection { + webdav_collection_href(&path) + } else { + webdav_href(&path) + }; let mut response_body = Vec::new(); WebDavAdapter::generate_lock_response(&mut response_body, &entry.info, &href).map_err( |e| AppError::internal_error(format!("Failed to generate LOCK response: {}", e)), @@ -1800,8 +1880,12 @@ async fn handle_lock( )) })?; - // Generate response - let href = format!("/webdav/{}", encode_uri_path(&path)); + // Generate response — collection vs file href chosen above. + let href = if is_collection { + webdav_collection_href(&path) + } else { + webdav_href(&path) + }; let mut response_body = Vec::new(); WebDavAdapter::generate_lock_response(&mut response_body, &entry.info, &href).map_err( |e| AppError::internal_error(format!("Failed to generate LOCK response: {}", e)), @@ -1865,3 +1949,49 @@ async fn handle_unlock( .body(Body::empty()) .unwrap()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_webdav_href_no_trailing_slash() { + assert_eq!( + webdav_href("Documents/report.pdf"), + "/webdav/Documents/report.pdf" + ); + assert_eq!(webdav_href("file.txt"), "/webdav/file.txt"); + } + + #[test] + fn test_webdav_collection_href_appends_slash_when_missing() { + assert_eq!(webdav_collection_href("Documents"), "/webdav/Documents/"); + assert_eq!( + webdav_collection_href("Documents/subfolder"), + "/webdav/Documents/subfolder/" + ); + } + + #[test] + fn test_webdav_collection_href_idempotent_when_already_slashed() { + // `encode_uri_path` never emits a trailing `/` of its own + // because the path argument is already trimmed by routing, + // but the helper still has to be robust to a path that + // happens to end in `/` — exercise the idempotence path. + assert_eq!(webdav_collection_href("Documents/"), "/webdav/Documents/"); + } + + #[test] + fn test_webdav_href_preserves_url_encoding() { + // Spaces and Unicode must percent-encode at the segment level, + // not get a verbatim `%20` re-encoded as `%2520`. + assert_eq!( + webdav_href("My Photos/vacation pic.jpg"), + "/webdav/My%20Photos/vacation%20pic.jpg" + ); + assert_eq!( + webdav_collection_href("My Photos/2024"), + "/webdav/My%20Photos/2024/" + ); + } +}