From 9f2ebd0758e63be14a4991c2f64fe7e3bd895af0 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 16 Jun 2026 23:17:40 +0200 Subject: [PATCH] =?UTF-8?q?fix(webdav):=20enforce=20LOCK=20on=20native=20P?= =?UTF-8?q?UT=20(RFC=204918=20=C2=A79.10.4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes N2. The native WebDAV PUT handler now consults the lock store before accepting a write: if the target path is exclusively locked, the request must carry the lock token in its If: header or the server returns 423 Locked. Without a matching token, the body is never consumed — a rejected PUT no longer wastes the upload bandwidth or hits the CDC ingester. Two helpers are introduced so the same enforcement plugs into the other mutator methods (delete/move/copy/proppatch) when their fixes land: extract_if_header_tokens — angle-bracket-scoop view of If: (sufficient for one-target writes; full §10.4 tagged-list grammar would only matter for multi-resource Ifs) enforce_native_lock — Some(423) when locked + no/wrong token, None otherwise Test N2 flipped from pinned 204 to assert 423. Added N2b: same PUT with the captured Lock-Token in If:(<...>) returns 204, so a regression that hard-rejected every PUT would still fail loudly. --- src/interfaces/api/handlers/webdav_handler.rs | 77 +++++++++++++++++++ tests/webdav/test_native_webdav_lifecycle.sh | 49 +++++------- 2 files changed, 96 insertions(+), 30 deletions(-) diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 4b8c2117..8d20bf2c 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -895,6 +895,66 @@ async fn handle_head( .unwrap()) } +/// Extract every `<...>` token from a WebDAV `If:` header value. +/// +/// RFC 4918 §10.4 defines a richer grammar (tagged-list / no-tag-list of +/// `(Condition)` items), but for our purposes the only thing that matters +/// is what lock tokens the caller is claiming to hold. Forgivingly scoop +/// every angle-bracketed value and let the caller compare against the +/// active lock token(s). +fn extract_if_header_tokens(if_header: &str) -> Vec { + let mut out = Vec::new(); + let mut current = String::new(); + let mut inside = false; + for c in if_header.chars() { + match (inside, c) { + (false, '<') => { + inside = true; + current.clear(); + } + (true, '>') => { + inside = false; + if !current.is_empty() { + out.push(std::mem::take(&mut current)); + } + } + (true, c) => current.push(c), + _ => {} + } + } + out +} + +/// RFC 4918 §9.10.4 — if `path` is locked, every mutating request MUST +/// carry the lock's token in its `If:` header. Returns `Some(Response)` +/// with a 423 Locked response when the request must be rejected; `None` +/// when the path is unlocked or the caller's `If:` header carries the +/// matching token (the cheap-and-cheerful submission check). +/// +/// Shared by `handle_put` now and will be reused by `handle_delete`, +/// `handle_move`, `handle_copy`, and `handle_proppatch` when each of +/// those gets the same enforcement. +fn enforce_native_lock( + lock_store: &crate::infrastructure::services::webdav_lock_service::WebDavLockStore, + if_header: Option<&str>, + path: &str, +) -> Option> { + let entry = lock_store.get_by_path(path)?; + if let Some(h) = if_header + && extract_if_header_tokens(h) + .iter() + .any(|t| t == &entry.info.token) + { + return None; + } + Some( + Response::builder() + .status(StatusCode::LOCKED) + .body(Body::empty()) + .unwrap(), + ) +} + /** * Handles PUT requests to create or update files. * @@ -926,6 +986,23 @@ async fn handle_put( return Err(AppError::bad_request("Cannot PUT to root folder")); } + // ── Active-lock guard (RFC 4918 §9.10.4) ────────────────────────── + // Reject a write that targets a locked resource unless the request + // carries the lock token in `If:`. Captured before we consume the + // body into the CDC ingester — a 423 mustn't waste any bandwidth. + 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); + } + // ── Ownership guard ──────────────────────────────────────── // Verify that the user owns the target file (update) or the // parent folder (create). Without this check a user could diff --git a/tests/webdav/test_native_webdav_lifecycle.sh b/tests/webdav/test_native_webdav_lifecycle.sh index 828d22bf..2e011c4a 100755 --- a/tests/webdav/test_native_webdav_lifecycle.sh +++ b/tests/webdav/test_native_webdav_lifecycle.sh @@ -332,44 +332,33 @@ LOCK_TOKEN=$(grep -i '^lock-token:' <<< "$HEADERS" | awk '{print $2}' | tr -d '\ pass "N1: LOCK → 200 + Lock-Token=$LOCK_TOKEN" # ───────────────────────────────────────────────────────────── -# N2 — PUT without the lock token → 423 Locked -# ───────────────────────────────────────────────────────────── -# ───────────────────────────────────────────────────────────── -# N2 — PUT to a locked file without the token +# N2 — PUT to a locked file without the token → 423 Locked # # RFC 4918 §9.10.4 + §6: a writeable resource under an # exclusive lock MUST reject conflicting writes with 423 -# Locked. OxiCloud's native handler currently does NOT consult -# the lock store before writing — LOCK just produces a token, -# and any PUT/DELETE/MOVE/PROPPATCH succeeds regardless. The -# class-2 DAV advertisement in M1 is therefore aspirational: -# the protocol surface exists, the enforcement doesn't. -# -# Where the fix lives: -# `interfaces/api/handlers/webdav_handler.rs::handle_put` (and -# the mutator paths in handle_delete / handle_move / handle_copy / -# handle_proppatch) — each needs to check the WebDAV lock service -# for an active lock on the target path and reject with 423 if -# the request doesn't carry a matching `If: ()` header. -# The lock store itself already records tokens — confirmed by N1 -# capturing one — so the gap is purely on the read-side check. +# Locked unless the request submits the lock token in `If:`. +# N2b verifies the inverse: same PUT with the correct +# `If: ()` header succeeds, proving the gate isn't +# blocking legitimate updates from the lock owner. # ───────────────────────────────────────────────────────────── -echo " N2: PUT /webdav/n-locked.txt without If:() — pinned: lock not enforced (RFC would 423)" +echo " N2: PUT /webdav/n-locked.txt without If:() → 423" STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PUT \ -H "Content-Type: text/plain" \ --data-binary 'tampered contents' \ "$DAV_BASE/n-locked.txt") -case "$STATUS" in - 204) - pass "N2: PUT succeeded despite active lock → 204 (KNOWN BUG: lock not enforced — pinned)" - ;; - 423) - fail "N2: server now returns 423 Locked. Lock enforcement was added — update this pin to assert == 423." - ;; - *) - fail "N2: unexpected status $STATUS" - ;; -esac +[[ "$STATUS" == "423" ]] \ + || fail "N2: expected 423 Locked for PUT to locked path without token, got $STATUS" +pass "N2: PUT to locked path without token → 423" + +echo " N2b: PUT /webdav/n-locked.txt WITH If:() → 204" +STATUS=$(dav_curl -o /dev/null -w "%{http_code}" -X PUT \ + -H "Content-Type: text/plain" \ + -H "If: (<$LOCK_TOKEN>)" \ + --data-binary 'authorised update' \ + "$DAV_BASE/n-locked.txt") +[[ "$STATUS" == "204" ]] \ + || fail "N2b: expected 204 No Content for PUT with correct lock token, got $STATUS" +pass "N2b: PUT with matching If:() → 204" # ───────────────────────────────────────────────────────────── # N3 — UNLOCK with token → 204; subsequent PUT succeeds