fix(webdav): enforce LOCK on native PUT (RFC 4918 §9.10.4)
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.
This commit is contained in:
Edouard Vanbelle
@@ -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<String> {
|
||||
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<Response<Body>> {
|
||||
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
|
||||
|
||||
@@ -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: (<token>)` 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: (<token>)` header succeeds, proving the gate isn't
|
||||
# blocking legitimate updates from the lock owner.
|
||||
# ─────────────────────────────────────────────────────────────
|
||||
echo " N2: PUT /webdav/n-locked.txt without If:(<token>) — pinned: lock not enforced (RFC would 423)"
|
||||
echo " N2: PUT /webdav/n-locked.txt without If:(<token>) → 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:(<token>) → 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:(<token>) → 204"
|
||||
|
||||
# ─────────────────────────────────────────────────────────────
|
||||
# N3 — UNLOCK with token → 204; subsequent PUT succeeds
|
||||
|
||||
Reference in New Issue
Block a user