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

fix(nc/webdav): MKCOL on a missing parent → 409 (RFC 4918 §9.3.1)

Browse Source 
Closes F11.

The NC MKCOL handler previously had `mkdir -p` semantics:
sending MKCOL on /a/b/c/ where neither a nor b exists silently
created both intermediates and returned 201. Sabre/DAV and the
actual NC server both 409 on that — our auto-create deviated.
NC desktop walks ancestors one MKCOL at a time during sync so
nothing real depended on the old behaviour.

Drop the segment-walking creation loop. New flow:
  target exists           → 405
  parent path missing     → 409
  parent ok, target new   → 201

The race-recovery branch for the loop's per-segment create is
also gone — single parent lookup, single create, no window.

Test F11 flipped from pinned-201 to strict 409 and asserts the
intermediate parent was not silently created. F11b and F11c added
as regression guards for the success path and the 'target already
exists' case.
This commit is contained in:
Edouard Vanbelle
2026-06-16 23:38:17 +02:00
parent e2702ac673
commit af3ce24062
2 changed files with 70 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/interfaces/nextcloud/webdav_handler.rs
+38 -46
View File
@@ -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(&current_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(&current_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)
tests/webdav/test_nc_put_mkcol_blake3.sh
+32 -27
View File
@@ -319,38 +319,43 @@ grep -q '<d:collection/>' <<< "$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