diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index 371c61bd..a13dc874 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -712,7 +712,18 @@ async fn handle_mkcol( let folder_service = &state.applications.folder_service; let internal_path = nc_to_internal_path(&user.username, subpath)?; - // If the folder already exists, return 405 per RFC 4918 §9.3.1 + // RFC 4918 §9.3.1: + // - target already exists → 405 Method Not Allowed + // - parent collection of the target does NOT exist → 409 Conflict + // - parent exists and target does not → 201 Created + // + // Previous behaviour effectively performed `mkdir -p` and returned + // 201 even when intermediate ancestors were missing. Sabre/DAV and + // the actual NC server both return 409 here, so the legacy + // auto-create deviated from the reference implementation. NC desktop + // walks ancestors one MKCOL at a time anyway, so dropping the + // auto-create doesn't break real clients. + if folder_service .get_folder_by_path(&internal_path) .await @@ -724,56 +735,37 @@ async fn handle_mkcol( .unwrap()); } - // Collect path segments that need to be created (walk from root to leaf) 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")); + } + let (target_name, parent_segments) = segments.split_last().expect("checked non-empty above"); let user_root = nc_to_internal_path(&user.username, "")?; - let mut current_path = user_root.clone(); - let mut parent_id = folder_service - .get_folder_by_path(&user_root) - .await - .map_err(|_| AppError::not_found("User root folder not found"))? - .id - .clone(); + let parent_path = if parent_segments.is_empty() { + user_root.clone() + } else { + format!("{}/{}", user_root, parent_segments.join("/")) + }; - for segment in &segments { - current_path = format!("{}/{}", current_path, segment); - match folder_service.get_folder_by_path(¤t_path).await { - Ok(existing) => { - parent_id = existing.id.clone(); - } - Err(_) => { - let dto = CreateFolderDto { - name: segment.to_string(), - parent_id: Some(parent_id.clone()), - }; - match folder_service.create_folder_with_perms(dto, user.id).await { - Ok(created) => { - parent_id = created.id.clone(); - } - Err(e) - if e.message.contains("already exists") - || e.message.contains("Already Exists") => - { - // Race condition — folder created concurrently - let folder = folder_service - .get_folder_by_path(¤t_path) - .await - .map_err(|_| { - AppError::internal_error("Folder exists but cannot be found") - })?; - parent_id = folder.id.clone(); - } - Err(e) => { - return Err(AppError::internal_error(format!( - "Failed to create folder: {}", - e - ))); - } - } - } + let parent_folder = match folder_service.get_folder_by_path(&parent_path).await { + Ok(folder) => folder, + Err(_) => { + return Ok(Response::builder() + .status(StatusCode::CONFLICT) + .body(Body::empty()) + .unwrap()); } - } + }; + + let dto = CreateFolderDto { + name: target_name.to_string(), + parent_id: Some(parent_folder.id.clone()), + }; + folder_service + .create_folder_with_perms(dto, user.id) + .await + .map_err(|e| AppError::internal_error(format!("Failed to create folder: {}", e)))?; Ok(Response::builder() .status(StatusCode::CREATED) diff --git a/tests/webdav/test_nc_put_mkcol_blake3.sh b/tests/webdav/test_nc_put_mkcol_blake3.sh index 5aea75d4..8c2e064e 100755 --- a/tests/webdav/test_nc_put_mkcol_blake3.sh +++ b/tests/webdav/test_nc_put_mkcol_blake3.sh @@ -319,38 +319,43 @@ grep -q '' <<< "$BODY" \ pass "F10: MKCOL creates folder, PROPFIND sees it as a collection" # ───────────────────────────────────────────────────────────── -# F11 — MKCOL with missing intermediate parent +# F11 / F11b / F11c — MKCOL parent semantics (RFC 4918 §9.3.1) # -# Pinned current behaviour: OxiCloud's MKCOL auto-creates -# missing intermediate parents (effectively `mkdir -p` -# semantics). Sending MKCOL on `/a/b/c/` where neither `a` nor -# `b` exists succeeds with 201 — both intermediates are -# silently created. +# F11 : missing intermediate parent → 409 Conflict +# F11b : parent exists, target new → 201 Created (positive case) +# F11c : target already exists → 405 Method Not Allowed # -# Strict RFC 4918 §9.3.1 requires 409 Conflict here ("when the -# parent collection does not exist"). NC desktop tolerates -# either behaviour (it always MKCOLs ancestors one at a time -# during sync), so the auto-create behaviour is harmless in -# practice — but if you ever want strict mode, the fix lives -# in `interfaces/nextcloud/webdav_handler.rs::handle_mkcol`: -# look up the parent path before creating; 409 if missing. +# Sabre/DAV and the actual NC server both 409 on a missing +# intermediate; our previous `mkdir -p` behaviour deviated. NC +# desktop walks ancestors one MKCOL at a time during sync so +# nothing real breaks from dropping the auto-create. # ───────────────────────────────────────────────────────────── -echo " F11: MKCOL with missing parent (pinned: auto-creates parents, RFC-4918 would 409)" +echo " F11: MKCOL with missing intermediate parent → 409" STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MKCOL \ "$NC_FILES_BASE/f11-nonexistent-parent/inner/") -case "$STATUS" in - 201) - pass "F11: MKCOL auto-created intermediate parents (201) — pinned current behaviour" - ;; - 409) - fail "F11: server now returns 409 (RFC-4918 strict). Bug? Improvement? — review and update pin to strict assertion." - ;; - *) - fail "F11: unexpected status $STATUS" - ;; -esac -# Cleanup the auto-created parent so subsequent tests don't see it. -nc_curl -o /dev/null -X DELETE "$NC_FILES_BASE/f11-nonexistent-parent/" > /dev/null 2>&1 || true +[[ "$STATUS" == "409" ]] \ + || fail "F11: expected 409 Conflict for MKCOL with missing parent, got $STATUS" +# The non-existent parent must NOT have been auto-created either. +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/f11-nonexistent-parent/")" == "404" ]] \ + || fail "F11: intermediate parent was silently created — auto-create still happening" +pass "F11: MKCOL with missing parent → 409, parent not silently created" + +echo " F11b: MKCOL with existing parent + new target → 201" +nc_curl -o /dev/null -X MKCOL "$NC_FILES_BASE/f11b-parent/" > /dev/null +STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MKCOL \ + "$NC_FILES_BASE/f11b-parent/child/") +[[ "$STATUS" == "201" ]] \ + || fail "F11b: expected 201 Created for MKCOL with existing parent, got $STATUS" +[[ "$(nc_status_propfind_depth0 "$NC_FILES_BASE/f11b-parent/child/")" == "207" ]] \ + || fail "F11b: target collection not visible via PROPFIND after MKCOL" +pass "F11b: MKCOL with existing parent → 201, target reachable" + +echo " F11c: MKCOL with target that already exists → 405" +STATUS=$(nc_curl -o /dev/null -w "%{http_code}" -X MKCOL \ + "$NC_FILES_BASE/f11b-parent/child/") +[[ "$STATUS" == "405" ]] \ + || fail "F11c: expected 405 Method Not Allowed for MKCOL on existing collection, got $STATUS" +pass "F11c: MKCOL on existing target → 405" # ───────────────────────────────────────────────────────────── # F12 — MKCOL on existing folder → 405