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

fix(webdav): enforce LOCK on every native mutator (RFC 4918 §9.10.4)

Browse Source 
Extends the N2/PUT lock guard introduced earlier to the rest of the
native mutator surface. Same helper, same If: capture before body
consumption, same 423-on-reject shape:

  - handle_delete    : check source path
  - handle_proppatch : check source path
  - handle_move      : check source AND destination paths
  - handle_copy      : check destination path only (source isn't
                       modified by a copy)

The class-2 DAV advertisement in OPTIONS is now honest across the
full surface, not just PUT.

New tests N2c-N2f run while n-locked.txt is still LOCKed (before the
existing N3 UNLOCK). Each asserts 423 without the token and verifies
the operation didn't half-apply: file present after DELETE-423,
source untouched + no destination after MOVE-423, locked destination's
content unchanged after COPY-423.

Positive (with-token) coverage is implicit via the M-series happy-
path tests that exercise each method on unlocked resources — a
regression that hard-rejected every call would fail there too.
This commit is contained in:
Edouard Vanbelle
2026-06-16 23:44:21 +02:00
parent af3ce24062
commit bacd5806d3
2 changed files with 141 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/interfaces/api/handlers/webdav_handler.rs
+73
View File
@@ -658,6 +658,21 @@ async fn handle_proppatch(
) -> Result<Response<Body>, AppError> {
let _user = extract_user(&req)?;
// Active-lock guard (RFC 4918 §9.10.4): PROPPATCH writes properties,
// so a lock on the target must release them via `If:`. Captured
// before the body is consumed below so a rejected request doesn't
// even parse the XML.
let if_header_owned = req
.headers()
.get("If")
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
if let Some(resp) =
enforce_native_lock(&state.webdav_lock_store, if_header_owned.as_deref(), &path)
{
return Ok(resp);
}
// 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
@@ -1237,6 +1252,18 @@ async fn handle_delete(
) -> Result<Response<Body>, AppError> {
let user = extract_user(&req)?;
// Active-lock guard (RFC 4918 §9.10.4).
let if_header_owned = req
.headers()
.get("If")
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
if let Some(resp) =
enforce_native_lock(&state.webdav_lock_store, if_header_owned.as_deref(), &path)
{
return Ok(resp);
}
// Get services from state
let file_retrieval_service = &state.applications.file_retrieval_service;
let file_management_service = &state.applications.file_management_service;
@@ -1293,6 +1320,23 @@ async fn handle_move(
let user = extract_user(&req)?;
let source_path = path;
// Captured up front so a rejected MOVE doesn't run any DB work.
let if_header_owned = req
.headers()
.get("If")
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
// Active-lock guard on the SOURCE (RFC 4918 §9.10.4): the move
// removes the source resource, which counts as modifying it.
if let Some(resp) = enforce_native_lock(
&state.webdav_lock_store,
if_header_owned.as_deref(),
&source_path,
) {
return Ok(resp);
}
// Get destination from Destination header
let destination = req
.headers()
@@ -1321,6 +1365,17 @@ async fn handle_move(
// SECURITY: reject path-traversal in destination
reject_path_traversal(&destination_path)?;
// Destination lock guard: MOVE also creates/replaces a resource at
// the destination. If that path is locked, the same If: header must
// satisfy it.
if let Some(resp) = enforce_native_lock(
&state.webdav_lock_store,
if_header_owned.as_deref(),
&destination_path,
) {
return Ok(resp);
}
// Get services from state
let file_retrieval_service = &state.applications.file_retrieval_service;
let file_management_service = &state.applications.file_management_service;
@@ -1455,6 +1510,15 @@ async fn handle_copy(
let user = extract_user(&req)?;
let source_path = path;
// Captured up front (cheap; used below for the destination lock guard).
// COPY doesn't mutate the source, so no source lock check — only the
// destination needs to clear (RFC 4918 §9.10.4).
let if_header_owned = req
.headers()
.get("If")
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
// Get destination from Destination header
let destination = req
.headers()
@@ -1483,6 +1547,15 @@ async fn handle_copy(
// SECURITY: reject path-traversal in destination
reject_path_traversal(&destination_path)?;
// Active-lock guard on the destination (RFC 4918 §9.10.4).
if let Some(resp) = enforce_native_lock(
&state.webdav_lock_store,
if_header_owned.as_deref(),
&destination_path,
) {
return Ok(resp);
}
// Get depth from Depth header
let depth = req
.headers()
tests/webdav/test_native_webdav_lifecycle.sh
+68
View File
@@ -302,6 +302,74 @@ STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PUT \
|| fail "N2b: expected 204 No Content for PUT with correct lock token, got $STATUS"
pass "N2b: PUT with matching If:(<token>) → 204"
# ─────────────────────────────────────────────────────────────
# N2c–N2f — Lock enforcement on the other mutator methods
#
# RFC 4918 §9.10.4: a lock binds every mutating method, not just
# PUT. The native handler's `enforce_native_lock` helper was
# designed to be called by handle_delete / handle_move /
# handle_copy / handle_proppatch as well — these tests prove the
# wire is in. Each case uses the n-locked.txt resource locked
# above and a `WITHOUT If:` request, expecting 423. Positive
# (with-token) coverage is implicit: the M-series above already
# exercises each method on unlocked resources and asserts the
# success codes, so a regression that hard-rejected every call
# would fail there.
#
# Order matters: each must run while the lock is still held,
# i.e. before N3 below releases it.
# ─────────────────────────────────────────────────────────────
echo " N2c: DELETE /webdav/n-locked.txt without If:(<token>) → 423"
STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X DELETE \
"$DAV_BASE/n-locked.txt")
[[ "$STATUS" == "423" ]] \
|| fail "N2c: expected 423 Locked for DELETE on locked path without token, got $STATUS"
# The file must still be present after a rejected DELETE.
[[ "$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/n-locked.txt")" == "207" ]] \
|| fail "N2c: file removed after rejected DELETE (423 was advisory only?)"
pass "N2c: DELETE on locked path without token → 423, resource preserved"
echo " N2d: MOVE /webdav/n-locked.txt without If:(<token>) → 423 (source-side lock)"
STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X MOVE \
-H "Destination: $DAV_BASE/n-locked-moved.txt" \
"$DAV_BASE/n-locked.txt")
[[ "$STATUS" == "423" ]] \
|| fail "N2d: expected 423 Locked for MOVE on locked source without token, got $STATUS"
[[ "$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/n-locked.txt")" == "207" ]] \
|| fail "N2d: source disappeared after rejected MOVE"
[[ "$(dav_curl -o /dev/null -w "%{http_code}" -X PROPFIND -H "Depth: 0" "$DAV_BASE/n-locked-moved.txt")" == "404" ]] \
|| fail "N2d: destination created after rejected MOVE"
pass "N2d: MOVE with locked source and no token → 423, no state mutated"
echo " N2e: COPY into /webdav/n-locked.txt (locked destination) without If:(<token>) → 423"
# Set up a fresh unlocked source for the COPY.
dav_curl -o /dev/null -X PUT -H "Content-Type: text/plain" \
--data-binary 'n2e copy source' \
"$DAV_BASE/n2e-copy-src.txt" > /dev/null
STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X COPY \
-H "Destination: $DAV_BASE/n-locked.txt" \
"$DAV_BASE/n2e-copy-src.txt")
[[ "$STATUS" == "423" ]] \
|| fail "N2e: expected 423 Locked for COPY into locked destination without token, got $STATUS"
# The locked destination's content must not have been replaced.
BODY=$(dav_curl -s "$DAV_BASE/n-locked.txt")
[[ "$BODY" == "authorised update" ]] \
|| fail "N2e: locked destination's content was overwritten (got '$BODY')"
pass "N2e: COPY into locked destination without token → 423, target untouched"
echo " N2f: PROPPATCH /webdav/n-locked.txt without If:(<token>) → 423"
PROPPATCH_BODY='<?xml version="1.0" encoding="utf-8"?>
<d:propertyupdate xmlns:d="DAV:">
<d:set><d:prop><d:displayname>tampered</d:displayname></d:prop></d:set>
</d:propertyupdate>'
STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PROPPATCH \
-H "Content-Type: application/xml" \
--data "$PROPPATCH_BODY" \
"$DAV_BASE/n-locked.txt")
[[ "$STATUS" == "423" ]] \
|| fail "N2f: expected 423 Locked for PROPPATCH on locked path without token, got $STATUS"
pass "N2f: PROPPATCH on locked path without token → 423"
# ─────────────────────────────────────────────────────────────
# N3 — UNLOCK with token → 204; subsequent PUT succeeds
# ─────────────────────────────────────────────────────────────